Fix isVisible racy access to options

grafana · Feb 10, 2025 · e92ec0e · e92ec0e
1 parent 4a04c08
commit e92ec0e
Show file tree

Hide file tree

Showing 5 changed files with 18 additions and 16 deletions.
diff --git a/internal/js/modules/k6/browser/browser/frame_mapping.go b/internal/js/modules/k6/browser/browser/frame_mapping.go
@@ -226,10 +226,14 @@ func mapFrame(vu moduleVU, f *common.Frame) mapping {
 				return f.IsHidden(selector, popts) //nolint:wrapcheck
 			}), nil
 		},
-		"isVisible": func(selector string, opts sobek.Value) *sobek.Promise {
+		"isVisible": func(selector string, opts sobek.Value) (*sobek.Promise, error) {
+			popts := common.NewFrameIsVisibleOptions()
+			if err := popts.Parse(vu.Context(), opts); err != nil {
+				return nil, fmt.Errorf("parse isVisible options of selector %q: %w", selector, err)
+			}
 			return k6ext.Promise(vu.Context(), func() (any, error) {
-				return f.IsVisible(selector, opts) //nolint:wrapcheck
-			})
+				return f.IsVisible(selector, popts) //nolint:wrapcheck
+			}), nil
 		},
 		"locator": func(selector string, opts sobek.Value) mapping {
 			return mapLocator(vu, f.Locator(selector, opts))

diff --git a/internal/js/modules/k6/browser/browser/page_mapping.go b/internal/js/modules/k6/browser/browser/page_mapping.go
@@ -260,11 +260,14 @@ func mapPage(vu moduleVU, p *common.Page) mapping { //nolint:gocognit,cyclop
 				return p.IsHidden(selector, popts) //nolint:wrapcheck
 			}), nil
 		},
-		"isVisible": func(selector string, opts sobek.Value) *sobek.Promise {
+		"isVisible": func(selector string, opts sobek.Value) (*sobek.Promise, error) {
+			popts := common.NewFrameIsVisibleOptions()
+			if err := popts.Parse(vu.Context(), opts); err != nil {
+				return nil, fmt.Errorf("parsing isVisible 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 p.IsVisible(selector, opts) //nolint:wrapcheck
-			})
+				return p.IsVisible(selector, popts) //nolint:wrapcheck
+			}), nil
 		},
 		"keyboard": mapKeyboard(vu, p.GetKeyboard()),
 		"locator": func(selector string, opts sobek.Value) *sobek.Object {

diff --git a/internal/js/modules/k6/browser/common/frame.go b/internal/js/modules/k6/browser/common/frame.go
@@ -1299,14 +1299,10 @@ func (f *Frame) isHidden(selector string, opts *FrameIsHiddenOptions) (bool, err
 
 // IsVisible returns true if the first element that matches the selector
 // is visible. Otherwise, returns false.
-func (f *Frame) IsVisible(selector string, opts sobek.Value) (bool, error) {
+func (f *Frame) IsVisible(selector string, opts *FrameIsVisibleOptions) (bool, error) {
 	f.log.Debugf("Frame:IsVisible", "fid:%s furl:%q sel:%q", f.ID(), f.URL(), selector)
 
-	popts := NewFrameIsVisibleOptions()
-	if err := popts.Parse(f.ctx, opts); err != nil {
-		return false, fmt.Errorf("parsing is visible options: %w", err)
-	}
-	visible, err := f.isVisible(selector, popts)
+	visible, err := f.isVisible(selector, opts)
 	if err != nil {
 		return false, err
 	}

diff --git a/internal/js/modules/k6/browser/common/page.go b/internal/js/modules/k6/browser/common/page.go
@@ -1120,7 +1120,7 @@ func (p *Page) IsHidden(selector string, opts *FrameIsHiddenOptions) (bool, erro
 
 // IsVisible will look for an element in the dom with given selector. It will
 // not wait for a match to occur. If no elements match `false` will be returned.
-func (p *Page) IsVisible(selector string, opts sobek.Value) (bool, error) {
+func (p *Page) IsVisible(selector string, opts *FrameIsVisibleOptions) (bool, error) {
 	p.logger.Debugf("Page:IsVisible", "sid:%v selector:%s", p.sessionID(), selector)
 
 	return p.MainFrame().IsVisible(selector, opts)

diff --git a/internal/js/modules/k6/browser/tests/page_test.go b/internal/js/modules/k6/browser/tests/page_test.go
@@ -1678,8 +1678,7 @@ func TestPageIsVisible(t *testing.T) {
 			)
 			require.NoError(t, err)
 
-			got, err := page.IsVisible(tc.selector, tb.toSobekValue(tc.options))
-
+			got, err := page.IsVisible(tc.selector, &tc.options)
 			if tc.wantErr != "" {
 				assert.ErrorContains(t, err, tc.wantErr)
 				return