Skip to content

Commit

Permalink
Fix potential nil exception when getting frame
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ankur22
ankur22 committed Jan 11, 2024
1 parent b7543e6 commit d921bb4
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 30 deletions.
22 changes: 16 additions & 6 deletions common/element_handle.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions common/frame.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
26 changes: 16 additions & 10 deletions common/frame_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
}
}
Expand All @@ -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()
}
}
Expand All @@ -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()
}
}
Expand Down Expand Up @@ -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) {
Expand Down
23 changes: 13 additions & 10 deletions common/frame_session.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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",
Expand Down
5 changes: 3 additions & 2 deletions common/network_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down

0 comments on commit d921bb4

Please sign in to comment.