Merge pull request #943 from grafana/fix/914-life-cycle-wait-fix

Fix race condition in navigation (and lifecycle events)

chromium/browser_type.go

@@ -326,7 +326,7 @@ func parseArgs(flags map[string]any) ([]string, error) {
 
 	// Force the first page to be blank, instead of the welcome page;
 	// --no-first-run doesn't enforce that.
-	// args = append(args, "about:blank")
+	// args = append(args, common.BlankPage)
 	// args = append(args, "--no-startup-window")
 	return args, nil
 }

common/browser.go

@@ -395,7 +395,7 @@ func (b *Browser) newPageInContext(id cdp.BrowserContextID) (*Page, error) {
 	defer removeEventHandler()
 
 	// create a new page.
-	action := target.CreateTarget("about:blank").WithBrowserContextID(id)
+	action := target.CreateTarget(BlankPage).WithBrowserContextID(id)
 	tid, err := action.Do(cdp.WithExecutor(ctx, b.conn))
 	if err != nil {
 		return nil, fmt.Errorf("creating a new blank page: %w", err)

common/browser_test.go

@@ -61,7 +61,7 @@ func TestBrowserNewPageInContext(t *testing.T) {
 				require.Equal(t, target.CommandCreateTarget, method)
 				require.IsType(t, params, &target.CreateTargetParams{})
 				tp, _ := params.(*target.CreateTargetParams)
-				require.Equal(t, "about:blank", tp.URL)
+				require.Equal(t, BlankPage, tp.URL)
 				require.Equal(t, browserContextID, tp.BrowserContextID)
 
 				// newPageInContext event handler will catch this target ID, and compare it to

common/frame.go

@@ -305,7 +305,10 @@ func (f *Frame) onLifecycleEvent(event LifecycleEvent) {
 	f.lifecycleEvents[event] = true
 	f.lifecycleEventsMu.Unlock()
 
-	f.emit(EventFrameAddLifecycle, event)
+	f.emit(EventFrameAddLifecycle, FrameLifecycleEvent{
+		URL:   f.URL(),
+		Event: event,
+	})
 }
 
 func (f *Frame) onLoadingStarted() {
@@ -1693,8 +1696,8 @@ func (f *Frame) WaitForLoadState(state string, opts goja.Value) {
 	lifecycleEvtCh, lifecycleEvtCancel := createWaitForEventPredicateHandler(
 		timeoutCtx, f, []string{EventFrameAddLifecycle},
 		func(data any) bool {
-			if le, ok := data.(LifecycleEvent); ok {
-				return le == waitUntil
+			if le, ok := data.(FrameLifecycleEvent); ok {
+				return le.Event == waitUntil
 			}
 			return false
 		})
@@ -1736,8 +1739,8 @@ func (f *Frame) WaitForNavigation(opts goja.Value) (api.Response, error) {
 	lifecycleEvtCh, lifecycleEvtCancel := createWaitForEventPredicateHandler(
 		timeoutCtx, f, []string{EventFrameAddLifecycle},
 		func(data any) bool {
-			if le, ok := data.(LifecycleEvent); ok {
-				return le == parsedOpts.WaitUntil
+			if le, ok := data.(FrameLifecycleEvent); ok {
+				return le.Event == parsedOpts.WaitUntil
 			}
 			return false
 		})
@@ -1773,7 +1776,7 @@ func (f *Frame) WaitForNavigation(opts goja.Value) (api.Response, error) {
 				sameDocNav = true
 				break
 			}
-			// request could be nil if navigating to e.g. about:blank
+			// request could be nil if navigating to e.g. BlankPage.
 			req := e.newDocument.request
 			if req != nil {
 				req.responseMu.RLock()

common/frame_manager.go

@@ -592,10 +592,23 @@ func (m *FrameManager) NavigateFrame(frame *Frame, url string, parsedOpts *Frame
 	lifecycleEvtCh, lifecycleEvtCancel := createWaitForEventPredicateHandler(
 		timeoutCtx, frame, []string{EventFrameAddLifecycle},
 		func(data any) bool {
-			if le, ok := data.(LifecycleEvent); ok {
-				return le == parsedOpts.WaitUntil
+			le, ok := data.(FrameLifecycleEvent)
+			if !ok {
+				return false
 			}
-			return false
+			// skip the initial blank page if we are navigating to a non-blank page.
+			// otherwise, we will get a lifecycle event for the initial blank page
+			// and return prematurely before waiting for the navigation to complete.
+			if url != BlankPage && le.URL == BlankPage {
+				m.logger.Debugf(
+					"FrameManager:NavigateFrame:createWaitForEventPredicateHandler",
+					"fmid:%d fid:%v furl:%s url:%s waitUntil:%s event.lifecycle:%q event.url:%q skipping %s",
+					fmid, fid, furl, url, parsedOpts.WaitUntil, le.Event, le.URL, BlankPage,
+				)
+				return false
+			}
+
+			return le.Event == parsedOpts.WaitUntil
 		})
 	defer lifecycleEvtCancel()
 
@@ -646,7 +659,7 @@ func (m *FrameManager) NavigateFrame(frame *Frame, url string, parsedOpts *Frame
 	case evt := <-navEvtCh:
 		if e, ok := evt.(*NavigationEvent); ok {
 			req := e.newDocument.request
-			// Request could be nil in case of navigation to e.g. about:blank
+			// Request could be nil in case of navigation to e.g. BlankPage.
 			if req != nil {
 				req.responseMu.RLock()
 				resp = req.response

common/page.go

@@ -762,8 +762,8 @@ func (p *Page) Reload(opts goja.Value) api.Response {
 	lifecycleEvtCh, lifecycleEvtCancel := createWaitForEventPredicateHandler(
 		timeoutCtx, p.frameManager.MainFrame(), []string{EventFrameAddLifecycle},
 		func(data any) bool {
-			if le, ok := data.(LifecycleEvent); ok {
-				return le == parsedOpts.WaitUntil
+			if le, ok := data.(FrameLifecycleEvent); ok {
+				return le.Event == parsedOpts.WaitUntil
 			}
 			return false
 		})

common/types.go

@@ -15,6 +15,9 @@ import (
 	"github.com/dop251/goja"
 )
 
+// BlankPage represents a blank page.
+const BlankPage = "about:blank"
+
 // ColorScheme represents a browser color scheme.
 type ColorScheme string
 
@@ -217,6 +220,15 @@ func (f *ImageFormat) UnmarshalJSON(b []byte) error {
 	return nil
 }
 
+// FrameLifecycleEvent is emitted when a frame lifecycle event occurs.
+type FrameLifecycleEvent struct {
+	// URL is the URL of the frame that emitted the event.
+	URL string
+
+	// Event is the lifecycle event that occurred.
+	Event LifecycleEvent
+}
+
 type LifecycleEvent int
 
 const (

tests/launch_options_slowmo_test.go

@@ -93,7 +93,7 @@ func TestBrowserOptionsSlowMo(t *testing.T) {
 			t.Parallel()
 			tb := newTestBrowser(t, withFileServer())
 			testPageSlowMoImpl(t, tb, func(_ *testBrowser, p *common.Page) {
-				_, err := p.Goto("about:blank", nil)
+				_, err := p.Goto(common.BlankPage, nil)
 				require.NoError(t, err)
 			})
 		})
@@ -223,7 +223,7 @@ func TestBrowserOptionsSlowMo(t *testing.T) {
 			t.Parallel()
 			tb := newTestBrowser(t, withFileServer())
 			testFrameSlowMoImpl(t, tb, func(_ *testBrowser, f api.Frame) {
-				f.Goto("about:blank", nil)
+				_, _ = f.Goto(common.BlankPage, nil)
 			})
 		})
 		t.Run("hover", func(t *testing.T) {

tests/page_test.go

@@ -11,6 +11,8 @@ import (
 	"github.com/dop251/goja"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/xk6-browser/common"
 )
 
 type emulateMediaOpts struct {
@@ -664,7 +666,7 @@ func TestPageURL(t *testing.T) {
 	b := newTestBrowser(t, withHTTPServer())
 
 	p := b.NewPage(nil)
-	assert.Equal(t, "about:blank", p.URL())
+	assert.Equal(t, common.BlankPage, p.URL())
 
 	resp, err := p.Goto(b.url("/get"), nil)
 	require.NoError(t, err)