Merge pull request #630 from grafana/bug/race-pendingdoc

Add mutex to prevent data race on pendingDoc and use mutex when reading frame.inflightRequests

Author: ankur22

common/frame.go

@@ -70,8 +70,9 @@ type Frame struct {
 	inflightRequestsMu sync.RWMutex
 	inflightRequests   map[network.RequestID]bool
 
-	currentDocument *DocumentInfo
-	pendingDocument *DocumentInfo
+	currentDocument   *DocumentInfo
+	pendingDocumentMu sync.RWMutex
+	pendingDocument   *DocumentInfo
 
 	log *log.Logger
 }
@@ -147,6 +148,18 @@ func (f *Frame) inflightRequestsLen() int {
 	return len(f.inflightRequests)
 }
 
+func (f *Frame) cloneInflightRequests() map[network.RequestID]bool {
+	f.inflightRequestsMu.RLock()
+	defer f.inflightRequestsMu.RUnlock()
+
+	ifr := make(map[network.RequestID]bool)
+	for k, v := range f.inflightRequests {
+		ifr[k] = v
+	}
+
+	return ifr
+}
+
 func (f *Frame) clearLifecycle() {
 	f.log.Debugf("Frame:clearLifecycle", "fid:%s furl:%q", f.ID(), f.URL())
 

common/frame_manager.go

@@ -117,10 +117,18 @@ func (m *FrameManager) frameAbortedNavigation(frameID cdp.FrameID, errorText, do
 	defer m.framesMu.Unlock()
 
 	frame := m.frames[frameID]
-	if frame == nil || frame.pendingDocument == nil {
+	if frame == nil {
+		return
+	}
+
+	frame.pendingDocumentMu.Lock()
+
+	if frame.pendingDocument == nil {
+		frame.pendingDocumentMu.Unlock()
 		return
 	}
 	if documentID != "" && frame.pendingDocument.documentID != documentID {
+		frame.pendingDocumentMu.Unlock()
 		return
 	}
 
@@ -135,6 +143,9 @@ func (m *FrameManager) frameAbortedNavigation(frameID cdp.FrameID, errorText, do
 		err:         errors.New(errorText),
 	}
 	frame.pendingDocument = nil
+
+	frame.pendingDocumentMu.Unlock()
+
 	frame.emit(EventFrameNavigation, ne)
 }
 
@@ -266,6 +277,9 @@ func (m *FrameManager) frameNavigated(frameID cdp.FrameID, parentFrameID cdp.Fra
 
 	frame.navigated(name, url, documentID)
 
+	frame.pendingDocumentMu.Lock()
+	defer frame.pendingDocumentMu.Unlock()
+
 	var (
 		keepPending     *DocumentInfo
 		pendingDocument = frame.pendingDocument
@@ -362,6 +376,9 @@ func (m *FrameManager) frameRequestedNavigation(frameID cdp.FrameID, url string,
 		b.AddFrameNavigation(frame)
 	}
 
+	frame.pendingDocumentMu.Lock()
+	defer frame.pendingDocumentMu.Unlock()
+
 	if frame.pendingDocument != nil && frame.pendingDocument.documentID == documentID {
 		m.logger.Debugf("FrameManager:frameRequestedNavigation:return",
 			"fmid:%d fid:%v furl:%s url:%s docid:%s pdocid:%s pdoc:dontSet",
@@ -433,29 +450,43 @@ func (m *FrameManager) requestFailed(req *Request, canceled bool) {
 	}
 	frame.deleteRequest(req.getID())
 
-	switch rc := frame.inflightRequestsLen(); {
+	ifr := frame.cloneInflightRequests()
+	switch rc := len(ifr); {
 	case rc == 0:
 		frame.startNetworkIdleTimer()
 	case rc <= 10:
-		for reqID := range frame.inflightRequests {
+		for reqID := range ifr {
 			req := frame.requestByID(reqID)
 
+			if req == nil {
+				m.logger.Debugf("FrameManager:requestFailed:rc<=10 request is nil",
+					"reqID:%s frameID:%s",
+					reqID, frame.ID())
+				continue
+			}
+
 			m.logger.Debugf("FrameManager:requestFailed:rc<=10",
 				"reqID:%s inflightURL:%s frameID:%s",
 				reqID, req.URL(), frame.ID())
 		}
 	}
 
+	frame.pendingDocumentMu.RLock()
 	if frame.pendingDocument == nil || frame.pendingDocument.request != req {
 		m.logger.Debugf("FrameManager:requestFailed:return", "fmid:%d pdoc:nil", m.ID())
+		frame.pendingDocumentMu.RUnlock()
 		return
 	}
+
 	errorText := req.errorText
 	if canceled {
 		errorText += "; maybe frame was detached?"
 	}
-	m.frameAbortedNavigation(cdp.FrameID(frame.ID()), errorText,
-		frame.pendingDocument.documentID)
+
+	docID := frame.pendingDocument.documentID
+	frame.pendingDocumentMu.RUnlock()
+
+	m.frameAbortedNavigation(cdp.FrameID(frame.ID()), errorText, docID)
 }
 
 func (m *FrameManager) requestFinished(req *Request) {
@@ -510,7 +541,9 @@ func (m *FrameManager) requestStarted(req *Request) {
 		frame.stopNetworkIdleTimer()
 	}
 	if req.documentID != "" {
+		frame.pendingDocumentMu.Lock()
 		frame.pendingDocument = &DocumentInfo{documentID: req.documentID, request: req}
+		frame.pendingDocumentMu.Unlock()
 	}
 	m.logger.Debugf("FrameManager:requestStarted", "fmid:%d rurl:%s pdoc:nil", m.ID(), req.URL())
 }