You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(headless): serialize the screenshot network event handlers - #2723
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.
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.
projectdiscovery/httpx#2695: Adds network-policy enforcement to the screenshot navigation and subresource request flow updated here.
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: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly identifies the main change: serializing screenshot network event handlers in headless mode.
Linked Issues check
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
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
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2722
setupPageAndNavigateran in separatego page.EachEvent(h)()goroutines and wrote to the same*NetworkRequestvalues. They are now registered in oneEachEventcall, which rod dispatches one event at a time from a single goroutine. The dialog handler stays separate.networkRequests.Slicewhile the handlers could still be appending. It now returnssnapshot(networkRequests), a copy taken under the slice's lock.TestScreenshotNetworkPolicyAndHeaders(Apply the network policy to screenshot requests #2695) already covers this under-race. Ondev(4f520f9) it fails 2 runs out of 3 withWARNING: DATA RACEatheadless.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