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

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.

Author: dgozman

src/chromium/crPage.ts

@@ -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) {

test/chromium/oopif.spec.js

@@ -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);