Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(chromium): fix a race in persistent context launch #1702

Merged
merged 1 commit into from
Apr 8, 2020
Merged

fix(chromium): fix a race in persistent context launch #1702

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 29 additions & 18 deletions src/chromium/crBrowser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,25 +50,36 @@ 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 }),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did this go?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use it except for initial target discovery (before auto attach is on).

session.send('Target.setAutoAttach', { autoAttach: true, waitForDebuggerOnStart: true, flatten: true }),
session.send('Target.setDiscoverTargets', { discover: false }),
];
const existingPageAttachPromises: Promise<any>[] = [];
if (isPersistent) {
// 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')
return;
existingPageAttachPromises.push(session.send('Target.attachToTarget', {targetId: targetInfo.targetId, flatten: true}));
}
session.on('Target.targetCreated', attachToExistingPage);
Promise.all(promises).then(() => session.off('Target.targetCreated', attachToExistingPage)).catch(debugError);
if (!isPersistent) {
await session.send('Target.setAutoAttach', { autoAttach: true, waitForDebuggerOnStart: true, flatten: true });
return browser;
}
await Promise.all(promises);
await Promise.all(existingPageAttachPromises);

const existingTargetAttachPromises: Promise<any>[] = [];
// First page, background pages and their service workers 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' && targetInfo.type !== 'service_worker')
return;
// TODO: should we handle the error during 'Target.attachToTarget'? Can the target disappear?
existingTargetAttachPromises.push(session.send('Target.attachToTarget', {targetId: targetInfo.targetId, flatten: true}));
}
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(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we again have to rely on the fact that no other messages will be handled before the response promise :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's our contract as far as I understand.

Copy link
Member

@yury-s yury-s Apr 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// All targets collected before setAutoAttach response will not be auto-attached, the rest will be.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we send them synchronously, but they arrive to the browser with arbitrary delay.

// TODO: We should fix this upstream and remove this tricky logic.
session.off('Target.targetCreated', attachToExistingPage);
return session.send('Target.setDiscoverTargets', { discover: false });
});
await Promise.all([
startDiscover,
autoAttachAndStopDiscover,
]);

// Wait for initial targets to arrive.
await Promise.all(existingTargetAttachPromises);
return browser;
}

Expand Down