fix(chromium): fix a race in persistent context launch #1702
Conversation
| @@ -50,24 +50,35 @@ export class CRBrowser extends BrowserBase { | |||
| const connection = new CRConnection(SlowMoTransport.wrap(transport, slowMo)); | |||
| const browser = new CRBrowser(connection); | |||
| const session = connection.rootSession; | |||
| const promises = [ | |||
| session.send('Target.setDiscoverTargets', { discover: true }), | |||
There was a problem hiding this comment.
We don't use it except for initial target discovery (before auto attach is on).
| @@ -50,24 +50,35 @@ export class CRBrowser extends BrowserBase { | |||
| const connection = new CRConnection(SlowMoTransport.wrap(transport, slowMo)); | |||
| const browser = new CRBrowser(connection); | |||
| const session = connection.rootSession; | |||
| const promises = [ | |||
| session.send('Target.setDiscoverTargets', { discover: true }), | |||
There was a problem hiding this comment.
We don't use it except for initial target discovery (before auto attach is on).
src/chromium/crBrowser.ts
Outdated
| // First page and background pages in the persistent context are created automatically | ||
| // and may be initialized before we enable auto-attach. | ||
| function attachToExistingPage({targetInfo}: Protocol.Target.targetCreatedPayload) { | ||
| if (targetInfo.type !== 'page' && targetInfo.type !== 'background_page') |
There was a problem hiding this comment.
Unrelated to thus PR but this should also include service workers I believe.
There was a problem hiding this comment.
Good point, I'll fix it right away.
|
|
||
| const startDiscover = session.send('Target.setDiscoverTargets', { discover: true }); | ||
| const autoAttachAndStopDiscover = session.send('Target.setAutoAttach', { autoAttach: true, waitForDebuggerOnStart: true, flatten: true }).then(() => { | ||
| // All targets collected before setAutoAttach response will not be auto-attached, the rest will be. |
There was a problem hiding this comment.
Can you help me understand how it can happen? Does it imply that between handling Target.setAutoAttach and Target.setDiscoverTargets (sent synchronously one after another ) the browser may create new targets?
There was a problem hiding this comment.
Yes, we send them synchronously, but they arrive to the browser with arbitrary delay.
| session.on('Target.targetCreated', attachToExistingPage); | ||
|
|
||
| const startDiscover = session.send('Target.setDiscoverTargets', { discover: true }); | ||
| const autoAttachAndStopDiscover = session.send('Target.setAutoAttach', { autoAttach: true, waitForDebuggerOnStart: true, flatten: true }).then(() => { |
There was a problem hiding this comment.
we again have to rely on the fact that no other messages will be handled before the response promise :(
There was a problem hiding this comment.
Well, that's our contract as far as I understand.
There was a problem hiding this comment.
I'd say this is an implementation detail which I'd rather not rely upon if possible. But this is not the only place though so it's fine. Alternatively, we could attach to all discovered targets after disabling discovery mode but preliminary checking if we haven't attached to it yet.
We should stop attaching to existing targets immediately after Target.setAutoAttach response arrives, otherwise we have a window for double attach.
We should stop attaching to existing targets immediately after Target.setAutoAttach response arrives, otherwise we have a window for double attach.