Skip to content

Commit

Permalink
Fix BrowsingContext re-initialisation race condition (#629)
Browse files Browse the repository at this point in the history
* Rely on `Runtime.executionContextCreated` event
* Remove unnecessary async event handlers

---------

Co-authored-by: Maksim Sadym <sadym@google.com>
  • Loading branch information
sadym-chromium and Maksim Sadym committed Apr 20, 2023
1 parent 66cc68d commit 55680d5
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 26 deletions.
21 changes: 12 additions & 9 deletions src/bidiMapper/domains/context/browsingContextImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,8 @@ export class BrowsingContextImpl {
return this.#loaderId;
}

async delete() {
await this.#deleteChildren();
delete() {
this.#deleteChildren();

this.#realmStorage.deleteRealms({
browsingContextId: this.contextId,
Expand Down Expand Up @@ -180,8 +180,8 @@ export class BrowsingContextImpl {
this.#children.set(child.contextId, child);
}

async #deleteChildren() {
await Promise.all(this.children.map((child) => child.delete()));
#deleteChildren() {
this.children.map((child) => child.delete());
}

get #defaultRealm(): Realm {
Expand Down Expand Up @@ -272,7 +272,7 @@ export class BrowsingContextImpl {

this.#cdpTarget.cdpClient.on(
'Page.frameNavigated',
async (params: Protocol.Page.FrameNavigatedEvent) => {
(params: Protocol.Page.FrameNavigatedEvent) => {
if (this.contextId !== params.frame.id) {
return;
}
Expand All @@ -281,10 +281,7 @@ export class BrowsingContextImpl {
// At the point the page is initiated, all the nested iframes from the
// previous page are detached and realms are destroyed.
// Remove context's children.
await this.#deleteChildren();

// Remove all the already created realms.
this.#realmStorage.deleteRealms({browsingContextId: this.contextId});
this.#deleteChildren();
}
);

Expand Down Expand Up @@ -406,6 +403,12 @@ export class BrowsingContextImpl {
});
}
);

this.#cdpTarget.cdpClient.on('Runtime.executionContextsCleared', () => {
this.#realmStorage.deleteRealms({
cdpSessionId: this.#cdpTarget.cdpSessionId,
});
});
}

#getOrigin(params: Protocol.Runtime.ExecutionContextCreatedEvent) {
Expand Down
35 changes: 18 additions & 17 deletions src/bidiMapper/domains/context/browsingContextProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ export class BrowsingContextProcessor {
* for creating and destroying all targets and browsing contexts.
*/
#setEventListeners(cdpClient: CdpClient) {
cdpClient.on('Target.attachedToTarget', async (params) => {
await this.#handleAttachedToTargetEvent(params, cdpClient);
cdpClient.on('Target.attachedToTarget', (params) => {
this.#handleAttachedToTargetEvent(params, cdpClient);
});
cdpClient.on('Target.detachedFromTarget', async (params) => {
await this.#handleDetachedFromTargetEvent(params);
cdpClient.on('Target.detachedFromTarget', (params) => {
this.#handleDetachedFromTargetEvent(params);
});

cdpClient.on(
Expand All @@ -78,8 +78,8 @@ export class BrowsingContextProcessor {
);
cdpClient.on(
'Page.frameDetached',
async (params: Protocol.Page.FrameDetachedEvent) => {
await this.#handleFrameDetachedEvent(params);
(params: Protocol.Page.FrameDetachedEvent) => {
this.#handleFrameDetachedEvent(params);
}
);
}
Expand Down Expand Up @@ -109,12 +109,12 @@ export class BrowsingContextProcessor {
// "params": {
// "frameId": "0A639AB1D9A392DF2CE02C53CC4ED3A6",
// "reason": "swap" } }
async #handleFrameDetachedEvent(params: Protocol.Page.FrameDetachedEvent) {
#handleFrameDetachedEvent(params: Protocol.Page.FrameDetachedEvent) {
// In case of OOPiF no need in deleting BrowsingContext.
if (params.reason === 'swap') {
return;
}
await this.#browsingContextStorage.findContext(params.frameId)?.delete();
this.#browsingContextStorage.findContext(params.frameId)?.delete();
}

// { "method": "Target.attachedToTarget",
Expand All @@ -129,7 +129,7 @@ export class BrowsingContextProcessor {
// "canAccessOpener": false,
// "browserContextId": "1B5244080EC3FF28D03BBDA73138C0E2" },
// "waitingForDebugger": false } }
async #handleAttachedToTargetEvent(
#handleAttachedToTargetEvent(
params: Protocol.Target.AttachedToTargetEvent,
parentSessionCdpClient: CdpClient
) {
Expand All @@ -138,12 +138,13 @@ export class BrowsingContextProcessor {
const targetCdpClient = this.#cdpConnection.getCdpClient(sessionId);

if (!this.#isValidTarget(targetInfo)) {
// DevTools or some other not supported by BiDi target.
await targetCdpClient.sendCommand('Runtime.runIfWaitingForDebugger');
await parentSessionCdpClient.sendCommand(
'Target.detachFromTarget',
params
);
// DevTools or some other not supported by BiDi target. Just release
// debugger and ignore them.
void targetCdpClient
.sendCommand('Runtime.runIfWaitingForDebugger')
.then(() =>
parentSessionCdpClient.sendCommand('Target.detachFromTarget', params)
);
return;
}

Expand Down Expand Up @@ -185,14 +186,14 @@ export class BrowsingContextProcessor {
// "params": {
// "sessionId": "7EFBFB2A4942A8989B3EADC561BC46E9",
// "targetId": "19416886405CBA4E03DBB59FA67FF4E8" } }
async #handleDetachedFromTargetEvent(
#handleDetachedFromTargetEvent(
params: Protocol.Target.DetachedFromTargetEvent
) {
// TODO: params.targetId is deprecated. Update this class to track using
// params.sessionId instead.
// https://github.com/GoogleChromeLabs/chromium-bidi/issues/60
const contextId = params.targetId!;
await this.#browsingContextStorage.findContext(contextId)?.delete();
this.#browsingContextStorage.findContext(contextId)?.delete();
}

async #getRealm(target: Script.Target): Promise<Realm> {
Expand Down

0 comments on commit 55680d5

Please sign in to comment.