chore(bidi): create as Bidi session before browser.close #36482
+16
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note that the test is still failing as we are closing the transport concurrently: |
dgozman
approved these changes
Jun 27, 2025
This comment has been minimized.
This comment has been minimized.
|
Turns out that we have to wait for the session to be created before sending |
This comment has been minimized.
This comment has been minimized.
whimboo
reviewed
Jul 1, 2025
| transport.send({ method: 'session.new', params: { | ||
| capabilities: { | ||
| alwaysMatch: { | ||
| unhandledPromptBehavior: { default: 'ignore' }, |
There was a problem hiding this comment.
Is there a reason why you need to set this capability? browser.close is closing all tabs first before shutting down the browser by not opening any beforeunload prompt.
There was a problem hiding this comment.
It was just copied over from the other place. Omitting all the params didn't work, so I just preserved all. I can drop this option if it makes no difference.
The bigger problem still stands. In order to close the browser here, we must create a Bidi session first. This means that even in the situation when browser initialization failed for some reason, we still have to create the session. With the pipe channel in the bundled browsers we can just close the Playwright's end of the pipe and have the browser exit gracefully. It would be more reliable to have something similar configurable with WebSocket in Bidi, as it's the only supported transport in Bidi. E.g. when we launch the browser from Playwright, we already know that it is going to be its exclusive client and if the socket connection is terminated, the browser should exit or otherwise the process will be killed by a signal anyway. If there is a use case when the browser needs to support multiple Bidi connections (parallel or connect/disconnect/connect scenario) over the WebSocket, the behavior could be configured via a flag at the browser launch time. @whimboo is it something that Firefox would consider?
There was a problem hiding this comment.
@yury-s I would suggest to file a BiDi issue so that we can get this scenario discussed at the spec level. If it's a feature that everyone agrees with we could implement it in a cross-browser way. If not we could consider a special treatment in Firefox.
whimboo
approved these changes
Jul 1, 2025
Unfortunately, they are not (or they are racy on the bots), see this comment above. |
yury-s
force-pushed
the
bidi-graceful-shutdown
branch
from
f6e996b to
363d52c
Compare
Yeah, dropped them. |
This is a shutdown hang @aslushnikov was working around as well in the browser repository. I thought that this was fixed in Firefox but maybe it's not (or fully yet). |
|
I believe it is a different issue. We are closing transport concurrently with sending the commands in the test scenario, the following patch fixes it, but we need a better way to handle it: diff --git a/packages/playwright-core/src/server/browserType.ts b/packages/playwright-core/src/server/browserType.ts
index 554382b3a..11839575f 100644
--- a/packages/playwright-core/src/server/browserType.ts
+++ b/packages/playwright-core/src/server/browserType.ts
@@ -260,7 +260,7 @@ export abstract class BrowserType extends SdkObject {
const stdio = launchedProcess.stdio as unknown as [NodeJS.ReadableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.ReadableStream];
transport = new PipeTransport(stdio[3], stdio[4]);
}
- progress.cleanupWhenAborted(() => transport.close());
+ // progress.cleanupWhenAborted(() => transport.close());
return { browserProcess, artifactsDir: prepared.artifactsDir, userDataDir: prepared.userDataDir, transport };
}
diff --git a/packages/playwright-core/src/server/transport.ts b/packages/playwright-core/src/server/transport.ts
index c2fefad6d..d90161453 100644
--- a/packages/playwright-core/src/server/transport.ts
+++ b/packages/playwright-core/src/server/transport.ts
@@ -84,7 +84,7 @@ export class WebSocketTransport implements ConnectionTransport {
const logUrl = stripQueryParams(url);
progress?.log(`<ws connecting> ${logUrl}`);
const transport = new WebSocketTransport(progress, url, logUrl, { ...options, followRedirects: !!options.followRedirects && hadRedirects });
- progress?.cleanupWhenAborted(() => transport.closeAndWait());
+ // progress?.cleanupWhenAborted(() => transport.closeAndWait());
const resultPromise = new Promise<{ transport?: WebSocketTransport, redirect?: IncomingMessage }>((fulfill, reject) => {
transport._ws.on('open', async () => {
progress?.log(`<ws connected> ${logUrl}`); |
Test results for "tests 1"1 failed 8 flaky47066 passed, 979 skipped Merge workflow run. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.