From 4518103c74499485881c0be33eeeb40c888810d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Tue, 20 Feb 2024 12:34:44 +0300 Subject: [PATCH 01/19] Add a bool result value to Connection.closeSession This allows us to detect whether a session that we're trying to close was in the connection's sessions. This way, we can skip. See the next commit for details. --- common/connection.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/common/connection.go b/common/connection.go index 1116f5bc6..cc49553a3 100644 --- a/common/connection.go +++ b/common/connection.go @@ -203,14 +203,22 @@ func (c *Connection) close(code int) error { return err } -func (c *Connection) closeSession(sid target.SessionID, tid target.ID) { +// closeSession closes the session with the given session ID. +// It returns true if the session was found and closed, false otherwise. +func (c *Connection) closeSession(sid target.SessionID, tid target.ID) bool { c.logger.Debugf("Connection:closeSession", "sid:%v tid:%v wsURL:%v", sid, tid, c.wsURL) + c.sessionsMu.Lock() - if session, ok := c.sessions[sid]; ok { - session.close() + defer c.sessionsMu.Unlock() + + session, ok := c.sessions[sid] + if !ok { + return false } + session.close() delete(c.sessions, sid) - c.sessionsMu.Unlock() + + return true } func (c *Connection) closeAllSessions() { From 6d6912cb53b8215ba60b3246c8fef9310b2abebc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Tue, 20 Feb 2024 12:35:50 +0300 Subject: [PATCH 02/19] Skip message propagation if session wasn't found The connection now skips doing session read and browser module event generation and management. This way, we prevent unnecessary communication overhead. --- common/connection.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/common/connection.go b/common/connection.go index cc49553a3..145ae6c62 100644 --- a/common/connection.go +++ b/common/connection.go @@ -348,7 +348,16 @@ func (c *Connection) recvLoop() { evt := ev.(*target.EventDetachedFromTarget) sid := evt.SessionID tid := c.findTargetIDForLog(sid) - c.closeSession(sid, tid) + ok := c.closeSession(sid, tid) + if !ok { + c.logger.Debugf( + "Connection:recvLoop:EventDetachedFromTarget", + "sid:%v tid:%v wsURL:%q, session not found", + sid, tid, c.wsURL, + ) + + continue + } } switch { From 08a9e81eba2740866fdcd1722d9407e1080578eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Wed, 21 Feb 2024 10:04:08 +0300 Subject: [PATCH 03/19] Add Connection.onTargetAttachedToTarget callback This callback will be called to manage the connection session handling from outside. Since Browser is the creator of Connection, we're adding this callback to let Browser hook into target attachment and tell Connection to stop attaching the target if Browser doesn't want it. This is a downside of the current design. There is a symbiotic relationship between Connection and Browser. Perhaps they should be together, not separate this way. But for the current design, this is the best we can do to improve Browser's session management. --- common/connection.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/common/connection.go b/common/connection.go index 145ae6c62..8917cd88c 100644 --- a/common/connection.go +++ b/common/connection.go @@ -131,6 +131,11 @@ type Connection struct { // Reuse the easyjson structs to avoid allocs per Read/Write. decoder jlexer.Lexer encoder jwriter.Writer + + // onTargetAttachedToTarget is called when a new target is attached to the browser. + // Returning false will prevent the session from being created. + // If onTargetAttachedToTarget is nil, the session will be created. + onTargetAttachedToTarget func(*target.EventAttachedToTarget) bool } // NewConnection creates a new browser. @@ -334,6 +339,15 @@ func (c *Connection) recvLoop() { eva := ev.(*target.EventAttachedToTarget) sid, tid := eva.SessionID, eva.TargetInfo.TargetID + if c.onTargetAttachedToTarget != nil { + // If onTargetAttachedToTarget is set, it will be called to determine + // if a session should be created for the target. + ok := c.onTargetAttachedToTarget(eva) + if !ok { + continue + } + } + c.sessionsMu.Lock() session := NewSession(c.ctx, c, sid, tid, c.logger, c.msgIDGen) c.logger.Debugf("Connection:recvLoop:EventAttachedToTarget", "sid:%v tid:%v wsURL:%q", sid, tid, c.wsURL) From 86e96a99ab7671954908a096940cdd2fe06bdbd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Tue, 20 Feb 2024 12:52:27 +0300 Subject: [PATCH 04/19] Add Browser.connectionOnAttachedToTarget --- common/browser.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/common/browser.go b/common/browser.go index 2ec1a563a..3b1dc6c0b 100644 --- a/common/browser.go +++ b/common/browser.go @@ -217,6 +217,12 @@ func (b *Browser) initEvents() error { return nil } +// connectionOnAttachedToTarget is called when Connection receives an attachedToTarget +// event. Returning false will stop the event from being processed by the connection. +func (b *Browser) connectionOnAttachedToTarget(_ *target.EventAttachedToTarget) bool { + return true +} + // onAttachedToTarget is called when a new page is attached to the browser. func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) { b.logger.Debugf("Browser:onAttachedToTarget", "sid:%v tid:%v bctxid:%v", From 62a02be82b85bab8e6f79312bfb3acad99893414 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Tue, 20 Feb 2024 12:53:36 +0300 Subject: [PATCH 05/19] Add binding for connectionOnAttachedToTarget This way, we can listen target attachment events from Connection, and veto Connection's decision of adding a Session to its internal list. --- common/browser.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/browser.go b/common/browser.go index 3b1dc6c0b..14271ffb9 100644 --- a/common/browser.go +++ b/common/browser.go @@ -116,6 +116,11 @@ func (b *Browser) connect() error { return fmt.Errorf("connecting to browser DevTools URL: %w", err) } + // hook into the connection to listen for target attachment events. + // this way, browser can manage the decision of target attachments. + // so that we can stop connection from doing unnecessary work. + conn.onTargetAttachedToTarget = b.connectionOnAttachedToTarget + b.conn = conn // We don't need to lock this because `connect()` is called only in NewBrowser From ecbd3a717e7383ef6bb6153aaccc1cfd22897442 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Tue, 20 Feb 2024 13:03:55 +0300 Subject: [PATCH 06/19] Add a check for target is in the same context This isn't yet functional until we add other mechanics in later commits. For now, it allows Connection to attach a target if it's in the same BrowserContext (or in the default context). We don't use `defer` for the lock because we don't want introduce any lock overhead. Local tests showed us that holding the lock for the entire function is prone to unexpected deadlock issues. This is actually for a future code addition that can easily miss this fact. --- common/browser.go | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/common/browser.go b/common/browser.go index 14271ffb9..27a1b825f 100644 --- a/common/browser.go +++ b/common/browser.go @@ -224,8 +224,27 @@ func (b *Browser) initEvents() error { // connectionOnAttachedToTarget is called when Connection receives an attachedToTarget // event. Returning false will stop the event from being processed by the connection. -func (b *Browser) connectionOnAttachedToTarget(_ *target.EventAttachedToTarget) bool { - return true +func (b *Browser) connectionOnAttachedToTarget(eva *target.EventAttachedToTarget) bool { + // Allow connection to attach to target as the browser context is not set + // (hence it's the first one we have—the default), or the target is in the + // same browser context as the current one. + // + // This allows to attach targets to the same browser context as the current + // one, and to the default browser context. + // + // We don't want to hold the lock for the entire function to prevent + // concurrency issues with the browser context being closed while we're + // checking it. So, we don't use defer. This is for a future code that might + // extend this function, add a defer, and cause this issue to be missed. + b.contextMu.RLock() + defaultContext := b.context == nil + if defaultContext || b.context.id == eva.TargetInfo.BrowserContextID { + b.contextMu.RUnlock() + return true + } + b.contextMu.RUnlock() + + return false } // onAttachedToTarget is called when a new page is attached to the browser. From 3ce1e835cb7ad8c4d2fac9e3c231036d41377c2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Tue, 20 Feb 2024 13:10:48 +0300 Subject: [PATCH 07/19] Stop sharing browser contexts This and the previous commit allow us to stop sharing browser contexts. --- common/browser.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/common/browser.go b/common/browser.go index 27a1b825f..b8a6b5f61 100644 --- a/common/browser.go +++ b/common/browser.go @@ -334,6 +334,15 @@ func (b *Browser) isAttachedPageValid(ev *target.EventAttachedToTarget, browserC ev.SessionID, targetPage.TargetID, targetPage.BrowserContextID, browserCtx == nil, targetPage.Type) return false } + // If the target is not in the same browser context as the current one, ignore it. + // We're not adding a log to prevent overhead. + if browserCtx.id != targetPage.BrowserContextID { + b.logger.Warnf( + "Browser:isAttachedPageValid", "incorrect browser context sid:%v tid:%v bctxid:%v target bctxid:%v", + ev.SessionID, targetPage.TargetID, targetPage.BrowserContextID, browserCtx.id, + ) + return false + } return true } From 8f75b40a02187dda3abfcab9aa95b5449df813e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Tue, 20 Feb 2024 13:24:31 +0300 Subject: [PATCH 08/19] Add Connection.stopWaitingForDebugger This method tells the browser to stop waiting for the attached target. Otherwise, the browser would indefinitely wait for the target that we don't want to be attached. This way, we prevent resource overuse. --- common/connection.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/common/connection.go b/common/connection.go index 8917cd88c..98655ea03 100644 --- a/common/connection.go +++ b/common/connection.go @@ -14,6 +14,7 @@ import ( "github.com/chromedp/cdproto" "github.com/chromedp/cdproto/cdp" + cdpruntime "github.com/chromedp/cdproto/runtime" "github.com/chromedp/cdproto/target" "github.com/dop251/goja" "github.com/gorilla/websocket" @@ -416,6 +417,25 @@ func (c *Connection) recvLoop() { } } +// stopWaitingForDebugger tells the browser to stop waiting for the +// debugger. Otherwise, the browser will wait for the debugger to +// attach indefinitely. +// +// We don't return an error because the browser might have already +// closed the connection. In that case, handling the error would +// be redundant. This operation is best-effort. +func (c *Connection) stopWaitingForDebugger(sid target.SessionID) { + msg := &cdproto.Message{ + ID: c.msgIDGen.newID(), + SessionID: sid, + Method: cdproto.MethodType(cdpruntime.CommandRunIfWaitingForDebugger), + } + err := c.send(c.ctx, msg, nil, nil) + if err != nil { + c.logger.Errorf("Connection:stopWaitingForDebugger", "sid:%v wsURL:%q, err:%v", sid, c.wsURL, err) + } +} + func (c *Connection) send(ctx context.Context, msg *cdproto.Message, recvCh chan *cdproto.Message, res easyjson.Unmarshaler) error { select { case c.sendCh <- msg: From 331085dc447c0278b5c1a0531c4730d66658d3a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Tue, 20 Feb 2024 13:25:59 +0300 Subject: [PATCH 09/19] Add force browser to stop waiting for debugger When Browser doesn't want a target to be attached, Connection tells the browser to stop waiting for debugger. This prevents the browser from indefinitely waiting for a target (that we don't want) to be attached. --- common/connection.go | 1 + 1 file changed, 1 insertion(+) diff --git a/common/connection.go b/common/connection.go index 98655ea03..351e43668 100644 --- a/common/connection.go +++ b/common/connection.go @@ -345,6 +345,7 @@ func (c *Connection) recvLoop() { // if a session should be created for the target. ok := c.onTargetAttachedToTarget(eva) if !ok { + c.stopWaitingForDebugger(sid) continue } } From fde1efbdd9d12773228b57a02c64c273fc3044a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Wed, 21 Feb 2024 10:40:02 +0300 Subject: [PATCH 10/19] Refactor Connection loops to a method This will be useful to register the onTargetAttachedToTarget event. See the next commit for the reason. --- common/connection.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/common/connection.go b/common/connection.go index 351e43668..3a2398de2 100644 --- a/common/connection.go +++ b/common/connection.go @@ -171,12 +171,20 @@ func NewConnection(ctx context.Context, wsURL string, logger *log.Logger) (*Conn sessions: make(map[target.SessionID]*Session), } - go c.recvLoop() - go c.sendLoop() + // starts the main control loop + // where the connection reads and writes messages + // in gorooutines. + c.start() return &c, nil } +// start starts the connection's main control loop. +func (c *Connection) start() { + go c.recvLoop() + go c.sendLoop() +} + func (c *Connection) close(code int) error { c.logger.Debugf("Connection:close", "code:%d", code) From 8478851c6fd22afc9740f38e0a2da69b4f83c42d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Wed, 21 Feb 2024 10:45:37 +0300 Subject: [PATCH 11/19] Fix a possible race condition in Connection NewConnection may start recv/send loop before the onTargetAttachedToTarget callback is attached. Extracting start() goroutine loops from NewConnection and calling it explicitly after registering the callback prevents this problem. --- common/browser.go | 3 +++ common/connection.go | 16 +++++++++++----- common/connection_test.go | 4 ++++ common/session_test.go | 1 + 4 files changed, 19 insertions(+), 5 deletions(-) diff --git a/common/browser.go b/common/browser.go index b8a6b5f61..be6798ffd 100644 --- a/common/browser.go +++ b/common/browser.go @@ -123,6 +123,9 @@ func (b *Browser) connect() error { b.conn = conn + // Start the connection to listen for CDP events. + conn.start() + // We don't need to lock this because `connect()` is called only in NewBrowser b.defaultContext, err = NewBrowserContext(b.ctx, b, "", NewBrowserContextOptions(), b.logger) if err != nil { diff --git a/common/connection.go b/common/connection.go index 3a2398de2..5f083ea56 100644 --- a/common/connection.go +++ b/common/connection.go @@ -136,10 +136,21 @@ type Connection struct { // onTargetAttachedToTarget is called when a new target is attached to the browser. // Returning false will prevent the session from being created. // If onTargetAttachedToTarget is nil, the session will be created. + // + // Register this only once, before the connection is used. + // It's not a constructor parameter to prevent polluting the constructor + // signature. Also, tests don't need to set this (or some other future code). + // + // Once the callback is registered, call start() to start the connection's + // main control loop where it will start reading and writing messages to the browser. onTargetAttachedToTarget func(*target.EventAttachedToTarget) bool } // NewConnection creates a new browser. +// +// Call start() to start the connection's main control loop +// where it will start reading and writing messages to the browser. +// Otherwise, the connection will not be able to send or receive messages. func NewConnection(ctx context.Context, wsURL string, logger *log.Logger) (*Connection, error) { var header http.Header var tlsConfig *tls.Config @@ -171,11 +182,6 @@ func NewConnection(ctx context.Context, wsURL string, logger *log.Logger) (*Conn sessions: make(map[target.SessionID]*Session), } - // starts the main control loop - // where the connection reads and writes messages - // in gorooutines. - c.start() - return &c, nil } diff --git a/common/connection_test.go b/common/connection_test.go index 6ad99c392..b6a7b419a 100644 --- a/common/connection_test.go +++ b/common/connection_test.go @@ -30,6 +30,7 @@ func TestConnection(t *testing.T) { url, _ := url.Parse(server.ServerHTTP.URL) wsURL := fmt.Sprintf("ws://%s/echo", url.Host) conn, err := NewConnection(ctx, wsURL, log.NewNullLogger()) + conn.start() conn.Close() require.NoError(t, err) @@ -48,6 +49,7 @@ func TestConnectionClosureAbnormal(t *testing.T) { url, _ := url.Parse(server.ServerHTTP.URL) wsURL := fmt.Sprintf("ws://%s/closure-abnormal", url.Host) conn, err := NewConnection(ctx, wsURL, log.NewNullLogger()) + conn.start() if assert.NoError(t, err) { action := target.SetDiscoverTargets(true) @@ -69,6 +71,7 @@ func TestConnectionSendRecv(t *testing.T) { url, _ := url.Parse(server.ServerHTTP.URL) wsURL := fmt.Sprintf("ws://%s/cdp", url.Host) conn, err := NewConnection(ctx, wsURL, log.NewNullLogger()) + conn.start() if assert.NoError(t, err) { action := target.SetDiscoverTargets(true) @@ -136,6 +139,7 @@ func TestConnectionCreateSession(t *testing.T) { url, _ := url.Parse(server.ServerHTTP.URL) wsURL := fmt.Sprintf("ws://%s/cdp", url.Host) conn, err := NewConnection(ctx, wsURL, log.NewNullLogger()) + conn.start() if assert.NoError(t, err) { session, err := conn.createSession(&target.Info{ diff --git a/common/session_test.go b/common/session_test.go index 14e64e241..f0a59f32e 100644 --- a/common/session_test.go +++ b/common/session_test.go @@ -91,6 +91,7 @@ func TestSessionCreateSession(t *testing.T) { conn, err := NewConnection(ctx, wsURL, log.NewNullLogger()) if assert.NoError(t, err) { + conn.start() session, err := conn.createSession(&target.Info{ Type: "page", TargetID: cdpTargetID, From 7fc4ae74c84d0cc14da950c8c885e444184d1655 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Wed, 21 Feb 2024 11:05:18 +0300 Subject: [PATCH 12/19] Fix Browser.getPages This method was returning +1 pages. This fix is necessary for the test in the following commit to work. --- common/browser_context.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/common/browser_context.go b/common/browser_context.go index 6a6c7fd8e..dc8ae216a 100644 --- a/common/browser_context.go +++ b/common/browser_context.go @@ -289,11 +289,7 @@ func (b *BrowserContext) NewPage() (*Page, error) { // Pages returns a list of pages inside this browser context. func (b *BrowserContext) Pages() []*Page { - pages := make([]*Page, 1) - for _, p := range b.browser.getPages() { - pages = append(pages, p) - } - return pages + return append([]*Page{}, b.browser.getPages()...) } // Route is not implemented. From 715ec068c95485d5ead374c0f24c30c6333556fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Wed, 21 Feb 2024 11:06:07 +0300 Subject: [PATCH 13/19] Add TestIsolateBrowserContexts This test ensures that we will no longer share pages across browser contexts. --- tests/browser_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/browser_test.go b/tests/browser_test.go index 9021f6f51..0fb42a0b0 100644 --- a/tests/browser_test.go +++ b/tests/browser_test.go @@ -378,3 +378,35 @@ func TestCloseContext(t *testing.T) { assert.Error(t, tb.CloseContext()) }) } + +func TestIsolateBrowserContexts(t *testing.T) { + tb := newTestBrowser(t) + + b1 := tb.Browser + b2, err := tb.browserType.Connect(tb.context(), tb.wsURL) + require.NoError(t, err) + t.Cleanup(b2.Close) + + bctx1, err := b1.NewContext(nil) + require.NoError(t, err) + bctx2, err := b2.NewContext(nil) + require.NoError(t, err) + + // both browser connections will receive onAttachedToTarget events. + // each Connection value should filter out events that are not related to + // the browser context it wasn't created from. + err = tb.run(tb.context(), func() error { + _, err := bctx1.NewPage() + return err + }, func() error { + _, err := bctx2.NewPage() + return err + }) + require.NoError(t, err) + + // assert.Len produces verbose output. so, use our own len. + bctx1PagesLen := len(bctx1.Pages()) + bctx2PagesLen := len(bctx2.Pages()) + assert.Equalf(t, 1, bctx1PagesLen, "browser context #1 should be attached to a single page, but got %d", bctx1PagesLen) + assert.Equalf(t, 1, bctx2PagesLen, "browser context #2 should be attached to a single page, but got %d", bctx2PagesLen) +} From 3ccee005826d62ecc514b23ca05ac2e09a370aa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 22 Feb 2024 09:44:55 +0300 Subject: [PATCH 14/19] Refactor Connection start and onTargetAttached - Removes start() - Moves onTargetAttachedToTarget to the constructor. - Moves implicit goroutine run to the constructor. - Updates tests to work with a nil hook. --- common/browser.go | 23 ++++++++++--------- common/connection.go | 48 ++++++++++++++++++--------------------- common/connection_test.go | 12 ++++------ common/session_test.go | 3 +-- 4 files changed, 39 insertions(+), 47 deletions(-) diff --git a/common/browser.go b/common/browser.go index be6798ffd..efec2a979 100644 --- a/common/browser.go +++ b/common/browser.go @@ -111,21 +111,22 @@ func newBrowser( func (b *Browser) connect() error { b.logger.Debugf("Browser:connect", "wsURL:%q", b.browserProc.WsURL()) - conn, err := NewConnection(b.ctx, b.browserProc.WsURL(), b.logger) + + // connectionOnAttachedToTarget hooks into the connection to listen + // for target attachment events. this way, browser can manage the + // decision of target attachments. so that we can stop connection + // from doing unnecessary work. + var err error + b.conn, err = NewConnection( + b.ctx, + b.browserProc.WsURL(), + b.logger, + b.connectionOnAttachedToTarget, + ) if err != nil { return fmt.Errorf("connecting to browser DevTools URL: %w", err) } - // hook into the connection to listen for target attachment events. - // this way, browser can manage the decision of target attachments. - // so that we can stop connection from doing unnecessary work. - conn.onTargetAttachedToTarget = b.connectionOnAttachedToTarget - - b.conn = conn - - // Start the connection to listen for CDP events. - conn.start() - // We don't need to lock this because `connect()` is called only in NewBrowser b.defaultContext, err = NewBrowserContext(b.ctx, b, "", NewBrowserContextOptions(), b.logger) if err != nil { diff --git a/common/connection.go b/common/connection.go index 5f083ea56..c86cf8461 100644 --- a/common/connection.go +++ b/common/connection.go @@ -140,18 +140,16 @@ type Connection struct { // Register this only once, before the connection is used. // It's not a constructor parameter to prevent polluting the constructor // signature. Also, tests don't need to set this (or some other future code). - // - // Once the callback is registered, call start() to start the connection's - // main control loop where it will start reading and writing messages to the browser. onTargetAttachedToTarget func(*target.EventAttachedToTarget) bool } // NewConnection creates a new browser. -// -// Call start() to start the connection's main control loop -// where it will start reading and writing messages to the browser. -// Otherwise, the connection will not be able to send or receive messages. -func NewConnection(ctx context.Context, wsURL string, logger *log.Logger) (*Connection, error) { +func NewConnection( + ctx context.Context, + wsURL string, + logger *log.Logger, + onTargetAttachedToTarget func(*target.EventAttachedToTarget) bool, +) (*Connection, error) { var header http.Header var tlsConfig *tls.Config wsd := websocket.Dialer{ @@ -167,28 +165,26 @@ func NewConnection(ctx context.Context, wsURL string, logger *log.Logger) (*Conn } c := Connection{ - BaseEventEmitter: NewBaseEventEmitter(ctx), - ctx: ctx, - wsURL: wsURL, - logger: logger, - conn: conn, - sendCh: make(chan *cdproto.Message, 32), // Avoid blocking in Execute - recvCh: make(chan *cdproto.Message), - closeCh: make(chan int), - errorCh: make(chan error), - done: make(chan struct{}), - closing: make(chan struct{}), - msgIDGen: &msgID{}, - sessions: make(map[target.SessionID]*Session), + BaseEventEmitter: NewBaseEventEmitter(ctx), + ctx: ctx, + wsURL: wsURL, + logger: logger, + conn: conn, + sendCh: make(chan *cdproto.Message, 32), // Avoid blocking in Execute + recvCh: make(chan *cdproto.Message), + closeCh: make(chan int), + errorCh: make(chan error), + done: make(chan struct{}), + closing: make(chan struct{}), + msgIDGen: &msgID{}, + sessions: make(map[target.SessionID]*Session), + onTargetAttachedToTarget: onTargetAttachedToTarget, } - return &c, nil -} - -// start starts the connection's main control loop. -func (c *Connection) start() { go c.recvLoop() go c.sendLoop() + + return &c, nil } func (c *Connection) close(code int) error { diff --git a/common/connection_test.go b/common/connection_test.go index b6a7b419a..d795c88d5 100644 --- a/common/connection_test.go +++ b/common/connection_test.go @@ -29,8 +29,7 @@ func TestConnection(t *testing.T) { ctx := context.Background() url, _ := url.Parse(server.ServerHTTP.URL) wsURL := fmt.Sprintf("ws://%s/echo", url.Host) - conn, err := NewConnection(ctx, wsURL, log.NewNullLogger()) - conn.start() + conn, err := NewConnection(ctx, wsURL, log.NewNullLogger(), nil) conn.Close() require.NoError(t, err) @@ -48,8 +47,7 @@ func TestConnectionClosureAbnormal(t *testing.T) { ctx := context.Background() url, _ := url.Parse(server.ServerHTTP.URL) wsURL := fmt.Sprintf("ws://%s/closure-abnormal", url.Host) - conn, err := NewConnection(ctx, wsURL, log.NewNullLogger()) - conn.start() + conn, err := NewConnection(ctx, wsURL, log.NewNullLogger(), nil) if assert.NoError(t, err) { action := target.SetDiscoverTargets(true) @@ -70,8 +68,7 @@ func TestConnectionSendRecv(t *testing.T) { ctx := context.Background() url, _ := url.Parse(server.ServerHTTP.URL) wsURL := fmt.Sprintf("ws://%s/cdp", url.Host) - conn, err := NewConnection(ctx, wsURL, log.NewNullLogger()) - conn.start() + conn, err := NewConnection(ctx, wsURL, log.NewNullLogger(), nil) if assert.NoError(t, err) { action := target.SetDiscoverTargets(true) @@ -138,8 +135,7 @@ func TestConnectionCreateSession(t *testing.T) { ctx := context.Background() url, _ := url.Parse(server.ServerHTTP.URL) wsURL := fmt.Sprintf("ws://%s/cdp", url.Host) - conn, err := NewConnection(ctx, wsURL, log.NewNullLogger()) - conn.start() + conn, err := NewConnection(ctx, wsURL, log.NewNullLogger(), nil) if assert.NoError(t, err) { session, err := conn.createSession(&target.Info{ diff --git a/common/session_test.go b/common/session_test.go index f0a59f32e..dd11909f0 100644 --- a/common/session_test.go +++ b/common/session_test.go @@ -88,10 +88,9 @@ func TestSessionCreateSession(t *testing.T) { ctx := context.Background() url, _ := url.Parse(server.ServerHTTP.URL) wsURL := fmt.Sprintf("ws://%s/cdp", url.Host) - conn, err := NewConnection(ctx, wsURL, log.NewNullLogger()) + conn, err := NewConnection(ctx, wsURL, log.NewNullLogger(), nil) if assert.NoError(t, err) { - conn.start() session, err := conn.createSession(&target.Info{ Type: "page", TargetID: cdpTargetID, From 8bb87798b78e5455edf23cb755fff23e40bf5b65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 22 Feb 2024 09:53:37 +0300 Subject: [PATCH 15/19] Move browser onAttached logic to a closure --- common/browser.go | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/common/browser.go b/common/browser.go index efec2a979..6bbc7469b 100644 --- a/common/browser.go +++ b/common/browser.go @@ -236,19 +236,18 @@ func (b *Browser) connectionOnAttachedToTarget(eva *target.EventAttachedToTarget // This allows to attach targets to the same browser context as the current // one, and to the default browser context. // - // We don't want to hold the lock for the entire function to prevent - // concurrency issues with the browser context being closed while we're - // checking it. So, we don't use defer. This is for a future code that might - // extend this function, add a defer, and cause this issue to be missed. - b.contextMu.RLock() - defaultContext := b.context == nil - if defaultContext || b.context.id == eva.TargetInfo.BrowserContextID { - b.contextMu.RUnlock() - return true - } - b.contextMu.RUnlock() - - return false + // We don't want to hold the lock for the entire function + // (connectionOnAttachedToTarget) run duration, because we want to avoid + // possible lock contention issues with the browser context being closed while + // we're waiting for it. So, we do the lock management in a function with its + // own defer. + isAllowedBrowserContext := func() bool { + b.contextMu.RLock() + defer b.contextMu.RUnlock() + return b.context == nil || b.context.id == eva.TargetInfo.BrowserContextID + } + + return isAllowedBrowserContext() } // onAttachedToTarget is called when a new page is attached to the browser. From 764400c092606bb2fecb19cc556f72da5f4ea40d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 22 Feb 2024 09:56:09 +0300 Subject: [PATCH 16/19] Update multiVU lock fix comments to clarify --- common/browser.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/common/browser.go b/common/browser.go index 6bbc7469b..9fad9d42e 100644 --- a/common/browser.go +++ b/common/browser.go @@ -229,10 +229,6 @@ func (b *Browser) initEvents() error { // connectionOnAttachedToTarget is called when Connection receives an attachedToTarget // event. Returning false will stop the event from being processed by the connection. func (b *Browser) connectionOnAttachedToTarget(eva *target.EventAttachedToTarget) bool { - // Allow connection to attach to target as the browser context is not set - // (hence it's the first one we have—the default), or the target is in the - // same browser context as the current one. - // // This allows to attach targets to the same browser context as the current // one, and to the default browser context. // @@ -338,7 +334,6 @@ func (b *Browser) isAttachedPageValid(ev *target.EventAttachedToTarget, browserC return false } // If the target is not in the same browser context as the current one, ignore it. - // We're not adding a log to prevent overhead. if browserCtx.id != targetPage.BrowserContextID { b.logger.Warnf( "Browser:isAttachedPageValid", "incorrect browser context sid:%v tid:%v bctxid:%v target bctxid:%v", From a6a8cb9d7736629c729277b9d0a12315a9ddebb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 22 Feb 2024 13:00:35 +0300 Subject: [PATCH 17/19] Reduce invalid browser context attachment log --- common/browser.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/browser.go b/common/browser.go index 9fad9d42e..e8680615f 100644 --- a/common/browser.go +++ b/common/browser.go @@ -335,7 +335,7 @@ func (b *Browser) isAttachedPageValid(ev *target.EventAttachedToTarget, browserC } // If the target is not in the same browser context as the current one, ignore it. if browserCtx.id != targetPage.BrowserContextID { - b.logger.Warnf( + b.logger.Debugf( "Browser:isAttachedPageValid", "incorrect browser context sid:%v tid:%v bctxid:%v target bctxid:%v", ev.SessionID, targetPage.TargetID, targetPage.BrowserContextID, browserCtx.id, ) From d646e4bf16799463ee97246de7b68ebf4cb07d71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 22 Feb 2024 14:16:31 +0300 Subject: [PATCH 18/19] Revise stopWaitingForDebugger comment --- common/connection.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/common/connection.go b/common/connection.go index c86cf8461..50da30a70 100644 --- a/common/connection.go +++ b/common/connection.go @@ -429,8 +429,14 @@ func (c *Connection) recvLoop() { } // stopWaitingForDebugger tells the browser to stop waiting for the -// debugger. Otherwise, the browser will wait for the debugger to -// attach indefinitely. +// debugger to attach to the page's session. +// +// Whether we're not sharing pages among browser contexts, Chromium +// still does so (since we're auto-attaching all browser targets). +// This means that if we don't stop waiting for the debugger, the +// browser will wait for the debugger to attach to the new page +// indefinitely, even if the page is not part of the browser context +// we're using. // // We don't return an error because the browser might have already // closed the connection. In that case, handling the error would From ac1c804ecbfc479e10c0340cc0bca0708554220b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 22 Feb 2024 14:19:57 +0300 Subject: [PATCH 19/19] Revise Connection hook comment --- common/connection.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/common/connection.go b/common/connection.go index 50da30a70..d8e1923c4 100644 --- a/common/connection.go +++ b/common/connection.go @@ -136,10 +136,6 @@ type Connection struct { // onTargetAttachedToTarget is called when a new target is attached to the browser. // Returning false will prevent the session from being created. // If onTargetAttachedToTarget is nil, the session will be created. - // - // Register this only once, before the connection is used. - // It's not a constructor parameter to prevent polluting the constructor - // signature. Also, tests don't need to set this (or some other future code). onTargetAttachedToTarget func(*target.EventAttachedToTarget) bool }