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.

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

fix: fail previous navigation on the next one #2569

Merged
merged 6 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
68 changes: 49 additions & 19 deletions src/bidiMapper/modules/context/BrowsingContextImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ export class BrowsingContextImpl {
// `None`, the command result should have `navigation` value, but mapper does not have
// it yet. This value will be set to `navigationId` after next .
#pendingNavigationId: string | undefined;
// Set if there is a pending navigation initiated by `BrowsingContext.navigate` command.
// The promise is resolved when the navigation is finished or rejected when canceled.
#pendingCommandNavigation: Deferred<void> | undefined;

#originalOpener?: string;

Expand Down Expand Up @@ -212,6 +215,9 @@ export class BrowsingContextImpl {
}

dispose(emitContextDestroyed: boolean) {
this.#pendingCommandNavigation?.reject(
new UnknownErrorException('navigation canceled by context disposal')
);
this.#deleteAllChildren();

this.#realmStorage.deleteRealms({
Expand Down Expand Up @@ -468,6 +474,12 @@ export class BrowsingContextImpl {
if (this.id !== params.frameId) {
return;
}
// If there is a pending navigation, reject it.
this.#pendingCommandNavigation?.reject(
new UnknownErrorException(
`navigation canceled, as new navigation is requested by ${params.reason}`
)
);
this.#pendingNavigationUrl = params.url;
});

Expand Down Expand Up @@ -815,16 +827,19 @@ export class BrowsingContextImpl {
throw new InvalidArgumentException(`Invalid URL: ${url}`);
}

this.#pendingCommandNavigation?.reject(
new UnknownErrorException('navigation canceled by concurrent navigation')
);
await this.targetUnblockedOrThrow();

// Set the pending navigation URL to provide it in `browsingContext.navigationStarted`
// event.
// TODO: detect navigation start not from CDP. Check if
// `Page.frameRequestedNavigation` can be used for this purpose.
this.#pendingNavigationUrl = url;

const navigationId = uuidv4();
this.#pendingNavigationId = navigationId;
this.#pendingCommandNavigation = new Deferred<void>();

// Navigate and wait for the result. If the navigation fails, the error event is
// emitted and the promise is rejected.
Expand Down Expand Up @@ -863,6 +878,9 @@ export class BrowsingContextImpl {

if (wait === BrowsingContext.ReadinessState.None) {
// Do not wait for the result of the navigation promise.
this.#pendingCommandNavigation.resolve();
this.#pendingCommandNavigation = undefined;

return {
navigation: navigationId,
url,
Expand All @@ -871,32 +889,44 @@ export class BrowsingContextImpl {

const cdpNavigateResult = await cdpNavigatePromise;

switch (wait) {
case BrowsingContext.ReadinessState.Interactive:
// No `loaderId` means same-document navigation.
if (cdpNavigateResult.loaderId === undefined) {
await this.#navigation.withinDocument;
} else {
await this.#lifecycle.DOMContentLoaded;
}
break;
case BrowsingContext.ReadinessState.Complete:
// No `loaderId` means same-document navigation.
if (cdpNavigateResult.loaderId === undefined) {
await this.#navigation.withinDocument;
} else {
await this.#lifecycle.load;
}
break;
}
// Wait for either the navigation is finished or canceled by another navigation.
await Promise.race([
// No `loaderId` means same-document navigation.
this.#waitNavigation(wait, cdpNavigateResult.loaderId === undefined),
// Throw an error if the navigation is canceled.
this.#pendingCommandNavigation,
]);

this.#pendingCommandNavigation.resolve();
this.#pendingCommandNavigation = undefined;
return {
navigation: navigationId,
// Url can change due to redirect get the latest one.
url: this.#url,
};
}

async #waitNavigation(
wait: BrowsingContext.ReadinessState,
withinDocument: boolean
) {
if (withinDocument) {
await this.#navigation.withinDocument;
return;
}
switch (wait) {
case BrowsingContext.ReadinessState.None:
return;
case BrowsingContext.ReadinessState.Interactive:
await this.#lifecycle.DOMContentLoaded;
return;
case BrowsingContext.ReadinessState.Complete:
await this.#lifecycle.load;
return;
}
}

// TODO: support concurrent navigations analogous to `navigate`.
async reload(
ignoreCache: boolean,
wait: BrowsingContext.ReadinessState
Expand Down
2 changes: 1 addition & 1 deletion tests/browsing_context/test_navigate.py
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ async def test_browsingContext_navigationStarted_browsingContextClosedBeforeNavi
'id': navigate_command_id,
'type': 'error',
'error': 'unknown error',
'message': 'navigation canceled',
'message': 'navigation canceled by context disposal',
})

assert close_command_result == AnyExtending({
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

Loading