Merge pull request #677 from grafana/fix/669-iframe-panic

ankur22 · web-flow · commit 9884e917ff62 · 2022-12-20T17:43:13.000Z
Fix: panic on navigating on remote iframes
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -43,6 +43,7 @@ jobs:
             args[1]="1"
             export GOMAXPROCS=1
           fi
+          export XK6_HEADLESS=true
           go test "${args[@]}" -timeout 5m ./...
 
   test-tip:
@@ -71,6 +72,7 @@ jobs:
           go version
           export GOMAXPROCS=2
           args=("-p" "2" "-race")
+          export XK6_HEADLESS=true
           go test "${args[@]}" -timeout 5m ./...
 
   test-current-cov:
@@ -99,6 +101,7 @@ jobs:
             args[1]="1"
             export GOMAXPROCS=1
           fi
+          export XK6_HEADLESS=true
           echo "mode: set" > coverage.txt
           for pkg in $(go list ./... | grep -v vendor); do
               list=$(go list -test -f  '{{ join .Deps  "\n"}}' $pkg | grep github.com/grafana/xk6-browser | grep -v vendor || true)
diff --git a/common/frame_manager.go b/common/frame_manager.go
@@ -15,6 +15,7 @@ import (
 
 	"github.com/chromedp/cdproto/cdp"
 	"github.com/chromedp/cdproto/network"
+	cdppage "github.com/chromedp/cdproto/page"
 )
 
 // FrameManager manages all frames in a page and their life-cycles, it's a purely internal component.
@@ -166,20 +167,26 @@ func (m *FrameManager) frameAttached(frameID cdp.FrameID, parentFrameID cdp.Fram
 	}
 }
 
-func (m *FrameManager) frameDetached(frameID cdp.FrameID) {
+func (m *FrameManager) frameDetached(frameID cdp.FrameID, reason cdppage.FrameDetachedReason) {
 	m.logger.Debugf("FrameManager:frameDetached", "fmid:%d fid:%v", m.ID(), frameID)
 
-	// TODO: use getFrameByID here
-	m.framesMu.RLock()
-	frame, ok := m.frames[frameID]
-	m.framesMu.RUnlock()
-	if !ok {
+	frame := m.getFrameByID(frameID)
+	if frame == nil {
 		m.logger.Debugf("FrameManager:frameDetached:return",
 			"fmid:%d fid:%v cannot find frame",
 			m.ID(), frameID)
 		return
 	}
-	// TODO: possible data race? the frame may have gone.
+
+	if reason == cdppage.FrameDetachedReasonSwap {
+		// When a local frame is swapped out for a remote
+		// 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
+	}
+
 	m.removeFramesRecursively(frame)
 }
 
diff --git a/common/frame_session.go b/common/frame_session.go
@@ -639,7 +639,7 @@ 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)
+	fs.manager.frameDetached(frameID, reason)
 }
 
 func (fs *FrameSession) onFrameNavigated(frame *cdp.Frame, initial bool) {
diff --git a/tests/frame_test.go b/tests/frame_test.go
@@ -1,6 +1,8 @@
 package tests
 
 import (
+	"os"
+	"strconv"
 	"testing"
 
 	"github.com/dop251/goja"
@@ -72,3 +74,46 @@ func TestFrameDismissDialogBox(t *testing.T) {
 		})
 	}
 }
+
+func TestFrameNoPanicWithEmbeddedIFrame(t *testing.T) {
+	if strValue, ok := os.LookupEnv("XK6_HEADLESS"); ok {
+		if value, err := strconv.ParseBool(strValue); err == nil && value {
+			// We're skipping this when running in headless
+			// environments since the bug that the test fixes
+			// only surfaces when in headfull mode.
+			// Remove this skip once we have headfull mode in
+			// CI: https://github.com/grafana/xk6-browser/issues/678
+			t.Skip("skipped when in headless mode")
+		}
+	}
+
+	t.Parallel()
+
+	opts := defaultLaunchOpts()
+	opts.Headless = false
+	b := newTestBrowser(t, withFileServer(), opts)
+	p := b.NewPage(nil)
+
+	var result string
+	err := b.await(func() error {
+		opts := b.toGojaValue(struct {
+			WaitUntil string `js:"waitUntil"`
+		}{
+			WaitUntil: "load",
+		})
+		pageGoto := p.Goto(
+			b.staticURL("embedded_iframe.html"),
+			opts,
+		)
+
+		b.promise(pageGoto).
+			then(func() {
+				result = p.TextContent("#doneDiv", nil)
+			})
+
+		return nil
+	})
+	require.NoError(t, err)
+
+	assert.EqualValues(t, "Done!", result)
+}
diff --git a/tests/static/embedded_iframe.html b/tests/static/embedded_iframe.html
@@ -0,0 +1,7 @@
+<html>
+    <head></head>
+    <body>
+        <div id='doneDiv'></div>
+        <iframe src='https://www.youtube.com/embed/gwO7k5RTE54?wmode=opaque&amp;enablejsapi=1' onload='document.getElementById("doneDiv").innerText = "Done!"'></iframe>
+    </body>
+</html>

Original file line number	Diff line number	Diff line change
`@@ -639,7 +639,7 @@ func (fs *FrameSession) onFrameDetached(frameID cdp.FrameID, reason cdppage.Fram`
`639`	`639`	`"sid:%v tid:%v fid:%v reason:%s",`
`640`	`640`	`fs.session.ID(), fs.targetID, frameID, reason)`
`641`	`641`
`642`		`- fs.manager.frameDetached(frameID)`
	`642`	`+ fs.manager.frameDetached(frameID, reason)`
`643`	`643`	`}`
`644`	`644`
`645`	`645`	`func (fs FrameSession) onFrameNavigated(frame cdp.Frame, initial bool) {`