Skip to content

[codex] fix page frames data race#626

Merged
mxschmitt merged 1 commit into
mainfrom
codex/fix-page-frames-race
Jul 17, 2026
Merged

[codex] fix page frames data race#626
mxschmitt merged 1 commit into
mainfrom
codex/fix-page-frames-race

Conversation

@mxschmitt

@mxschmitt mxschmitt commented Jul 17, 2026

Copy link
Copy Markdown
Owner

What changed

  • protect pageImpl.frames with a dedicated framesMu
  • return a cloned frame snapshot from Page.Frames()
  • make Page.Frame() search the same protected snapshot
  • keep frame event emission outside the lock
  • add a race-detector regression test that attaches and removes iframes while reading the page frame list

Why

Frame attach and detach events mutate pageImpl.frames on the connection dispatch goroutine while user goroutines can call Page.Frames() and Page.Frame(). This causes concurrent slice reads and writes.

A dedicated mutex is used instead of the embedded channelOwner mutex. The latter also protects routing state and can be held across protocol calls; making frame event dispatch acquire it could block the sole protocol dispatcher while another goroutine waits for a reply.

Impact

Concurrent frame-list reads no longer race with frame lifecycle events. Returning a clone also prevents callers from retaining the internal slice backing array.

Validation

  • reproduced multiple race reports on the unpatched base
  • go test -race ./tests -run '^TestPageFramesRace$' -count=10 -v
  • full Chromium browser integration package passed
  • gofumpt -w page.go tests/page_frames_race_test.go
  • git diff --check

closes #625

@mxschmitt
mxschmitt marked this pull request as ready for review July 17, 2026 00:57
@mxschmitt
mxschmitt merged commit 9af1314 into main Jul 17, 2026
21 of 23 checks passed
@mxschmitt
mxschmitt deleted the codex/fix-page-frames-race branch July 17, 2026 02:14
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.

1 participant