diff --git a/browser/js_handle_mapping.go b/browser/js_handle_mapping.go index a5a11389d..691cabdb4 100644 --- a/browser/js_handle_mapping.go +++ b/browser/js_handle_mapping.go @@ -15,7 +15,7 @@ func mapJSHandle(vu moduleVU, jsh common.JSHandleAPI) mapping { return rt.ToValue(m).ToObject(rt) }, "dispose": jsh.Dispose, - "evaluate": func(pageFunc goja.Value, gargs ...goja.Value) any { + "evaluate": func(pageFunc goja.Value, gargs ...goja.Value) (any, error) { args := make([]any, 0, len(gargs)) for _, a := range gargs { args = append(args, exportArg(a)) diff --git a/common/element_handle.go b/common/element_handle.go index c0ff9e78e..a4152e8e7 100644 --- a/common/element_handle.go +++ b/common/element_handle.go @@ -64,7 +64,10 @@ func (h *ElementHandle) boundingBox() (*Rect, error) { } func (h *ElementHandle) checkHitTargetAt(apiCtx context.Context, point Position) (bool, error) { - frame := h.ownerFrame(apiCtx) + frame, err := h.ownerFrame(apiCtx) + if err != nil { + return false, fmt.Errorf("checking hit target at %v: %w", point, err) + } if frame != nil && frame.parentFrame != nil { el, err := h.frame.FrameElement() if err != nil { @@ -424,22 +427,26 @@ func (h *ElementHandle) offsetPosition(apiCtx context.Context, offset *Position) }, nil } -func (h *ElementHandle) ownerFrame(apiCtx context.Context) *Frame { - frameId := h.frame.page.getOwnerFrame(apiCtx, h) - if frameId == "" { - return nil +func (h *ElementHandle) ownerFrame(apiCtx context.Context) (*Frame, error) { + frameID, err := h.frame.page.getOwnerFrame(apiCtx, h) + if err != nil { + return nil, err + } + if frameID == "" { + return nil, nil //nolint:nilnil } - frame, ok := h.frame.page.frameManager.getFrameByID(frameId) + frame, ok := h.frame.page.frameManager.getFrameByID(frameID) if ok { - return frame + return frame, nil } for _, page := range h.frame.page.browserCtx.browser.pages { - frame, ok = page.frameManager.getFrameByID(frameId) + frame, ok = page.frameManager.getFrameByID(frameID) if ok { - return frame + return frame, nil } } - return nil + + return nil, nil //nolint:nilnil } func (h *ElementHandle) scrollRectIntoViewIfNeeded(apiCtx context.Context, rect *dom.Rect) error { @@ -1011,7 +1018,7 @@ func (h *ElementHandle) IsVisible() (bool, error) { } // OwnerFrame returns the frame containing this element. -func (h *ElementHandle) OwnerFrame() (*Frame, error) { +func (h *ElementHandle) OwnerFrame() (_ *Frame, rerr error) { fn := ` (node, injected) => { return injected.getDocumentElement(node); @@ -1033,7 +1040,13 @@ func (h *ElementHandle) OwnerFrame() (*Frame, error) { if !ok { return nil, fmt.Errorf("unexpected result type while getting document element: %T", res) } - defer documentHandle.Dispose() + defer func() { + if err := documentHandle.Dispose(); err != nil { + err = fmt.Errorf("disposing document element: %w", err) + rerr = errors.Join(err, rerr) + } + }() + if documentHandle.remoteObject.ObjectID == "" { return nil, err } @@ -1079,7 +1092,7 @@ func (h *ElementHandle) Press(key string, opts goja.Value) error { // Query runs "element.querySelector" within the page. If no element matches the selector, // the return value resolves to "null". -func (h *ElementHandle) Query(selector string, strict bool) (*ElementHandle, error) { +func (h *ElementHandle) Query(selector string, strict bool) (_ *ElementHandle, rerr error) { parsedSelector, err := NewSelector(selector) if err != nil { return nil, fmt.Errorf("parsing selector %q: %w", selector, err) @@ -1106,7 +1119,12 @@ func (h *ElementHandle) Query(selector string, strict bool) (*ElementHandle, err } element := handle.AsElement() if element == nil { - handle.Dispose() + defer func() { + if err := handle.Dispose(); err != nil { + err = fmt.Errorf("disposing element handle: %w", err) + rerr = errors.Join(err, rerr) + } + }() return nil, fmt.Errorf("querying selector %q", selector) } @@ -1124,7 +1142,7 @@ func (h *ElementHandle) QueryAll(selector string) ([]*ElementHandle, error) { return handles, nil } -func (h *ElementHandle) queryAll(selector string, eval evalFunc) ([]*ElementHandle, error) { +func (h *ElementHandle) queryAll(selector string, eval evalFunc) (_ []*ElementHandle, rerr error) { parsedSelector, err := NewSelector(selector) if err != nil { return nil, fmt.Errorf("parsing selector %q: %w", selector, err) @@ -1147,7 +1165,12 @@ func (h *ElementHandle) queryAll(selector string, eval evalFunc) ([]*ElementHand if !ok { return nil, fmt.Errorf("getting element handle for selector %q: %w", selector, ErrJSHandleInvalid) } - defer handles.Dispose() + defer func() { + if err := handles.Dispose(); err != nil { + err = fmt.Errorf("disposing element handles: %w", err) + rerr = errors.Join(err, rerr) + } + }() props, err := handles.GetProperties() if err != nil { @@ -1159,8 +1182,8 @@ func (h *ElementHandle) queryAll(selector string, eval evalFunc) ([]*ElementHand for _, prop := range props { if el := prop.AsElement(); el != nil { els = append(els, el) - } else { - prop.Dispose() + } else if err := prop.Dispose(); err != nil { + return nil, fmt.Errorf("disposing property while querying all selectors %q: %w", selector, err) } } diff --git a/common/element_handle_test.go b/common/element_handle_test.go index 79bb8a947..a9a2a8c6d 100644 --- a/common/element_handle_test.go +++ b/common/element_handle_test.go @@ -197,8 +197,9 @@ func (s *jsHandleStub) AsElement() *ElementHandle { return s.asElementFn() } -func (s *jsHandleStub) Dispose() { +func (s *jsHandleStub) Dispose() error { s.disposeCalls++ + return nil } func (s *jsHandleStub) GetProperties() (map[string]JSHandleAPI, error) { diff --git a/common/frame.go b/common/frame.go index 79f451861..04fdbedba 100644 --- a/common/frame.go +++ b/common/frame.go @@ -228,7 +228,7 @@ func (f *Frame) clearLifecycle() { f.inflightRequestsMu.Unlock() } -func (f *Frame) detach() { +func (f *Frame) detach() error { f.log.Debugf("Frame:detach", "fid:%s furl:%q", f.ID(), f.URL()) f.setDetached(true) @@ -236,12 +236,18 @@ func (f *Frame) detach() { f.parentFrame.removeChildFrame(f) } f.parentFrame = nil + // detach() is called by the same frame Goroutine that manages execution // context switches. so this should be safe. // we don't need to protect the following with executionContextMu. - if f.documentHandle != nil { - f.documentHandle.Dispose() + if f.documentHandle == nil { + return nil + } + if err := f.documentHandle.Dispose(); err != nil { + return fmt.Errorf("disposing document handle while detaching frame: %w", err) } + + return nil } func (f *Frame) defaultTimeout() time.Duration { @@ -516,7 +522,7 @@ func (f *Frame) waitForSelectorRetry( return nil, err } -func (f *Frame) waitForSelector(selector string, opts *FrameWaitForSelectorOptions) (*ElementHandle, error) { +func (f *Frame) waitForSelector(selector string, opts *FrameWaitForSelectorOptions) (_ *ElementHandle, rerr error) { f.log.Debugf("Frame:waitForSelector", "fid:%s furl:%q sel:%q", f.ID(), f.URL(), selector) document, err := f.document() @@ -543,9 +549,14 @@ 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 handle.Dispose() + defer func() { + if err := handle.Dispose(); err != nil { + err = fmt.Errorf("disposing element handle: %w", err) + rerr = errors.Join(err, rerr) + } + }() if handle, err = ec.adoptElementHandle(handle); err != nil { - return nil, fmt.Errorf("adopting element handle while waiting for selector %q: %w", selector, err) + return nil, fmt.Errorf("waiting for selector %q: adopting element handle: %w", selector, err) } } diff --git a/common/frame_manager.go b/common/frame_manager.go index 6638aa106..ae41ef696 100644 --- a/common/frame_manager.go +++ b/common/frame_manager.go @@ -162,7 +162,7 @@ func (m *FrameManager) frameAttached(frameID cdp.FrameID, parentFrameID cdp.Fram } } -func (m *FrameManager) frameDetached(frameID cdp.FrameID, reason cdppage.FrameDetachedReason) { +func (m *FrameManager) frameDetached(frameID cdp.FrameID, reason cdppage.FrameDetachedReason) error { m.logger.Debugf("FrameManager:frameDetached", "fmid:%d fid:%v", m.ID(), frameID) frame, ok := m.getFrameByID(frameID) @@ -170,7 +170,7 @@ func (m *FrameManager) frameDetached(frameID cdp.FrameID, reason cdppage.FrameDe m.logger.Debugf("FrameManager:frameDetached:return", "fmid:%d fid:%v cannot find frame", m.ID(), frameID) - return + return nil } // This helps prevent an iframe and its child frames from being removed @@ -187,7 +187,7 @@ func (m *FrameManager) frameDetached(frameID cdp.FrameID, reason cdppage.FrameDe m.logger.Debugf("FrameManager:frameDetached:notSameSession:return", "fmid:%d fid:%v event session and frame session do not match", m.ID(), frameID) - return + return nil } } @@ -196,11 +196,10 @@ func (m *FrameManager) frameDetached(frameID cdp.FrameID, reason cdppage.FrameDe // frame, we want to keep the current frame which is // still referenced by the (incoming) remote frame, but // remove all its child frames. - m.removeChildFramesRecursively(frame) - return + return m.removeChildFramesRecursively(frame) } - m.removeFramesRecursively(frame) + return m.removeFramesRecursively(frame) } func (m *FrameManager) frameLifecycleEvent(frameID cdp.FrameID, event LifecycleEvent) { @@ -257,7 +256,10 @@ func (m *FrameManager) frameNavigated(frameID cdp.FrameID, parentFrameID cdp.Fra if frame != nil { m.framesMu.Unlock() for _, child := range frame.ChildFrames() { - m.removeFramesRecursively(child) + if err := m.removeFramesRecursively(child); err != nil { + m.framesMu.Lock() + return fmt.Errorf("removing child frames recursively: %w", err) + } } m.framesMu.Lock() } @@ -419,22 +421,30 @@ func (m *FrameManager) getFrameByID(id cdp.FrameID) (*Frame, bool) { return frame, ok } -func (m *FrameManager) removeChildFramesRecursively(frame *Frame) { +func (m *FrameManager) removeChildFramesRecursively(frame *Frame) error { for _, child := range frame.ChildFrames() { - m.removeFramesRecursively(child) + if err := m.removeFramesRecursively(child); err != nil { + return fmt.Errorf("removing child frames recursively: %w", err) + } } + + return nil } -func (m *FrameManager) removeFramesRecursively(frame *Frame) { +func (m *FrameManager) removeFramesRecursively(frame *Frame) error { for _, child := range frame.ChildFrames() { m.logger.Debugf("FrameManager:removeFramesRecursively", "fmid:%d cfid:%v pfid:%v cfname:%s cfurl:%s", m.ID(), child.ID(), frame.ID(), child.Name(), child.URL()) - m.removeFramesRecursively(child) + if err := m.removeFramesRecursively(child); err != nil { + return fmt.Errorf("removing frames recursively: %w", err) + } } - frame.detach() + if err := frame.detach(); err != nil { + return fmt.Errorf("removing frames recursively: detaching frame: %w", err) + } m.framesMu.Lock() m.logger.Debugf("FrameManager:removeFramesRecursively:delParentFrame", @@ -451,6 +461,8 @@ func (m *FrameManager) removeFramesRecursively(frame *Frame) { m.page.emit(EventPageFrameDetached, frame) } + + return nil } func (m *FrameManager) requestFailed(req *Request, canceled bool) { diff --git a/common/frame_session.go b/common/frame_session.go index f2eb92841..8fb82115e 100644 --- a/common/frame_session.go +++ b/common/frame_session.go @@ -755,7 +755,9 @@ func (fs *FrameSession) onFrameDetached(frameID cdp.FrameID, reason cdppage.Fram "sid:%v tid:%v fid:%v reason:%s", fs.session.ID(), fs.targetID, frameID, reason) - fs.manager.frameDetached(frameID, reason) + if err := fs.manager.frameDetached(frameID, reason); err != nil { + k6ext.Panic(fs.ctx, "handling frameDetached event: %w", err) + } } func (fs *FrameSession) onFrameNavigated(frame *cdp.Frame, initial bool) { diff --git a/common/js_handle.go b/common/js_handle.go index f52469e24..3f17b79ac 100644 --- a/common/js_handle.go +++ b/common/js_handle.go @@ -6,7 +6,6 @@ import ( "fmt" "strings" - "github.com/grafana/xk6-browser/k6ext" "github.com/grafana/xk6-browser/log" "github.com/chromedp/cdproto/cdp" @@ -21,8 +20,8 @@ import ( // JSHandleAPI interface. type JSHandleAPI interface { AsElement() *ElementHandle - Dispose() - Evaluate(pageFunc string, args ...any) any + Dispose() error + Evaluate(pageFunc string, args ...any) (any, error) EvaluateHandle(pageFunc string, args ...any) (JSHandleAPI, error) GetProperties() (map[string]JSHandleAPI, error) JSONValue() (string, error) @@ -79,30 +78,33 @@ func (h *BaseJSHandle) AsElement() *ElementHandle { } // Dispose releases the remote object. -func (h *BaseJSHandle) Dispose() { - if err := h.dispose(); err != nil { - // We do not want to panic on an error when the error is a closed - // context. The reason the context would be closed is due to the - // iteration ending and therefore the associated browser and its assets - // will be automatically deleted. - if errors.Is(err, context.Canceled) { - h.logger.Debugf("BaseJSHandle:Dispose", "%v", err) - return - } - // The following error indicates that the object we're trying to release - // cannot be found, which would mean that the object has already been - // removed/deleted. This can occur when a navigation occurs, usually when - // a page contains an iframe. - if strings.Contains(err.Error(), "Cannot find context with specified id") { - h.logger.Debugf("BaseJSHandle:Dispose", "%v", err) - return - } +func (h *BaseJSHandle) Dispose() error { + err := h.dispose() + if err == nil { // no error + return nil + } - k6ext.Panic(h.ctx, "dispose: %w", err) + // We do not want to return an error when the error is a closed + // context. The reason the context would be closed is due to the + // iteration ending and therefore the associated browser and its assets + // will be automatically deleted. + if errors.Is(err, context.Canceled) { + h.logger.Debugf("BaseJSHandle:Dispose", "%v", err) + return nil } + // The following error indicates that the object we're trying to release + // cannot be found, which would mean that the object has already been + // removed/deleted. This can occur when a navigation occurs, usually when + // a page contains an iframe. + if strings.Contains(err.Error(), "Cannot find context with specified id") { + h.logger.Debugf("BaseJSHandle:Dispose", "%v", err) + return nil + } + + return fmt.Errorf("disposing element with ID %s: %w", h.remoteObject.ObjectID, err) } -// dispose is like Dispose, but does not panic. +// dispose sends a command to the browser to release the remote object. func (h *BaseJSHandle) dispose() error { if h.disposed { return nil @@ -121,12 +123,13 @@ func (h *BaseJSHandle) dispose() error { } // Evaluate will evaluate provided page function within an execution context. -func (h *BaseJSHandle) Evaluate(pageFunc string, args ...any) any { +func (h *BaseJSHandle) Evaluate(pageFunc string, args ...any) (any, error) { res, err := h.execCtx.Eval(h.ctx, pageFunc, args...) if err != nil { - k6ext.Panic(h.ctx, "%w", err) + return nil, fmt.Errorf("evaluating element: %w", err) } - return res + + return res, nil } // EvaluateHandle will evaluate provided page function within an execution context. diff --git a/common/page.go b/common/page.go index 822ac468a..aebb02342 100644 --- a/common/page.go +++ b/common/page.go @@ -441,7 +441,7 @@ func (p *Page) getFrameElement(f *Frame) (handle *ElementHandle, _ error) { return parent.adoptBackendNodeID(mainWorld, backendNodeId) } -func (p *Page) getOwnerFrame(apiCtx context.Context, h *ElementHandle) cdp.FrameID { +func (p *Page) getOwnerFrame(apiCtx context.Context, h *ElementHandle) (cdp.FrameID, error) { p.logger.Debugf("Page:getOwnerFrame", "sid:%v", p.sessionID()) // document.documentElement has frameId of the owner frame @@ -461,34 +461,37 @@ func (p *Page) getOwnerFrame(apiCtx context.Context, h *ElementHandle) cdp.Frame result, err := h.execCtx.eval(apiCtx, opts, pageFn, h) if err != nil { p.logger.Debugf("Page:getOwnerFrame:return", "sid:%v err:%v", p.sessionID(), err) - return "" + return "", nil } switch result.(type) { case nil: p.logger.Debugf("Page:getOwnerFrame:return", "sid:%v result:nil", p.sessionID()) - return "" + return "", nil } documentElement := result.(*ElementHandle) if documentElement == nil { p.logger.Debugf("Page:getOwnerFrame:return", "sid:%v docel:nil", p.sessionID()) - return "" + return "", nil } if documentElement.remoteObject.ObjectID == "" { p.logger.Debugf("Page:getOwnerFrame:return", "sid:%v robjid:%q", p.sessionID(), "") - return "" + return "", nil } action := dom.DescribeNode().WithObjectID(documentElement.remoteObject.ObjectID) node, err := action.Do(cdp.WithExecutor(p.ctx, p.session)) if err != nil { p.logger.Debugf("Page:getOwnerFrame:DescribeNode:return", "sid:%v err:%v", p.sessionID(), err) - return "" + return "", nil } frameID := node.FrameID - documentElement.Dispose() - return frameID + if err := documentElement.Dispose(); err != nil { + return "", fmt.Errorf("disposing document element while getting owner frame: %w", err) + } + + return frameID, nil } func (p *Page) attachFrameSession(fid cdp.FrameID, fs *FrameSession) { diff --git a/common/screenshotter.go b/common/screenshotter.go index d25b79a3b..4b4002146 100644 --- a/common/screenshotter.go +++ b/common/screenshotter.go @@ -282,18 +282,29 @@ func (s *screenshotter) screenshotElement(h *ElementHandle, opts *ElementHandleS } } - documentRect := bbox - scrollOffset := h.Evaluate(`() => { return {x: window.scrollX, y: window.scrollY};}`) + scrollOffset, err := h.Evaluate(`() => { return {x: window.scrollX, y: window.scrollY};}`) + if err != nil { + return nil, fmt.Errorf("evaluating scroll offset: %w", err) + } var returnVal Position if err := convert(scrollOffset, &returnVal); err != nil { return nil, fmt.Errorf("unpacking scroll offset: %w", err) } + documentRect := bbox documentRect.X += returnVal.X documentRect.Y += returnVal.Y - buf, err := s.screenshot(h.frame.page.session, documentRect.enclosingIntRect(), nil, format, opts.OmitBackground, opts.Quality, opts.Path) + buf, err := s.screenshot( + h.frame.page.session, + documentRect.enclosingIntRect(), + nil, // viewportRect + format, + opts.OmitBackground, + opts.Quality, + opts.Path, + ) if err != nil { return nil, err } @@ -302,6 +313,7 @@ func (s *screenshotter) screenshotElement(h *ElementHandle, opts *ElementHandleS return nil, fmt.Errorf("restoring viewport: %w", err) } } + return buf, nil } diff --git a/tests/element_handle_test.go b/tests/element_handle_test.go index 51d5cd9d8..7fc00e9f6 100644 --- a/tests/element_handle_test.go +++ b/tests/element_handle_test.go @@ -281,7 +281,7 @@ func TestElementHandleInputValue(t *testing.T) { value, err := element.InputValue(nil) require.NoError(t, err) - element.Dispose() + require.NoError(t, element.Dispose()) assert.Equal(t, value, "hello1", `expected input value "hello1", got %q`, value) element, err = p.Query("select") @@ -289,7 +289,7 @@ func TestElementHandleInputValue(t *testing.T) { value, err = element.InputValue(nil) require.NoError(t, err) - element.Dispose() + require.NoError(t, element.Dispose()) assert.Equal(t, value, "hello2", `expected input value "hello2", got %q`, value) element, err = p.Query("textarea") @@ -297,7 +297,7 @@ func TestElementHandleInputValue(t *testing.T) { value, err = element.InputValue(nil) require.NoError(t, err) - element.Dispose() + require.NoError(t, element.Dispose()) assert.Equal(t, value, "hello3", `expected input value "hello3", got %q`, value) } @@ -314,7 +314,7 @@ func TestElementHandleIsChecked(t *testing.T) { checked, err := element.IsChecked() require.NoError(t, err) assert.True(t, checked, "expected checkbox to be checked") - element.Dispose() + require.NoError(t, element.Dispose()) err = p.SetContent(``, nil) require.NoError(t, err) @@ -323,7 +323,7 @@ func TestElementHandleIsChecked(t *testing.T) { checked, err = element.IsChecked() require.NoError(t, err) assert.False(t, checked, "expected checkbox to be unchecked") - element.Dispose() + require.NoError(t, element.Dispose()) } func TestElementHandleQueryAll(t *testing.T) { @@ -463,7 +463,7 @@ func TestElementHandleWaitForSelector(t *testing.T) { require.NoError(t, err) require.NotNil(t, element, "expected element to have been found after wait") - element.Dispose() + require.NoError(t, element.Dispose()) } func TestElementHandlePress(t *testing.T) {