From d921bb4c8d192b4f828905ca2457ac1021ef2623 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Wed, 10 Jan 2024 17:20:29 +0000 Subject: [PATCH] Fix potential nil exception when getting frame When a part of the browser module needs to retrieve the frame given an id, it should check whether the frame is nil before using it. This is not safe, since it's easy to forget to check for nil. This change avoids the need to check for nil and reinforces a requirement to check with an addition of a bool that is also returned to indicate whether the frame was found or not. --- common/element_handle.go | 22 ++++++++++++++++------ common/frame.go | 4 ++-- common/frame_manager.go | 26 ++++++++++++++++---------- common/frame_session.go | 23 +++++++++++++---------- common/network_manager.go | 5 +++-- 5 files changed, 50 insertions(+), 30 deletions(-) diff --git a/common/element_handle.go b/common/element_handle.go index 32757db01..763c3f623 100644 --- a/common/element_handle.go +++ b/common/element_handle.go @@ -440,13 +440,13 @@ func (h *ElementHandle) ownerFrame(apiCtx context.Context) *Frame { if frameId == "" { return nil } - frame := h.frame.page.frameManager.getFrameByID(frameId) - if frame != nil { + frame, found := h.frame.page.frameManager.getFrameByID(frameId) + if found { return frame } for _, page := range h.frame.page.browserCtx.browser.pages { - frame = page.frameManager.getFrameByID(frameId) - if frame != nil { + frame, found = page.frameManager.getFrameByID(frameId) + if found { return frame } } @@ -766,7 +766,12 @@ func (h *ElementHandle) ContentFrame() (*Frame, error) { return nil, fmt.Errorf("element is not an iframe") } - return h.frame.manager.getFrameByID(node.FrameID), nil + frame, found := h.frame.manager.getFrameByID(node.FrameID) + if !found { + return nil, fmt.Errorf("frame not found for id") + } + + return frame, nil } func (h *ElementHandle) Dblclick(opts goja.Value) { @@ -1003,7 +1008,12 @@ func (h *ElementHandle) OwnerFrame() (*Frame, error) { return nil, fmt.Errorf("no frame found for node: %w", err) } - return h.frame.manager.getFrameByID(node.FrameID), nil + frame, found := h.frame.manager.getFrameByID(node.FrameID) + if !found { + return nil, fmt.Errorf("no frame found for id") + } + + return frame, nil } func (h *ElementHandle) Press(key string, opts goja.Value) { diff --git a/common/frame.go b/common/frame.go index e298cd516..c96e69523 100644 --- a/common/frame.go +++ b/common/frame.go @@ -377,8 +377,8 @@ func (f *Frame) onLoadingStopped() { } func (f *Frame) position() (*Position, error) { - frame := f.manager.getFrameByID(cdp.FrameID(f.page.targetID)) - if frame == nil { + frame, found := f.manager.getFrameByID(cdp.FrameID(f.page.targetID)) + if !found { return nil, fmt.Errorf("could not find frame with id %s", f.page.targetID) } if frame == f.page.frameManager.MainFrame() { diff --git a/common/frame_manager.go b/common/frame_manager.go index 666381d34..c4d3dc455 100644 --- a/common/frame_manager.go +++ b/common/frame_manager.go @@ -165,8 +165,8 @@ func (m *FrameManager) frameAttached(frameID cdp.FrameID, parentFrameID cdp.Fram func (m *FrameManager) frameDetached(frameID cdp.FrameID, reason cdppage.FrameDetachedReason) { m.logger.Debugf("FrameManager:frameDetached", "fmid:%d fid:%v", m.ID(), frameID) - frame := m.getFrameByID(frameID) - if frame == nil { + frame, found := m.getFrameByID(frameID) + if !found { m.logger.Debugf("FrameManager:frameDetached:return", "fmid:%d fid:%v cannot find frame", m.ID(), frameID) @@ -190,8 +190,8 @@ func (m *FrameManager) frameLifecycleEvent(frameID cdp.FrameID, event LifecycleE "fmid:%d fid:%v event:%s", m.ID(), frameID, lifecycleEventToString[event]) - frame := m.getFrameByID(frameID) - if frame != nil { + frame, found := m.getFrameByID(frameID) + if found { frame.onLifecycleEvent(event) } } @@ -200,8 +200,8 @@ func (m *FrameManager) frameLoadingStarted(frameID cdp.FrameID) { m.logger.Debugf("FrameManager:frameLoadingStarted", "fmid:%d fid:%v", m.ID(), frameID) - frame := m.getFrameByID(frameID) - if frame != nil { + frame, found := m.getFrameByID(frameID) + if found { frame.onLoadingStarted() } } @@ -210,8 +210,8 @@ func (m *FrameManager) frameLoadingStopped(frameID cdp.FrameID) { m.logger.Debugf("FrameManager:frameLoadingStopped", "fmid:%d fid:%v", m.ID(), frameID) - frame := m.getFrameByID(frameID) - if frame != nil { + frame, found := m.getFrameByID(frameID) + if found { frame.onLoadingStopped() } } @@ -390,10 +390,16 @@ func (m *FrameManager) frameRequestedNavigation(frameID cdp.FrameID, url string, return nil } -func (m *FrameManager) getFrameByID(id cdp.FrameID) *Frame { +// getFrameByID will attempt to find a frame with the given id. If one is found +// the frame and true will be returned. If one isn't found nil and false will +// be returned. +func (m *FrameManager) getFrameByID(id cdp.FrameID) (*Frame, bool) { m.framesMu.RLock() defer m.framesMu.RUnlock() - return m.frames[id] + + frame, ok := m.frames[id] + + return frame, ok } func (m *FrameManager) removeChildFramesRecursively(frame *Frame) { diff --git a/common/frame_session.go b/common/frame_session.go index 988ae7690..4a7105896 100644 --- a/common/frame_session.go +++ b/common/frame_session.go @@ -434,7 +434,10 @@ func (fs *FrameSession) initIsolatedWorld(name string) error { if fs.isMainFrame() { frames = fs.manager.Frames() } else { - frames = []*Frame{fs.manager.getFrameByID(cdp.FrameID(fs.targetID))} + frame, found := fs.manager.getFrameByID(cdp.FrameID(fs.targetID)) + if found { + frames = []*Frame{frame} + } } for _, frame := range frames { // A frame could have been removed before we execute this, so don't wait around for a reply. @@ -659,8 +662,8 @@ func (fs *FrameSession) onExecutionContextCreated(event *cdpruntime.EventExecuti k6ext.Panic(fs.ctx, "unmarshaling executionContextCreated event JSON: %w", err) } var world executionWorld - frame := fs.manager.getFrameByID(i.FrameID) - if frame != nil { + frame, found := fs.manager.getFrameByID(i.FrameID) + if found { if i.IsDefault { world = mainWorld } else if event.Context.Name == utilityWorldName && !frame.hasContext(utilityWorld) { @@ -760,12 +763,12 @@ func (fs *FrameSession) onFrameNavigated(frame *cdp.Frame, initial bool) { ) var ( - spanID = fs.mainFrameSpan.SpanContext().SpanID().String() - newFrame = fs.manager.getFrameByID(frame.ID) + spanID = fs.mainFrameSpan.SpanContext().SpanID().String() + newFrame, found = fs.manager.getFrameByID(frame.ID) ) // Only set the k6SpanId reference if it's a new frame. - if newFrame == nil { + if !found { return } @@ -851,8 +854,8 @@ func (fs *FrameSession) onPageLifecycle(event *cdppage.EventLifecycleEvent) { "sid:%v tid:%v fid:%v event:%s eventTime:%q", fs.session.ID(), fs.targetID, event.FrameID, event.Name, event.Timestamp.Time()) - frame := fs.manager.getFrameByID(event.FrameID) - if frame == nil { + _, found := fs.manager.getFrameByID(event.FrameID) + if !found { return } @@ -953,8 +956,8 @@ func (fs *FrameSession) onAttachedToTarget(event *target.EventAttachedToTarget) // attachIFrameToTarget attaches an IFrame target to a given session. func (fs *FrameSession) attachIFrameToTarget(ti *target.Info, sid target.SessionID) error { - fr := fs.manager.getFrameByID(cdp.FrameID(ti.TargetID)) - if fr == nil { + fr, found := fs.manager.getFrameByID(cdp.FrameID(ti.TargetID)) + if !found { // IFrame should be attached to fs.page with EventFrameAttached // event before. fs.logger.Debugf("FrameSession:attachIFrameToTarget:return", diff --git a/common/network_manager.go b/common/network_manager.go index 6327d11a0..bf2449ad5 100644 --- a/common/network_manager.go +++ b/common/network_manager.go @@ -458,10 +458,11 @@ func (m *NetworkManager) onRequest(event *network.EventRequestWillBeSent, interc } var frame *Frame = nil + var found bool if event.FrameID != "" { - frame = m.frameManager.getFrameByID(event.FrameID) + frame, found = m.frameManager.getFrameByID(event.FrameID) } - if frame == nil { + if !found { m.logger.Debugf("NetworkManager:onRequest", "url:%s method:%s type:%s fid:%s frame is nil", event.Request.URL, event.Request.Method, event.Initiator.Type, event.FrameID) }