Skip to content

Commit

Permalink
fix!: emit browsingContext.contextDestroyed once (#2563)
Browse files Browse the repository at this point in the history
Rely on the new CDP event `Page.frameSubtreeWillBeDetached`, do not emit
extra `browsingContext.contextDestroyed`, only for the top-level frame
to be destroyed. Emit the event for nested iframes on document change.

---------

Co-authored-by: browser-automation-bot <133232582+browser-automation-bot@users.noreply.github.com>
  • Loading branch information
sadym-chromium and browser-automation-bot authored Sep 3, 2024
1 parent e8827d5 commit 930d401
Show file tree
Hide file tree
Showing 8 changed files with 43 additions and 38 deletions.
14 changes: 12 additions & 2 deletions src/bidiMapper/modules/cdp/CdpTargetManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ export class CdpTargetManager {
'Page.frameDetached',
this.#handleFrameDetachedEvent.bind(this)
);
cdpClient.on(
'Page.frameSubtreeWillBeDetached',
this.#handleFrameSubtreeWillBeDetached.bind(this)
);
}

#handleFrameAttachedEvent(params: Protocol.Page.FrameAttachedEvent) {
Expand Down Expand Up @@ -143,7 +147,13 @@ export class CdpTargetManager {
if (params.reason === 'swap') {
return;
}
this.#browsingContextStorage.findContext(params.frameId)?.dispose();
this.#browsingContextStorage.findContext(params.frameId)?.dispose(true);
}

#handleFrameSubtreeWillBeDetached(
params: Protocol.Page.FrameSubtreeWillBeDetachedEvent
) {
this.#browsingContextStorage.findContext(params.frameId)?.dispose(true);
}

#handleAttachedToTargetEvent(
Expand Down Expand Up @@ -327,7 +337,7 @@ export class CdpTargetManager {
const context =
this.#browsingContextStorage.findContextBySession(sessionId);
if (context) {
context.dispose();
context.dispose(true);
return;
}

Expand Down
34 changes: 19 additions & 15 deletions src/bidiMapper/modules/context/BrowsingContextImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,29 +211,31 @@ export class BrowsingContextImpl {
return this.#navigationId;
}

dispose() {
dispose(emitContextDestroyed: boolean) {
this.#deleteAllChildren();

this.#realmStorage.deleteRealms({
browsingContextId: this.id,
});

// Remove context from the parent.
// Delete context from the parent.
if (!this.isTopLevelContext()) {
this.parent!.#children.delete(this.id);
}

// Fail all ongoing navigations.
this.#failLifecycleIfNotFinished();

this.#eventManager.registerEvent(
{
type: 'event',
method: ChromiumBidi.BrowsingContext.EventNames.ContextDestroyed,
params: this.serializeToBidiValue(),
},
this.id
);
if (emitContextDestroyed) {
this.#eventManager.registerEvent(
{
type: 'event',
method: ChromiumBidi.BrowsingContext.EventNames.ContextDestroyed,
params: this.serializeToBidiValue(),
},
this.id
);
}
this.#browsingContextStorage.deleteContextById(this.id);
}

Expand Down Expand Up @@ -307,8 +309,8 @@ export class BrowsingContextImpl {
this.#children.add(childId);
}

#deleteAllChildren() {
this.directChildren.map((child) => child.dispose());
#deleteAllChildren(emitContextDestroyed: boolean = false) {
this.directChildren.map((child) => child.dispose(emitContextDestroyed));
}

get cdpTarget(): CdpTarget {
Expand Down Expand Up @@ -399,7 +401,7 @@ export class BrowsingContextImpl {

// At the point the page is initialized, all the nested iframes from the
// previous page are detached and realms are destroyed.
// Remove children from context.
// Delete children from context.
this.#deleteAllChildren();
});

Expand Down Expand Up @@ -749,8 +751,8 @@ export class BrowsingContextImpl {
}

#documentChanged(loaderId?: Protocol.Network.LoaderId) {
// Same document navigation.
if (loaderId === undefined || this.#loaderId === loaderId) {
// Same document navigation. Document didn't change.
if (this.#navigation.withinDocument.isFinished) {
this.#navigation.withinDocument = new Deferred();
} else {
Expand All @@ -762,9 +764,11 @@ export class BrowsingContextImpl {
return;
}

// Document changed.
this.#resetLifecycleIfFinished();

this.#loaderId = loaderId;
// Delete all child iframes and notify about top level destruction.
this.#deleteAllChildren(true);
}

#resetLifecycleIfFinished() {
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
[error.py]
[test_with_new_navigation_inside_page]
expected: FAIL
expected: [FAIL, PASS]

[test_with_new_navigation]
expected: [FAIL, PASS]

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
[error.py]
[test_with_new_navigation_inside_page]
expected: FAIL
expected: [FAIL, PASS]

[test_with_new_navigation]
expected: [FAIL, PASS]

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
[error.py]
[test_with_new_navigation_inside_page]
expected: FAIL
expected: [FAIL, PASS]

[test_with_new_navigation]
expected: [FAIL, PASS]

0 comments on commit 930d401

Please sign in to comment.