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

test(bidi): Update beforeunload tests to avoid hangs with bidi #34281

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

test(bidi): Update beforeunload tests to avoid hangs with bidi #34281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac11624
chore(bidi): Avoid errors if url is unavailable in navigationStarted …
juliandescottes Jan 10, 2025
8778e02
chore(bidi): Do not wait for browsingContext.close if runBeforeUnload…
juliandescottes Jan 10, 2025
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
15 changes: 14 additions & 1 deletion packages/playwright-core/src/server/bidi/bidiPage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,12 @@ export class BidiPage implements PageDelegate {
const frameId = params.context;
this._page._frameManager.frameRequestedNavigation(frameId, params.navigation!);

// url is missing from navigationStarted events on Firefox when the
// navigation is interrupted by a beforeunload prompt.
// See https://bugzilla.mozilla.org/show_bug.cgi?id=1908952
if (!params.url)
Copy link
Member

Choose a reason for hiding this comment

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

This would just mask the problem and potentially other issues elsewhere. Let's revert this and keep the test failing (it stops timing out with the other fix in this PR, right?). Our primary objective is to ensure the logic functions correctly when the protocol implementation provides the property, rather than working around the current issues in the Bidi protocol.

return;

const url = params.url.toLowerCase();
if (url.startsWith('file:') || url.startsWith('data:') || url === 'about:blank') {
// Navigation to file urls doesn't emit network events, so we fire 'commit' event right when navigation is started.
Expand Down Expand Up @@ -384,10 +390,17 @@ export class BidiPage implements PageDelegate {
}

async closePage(runBeforeUnload: boolean): Promise<void> {
await this._session.send('browsingContext.close', {
const onClose = this._session.send('browsingContext.close', {
context: this._session.sessionId,
promptUnload: runBeforeUnload,
});

// Only wait for the browsingContext to close if runBeforeUnload is false.
// Otherwise a before unload prompt might be displayed and should be handled
// by the caller.
// See NOTE on https://playwright.dev/docs/api/class-page#page-close
if (!runBeforeUnload)
Copy link
Member

Choose a reason for hiding this comment

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

Need to catch potential exceptions if not awaiting to avoid unhandled promise rejection.

await onClose;
}

async setBackgroundColor(color?: { r: number; g: number; b: number; a: number; }): Promise<void> {
Expand Down
2 changes: 0 additions & 2 deletions tests/bidi/expectations/bidi-chromium-library.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
library/beforeunload.spec.ts › should access page after beforeunload [timeout]
library/beforeunload.spec.ts › should run beforeunload if asked for @smoke [timeout]
library/browsercontext-events.spec.ts › console event should work in immediately closed popup [timeout]
library/browsercontext-events.spec.ts › console event should work in popup 2 [timeout]
library/browsercontext-events.spec.ts › dialog event should work in immediately closed popup [timeout]
Expand Down
Loading