From b150ad5177b0afe6f1bb55aa36dfccb17fbe3a56 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Tue, 20 Aug 2024 13:53:52 +0100 Subject: [PATCH 1/4] Refactor windowID to pointer When working with iframes, there are some cases where the frame that contains the iframe doesn't have a windowID. Trying to retrieve one will result in an error. Changing windowID to a pointer will enable us to either have one or not. --- common/frame_session.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/common/frame_session.go b/common/frame_session.go index 8fb82115e..ef2e0909f 100644 --- a/common/frame_session.go +++ b/common/frame_session.go @@ -57,7 +57,8 @@ type FrameSession struct { k6Metrics *k6ext.CustomMetrics targetID target.ID - windowID browser.WindowID + // windowID can be nil when it is associated to an iframe. + windowID *browser.WindowID // To understand the concepts of Isolated Worlds, Contexts and Frames and // the relationship betwween them have a look at the following doc: @@ -121,7 +122,8 @@ func NewFrameSession( } action := browser.GetWindowForTarget().WithTargetID(fs.targetID) - if fs.windowID, _, err = action.Do(cdp.WithExecutor(fs.ctx, fs.session)); err != nil { + var windowID browser.WindowID + if windowID, _, err = action.Do(cdp.WithExecutor(fs.ctx, fs.session)); err != nil { l.Debugf( "NewFrameSession:GetWindowForTarget", "sid:%v tid:%v err:%v", @@ -129,6 +131,7 @@ func NewFrameSession( return nil, fmt.Errorf("getting browser window ID: %w", err) } + fs.windowID = &windowID fs.initEvents() if err = fs.initFrameTree(); err != nil { @@ -1193,7 +1196,7 @@ func (fs *FrameSession) updateViewport() error { fs.page.browserCtx.browser.browserOpts.Headless, runtime.GOOS, ) - action2 := browser.SetWindowBounds(fs.windowID, &browser.Bounds{ + action2 := browser.SetWindowBounds(*fs.windowID, &browser.Bounds{ Width: viewport.Width, Height: viewport.Height, }) From 72d28193d760ab0ab52a8e932697ed9ba2dace61 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Tue, 20 Aug 2024 13:56:55 +0100 Subject: [PATCH 2/4] Update NewFrameSession to take hasUIWindow flag When we create a new FrameSession for a frame associated with a frame, we don't want to retrieve the windowID as it could cause chromium to error and therefore the browser module to panic. Instead we will avoid calling out to retrieve the windowID in those cases. --- common/frame_session.go | 5 +++-- common/page.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/common/frame_session.go b/common/frame_session.go index ef2e0909f..99c75acdf 100644 --- a/common/frame_session.go +++ b/common/frame_session.go @@ -83,7 +83,7 @@ type FrameSession struct { // //nolint:funlen func NewFrameSession( - ctx context.Context, s session, p *Page, parent *FrameSession, tid target.ID, l *log.Logger, + ctx context.Context, s session, p *Page, parent *FrameSession, tid target.ID, l *log.Logger, hasUIWindow bool, ) (_ *FrameSession, err error) { l.Debugf("NewFrameSession", "sid:%v tid:%v", s.ID(), tid) @@ -997,7 +997,8 @@ func (fs *FrameSession) attachIFrameToTarget(ti *target.Info, sid target.Session fs.ctx, fs.page.browserCtx.getSession(sid), fs.page, fs, ti.TargetID, - fs.logger) + fs.logger, + false) if err != nil { return fmt.Errorf("attaching iframe target ID %v to session ID %v: %w", ti.TargetID, sid, err) diff --git a/common/page.go b/common/page.go index 4abdf2a9b..1c2b96394 100644 --- a/common/page.go +++ b/common/page.go @@ -296,7 +296,7 @@ func NewPage( var err error p.frameManager = NewFrameManager(ctx, s, &p, p.timeoutSettings, p.logger) - p.mainFrameSession, err = NewFrameSession(ctx, s, &p, nil, tid, p.logger) + p.mainFrameSession, err = NewFrameSession(ctx, s, &p, nil, tid, p.logger, true) if err != nil { p.logger.Debugf("Page:NewPage:NewFrameSession:return", "sid:%v tid:%v err:%v", p.sessionID(), tid, err) From 0d6514a541ea28ab2d1a726131cd847f70a628b9 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Tue, 20 Aug 2024 13:58:51 +0100 Subject: [PATCH 3/4] Use hasUIWindow to avoid getting windowID Now we can avoid the call to retrieve windowID and work with it if the windowID is nil. This change will avoid panics when working with certain iframes that do not contain UI elements, and therefore chromium doesn't track a windowID for it. --- common/frame_session.go | 52 ++++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/common/frame_session.go b/common/frame_session.go index 99c75acdf..5c2137d02 100644 --- a/common/frame_session.go +++ b/common/frame_session.go @@ -121,17 +121,23 @@ func NewFrameSession( return nil, err } - action := browser.GetWindowForTarget().WithTargetID(fs.targetID) - var windowID browser.WindowID - if windowID, _, err = action.Do(cdp.WithExecutor(fs.ctx, fs.session)); err != nil { - l.Debugf( - "NewFrameSession:GetWindowForTarget", - "sid:%v tid:%v err:%v", - s.ID(), tid, err) - - return nil, fmt.Errorf("getting browser window ID: %w", err) + // When a frame creates a new FrameSession without UI (e.g. some iframes) we cannot + // retrieve the windowID. Doing so would lead to an error from chromium. For now all + // iframes that are attached are setup with hasUIWindow as false which seems to work + // as expected for iframes with and without UI elements. + if hasUIWindow { + action := browser.GetWindowForTarget().WithTargetID(fs.targetID) + var windowID browser.WindowID + if windowID, _, err = action.Do(cdp.WithExecutor(fs.ctx, fs.session)); err != nil { + l.Debugf( + "NewFrameSession:GetWindowForTarget", + "sid:%v tid:%v err:%v", + s.ID(), tid, err) + + return nil, fmt.Errorf("getting browser window ID: %w", err) + } + fs.windowID = &windowID } - fs.windowID = &windowID fs.initEvents() if err = fs.initFrameTree(); err != nil { @@ -1191,18 +1197,20 @@ func (fs *FrameSession) updateViewport() error { return fmt.Errorf("emulating viewport: %w", err) } - // add an inset to viewport depending on the operating system. - // this won't add an inset if we're running in headless mode. - viewport.calculateInset( - fs.page.browserCtx.browser.browserOpts.Headless, - runtime.GOOS, - ) - action2 := browser.SetWindowBounds(*fs.windowID, &browser.Bounds{ - Width: viewport.Width, - Height: viewport.Height, - }) - if err := action2.Do(cdp.WithExecutor(fs.ctx, fs.session)); err != nil { - return fmt.Errorf("setting window bounds: %w", err) + if fs.windowID != nil { + // add an inset to viewport depending on the operating system. + // this won't add an inset if we're running in headless mode. + viewport.calculateInset( + fs.page.browserCtx.browser.browserOpts.Headless, + runtime.GOOS, + ) + action2 := browser.SetWindowBounds(*fs.windowID, &browser.Bounds{ + Width: viewport.Width, + Height: viewport.Height, + }) + if err := action2.Do(cdp.WithExecutor(fs.ctx, fs.session)); err != nil { + return fmt.Errorf("setting window bounds: %w", err) + } } return nil From ed17d77f74c91ce7905ad3ba326cf01b8bac573c Mon Sep 17 00:00:00 2001 From: ankur22 Date: Wed, 21 Aug 2024 09:23:33 +0100 Subject: [PATCH 4/4] Add hasUIWindow as field on FrameSession This makes it more explicit as to what is going on when windowID has not been retrieved and not used later on. This change also means we can revert windowID back to a non-pointer field. --- common/frame_session.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/common/frame_session.go b/common/frame_session.go index 5c2137d02..b7ae24a8a 100644 --- a/common/frame_session.go +++ b/common/frame_session.go @@ -57,8 +57,9 @@ type FrameSession struct { k6Metrics *k6ext.CustomMetrics targetID target.ID - // windowID can be nil when it is associated to an iframe. - windowID *browser.WindowID + // windowID can be 0 when it is associated to an iframe or frame with no UI. + windowID browser.WindowID + hasUIWindow bool // To understand the concepts of Isolated Worlds, Contexts and Frames and // the relationship betwween them have a look at the following doc: @@ -104,6 +105,7 @@ func NewFrameSession( vu: k6ext.GetVU(ctx), k6Metrics: k6Metrics, logger: l, + hasUIWindow: hasUIWindow, } if err := cdpruntime.RunIfWaitingForDebugger().Do(cdp.WithExecutor(fs.ctx, fs.session)); err != nil { @@ -125,10 +127,9 @@ func NewFrameSession( // retrieve the windowID. Doing so would lead to an error from chromium. For now all // iframes that are attached are setup with hasUIWindow as false which seems to work // as expected for iframes with and without UI elements. - if hasUIWindow { + if fs.hasUIWindow { action := browser.GetWindowForTarget().WithTargetID(fs.targetID) - var windowID browser.WindowID - if windowID, _, err = action.Do(cdp.WithExecutor(fs.ctx, fs.session)); err != nil { + if fs.windowID, _, err = action.Do(cdp.WithExecutor(fs.ctx, fs.session)); err != nil { l.Debugf( "NewFrameSession:GetWindowForTarget", "sid:%v tid:%v err:%v", @@ -136,7 +137,6 @@ func NewFrameSession( return nil, fmt.Errorf("getting browser window ID: %w", err) } - fs.windowID = &windowID } fs.initEvents() @@ -1197,14 +1197,14 @@ func (fs *FrameSession) updateViewport() error { return fmt.Errorf("emulating viewport: %w", err) } - if fs.windowID != nil { + if fs.hasUIWindow { // add an inset to viewport depending on the operating system. // this won't add an inset if we're running in headless mode. viewport.calculateInset( fs.page.browserCtx.browser.browserOpts.Headless, runtime.GOOS, ) - action2 := browser.SetWindowBounds(*fs.windowID, &browser.Bounds{ + action2 := browser.SetWindowBounds(fs.windowID, &browser.Bounds{ Width: viewport.Width, Height: viewport.Height, })