test: live CDP download guid coverage and unroute interception deadlock fix

browser_context.go

@@ -273,13 +273,15 @@ func (b *browserContextImpl) ExposeFunction(name string, binding ExposedFunction
 
 func (b *browserContextImpl) Route(url any, handler routeHandler, times ...int) error {
 	b.Lock()
-	defer b.Unlock()
 	b.routes = slices.Insert(b.routes, 0, newRouteHandlerEntry(newURLMatcher(url, b.options.BaseURL), handler, times...))
+	b.Unlock()
 	return b.updateInterceptionPatterns()
 }
 
 func (b *browserContextImpl) Unroute(url any, handlers ...routeHandler) error {
+	b.Lock()
 	removed, remaining, err := unroute(b.routes, url, handlers...)
+	b.Unlock()
 	if err != nil {
 		return err
 	}
@@ -288,8 +290,8 @@ func (b *browserContextImpl) Unroute(url any, handlers ...routeHandler) error {
 
 func (b *browserContextImpl) unrouteInternal(removed []*routeHandlerEntry, remaining []*routeHandlerEntry, behavior *UnrouteBehavior) error {
 	b.Lock()
-	defer b.Unlock()
 	b.routes = remaining
+	b.Unlock()
 	if err := b.updateInterceptionPatterns(); err != nil {
 		return err
 	}
@@ -313,8 +315,11 @@ func (b *browserContextImpl) UnrouteAll(options ...BrowserContextUnrouteAllOptio
 	if len(options) == 1 {
 		behavior = options[0].Behavior
 	}
+	b.Lock()
+	removed := b.routes
+	b.Unlock()
 	defer b.disposeHarRouters()
-	return b.unrouteInternal(b.routes, []*routeHandlerEntry{}, behavior)
+	return b.unrouteInternal(removed, []*routeHandlerEntry{}, behavior)
 }
 
 func (b *browserContextImpl) disposeHarRouters() {
@@ -634,16 +639,21 @@ func (b *browserContextImpl) onRoute(route *routeImpl) {
 	b.Unlock()
 
 	checkInterceptionIfNeeded := func() {
+		// Do not hold the context mutex across the protocol round-trip:
+		// dispatch may need the same lock (e.g. page.onClose) to deliver the
+		// setNetworkInterceptionPatterns reply.
 		b.Lock()
-		defer b.Unlock()
-		if len(b.routes) == 0 {
-			_, err := b.connection.WrapAPICall(func() (any, error) {
-				err := b.updateInterceptionPatterns()
-				return nil, err
-			}, true)
-			if err != nil {
-				logger.Error("could not update interception patterns", "error", err)
-			}
+		empty := len(b.routes) == 0
+		b.Unlock()
+		if !empty {
+			return
+		}
+		_, err := b.connection.WrapAPICall(func() (any, error) {
+			err := b.updateInterceptionPatterns()
+			return nil, err
+		}, true)
+		if err != nil {
+			logger.Error("could not update interception patterns", "error", err)
 		}
 	}
 
@@ -656,16 +666,19 @@ func (b *browserContextImpl) onRoute(route *routeImpl) {
 		if !handlerEntry.Matches(url) {
 			continue
 		}
-		if !slices.ContainsFunc(b.routes, func(entry *routeHandlerEntry) bool {
+		b.Lock()
+		stillListed := slices.ContainsFunc(b.routes, func(entry *routeHandlerEntry) bool {
 			return entry == handlerEntry
-		}) {
-			continue
-		}
-		if handlerEntry.WillExceed() {
+		})
+		if stillListed && handlerEntry.WillExceed() {
 			b.routes = slices.DeleteFunc(b.routes, func(rhe *routeHandlerEntry) bool {
 				return rhe == handlerEntry
 			})
 		}
+		b.Unlock()
+		if !stillListed {
+			continue
+		}
 		handled := handlerEntry.Handle(route)
 		checkInterceptionIfNeeded()
 		yes := <-handled
@@ -679,7 +692,9 @@ func (b *browserContextImpl) onRoute(route *routeImpl) {
 }
 
 func (b *browserContextImpl) updateInterceptionPatterns() error {
+	b.Lock()
 	patterns := prepareInterceptionPatterns(b.routes)
+	b.Unlock()
 	_, err := b.channel.Send("setNetworkInterceptionPatterns", map[string]any{
 		"patterns": patterns,
 	})

page.go

@@ -313,17 +313,18 @@ func (p *pageImpl) URL() string {
 
 func (p *pageImpl) Unroute(url any, handlers ...routeHandler) error {
 	p.Lock()
-	defer p.Unlock()
-
 	removed, remaining, err := unroute(p.routes, url, handlers...)
+	p.Unlock()
 	if err != nil {
 		return err
 	}
 	return p.unrouteInternal(removed, remaining, UnrouteBehaviorDefault)
 }
 
 func (p *pageImpl) unrouteInternal(removed []*routeHandlerEntry, remaining []*routeHandlerEntry, behavior *UnrouteBehavior) error {
+	p.Lock()
 	p.routes = remaining
+	p.Unlock()
 	err := p.updateInterceptionPatterns()
 	if err != nil {
 		return err
@@ -356,9 +357,10 @@ func (p *pageImpl) UnrouteAll(options ...PageUnrouteAllOptions) error {
 		behavior = options[0].Behavior
 	}
 	p.Lock()
-	defer p.Unlock()
+	removed := p.routes
+	p.Unlock()
 	defer p.disposeHarRouters()
-	return p.unrouteInternal(p.routes, []*routeHandlerEntry{}, behavior)
+	return p.unrouteInternal(removed, []*routeHandlerEntry{}, behavior)
 }
 
 func (p *pageImpl) Content() (string, error) {
@@ -799,8 +801,8 @@ func (p *pageImpl) ExpectWorker(cb func() error, options ...PageExpectWorkerOpti
 
 func (p *pageImpl) Route(url any, handler routeHandler, times ...int) error {
 	p.Lock()
-	defer p.Unlock()
 	p.routes = slices.Insert(p.routes, 0, newRouteHandlerEntry(newURLMatcher(url, p.browserContext.options.BaseURL), handler, times...))
+	p.Unlock()
 	return p.updateInterceptionPatterns()
 }
 
@@ -1099,16 +1101,20 @@ func (p *pageImpl) onRoute(route *routeImpl) {
 	p.Unlock()
 
 	checkInterceptionIfNeeded := func() {
+		// Do not hold the page mutex across the protocol round-trip; dispatch
+		// may need to run other handlers before the reply can be delivered.
 		p.Lock()
-		defer p.Unlock()
-		if len(p.routes) == 0 {
-			_, err := p.connection.WrapAPICall(func() (any, error) {
-				err := p.updateInterceptionPatterns()
-				return nil, err
-			}, true)
-			if err != nil {
-				logger.Error("could not update interception patterns", "error", err)
-			}
+		empty := len(p.routes) == 0
+		p.Unlock()
+		if !empty {
+			return
+		}
+		_, err := p.connection.WrapAPICall(func() (any, error) {
+			err := p.updateInterceptionPatterns()
+			return nil, err
+		}, true)
+		if err != nil {
+			logger.Error("could not update interception patterns", "error", err)
 		}
 	}
 
@@ -1123,16 +1129,19 @@ func (p *pageImpl) onRoute(route *routeImpl) {
 		if !handlerEntry.Matches(url) {
 			continue
 		}
-		if !slices.ContainsFunc(p.routes, func(entry *routeHandlerEntry) bool {
+		p.Lock()
+		stillListed := slices.ContainsFunc(p.routes, func(entry *routeHandlerEntry) bool {
 			return entry == handlerEntry
-		}) {
-			continue
-		}
-		if handlerEntry.WillExceed() {
+		})
+		if stillListed && handlerEntry.WillExceed() {
 			p.routes = slices.DeleteFunc(p.routes, func(rhe *routeHandlerEntry) bool {
 				return rhe == handlerEntry
 			})
 		}
+		p.Unlock()
+		if !stillListed {
+			continue
+		}
 		handled := handlerEntry.Handle(route)
 		checkInterceptionIfNeeded()
 
@@ -1144,7 +1153,9 @@ func (p *pageImpl) onRoute(route *routeImpl) {
 }
 
 func (p *pageImpl) updateInterceptionPatterns() error {
+	p.Lock()
 	patterns := prepareInterceptionPatterns(p.routes)
+	p.Unlock()
 	_, err := p.channel.Send("setNetworkInterceptionPatterns", map[string]any{
 		"patterns": patterns,
 	})

tests/cdp_session_test.go

@@ -1,7 +1,11 @@
 package playwright_test
 
 import (
+	"fmt"
+	"log"
+	"net/http"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 )
@@ -40,6 +44,61 @@ func TestCDPSessionOn(t *testing.T) {
 	}
 }
 
+// Chrome's Page.downloadWillBegin / Page.downloadProgress payloads carry a
+// download guid that is not a Playwright channel. A CDPSession with Page.enable
+// relays those maps as events; they must not tear down the driver connection.
+func TestCDPSessionDownloadDoesNotCloseConnection(t *testing.T) {
+	BeforeEach(t)
+
+	cdpSession, err := page.Context().NewCDPSession(page)
+	if !isChromium {
+		require.Error(t, err)
+		return
+	}
+	require.NoError(t, err)
+
+	server.SetRoute("/downloadWithFilename", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Add("Content-Type", "application/octet-stream")
+		w.Header().Add("Content-Disposition", "attachment; filename=file.txt")
+		if _, err := w.Write([]byte("foobar")); err != nil {
+			log.Printf("could not write: %v", err)
+		}
+	})
+
+	began := make(chan map[string]any, 1)
+	cdpSession.On("Page.downloadWillBegin", func(params map[string]any) {
+		began <- params
+	})
+	_, err = cdpSession.Send("Page.enable", nil)
+	require.NoError(t, err)
+	_, err = cdpSession.Send("Page.setDownloadBehavior", map[string]any{
+		"behavior":     "allow",
+		"downloadPath": t.TempDir(),
+	})
+	require.NoError(t, err)
+
+	require.NoError(t, page.SetContent(
+		fmt.Sprintf(`<a href="%s/downloadWithFilename">download</a>`, server.PREFIX),
+	))
+	download, err := page.ExpectDownload(func() error {
+		return page.Locator("a").Click()
+	})
+	require.NoError(t, err)
+	require.Equal(t, "file.txt", download.SuggestedFilename())
+
+	select {
+	case params := <-began:
+		require.Equal(t, "file.txt", params["suggestedFilename"])
+		require.NotEmpty(t, params["guid"])
+	case <-time.After(5 * time.Second):
+		t.Fatal("timed out waiting for Page.downloadWillBegin")
+	}
+
+	ctx, err := browser.NewContext()
+	require.NoError(t, err, "connection must survive an unbound CDP download guid")
+	require.NoError(t, ctx.Close())
+}
+
 func TestCDPSessionDetach(t *testing.T) {
 	BeforeEach(t)