Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

browser: Fix options races p5 #4533

Merged
merged 3 commits into from
Feb 11, 2025
Merged

browser: Fix options races p5 #4533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d0d126
Fix press racy access to options
codebien Feb 10, 2025
1a47a84
Fix reload racy access to options
codebien Feb 10, 2025
67e4019
Fix setContent racy access to options
codebien Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions internal/js/modules/k6/browser/browser/frame_mapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,14 @@ func mapFrame(vu moduleVU, f *common.Frame) mapping {
"parentFrame": func() mapping {
return mapFrame(vu, f.ParentFrame())
},
"press": func(selector, key string, opts sobek.Value) *sobek.Promise {
"press": func(selector, key string, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFramePressOptions(f.Timeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parse press options of selector %q on key %q: %w", selector, key, err)
}
return k6ext.Promise(vu.Context(), func() (any, error) {
return nil, f.Press(selector, key, opts) //nolint:wrapcheck
})
return nil, f.Press(selector, key, popts) //nolint:wrapcheck
}), nil
},
"selectOption": func(selector string, values sobek.Value, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameSelectOptionOptions(f.Timeout())
Expand All @@ -271,10 +275,14 @@ func mapFrame(vu moduleVU, f *common.Frame) mapping {
return nil, f.SetChecked(selector, checked, popts) //nolint:wrapcheck
}), nil
},
"setContent": func(html string, opts sobek.Value) *sobek.Promise {
"setContent": func(html string, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameSetContentOptions(f.Page().NavigationTimeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing setContent options: %w", err)
}
return k6ext.Promise(vu.Context(), func() (any, error) {
return nil, f.SetContent(html, opts) //nolint:wrapcheck
})
return nil, f.SetContent(html, popts) //nolint:wrapcheck
}), nil
},
"setInputFiles": func(selector string, files sobek.Value, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameSetInputFilesOptions(f.Timeout())
Expand Down
37 changes: 22 additions & 15 deletions internal/js/modules/k6/browser/browser/page_mapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,26 +285,30 @@ func mapPage(vu moduleVU, p *common.Page) mapping { //nolint:gocognit,cyclop
return p.Opener(), nil
})
},
"press": func(selector string, key string, opts sobek.Value) *sobek.Promise {
"press": func(selector string, key string, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFramePressOptions(p.Timeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing press options of selector %q: %w", selector, err)
}
return k6ext.Promise(vu.Context(), func() (any, error) {
// TODO(@mstoykov): don't use sobek Values in a separate goroutine
return nil, p.Press(selector, key, opts) //nolint:wrapcheck
})
return nil, p.Press(selector, key, popts) //nolint:wrapcheck
}), nil
},
"reload": func(opts sobek.Value) *sobek.Promise {
"reload": func(opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewPageReloadOptions(common.LifecycleEventLoad, p.NavigationTimeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing reload options: %w", err)
}
return k6ext.Promise(vu.Context(), func() (any, error) {
// TODO(@mstoykov): don't use sobek Values in a separate goroutine
resp, err := p.Reload(opts)
resp, err := p.Reload(popts)
if err != nil {
return nil, err //nolint:wrapcheck
}

if resp == nil {
return nil, nil
return nil, nil //nolint:nilnil
}

return mapResponse(vu, resp), nil
})
}), nil
},
"screenshot": func(opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewPageScreenshotOptions()
Expand Down Expand Up @@ -345,11 +349,14 @@ func mapPage(vu moduleVU, p *common.Page) mapping { //nolint:gocognit,cyclop
return nil, p.SetChecked(selector, checked, popts) //nolint:wrapcheck
}), nil
},
"setContent": func(html string, opts sobek.Value) *sobek.Promise {
"setContent": func(html string, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameSetContentOptions(p.MainFrame().NavigationTimeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing setContent options: %w", err)
}
return k6ext.Promise(vu.Context(), func() (any, error) {
// TODO(@mstoykov): don't use sobek Values in a separate goroutine
return nil, p.SetContent(html, opts) //nolint:wrapcheck
})
return nil, p.SetContent(html, popts) //nolint:wrapcheck
}), nil
},
"setDefaultNavigationTimeout": p.SetDefaultNavigationTimeout,
"setDefaultTimeout": p.SetDefaultTimeout,
Expand Down
17 changes: 3 additions & 14 deletions internal/js/modules/k6/browser/common/frame.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1391,14 +1391,10 @@ func (f *Frame) ParentFrame() *Frame {
}

// Press presses the given key for the first element found that matches the selector.
func (f *Frame) Press(selector, key string, opts sobek.Value) error {
func (f *Frame) Press(selector, key string, opts *FramePressOptions) error {
f.log.Debugf("Frame:Press", "fid:%s furl:%q sel:%q key:%q", f.ID(), f.URL(), selector, key)

popts := NewFramePressOptions(f.defaultTimeout())
if err := popts.Parse(f.ctx, opts); err != nil {
return fmt.Errorf("parsing press options: %w", err)
}
if err := f.press(selector, key, popts); err != nil {
if err := f.press(selector, key, opts); err != nil {
return fmt.Errorf("pressing %q on %q: %w", key, selector, err)
}

Expand Down Expand Up @@ -1478,16 +1474,9 @@ func (f *Frame) selectOption(selector string, values sobek.Value, opts *FrameSel
}

// SetContent replaces the entire HTML document content.
func (f *Frame) SetContent(html string, opts sobek.Value) error {
func (f *Frame) SetContent(html string, _ *FrameSetContentOptions) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like here (and maybe upstream) we can clean up the opts completely? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, for this reason, I would prefer if we defer this to the next release. In my opinion, it's better to coordinate with @inancgumus on similar refactors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@inancgumus do you have any plan in mind for it? Can we remove it or do we expect to add features where it is needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to keep it and add a TODO comment instead because here, we (the early-browser team) forgot to respect the options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@inancgumus Unfortunately, I don't have the knowledge to add a meaningful TODO. Can you add one, please? Happy to review the pull request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4538

f.log.Debugf("Frame:SetContent", "fid:%s furl:%q", f.ID(), f.URL())

parsedOpts := NewFrameSetContentOptions(
f.manager.timeoutSettings.navigationTimeout(),
)
if err := parsedOpts.Parse(f.ctx, opts); err != nil {
return fmt.Errorf("parsing set content options: %w", err)
}

js := `(html) => {
window.stop();
document.open();
Expand Down
22 changes: 6 additions & 16 deletions internal/js/modules/k6/browser/common/page.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,7 +1192,7 @@ func (p *Page) Opener() *Page {
}

// Press presses the given key for the first element found that matches the selector.
func (p *Page) Press(selector string, key string, opts sobek.Value) error {
func (p *Page) Press(selector string, key string, opts *FramePressOptions) error {
p.logger.Debugf("Page:Press", "sid:%v selector:%s", p.sessionID(), selector)

return p.MainFrame().Press(selector, key, opts)
Expand All @@ -1213,22 +1213,12 @@ func (p *Page) QueryAll(selector string) ([]*ElementHandle, error) {
}

// Reload will reload the current page.
func (p *Page) Reload(opts sobek.Value) (*Response, error) { //nolint:funlen
func (p *Page) Reload(opts *PageReloadOptions) (*Response, error) { //nolint:funlen
p.logger.Debugf("Page:Reload", "sid:%v", p.sessionID())
_, span := TraceAPICall(p.ctx, p.targetID.String(), "page.reload")
defer span.End()

reloadOpts := NewPageReloadOptions(
LifecycleEventLoad,
p.timeoutSettings.navigationTimeout(),
)
if err := reloadOpts.Parse(p.ctx, opts); err != nil {
err := fmt.Errorf("parsing reload options: %w", err)
spanRecordError(span, err)
return nil, err
}

timeoutCtx, timeoutCancelFn := context.WithTimeout(p.ctx, reloadOpts.Timeout)
timeoutCtx, timeoutCancelFn := context.WithTimeout(p.ctx, opts.Timeout)
defer timeoutCancelFn()

waitForFrameNavigation, cancelWaitingForFrameNavigation := createWaitForEventHandler(
Expand All @@ -1245,7 +1235,7 @@ func (p *Page) Reload(opts sobek.Value) (*Response, error) { //nolint:funlen
[]string{EventFrameAddLifecycle},
func(data any) bool {
if le, ok := data.(FrameLifecycleEvent); ok {
return le.Event == reloadOpts.WaitUntil
return le.Event == opts.WaitUntil
}
return false
})
Expand All @@ -1262,7 +1252,7 @@ func (p *Page) Reload(opts sobek.Value) (*Response, error) { //nolint:funlen
if errors.Is(err, context.DeadlineExceeded) {
err = &k6ext.UserFriendlyError{
Err: err,
Timeout: reloadOpts.Timeout,
Timeout: opts.Timeout,
}
return fmt.Errorf("reloading page: %w", err)
}
Expand Down Expand Up @@ -1342,7 +1332,7 @@ func (p *Page) SelectOption(selector string, values sobek.Value, popts *FrameSel
}

// SetContent replaces the entire HTML document content.
func (p *Page) SetContent(html string, opts sobek.Value) error {
func (p *Page) SetContent(html string, opts *FrameSetContentOptions) error {
p.logger.Debugf("Page:SetContent", "sid:%v", p.sessionID())

return p.MainFrame().SetContent(html, opts)
Expand Down
7 changes: 4 additions & 3 deletions internal/js/modules/k6/browser/tests/frame_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ func TestFramePress(t *testing.T) {

f := p.Frames()[0]

require.NoError(t, f.Press("#text1", "Shift+KeyA", nil))
require.NoError(t, f.Press("#text1", "KeyB", nil))
require.NoError(t, f.Press("#text1", "Shift+KeyC", nil))
opts := common.NewFramePressOptions(f.Timeout())
require.NoError(t, f.Press("#text1", "Shift+KeyA", opts))
require.NoError(t, f.Press("#text1", "KeyB", opts))
require.NoError(t, f.Press("#text1", "Shift+KeyC", opts))

inputValue, err := f.InputValue("#text1", common.NewFrameInputValueOptions(p.MainFrame().Timeout()))
require.NoError(t, err)
Expand Down
7 changes: 4 additions & 3 deletions internal/js/modules/k6/browser/tests/launch_options_slowmo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,16 @@ func TestBrowserOptionsSlowMo(t *testing.T) {
t.Parallel()
tb := newTestBrowser(t, withFileServer())
testPageSlowMoImpl(t, tb, func(_ *testBrowser, p *common.Page) {
err := p.Press("button", "Enter", nil)
err := p.Press("button", "Enter", common.NewFramePressOptions(p.MainFrame().Timeout()))
require.NoError(t, err)
})
})
t.Run("reload", func(t *testing.T) {
t.Parallel()
tb := newTestBrowser(t, withFileServer())
testPageSlowMoImpl(t, tb, func(_ *testBrowser, p *common.Page) {
_, err := p.Reload(nil)
opts := common.NewPageReloadOptions(common.LifecycleEventLoad, p.MainFrame().NavigationTimeout())
_, err := p.Reload(opts)
require.NoError(t, err)
})
})
Expand Down Expand Up @@ -262,7 +263,7 @@ func TestBrowserOptionsSlowMo(t *testing.T) {
t.Parallel()
tb := newTestBrowser(t, withFileServer())
testFrameSlowMoImpl(t, tb, func(_ *testBrowser, f *common.Frame) {
err := f.Press("button", "Enter", nil)
err := f.Press("button", "Enter", common.NewFramePressOptions(f.Timeout()))
require.NoError(t, err)
})
})
Expand Down
4 changes: 2 additions & 2 deletions internal/js/modules/k6/browser/tests/lifecycle_wait_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,10 +415,10 @@ func TestLifecycleReload(t *testing.T) {
}
tt.pingJSTextAssert(result)

opts := tb.toSobekValue(common.PageReloadOptions{
opts := &common.PageReloadOptions{
WaitUntil: tt.waitUntil,
Timeout: 30 * time.Second,
})
}
_, err = p.Reload(opts)
require.NoError(t, err)

Expand Down
7 changes: 4 additions & 3 deletions internal/js/modules/k6/browser/tests/page_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,9 +961,10 @@ func TestPagePress(t *testing.T) {
err := p.SetContent(`<input id="text1">`, nil)
require.NoError(t, err)

require.NoError(t, p.Press("#text1", "Shift+KeyA", nil))
require.NoError(t, p.Press("#text1", "KeyB", nil))
require.NoError(t, p.Press("#text1", "Shift+KeyC", nil))
opts := common.NewFramePressOptions(p.MainFrame().Timeout())
require.NoError(t, p.Press("#text1", "Shift+KeyA", opts))
require.NoError(t, p.Press("#text1", "KeyB", opts))
require.NoError(t, p.Press("#text1", "Shift+KeyC", opts))

inputValue, err := p.InputValue("#text1", common.NewFrameInputValueOptions(p.MainFrame().Timeout()))
require.NoError(t, err)
Expand Down
Loading