From 86e90c4f9059cf95a7f30f5cc46e9f29b026718e Mon Sep 17 00:00:00 2001 From: ankur22 Date: Fri, 24 Jan 2025 16:25:49 +0000 Subject: [PATCH 1/2] Fix an NPD when handle is overwritten with nil Call the defer function with the non nil handle, and allow the handle to be overwritten later. Now the handle in the defer will still be non-nil. --- internal/js/modules/k6/browser/common/frame.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/js/modules/k6/browser/common/frame.go b/internal/js/modules/k6/browser/common/frame.go index 3742c27ea29..f09dfee7198 100644 --- a/internal/js/modules/k6/browser/common/frame.go +++ b/internal/js/modules/k6/browser/common/frame.go @@ -500,12 +500,12 @@ func (f *Frame) waitForSelector(selector string, opts *FrameWaitForSelectorOptio // an element should belong to the current execution context. // otherwise, we should adopt it to this execution context. if ec != handle.execCtx { - defer func() { + defer func(handle *ElementHandle) { if err := handle.Dispose(); err != nil { err = fmt.Errorf("disposing element handle: %w", err) rerr = errors.Join(err, rerr) } - }() + }(handle) if handle, err = ec.adoptElementHandle(handle); err != nil { return nil, fmt.Errorf("waiting for selector %q: adopting element handle: %w", selector, err) } From ff4fcad3b9c06149e239a5c29132379d40de45c0 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Mon, 27 Jan 2025 10:04:04 +0000 Subject: [PATCH 2/2] Fix dispose of original handle We don't want to dispose the new handle, only the original one, and only when the handle is successfully adopted. We also don't want to end the iteration if we fail to dispose of the original handle since we're not going to be working with it. Chances are that chrome will have dealt with it already. --- .../js/modules/k6/browser/common/frame.go | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/internal/js/modules/k6/browser/common/frame.go b/internal/js/modules/k6/browser/common/frame.go index f09dfee7198..9dc34f88b25 100644 --- a/internal/js/modules/k6/browser/common/frame.go +++ b/internal/js/modules/k6/browser/common/frame.go @@ -478,7 +478,7 @@ func (f *Frame) waitForSelectorRetry( // It will auto retry on certain errors until the retryCount is below 0. The // retry workaround is needed since the underlying DOM can change when the // wait action is performed during a navigation. -func (f *Frame) waitForSelector(selector string, opts *FrameWaitForSelectorOptions) (_ *ElementHandle, rerr error) { +func (f *Frame) waitForSelector(selector string, opts *FrameWaitForSelectorOptions) (*ElementHandle, error) { f.log.Debugf("Frame:waitForSelector", "fid:%s furl:%q sel:%q", f.ID(), f.URL(), selector) handle, err := f.waitFor(selector, opts, 20) @@ -497,21 +497,25 @@ func (f *Frame) waitForSelector(selector string, opts *FrameWaitForSelectorOptio if ec == nil { return nil, fmt.Errorf("waiting for selector %q: execution context %q not found", selector, mainWorld) } + // an element should belong to the current execution context. // otherwise, we should adopt it to this execution context. + adopted := handle if ec != handle.execCtx { - defer func(handle *ElementHandle) { - if err := handle.Dispose(); err != nil { - err = fmt.Errorf("disposing element handle: %w", err) - rerr = errors.Join(err, rerr) - } - }(handle) - if handle, err = ec.adoptElementHandle(handle); err != nil { + if adopted, err = ec.adoptElementHandle(handle); err != nil { return nil, fmt.Errorf("waiting for selector %q: adopting element handle: %w", selector, err) } + + if err = handle.Dispose(); err != nil { + f.log.Warnf( + "Frame:waitForSelector", + "fid:%s furl:%q sel:%q disposing element handle: %v", + f.ID(), f.URL(), selector, err, + ) + } } - return handle, nil + return adopted, nil } func (f *Frame) waitFor(