Skip to content

fix(headless): serialize the screenshot network event handlers - #2723

Merged
dwisiswant0 merged 1 commit into
projectdiscovery:devfrom
antcybersec:fix/screenshot-network-requests-race
Nov 5, 2026
Merged

dwisiswant0 merged 1 commit into
projectdiscovery:devfrom
antcybersec:fix/screenshot-network-requests-race

Conversation

@antcybersec

@antcybersec antcybersec commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2722

  • The four network event handlers in setupPageAndNavigate ran in separate go page.EachEvent(h)() goroutines and wrote to the same *NetworkRequest values. They are now registered in one EachEvent call, which rod dispatches one event at a time from a single goroutine. The dialog handler stays separate.
  • The function returned networkRequests.Slice while the handlers could still be appending. It now returns snapshot(networkRequests), a copy taken under the slice's lock.
  • No new test: TestScreenshotNetworkPolicyAndHeaders (Apply the network policy to screenshot requests #2695) already covers this under -race. On dev (4f520f9) it fails 2 runs out of 3 with WARNING: DATA RACE at headless.go:187/197. With this change it passed 10 of 10 runs, and the full ./runner/ ./common/... suite passes with -race.

Tested with:

  • go test -race ./runner/ ./common/...
  • go vet ./runner/
  • golangci-lint run ./runner/...

Summary by CodeRabbit

  • Bug Fixes
    • Improved consistency when collecting and returning network activity during page navigation.

Signed-off-by: anant <170962632+antcybersec@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

setupPageAndNavigate now processes network lifecycle events in one sequential EachEvent loop. It returns a copied request snapshot on success and error paths.

Changes

Screenshot Network Tracking

Layer / File(s) Summary
Serialize event handling and snapshot results
runner/headless.go
One EachEvent loop processes outbound, response, completion, and failure events. A new snapshot helper copies collected requests, and navigation uses that copy for success and error returns.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related PRs

Suggested reviewers: mzack9999, serizao

Merge Risk: Minimal · up to 02231

The change serializes network-event updates and returns independent request snapshots. No actionable merge-blocking issue remains.

Pre-merge checks: Passed 5

Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies the main change: serializing screenshot network event handlers in headless mode.
Linked Issues check Passed Issue #2722 has two coding requirements. Both are implemented in runner/headless.go.

1. Serialize the network handlers. The diff replaces the four go page.EachEvent(h)() calls (RequestWillBeSent, R…
Out of Scope Changes check Passed The PR changes only runner/headless.go (diff against merge base 4f520f9). Every hunk falls inside setupPageAndNavigate or is the new snapshot helper it calls. The four network handlers are merged…
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Linked Issues check

Explanation

Issue #2722 has two coding requirements. Both are implemented in runner/headless.go.

  1. Serialize the network handlers. The diff replaces the four go page.EachEvent(h)() calls (RequestWillBeSent, ResponseReceived, LoadingFinished, LoadingFailed) with one go page.EachEvent(h1, h2, h3, h4)() call. The handlers share *NetworkRequest values through requestsMap, so one dispatch loop removes the concurrent writes that the issue reports at headless.go:187/197. The issue states that rod calls the handlers one at a time from one goroutine. That rod behavior comes from the external library and is not visible in this repository. I accept it because the issue and the PR both rely on it.

  2. Avoid reading the slice while handlers append. setupPageAndNavigate now returns snapshot(networkRequests) on every return path. The new snapshot helper copies entries through SyncSlice.Each and Len, so the copy is taken through the slice's synchronized API. The return values are unchanged in type and signature.

The issue expects the test to pass under -race. The test TestScreenshotNetworkPolicyAndHeaders already exists from #2695 (commit f1c43fc), and the PR adds no new test. This assessment relies on the PR's reported test runs, which I did not execute. The code changes address the race that the issue describes.

Full details: Out of Scope Changes check

Explanation

The PR changes only runner/headless.go (diff against merge base 4f520f9). Every hunk falls inside setupPageAndNavigate or is the new snapshot helper it calls. The four network handlers are merged into one EachEvent call, and the return statements now use snapshot. The dialog handler is unchanged and remains a separate EachEvent, which is consistent with the issue because the dialog handler does not touch the network request state. The snapshot helper supports the locked-copy requirement from #2722. The changes do not add unrelated behavior, refactoring, or public declaration changes.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit watched requests arrive,
Four event paths now run in line.
A snapshot keeps the list in view,
Each copied entry clear and true.
The rabbit hops, pleased with the fix.

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d56fd40e-ce1f-4b69-939c-f53603d5dd4a
Commits

Reviewing files that changed from the base of the PR and between 0106d7f and 02231c0.

Files selected for processing: 1

  • runner/headless.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@dwisiswant0
dwisiswant0 merged commit b459738 into projectdiscovery:dev Nov 5, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

data race between the screenshot network event handlers

2 participants