From 32fc1b59574db16d58c85163dc39e78ff71102e4 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Tue, 23 Jan 2024 16:32:21 +0000 Subject: [PATCH 1/2] Refactor parse out of frame.waitForNavigation The parsing of the options for frame.waitForNavigation has been moved outside of the promise. This change will mitigate any issues when working with the goja runtime with multiple goroutines (goja is not thread safe). --- browser/mapping.go | 11 ++++++++--- common/frame.go | 16 +++++----------- common/page.go | 7 ++++++- 3 files changed, 19 insertions(+), 15 deletions(-) diff --git a/browser/mapping.go b/browser/mapping.go index 2f2c9511f..985c605d9 100644 --- a/browser/mapping.go +++ b/browser/mapping.go @@ -423,14 +423,19 @@ func mapFrame(vu moduleVU, f *common.Frame) mapping { }) }, "waitForLoadState": f.WaitForLoadState, - "waitForNavigation": func(opts goja.Value) *goja.Promise { + "waitForNavigation": func(opts goja.Value) (*goja.Promise, error) { + popts := common.NewFrameWaitForNavigationOptions(f.Timeout()) + if err := popts.Parse(vu.Context(), opts); err != nil { + return nil, fmt.Errorf("parsing frame wait for navigation options: %w", err) + } + return k6ext.Promise(vu.Context(), func() (result any, reason error) { - resp, err := f.WaitForNavigation(opts) + resp, err := f.WaitForNavigation(popts) if err != nil { return nil, err //nolint:wrapcheck } return mapResponse(vu, resp), nil - }) + }), nil }, "waitForSelector": func(selector string, opts goja.Value) (mapping, error) { eh, err := f.WaitForSelector(selector, opts) diff --git a/common/frame.go b/common/frame.go index 630fcfaa6..516577957 100644 --- a/common/frame.go +++ b/common/frame.go @@ -1787,19 +1787,13 @@ func (f *Frame) WaitForLoadState(state string, opts goja.Value) { // WaitForNavigation waits for the given navigation lifecycle event to happen. // //nolint:funlen,cyclop -func (f *Frame) WaitForNavigation(opts goja.Value) (*Response, error) { +func (f *Frame) WaitForNavigation(opts *FrameWaitForNavigationOptions) (*Response, error) { f.log.Debugf("Frame:WaitForNavigation", "fid:%s furl:%s", f.ID(), f.URL()) defer f.log.Debugf("Frame:WaitForNavigation:return", "fid:%s furl:%s", f.ID(), f.URL()) - parsedOpts := NewFrameWaitForNavigationOptions( - f.manager.timeoutSettings.timeout()) - if err := parsedOpts.Parse(f.ctx, opts); err != nil { - k6ext.Panic(f.ctx, "parsing wait for navigation options: %w", err) - } - - timeoutCtx, timeoutCancel := context.WithTimeout(f.ctx, parsedOpts.Timeout) + timeoutCtx, timeoutCancel := context.WithTimeout(f.ctx, opts.Timeout) navEvtCh, navEvtCancel := createWaitForEventHandler(timeoutCtx, f, []string{EventFrameNavigation}, func(data any) bool { @@ -1810,7 +1804,7 @@ func (f *Frame) WaitForNavigation(opts goja.Value) (*Response, error) { timeoutCtx, f, []string{EventFrameAddLifecycle}, func(data any) bool { if le, ok := data.(FrameLifecycleEvent); ok { - return le.Event == parsedOpts.WaitUntil + return le.Event == opts.WaitUntil } return false }) @@ -1821,7 +1815,7 @@ func (f *Frame) WaitForNavigation(opts goja.Value) (*Response, error) { if err != nil { e := &k6ext.UserFriendlyError{ Err: err, - Timeout: parsedOpts.Timeout, + Timeout: opts.Timeout, } return fmt.Errorf("waiting for navigation: %w", e) } @@ -1861,7 +1855,7 @@ func (f *Frame) WaitForNavigation(opts goja.Value) (*Response, error) { // A lifecycle event won't be received when navigating within the same // document, so don't wait for it. The event might've also already been // fired once we're here, so also skip waiting in that case. - if !sameDocNav && !f.hasLifecycleEventFired(parsedOpts.WaitUntil) { + if !sameDocNav && !f.hasLifecycleEventFired(opts.WaitUntil) { select { case <-lifecycleEvtCh: case <-timeoutCtx.Done(): diff --git a/common/page.go b/common/page.go index 927b7acb5..2ad05f6fb 100644 --- a/common/page.go +++ b/common/page.go @@ -1312,7 +1312,12 @@ func (p *Page) WaitForNavigation(opts goja.Value) (*Response, error) { _, span := TraceAPICall(p.ctx, p.targetID.String(), "page.waitForNavigation") defer span.End() - return p.frameManager.MainFrame().WaitForNavigation(opts) + popts := NewFrameWaitForNavigationOptions(p.frameManager.timeoutSettings.timeout()) + if err := popts.Parse(p.ctx, opts); err != nil { + return nil, fmt.Errorf("parsing page wait for navigation options: %w", err) + } + + return p.frameManager.MainFrame().WaitForNavigation(popts) } // WaitForRequest is not implemented. From 7ab8dd7a72a1eb6322448b4780bd67c349c44124 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Tue, 23 Jan 2024 16:50:29 +0000 Subject: [PATCH 2/2] Refactor parse out of page.waitForNavigation The parsing of the options for page.waitForNavigation has been moved outside of the promise. This change will mitigate any issues when working with the goja runtime with multiple goroutines (goja is not thread safe). --- browser/mapping.go | 11 ++++++++--- common/page.go | 9 ++------- tests/frame_manager_test.go | 10 ++++------ tests/frame_test.go | 7 ++++++- tests/lifecycle_wait_test.go | 12 ++++++------ tests/page_test.go | 4 +++- tests/webvital_test.go | 7 ++++++- 7 files changed, 35 insertions(+), 25 deletions(-) diff --git a/browser/mapping.go b/browser/mapping.go index 985c605d9..6f69d4f14 100644 --- a/browser/mapping.go +++ b/browser/mapping.go @@ -634,14 +634,19 @@ func mapPage(vu moduleVU, p *common.Page) mapping { }) }, "waitForLoadState": p.WaitForLoadState, - "waitForNavigation": func(opts goja.Value) *goja.Promise { + "waitForNavigation": func(opts goja.Value) (*goja.Promise, error) { + popts := common.NewFrameWaitForNavigationOptions(p.Timeout()) + if err := popts.Parse(vu.Context(), opts); err != nil { + return nil, fmt.Errorf("parsing page wait for navigation options: %w", err) + } + return k6ext.Promise(vu.Context(), func() (result any, reason error) { - resp, err := p.WaitForNavigation(opts) + resp, err := p.WaitForNavigation(popts) if err != nil { return nil, err //nolint:wrapcheck } return mapResponse(vu, resp), nil - }) + }), nil }, "waitForRequest": p.WaitForRequest, "waitForResponse": p.WaitForResponse, diff --git a/common/page.go b/common/page.go index 2ad05f6fb..8898bf8c4 100644 --- a/common/page.go +++ b/common/page.go @@ -1307,17 +1307,12 @@ func (p *Page) WaitForLoadState(state string, opts goja.Value) { } // WaitForNavigation waits for the given navigation lifecycle event to happen. -func (p *Page) WaitForNavigation(opts goja.Value) (*Response, error) { +func (p *Page) WaitForNavigation(opts *FrameWaitForNavigationOptions) (*Response, error) { p.logger.Debugf("Page:WaitForNavigation", "sid:%v", p.sessionID()) _, span := TraceAPICall(p.ctx, p.targetID.String(), "page.waitForNavigation") defer span.End() - popts := NewFrameWaitForNavigationOptions(p.frameManager.timeoutSettings.timeout()) - if err := popts.Parse(p.ctx, opts); err != nil { - return nil, fmt.Errorf("parsing page wait for navigation options: %w", err) - } - - return p.frameManager.MainFrame().WaitForNavigation(popts) + return p.frameManager.MainFrame().WaitForNavigation(opts) } // WaitForRequest is not implemented. diff --git a/tests/frame_manager_test.go b/tests/frame_manager_test.go index 97daf73f4..f2e76092c 100644 --- a/tests/frame_manager_test.go +++ b/tests/frame_manager_test.go @@ -42,9 +42,7 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { require.NotNil(t, resp) waitForNav := func() error { - opts := tb.toGojaValue(&common.FrameWaitForNavigationOptions{ - Timeout: time.Duration(timeout.Milliseconds()), // interpreted as ms - }) + opts := &common.FrameWaitForNavigationOptions{Timeout: timeout} _, err := p.WaitForNavigation(opts) return err } @@ -98,9 +96,9 @@ func TestWaitForFrameNavigation(t *testing.T) { require.NoError(t, err) waitForNav := func() error { - opts := tb.toGojaValue(&common.FrameWaitForNavigationOptions{ - Timeout: 5000, // interpreted as ms - }) + opts := &common.FrameWaitForNavigationOptions{ + Timeout: 5000 * time.Millisecond, + } _, err := p.WaitForNavigation(opts) return err } diff --git a/tests/frame_test.go b/tests/frame_test.go index be59f1609..d2a8e606d 100644 --- a/tests/frame_test.go +++ b/tests/frame_test.go @@ -137,7 +137,12 @@ func TestFrameNoPanicNavigateAndClickOnPageWithIFrames(t *testing.T) { err = tb.run( ctx, func() error { return p.Click(`a[href="/iframeSignIn"]`, common.NewFrameClickOptions(p.Timeout())) }, - func() error { _, err := p.WaitForNavigation(nil); return err }, + func() error { + _, err := p.WaitForNavigation( + common.NewFrameWaitForNavigationOptions(p.Timeout()), + ) + return err + }, ) require.NoError(t, err) diff --git a/tests/lifecycle_wait_test.go b/tests/lifecycle_wait_test.go index 467816eba..263c340c2 100644 --- a/tests/lifecycle_wait_test.go +++ b/tests/lifecycle_wait_test.go @@ -128,10 +128,10 @@ func TestLifecycleWaitForNavigation(t *testing.T) { result := p.TextContent("#pingRequestText", nil) assert.EqualValues(t, "Waiting... pong 10 - for loop complete", result) - opts := tb.toGojaValue(&common.FrameWaitForNavigationOptions{ - Timeout: 1000, + opts := &common.FrameWaitForNavigationOptions{ + Timeout: 1000 * time.Millisecond, WaitUntil: common.LifecycleEventNetworkIdle, - }) + } _, err := p.WaitForNavigation(opts) return err @@ -167,10 +167,10 @@ func TestLifecycleWaitForNavigation(t *testing.T) { tt.pingJSTextAssert(result) waitForNav := func() error { - opts := tb.toGojaValue(&common.FrameWaitForNavigationOptions{ - Timeout: 30000, + opts := &common.FrameWaitForNavigationOptions{ + Timeout: 30000 * time.Millisecond, WaitUntil: tt.waitUntil, - }) + } _, err := p.WaitForNavigation(opts) return err } diff --git a/tests/page_test.go b/tests/page_test.go index f320f7191..e86ad542a 100644 --- a/tests/page_test.go +++ b/tests/page_test.go @@ -737,7 +737,9 @@ func TestPageWaitForNavigationErrOnCtxDone(t *testing.T) { p := b.NewPage(nil) go b.cancelContext() <-b.context().Done() - _, err := p.WaitForNavigation(nil) + _, err := p.WaitForNavigation( + common.NewFrameWaitForNavigationOptions(p.Timeout()), + ) require.ErrorContains(t, err, "canceled") } diff --git a/tests/webvital_test.go b/tests/webvital_test.go index 3228a8e54..f91b9f777 100644 --- a/tests/webvital_test.go +++ b/tests/webvital_test.go @@ -70,7 +70,12 @@ func TestWebVitalMetric(t *testing.T) { err = browser.run( ctx, func() error { return page.Click("#clickMe", common.NewFrameClickOptions(page.Timeout())) }, - func() error { _, err := page.WaitForNavigation(nil); return err }, + func() error { + _, err := page.WaitForNavigation( + common.NewFrameWaitForNavigationOptions(page.Timeout()), + ) + return err + }, ) require.NoError(t, err)