From a3e54843443497aeca75a2ebcb5aef7cab8fc154 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Sun, 2 Nov 2025 21:23:00 -0500 Subject: [PATCH 1/2] Add failing test for data race --- tests/browser_context_close_race_test.go | 108 +++++++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 tests/browser_context_close_race_test.go diff --git a/tests/browser_context_close_race_test.go b/tests/browser_context_close_race_test.go new file mode 100644 index 00000000..9f8a7dd5 --- /dev/null +++ b/tests/browser_context_close_race_test.go @@ -0,0 +1,108 @@ +package playwright_test + +import ( + "os" + "testing" + "time" + + "github.com/playwright-community/playwright-go" + "github.com/stretchr/testify/require" +) + +// TestBrowserContextCloseRace tests for a data race between Close() and route handlers. +// This reproduces a race condition where Close() writes to closeWasCalled while +// route handler goroutines read it during page navigation. +// +// See: https://github.com/playwright-community/playwright-go/issues/XXX +func TestBrowserContextCloseRace(t *testing.T) { + // Create a minimal HAR file + harContent := `{ + "log": { + "version": "1.2", + "creator": {"name": "test", "version": "1.0"}, + "entries": [ + { + "request": { + "method": "GET", + "url": "https://example.com/", + "httpVersion": "HTTP/2.0", + "headers": [], + "queryString": [], + "cookies": [], + "headersSize": -1, + "bodySize": 0 + }, + "response": { + "status": 200, + "statusText": "OK", + "httpVersion": "HTTP/2.0", + "headers": [{"name": "Content-Type", "value": "text/html"}], + "cookies": [], + "content": { + "size": 13, + "mimeType": "text/html", + "text": "Hello, World!" + }, + "redirectURL": "", + "headersSize": -1, + "bodySize": 13 + }, + "cache": {}, + "timings": {"send": 0, "wait": 0, "receive": 0} + } + ] + } +}` + + harFile, err := os.CreateTemp("", "test-*.har") + require.NoError(t, err) + defer os.Remove(harFile.Name()) + + _, err = harFile.WriteString(harContent) + require.NoError(t, err) + harFile.Close() + + // Create a new context for this test (don't use BeforeEach) + testContext, err := browser.NewContext() + require.NoError(t, err) + defer testContext.Close() + + // Set up HAR replay - registers internal route handlers + err = testContext.RouteFromHAR(harFile.Name(), playwright.BrowserContextRouteFromHAROptions{ + NotFound: playwright.HarNotFoundAbort, + }) + require.NoError(t, err) + + // Add custom route handler + err = testContext.Route("**/version.json*", func(route playwright.Route) { + time.Sleep(5 * time.Millisecond) // Increase race window + _ = route.Fulfill(playwright.RouteFulfillOptions{ + Status: playwright.Int(200), + ContentType: playwright.String("application/json"), + Body: playwright.String(`{"version": "1.0"}`), + }) + }) + require.NoError(t, err) + + testPage, err := testContext.NewPage() + require.NoError(t, err) + + // Start navigation in background + done := make(chan error, 1) + go func() { + _, err := testPage.Goto("https://example.com/") + done <- err + }() + + // Give route handlers time to start processing + time.Sleep(20 * time.Millisecond) + + // Close context while route handlers are actively running + // This triggers the race between Close() and the route handler goroutines + // Without proper synchronization, this will be detected by -race flag + err = testContext.Close() + require.NoError(t, err) + + // Wait for navigation to complete + <-done +} From 17d86a42ce0af7a15e667caf7c9194cb4259bf97 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Sun, 2 Nov 2025 21:26:27 -0500 Subject: [PATCH 2/2] Implement closeWasCalled with atomic.Bool to prevent data race --- browser_context.go | 9 +++++---- page.go | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/browser_context.go b/browser_context.go index 1d420d3b..b825285a 100644 --- a/browser_context.go +++ b/browser_context.go @@ -9,6 +9,7 @@ import ( "slices" "strings" "sync" + "sync/atomic" "github.com/playwright-community/playwright-go/internal/safe" ) @@ -16,7 +17,7 @@ import ( type browserContextImpl struct { channelOwner timeoutSettings *timeoutSettings - closeWasCalled bool + closeWasCalled atomic.Bool options *BrowserNewContextOptions pages []Page routes []*routeHandlerEntry @@ -411,13 +412,13 @@ func (b *browserContextImpl) ExpectPage(cb func() error, options ...BrowserConte } func (b *browserContextImpl) Close(options ...BrowserContextCloseOptions) error { - if b.closeWasCalled { + if b.closeWasCalled.Load() { return nil } if len(options) == 1 { b.closeReason = options[0].Reason } - b.closeWasCalled = true + b.closeWasCalled.Store(true) _, err := b.channel.connection.WrapAPICall(func() (interface{}, error) { return nil, b.request.Dispose(APIRequestContextDisposeOptions{ @@ -597,7 +598,7 @@ func (b *browserContextImpl) onRoute(route *routeImpl) { url := route.Request().URL() for _, handlerEntry := range routes { // If the page or the context was closed we stall all requests right away. - if (page != nil && page.closeWasCalled) || b.closeWasCalled { + if (page != nil && page.closeWasCalled) || b.closeWasCalled.Load() { return } if !handlerEntry.Matches(url) { diff --git a/page.go b/page.go index d4271a04..2706f2a0 100644 --- a/page.go +++ b/page.go @@ -942,7 +942,7 @@ func (p *pageImpl) onRoute(route *routeImpl) { url := route.Request().URL() for _, handlerEntry := range routes { // If the page was closed we stall all requests right away. - if p.closeWasCalled || p.browserContext.closeWasCalled { + if p.closeWasCalled || p.browserContext.closeWasCalled.Load() { return } if !handlerEntry.Matches(url) {