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

[juliandescottes]

The WebDriver BiDi browsingContext.close command will only resolve when the context closes.
Some beforeunload tests are awaiting on browsingContext.close before dismissing the beforeunload prompt and therefore will deadlock and timeout.

[juliandescottes]

Note that on Firefox + BiDi the tests also fail at the moment because the URL is missing from the corresponding navigationStarted event, which is tracked at https://bugzilla.mozilla.org/show_bug.cgi?id=1908952

We should workaround this by modifying the _onNavigationStarted handler on playwright's side, but this can be handled in a different PR.

playwright/packages/playwright-core/src/server/bidi/bidiPage.ts

Lines 183 to 190 in 13bdd3c

	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.
	// Doing it in domcontentload would be too late as we'd clear frame tree.
	const frame = this._page._frameManager.frame(frameId)!;
	if (frame)
	this._page._frameManager.frameCommittedNewDocumentNavigation(frameId, params.url, '', params.navigation!, /* initial */ false);
	}

[yury-s]

According to the documentation, "If runBeforeUnload is true the method will run unload handlers, but will not wait for the page to close.". So this behavior seems to be intentional. Also the first test ('should run beforeunload if asked for @smoke') passes when I run it locally. I'd rather try to align Bidi behavior with the existing implementation or at least understand why it is different.

[juliandescottes]

According to the documentation, "If runBeforeUnload is true the method will run unload handlers, but will not wait for the page to close.".

Thanks for taking a look!
On BiDi's side browsingContext.close will only return once the prompt has been handled. I guess we could special case this in bidiPage.ts' closePage. In parallel we can also discuss it on the spec level to see if it should change directly in BiDi.

Also the first test ('should run beforeunload if asked for @smoke') passes when I run it locally.

Did you remove it from the timeout expectations? For me it fails first because of the _onNavigationStarted issue I mentioned.

And if I fix that, then it hangs.

[yury-s]

On BiDi's side browsingContext.close will only return once the prompt has been handled. I guess we could special case this in bidiPage.ts' closePage. In parallel we can also discuss it on the spec level to see if it should change directly in BiDi.

Yeah, blocking browsingContext.close on before unload dialog which may never be handled sounds wrong to me, not sure why it is done that way. If the dialog is dismissed and the page stays open, will the method not return or what is the point? I tried the following workaround locally and got the the same exception in params.url.toLowerCase(); call as you mentioned:

diff --git a/packages/playwright-core/src/server/bidi/bidiPage.ts b/packages/playwright-core/src/server/bidi/bidiPage.ts
index cf0662738..61d5dd20b 100644
--- a/packages/playwright-core/src/server/bidi/bidiPage.ts
+++ b/packages/playwright-core/src/server/bidi/bidiPage.ts
@@ -384,10 +384,12 @@ export class BidiPage implements PageDelegate {
   }
 
   async closePage(runBeforeUnload: boolean): Promise<void> {
-    await this._session.send('browsingContext.close', {
+    const promise = this._session.send('browsingContext.close', {
       context: this._session.sessionId,
       promptUnload: runBeforeUnload,
     });
+    if (!runBeforeUnload)
+      await promise;
   }
 
   async setBackgroundColor(color?: { r: number; g: number; b: number; a: number; }): Promise<void> {

Did you remove it from the timeout expectations? For me it fails first because of the _onNavigationStarted issue I mentioned.

I'm running without PWTEST_USE_BIDI_EXPECTATIONS so the expectations should be ignored, it just passes. I'm running on Mac.

Update: fails on Linux:

    TypeError: Cannot read properties of undefined (reading 'toLowerCase')

       at ../packages/playwright-core/src/server/bidi/bidiPage.ts:183

      181 |     this._page._frameManager.frameRequestedNavigation(frameId, params.navigation!);
      182 |
    > 183 |     const url = params.url.toLowerCase();
          |                            ^
      184 |     if (url.startsWith('file:') || url.startsWith('data:') || url === 'about:blank') {
      185 |       // Navigation to file urls doesn't emit network events, so we fire 'commit' event right when navigation is started.
      186 |       // Doing it in domcontentload would be too late as we'd clear frame tree.
        at BidiPage._onNavigationStarted (/home/pwuser/playwright/packages/playwright-core/src/server/bidi/bidiPage.ts:183:28)
        at BidiSession.emit (node:events:517:28)
        at /home/pwuser/playwright/packages/playwright-core/src/server/bidi/bidiConnection.ts:227:41

FYI, you can run with PWDEBUGIMPL=1 to see full stack trace where the error was thrown.

[juliandescottes]

If the dialog is dismissed and the page stays open, will the method not return or what is the point?

I will open an issue on the BiDi spec to discuss this, but having a consistent behavior for the command (ie always waiting for the tab to close) might be an argument in favor of the current approach. If consumers want to cancel the navigation, arguably they can just not wait on the command.

I tried the following workaround locally and got the the same exception in params.url.toLowerCase(); call as you mentioned:

Not sure what happens. I am on mac as well. In my case, I always get first the error with params.url, so it needs to be fixed to get the timeout. I will update the PR with fixes for both issues, I can't really see how the current test would not timeout with BiDi (once the params.url issue is fixed, that is).

So to be clear, running the should run beforeunload if asked for @smoke test today with Firefox + BiDi:

• first fails early because of the params.url issue
• then times out if the previous issue is fixed

[yury-s]

To summarize, I think the test failures highlight two issues in Bidi:

1. Spec: browsingContext.close should not block when promptUnload: true.
2. browsingContext.navigationStarted should have NavigationInfo.url properly populated.

If those issues are resolved, both tests should start passing.

[yury-s]

Sorry, I posted my response before seeing yours, let me see what happens if url problem is resolved.

[juliandescottes]

Filed the spec issue, I'll leave it up to you if you prefer to wait for a decision there. It feels to me like both approaches can make sense, so it seems reasonable to handle it on the Playwright side, but that's not for me to decide :)

About the current bug for the missing url in navigationStarted, no strong opinion either. On the Firefox side it's challenging to return the actual URL but we should probably at least return a string here.

[yury-s]

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.

[yury-s]

Filed the spec issue, I'll leave it up to you if you prefer to wait for a decision there. It feels to me like both approaches can make sense, so it seems reasonable to handle it on the Playwright side, but that's not for me to decide :)

Thanks! I'd rather have the problem resolved in the spec. The problem with the workaround on playwright end is that we have to also catch potential errors in this._session.send('browsingContext.close',, which may occur due to other issues (e.g. remote browser disconnects). If we simply do this._session.send('browsingContext.close', {...}).catch(e => {}); we won't know if the command even reached the browser, which is suboptimal.

[yury-s]

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

[github-actions]

Test results for "tests 1"

6 flaky

⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsercontext-clearcookies.spec.ts:116:3 › should remove cookies by path @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/proxy.spec.ts:93:11 › should proxy local network requests › by default › localhost @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/proxy.spec.ts:93:11 › should proxy local network requests › with other bypasses › loopback address @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-set-input-files.spec.ts:205:3 › should upload multiple large files @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

37569 passed, 648 skipped
✔️✔️✔️

Merge workflow run.