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

Jump to bottom

fix: handle missing detach events for restored bfcache targets #10967

Merged
merged 1 commit into from
Sep 21, 2023
Merged

fix: handle missing detach events for restored bfcache targets #10967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 4 additions & 8 deletions packages/puppeteer-core/src/cdp/ChromeTargetManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {debugError} from '../common/util.js';
import {assert} from '../util/assert.js';
import {Deferred} from '../util/Deferred.js';

import {type CdpCDPSession} from './CDPSession.js';
import {type Connection} from './Connection.js';
import {CdpTarget, InitializationStatus} from './Target.js';
import {
Expand Down Expand Up @@ -358,10 +359,7 @@ export class ChromeTargetManager
// `this.#connection.isAutoAttached(targetInfo.targetId)`. In the future, we
// should determine if a target is auto-attached or not with the help of
// CDP.
if (
targetInfo.type === 'service_worker' &&
this.#connection.isAutoAttached(targetInfo.targetId)
) {
if (targetInfo.type === 'service_worker') {
Copy link
Collaborator Author

@OrKoN OrKoN Sep 20, 2023 •
edited

Choose a reason for hiding this comment

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

unrelated change: this condition is redundant because of the check on line 351 (improved for readability)

this.#finishInitializationIfReady(targetInfo.targetId);
await silentDetach();
if (this.#attachedTargetsByTargetId.has(targetInfo.targetId)) {
Expand Down Expand Up @@ -393,18 +391,16 @@ export class ChromeTargetManager
return;
}

if (!isExistingTarget) {
target._initialize();
}

this.#setupAttachmentListeners(session);

if (isExistingTarget) {
(session as CdpCDPSession)._setTarget(target);
this.#attachedTargetsBySessionId.set(
session.id(),
this.#attachedTargetsByTargetId.get(targetInfo.targetId)!
);
} else {
target._initialize();
this.#attachedTargetsByTargetId.set(targetInfo.targetId, target);
this.#attachedTargetsBySessionId.set(session.id(), target);
}
Expand Down
6 changes: 6 additions & 0 deletions test/TestExpectations.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,6 +1397,12 @@
"parameters": ["chrome", "webDriverBiDi"],
"expectations": ["PASS"]
},
{
"testIdPattern": "[bfcache.spec] BFCache can navigate to a BFCached page containing an OOPIF and a worker",
"platforms": ["darwin", "linux", "win32"],
"parameters": ["cdp", "firefox"],
"expectations": ["SKIP"]
},
{
"testIdPattern": "[browser.spec] Browser specs Browser.isConnected should set the browser connected state",
"platforms": ["darwin", "linux", "win32"],
Expand Down
11 changes: 11 additions & 0 deletions test/assets/cached/bfcache/worker-iframe-container.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<body>BFCached<a href="target.html">next</a></body>
<script>
window.addEventListener('DOMContentLoaded', () => {
const iframe = document.createElement('iframe');
const url = new URL(location.href);
url.hostname = url.hostname === 'localhost' ? '127.0.0.1' : 'localhost';
url.pathname = '/cached/bfcache/worker-iframe.html';
iframe.src = url.toString();
document.body.appendChild(iframe);
}, false);
</script>
3 changes: 3 additions & 0 deletions test/assets/cached/bfcache/worker-iframe.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<script>
const worker = new Worker('worker.mjs', {type: 'module'})
</script>
1 change: 1 addition & 0 deletions test/assets/cached/bfcache/worker.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('HELLO');
30 changes: 29 additions & 1 deletion test/src/bfcache.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
*/

import expect from 'expect';
import {PageEvent} from 'puppeteer-core';

import {launch} from './mocha-utils.js';
import {waitEvent} from './utils.js';

describe('BFCache', function () {
it('can navigate to a BFCached page', async () => {
Expand All @@ -25,7 +27,7 @@ describe('BFCache', function () {
});

try {
page.setDefaultTimeout(2000);
page.setDefaultTimeout(3000);

await page.goto(httpsServer.PREFIX + '/cached/bfcache/index.html');

Expand All @@ -44,4 +46,30 @@ describe('BFCache', function () {
await close();
}
});

it('can navigate to a BFCached page containing an OOPIF and a worker', async () => {
const {httpsServer, page, close} = await launch({
ignoreHTTPSErrors: true,
});
try {
page.setDefaultTimeout(3000);
const [worker1] = await Promise.all([
waitEvent(page, PageEvent.WorkerCreated),
page.goto(
httpsServer.PREFIX + '/cached/bfcache/worker-iframe-container.html'
),
]);
expect(await worker1.evaluate('1 + 1')).toBe(2);
await Promise.all([page.waitForNavigation(), page.locator('a').click()]);

const [worker2] = await Promise.all([
waitEvent(page, PageEvent.WorkerCreated),
page.waitForNavigation(),
page.goBack(),
]);
expect(await worker2.evaluate('1 + 1')).toBe(2);
} finally {
await close();
}
});
});