Filip/playwright
1
0
Fork 0
Code Issues Pull requests Actions Packages Projects Releases Wiki Activity

fix(oopif): race between detachedFromTarget and frameAttached (#2419)

Browse source 
During remote -> local transition, these two events come in unpredictable order, so we try to handle both cases. Also, remote frame detach was not handled at all.
This commit is contained in:
Dmitry Gozman 2020-06-01 13:47:02 -07:00 committed by GitHub
parent de0bbd3031
commit 454411062b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 51 additions and 27 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

66
src/chromium/crPage.ts
View file

@ -89,7 +89,7 @@ export class CRPage implements PageDelegate {
await Promise.all(Array.from(this._sessions.values()).map(frame => cb(frame)));
}
_sessionForFrame(frame: frames.Frame): FrameSession {
private _sessionForFrame(frame: frames.Frame): FrameSession {
// Frame id equals target id.
while (!this._sessions.has(frame._id)) {
const parent = frame.parentFrame();
@ -105,29 +105,6 @@ export class CRPage implements PageDelegate {
return this._sessionForFrame(frame);
}
addFrameSession(targetId: Protocol.Target.TargetID, session: CRSession) {
// Frame id equals target id.
const frame = this._page._frameManager.frame(targetId);
assert(frame);
const parentSession = this._sessionForFrame(frame);
this._page._frameManager.removeChildFramesRecursively(frame);
const frameSession = new FrameSession(this, session, targetId, parentSession);
this._sessions.set(targetId, frameSession);
frameSession._initialize(false).catch(e => e);
}
removeFrameSession(targetId: Protocol.Target.TargetID) {
const frameSession = this._sessions.get(targetId);
if (!frameSession)
return;
// Frame id equals target id.
const frame = this._page._frameManager.frame(targetId);
if (frame)
this._page._frameManager.removeChildFramesRecursively(frame);
frameSession.dispose();
this._sessions.delete(targetId);
}
async pageOrError(): Promise<Page | Error> {
return this._pagePromise;
}
@ -340,6 +317,9 @@ class FrameSession {
private _firstNonInitialNavigationCommittedFulfill = () => {};
private _firstNonInitialNavigationCommittedReject = (e: Error) => {};
private _windowId: number | undefined;
// Marks the oopif session that remote -> local transition has happened in the parent.
// See Target.detachedFromTarget handler for details.
private _swappedIn = false;
constructor(crPage: CRPage, client: CRSession, targetId: string, parentSession: FrameSession | null) {
this._client = client;
@ -471,6 +451,7 @@ class FrameSession {
dispose() {
helper.removeEventListeners(this._eventListeners);
this._networkManager.dispose();
this._crPage._sessions.delete(this._targetId);
}
async _navigate(frame: frames.Frame, url: string, referrer: string | undefined): Promise<frames.GotoResult> {
@ -502,8 +483,10 @@ class FrameSession {
}
_onFrameAttached(frameId: string, parentFrameId: string | null) {
if (this._crPage._sessions.has(frameId) && frameId !== this._targetId) {
const frameSession = this._crPage._sessions.get(frameId);
if (frameSession && frameId !== this._targetId) {
// This is a remote -> local frame transition.
frameSession._swappedIn = true;
const frame = this._page._frameManager.frame(frameId)!;
this._page._frameManager.removeChildFramesRecursively(frame);
return;
@ -565,7 +548,13 @@ class FrameSession {
const session = CRConnection.fromSession(this._client).session(event.sessionId)!;
if (event.targetInfo.type === 'iframe') {
this._crPage.addFrameSession(event.targetInfo.targetId, session);
// Frame id equals target id.
const targetId = event.targetInfo.targetId;
const frame = this._page._frameManager.frame(targetId)!;
this._page._frameManager.removeChildFramesRecursively(frame);
const frameSession = new FrameSession(this._crPage, session, targetId, this);
this._crPage._sessions.set(targetId, frameSession);
frameSession._initialize(false).catch(e => e);
return;
}
@ -598,8 +587,31 @@ class FrameSession {
}
_onDetachedFromTarget(event: Protocol.Target.detachedFromTargetPayload) {
this._crPage.removeFrameSession(event.targetId!);
// This might be a worker...
this._page._removeWorker(event.sessionId);
// ... or an oopif.
const childFrameSession = this._crPage._sessions.get(event.targetId!);
if (!childFrameSession)
return;
// Usually, we get frameAttached in this session first and mark child as swappedIn.
if (childFrameSession._swappedIn) {
childFrameSession.dispose();
return;
}
// However, sometimes we get detachedFromTarget before frameAttached.
// In this case we don't know wheter this is a remote frame detach,
// or just a remote -> local transition. In the latter case, frameAttached
// is already inflight, so let's make a safe roundtrip to ensure it arrives.
this._client.send('Page.enable').catch(e => null).then(() => {
// Child was not swapped in - that means frameAttached did not happen and
// this is remote detach rather than remote -> local swap.
if (!childFrameSession._swappedIn)
this._page._frameManager.frameDetached(event.targetId!);
childFrameSession.dispose();
});
}
_onWindowOpen(event: Protocol.Page.windowOpenPayload) {

12
test/chromium/oopif.spec.js
View file

@ -41,6 +41,18 @@ describe('OOPIF', function() {
expect(page.frames().length).toBe(2);
expect(await page.frames()[1].evaluate(() => '' + location.href)).toBe(server.CROSS_PROCESS_PREFIX + '/grid.html');
});
it('should handle oopif detach', async function({browser, page, server, context}) {
await page.goto(server.PREFIX + '/dynamic-oopif.html');
expect(await countOOPIFs(browser)).toBe(1);
expect(page.frames().length).toBe(2);
const frame = page.frames()[1];
expect(await frame.evaluate(() => '' + location.href)).toBe(server.CROSS_PROCESS_PREFIX + '/grid.html');
const [detachedFrame] = await Promise.all([
page.waitForEvent('framedetached'),
page.evaluate(() => document.querySelector('iframe').remove()),
]);
expect(detachedFrame).toBe(frame);
});
it('should handle remote -> local -> remote transitions', async function({browser, page, server, context}) {
await page.goto(server.PREFIX + '/dynamic-oopif.html');
expect(page.frames().length).toBe(2);