From e1e13bf2924ea693f24cec26605ab4423a14bf04 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Fri, 19 Jan 2024 15:30:14 +0000 Subject: [PATCH] Refactor elementHandle.click parsing Move the parsing of the options for elementHandle.click outside of the promise goroutine and back onto the main goja thread in the mapping layer. This will help mitigate the risk of a panic if more than one goroutine is accessing the goja runtime off the main goja thread. This doesn't solve the problem completely though since this API calls to other areas of the codebase which does still interact with the goja runtime. See https://github.com/grafana/xk6-browser/issues/1170 for further details. --- browser/mapping.go | 13 ++++++++++--- common/element_handle.go | 12 ++++-------- tests/element_handle_test.go | 36 +++++++++++++++--------------------- tests/page_test.go | 2 +- 4 files changed, 30 insertions(+), 33 deletions(-) diff --git a/browser/mapping.go b/browser/mapping.go index 480cf6726..cb0b57092 100644 --- a/browser/mapping.go +++ b/browser/mapping.go @@ -219,11 +219,18 @@ func mapElementHandle(vu moduleVU, eh *common.ElementHandle) mapping { maps := mapping{ "boundingBox": eh.BoundingBox, "check": eh.Check, - "click": func(opts goja.Value) *goja.Promise { + "click": func(opts goja.Value) (*goja.Promise, error) { + ctx := vu.Context() + + popts := common.NewElementHandleClickOptions(eh.Timeout()) + if err := popts.Parse(ctx, opts); err != nil { + return nil, fmt.Errorf("parsing element click options: %w", err) + } + return k6ext.Promise(vu.Context(), func() (any, error) { - err := eh.Click(opts) + err := eh.Click(popts) return nil, err //nolint:wrapcheck - }) + }), nil }, "contentFrame": func() (mapping, error) { f, err := eh.ContentFrame() diff --git a/common/element_handle.go b/common/element_handle.go index 04b876b0a..ebb4faff1 100644 --- a/common/element_handle.go +++ b/common/element_handle.go @@ -733,18 +733,14 @@ func (h *ElementHandle) BoundingBox() *Rect { // Click scrolls element into view and clicks in the center of the element // TODO: look into making more robust using retries // (see: https://github.com/microsoft/playwright/blob/master/src/server/dom.ts#L298) -func (h *ElementHandle) Click(opts goja.Value) error { - actionOpts := NewElementHandleClickOptions(h.defaultTimeout()) - if err := actionOpts.Parse(h.ctx, opts); err != nil { - k6ext.Panic(h.ctx, "parsing element click options: %v", err) - } +func (h *ElementHandle) Click(opts *ElementHandleClickOptions) error { click := h.newPointerAction( func(apiCtx context.Context, handle *ElementHandle, p *Position) (any, error) { - return nil, handle.click(p, actionOpts.ToMouseClickOptions()) + return nil, handle.click(p, opts.ToMouseClickOptions()) }, - &actionOpts.ElementHandleBasePointerOptions, + &opts.ElementHandleBasePointerOptions, ) - if _, err := call(h.ctx, click, actionOpts.Timeout); err != nil { + if _, err := call(h.ctx, click, opts.Timeout); err != nil { return fmt.Errorf("clicking on element: %w", err) } applySlowMo(h.ctx) diff --git a/tests/element_handle_test.go b/tests/element_handle_test.go index 5a8e790b6..4016d72ef 100644 --- a/tests/element_handle_test.go +++ b/tests/element_handle_test.go @@ -103,13 +103,11 @@ func TestElementHandleClick(t *testing.T) { button, err := p.Query("button") require.NoError(t, err) - err = button.Click(tb.toGojaValue(struct { - NoWaitAfter bool `js:"noWaitAfter"` - }{ - // FIX: this is just a workaround because navigation is never triggered - // and we'd be waiting for it to happen otherwise! - NoWaitAfter: true, - })) + opts := common.NewElementHandleClickOptions(button.Timeout()) + // FIX: this is just a workaround because navigation is never triggered + // and we'd be waiting for it to happen otherwise! + opts.NoWaitAfter = true + err = button.Click(opts) require.NoError(t, err) res := tb.asGojaValue(p.Evaluate(tb.toGojaValue("() => window['result']"))) @@ -130,13 +128,11 @@ func TestElementHandleClickWithNodeRemoved(t *testing.T) { button, err := p.Query("button") require.NoError(t, err) - err = button.Click(tb.toGojaValue(struct { - NoWaitAfter bool `js:"noWaitAfter"` - }{ - // FIX: this is just a workaround because navigation is never triggered - // and we'd be waiting for it to happen otherwise! - NoWaitAfter: true, - })) + opts := common.NewElementHandleClickOptions(button.Timeout()) + // FIX: this is just a workaround because navigation is never triggered + // and we'd be waiting for it to happen otherwise! + opts.NoWaitAfter = true + err = button.Click(opts) require.NoError(t, err) res := tb.asGojaValue(p.Evaluate(tb.toGojaValue("() => window['result']"))) @@ -156,13 +152,11 @@ func TestElementHandleClickWithDetachedNode(t *testing.T) { // Detach node to panic when clicked p.Evaluate(tb.toGojaValue("button => button.remove()"), tb.toGojaValue(button)) - err = button.Click(tb.toGojaValue(struct { - NoWaitAfter bool `js:"noWaitAfter"` - }{ - // FIX: this is just a workaround because navigation is never triggered and we'd be waiting for - // it to happen otherwise! - NoWaitAfter: true, - })) + opts := common.NewElementHandleClickOptions(button.Timeout()) + // FIX: this is just a workaround because navigation is never triggered + // and we'd be waiting for it to happen otherwise! + opts.NoWaitAfter = true + err = button.Click(opts) assert.ErrorContains( t, err, "element is not attached to the DOM", diff --git a/tests/page_test.go b/tests/page_test.go index df3976ea9..82d7ee101 100644 --- a/tests/page_test.go +++ b/tests/page_test.go @@ -64,7 +64,7 @@ func TestNestedFrames(t *testing.T) { assert.Nil(t, err) assert.NotNil(t, button1Handle) - err = button1Handle.Click(nil) + err = button1Handle.Click(common.NewElementHandleClickOptions(button1Handle.Timeout())) assert.Nil(t, err) v := frame2.Evaluate(tb.toGojaValue(`() => window.buttonClicked`))