Skip to content

Commit

Permalink
fix(chromium): detach from shared workers
Browse files Browse the repository at this point in the history
This prevents shared workers from stalling upon restart.

We receive `Inspector.targetCrashed` and `Inspector.targetReloadedAfterCrash`
events that assume `Runtime.runIfWaitingForDebugger` from any
attached client. It is easier and more stable to just detach
from shared workers, because we do not inspect them.

For service workers, we should actually issue
`Runtime.runIfWaitingForDebugger` in such cases, because
we attach to them.
  • Loading branch information
dgozman committed Nov 22, 2022
1 parent b5d7566 commit f97730d
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 0 deletions.
6 changes: 6 additions & 0 deletions packages/playwright-core/src/server/chromium/crBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,12 @@ export class CRBrowser extends Browser {
context.emit(CRBrowserContext.CREvents.ServiceWorker, serviceWorker);
return;
}

// Detach from any targets we are not interested in.
// Ideally, detaching should resume any target, but there is a bug in the backend.
session._sendMayFail('Runtime.runIfWaitingForDebugger').then(() => {
this._session._sendMayFail('Target.detachFromTarget', { sessionId });
});
}

_onDetachedFromTarget(payload: Protocol.Target.detachFromTargetParameters) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ export class CRServiceWorker extends Worker {

session.send('Runtime.enable', {}).catch(e => { });
session.send('Runtime.runIfWaitingForDebugger').catch(e => { });
session.on('Inspector.targetReloadedAfterCrash', () => {
// Resume service worker after restart.
session._sendMayFail('Runtime.runIfWaitingForDebugger', {});
});
}

async updateOffline(initial: boolean): Promise<void> {
Expand Down
7 changes: 7 additions & 0 deletions tests/assets/shared-worker/shared-worker.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<script>
window.sharedWorkerResponsePromise = new Promise(f => {
window.myWorker = new SharedWorker("shared-worker.js");
window.myWorker.port.postMessage('hello');
window.myWorker.port.onmessage = e => f(e.data);
});
</script>
4 changes: 4 additions & 0 deletions tests/assets/shared-worker/shared-worker.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
onconnect = event => {
const port = event.ports[0];
port.onmessage = e => port.postMessage('echo:' + e.data);
};
29 changes: 29 additions & 0 deletions tests/library/shared-worker.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* Copyright (c) Microsoft Corporation.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { contextTest as test, expect } from '../config/browserTest';

test('should survive shared worker restart', async ({ context, server }) => {
const page1 = await context.newPage();
await page1.goto(server.PREFIX + '/shared-worker/shared-worker.html');
expect(await page1.evaluate('window.sharedWorkerResponsePromise')).toBe('echo:hello');
await page1.close();

const page2 = await context.newPage();
await page2.goto(server.PREFIX + '/shared-worker/shared-worker.html');
expect(await page2.evaluate('window.sharedWorkerResponsePromise')).toBe('echo:hello');
await page2.close();
});

0 comments on commit f97730d

Please sign in to comment.