Revise how guest notifies host and sidebar when it is unloaded

Change how the guest notifies other frames, specifically the sidebar and host,
when it is unloaded. The host and sidebar now receive a `frameDestroyed` message
from the corresponding guest port, which allows them to easily close the right
port and remove it from the list of active guest ports.

Due to a Safari bug (see code comments) we can't send the `frameDestroyed`
messages from the guest frame while it is being unloaded. However it is possible
to first transfer the port to the host frame and then have the host frame send
the message on the same port. I also tried sending the message from the guest
frame, and then transferring the port to the host frame but that didn't work.
This workaround has the advantage that it is transparent to the receiver of the
`frameDestroyed` message.

This change is also a step towards possibly not relying on user-provided guest
frame identifiers in the sidebar, which only become available once the
`documentInfoChanged` call has been received. Instead the sidebar could
use its own internal IDs for guest frames, avoiding the possibility for
conflicts.

Changes in detail:

 - Add `disconnect` method to PortRPC

 - When guest is unloaded, transfer ports to the host frame in a
   `hypothesisGuestUnloaded` message, and make the host frame dispatch
   `frameDestroyed` calls on these ports.

 - Handle `frameDestroyed` in sidebar by closing port, removing it from
   the active guest list and removing the associated frame from the
   store

 - Handle `frameDestroyed` in host frame by closing port and removing it
   from the active guest list

src/annotator/guest.js

@@ -225,7 +225,7 @@ export class Guest {
      */
     this._focusedAnnotations = new Set();
 
-    this._listeners.add(window, 'unload', () => this._notifyGuestUnload());
+    this._listeners.add(window, 'unload', () => this._sendUnloadNotification());
   }
 
   // Add DOM event listeners for clicks, taps etc. on the document and
@@ -452,8 +452,12 @@ export class Guest {
   }
 
   destroy() {
+    this._sendUnloadNotification();
+
     this._portFinder.destroy();
-    this._notifyGuestUnload();
+    this._hostRPC.destroy();
+    this._sidebarRPC.destroy();
+
     this._hypothesisInjector.destroy();
     this._listeners.removeAll();
 
@@ -464,21 +468,35 @@ export class Guest {
     removeAllHighlights(this.element);
 
     this._integration.destroy();
-    this._sidebarRPC.destroy();
   }
 
   /**
-   * Notify the host frame that the guest is being unloaded.
+   * Notify other frames that the guest is being unloaded.
+   *
+   * In order to work around a bug in Safari 15 and below [1], this is done by
+   * first sending ports to the host frame and then, from the host frame,
+   * sending messages on the ports.
    *
-   * The host frame in turn notifies the sidebar app that the guest has gone away.
+   * [1] https://bugs.webkit.org/show_bug.cgi?id=231167.
    */
-  _notifyGuestUnload() {
-    const message = {
-      type: 'hypothesisGuestUnloaded',
-      frameIdentifier: this._frameIdentifier,
-    };
+  _sendUnloadNotification() {
+    const ports = [];
+
+    const sidebarPort = this._sidebarRPC.disconnect();
+    if (sidebarPort) {
+      ports.push(sidebarPort);
+    }
 
-    this._hostFrame.postMessage(message, '*');
+    const hostPort = this._hostRPC.disconnect();
+    if (hostPort) {
+      ports.push(hostPort);
+    }
+
+    this._hostFrame.postMessage(
+      { type: 'hypothesisGuestUnloaded' },
+      '*',
+      ports
+    );
   }
 
   /**

src/annotator/sidebar.js

@@ -219,14 +219,18 @@ export class Sidebar {
       }
     });
 
-    // Notify sidebar when a guest is unloaded. This message is routed via
-    // the host frame because in Safari guest frames are unable to send messages
-    // directly to the sidebar during a window's 'unload' event.
-    // See https://bugs.webkit.org/show_bug.cgi?id=231167.
+    // Notify other frames when a guest is unloaded. The ports are first
+    // transferred from the guest to the host frame to work around a bug in
+    // Safari <= 15. See https://bugs.webkit.org/show_bug.cgi?id=231167.
     this._listeners.add(window, 'message', event => {
-      const { data } = /** @type {MessageEvent} */ (event);
+      const { data, ports } = /** @type {MessageEvent} */ (event);
       if (data?.type === 'hypothesisGuestUnloaded') {
-        this._sidebarRPC.call('frameDestroyed', data.frameIdentifier);
+        for (let port of ports) {
+          const rpc = new PortRPC();
+          rpc.connect(port);
+          rpc.call('frameDestroyed');
+          rpc.destroy();
+        }
       }
     });
   }
@@ -257,7 +261,7 @@ export class Sidebar {
   onFrameConnected(source, port) {
     switch (source) {
       case 'guest':
-        this._guestRPC.push(this._connectGuest(port));
+        this._connectGuest(port);
         break;
       case 'sidebar':
         this._sidebarRPC.connect(port);
@@ -307,9 +311,13 @@ export class Sidebar {
       }
     );
 
-    guestRPC.connect(port);
+    guestRPC.on('frameDestroyed', () => {
+      guestRPC.destroy();
+      this._guestRPC = this._guestRPC.filter(rpc => rpc !== guestRPC);
+    });
 
-    return guestRPC;
+    guestRPC.connect(port);
+    this._guestRPC.push(guestRPC);
   }
 
   _setupSidebarEvents() {

src/annotator/test/guest-test.js

@@ -109,6 +109,7 @@ describe('Guest', () => {
         call: sinon.stub(),
         connect: sinon.stub(),
         destroy: sinon.stub(),
+        disconnect: sinon.stub(),
         on: sinon.stub(),
       };
       fakePortRPCs.push(rpc);
@@ -1247,7 +1248,12 @@ describe('Guest', () => {
     });
 
     it('notifies host frame that guest has been unloaded', () => {
+      const sidebarPort = {};
+      const hostPort = {};
+
       const guest = createGuest({ subFrameIdentifier: 'frame-id' });
+      hostRPC().disconnect.returns(hostPort);
+      sidebarRPC().disconnect.returns(sidebarPort);
 
       guest.destroy();
 
@@ -1255,15 +1261,19 @@ describe('Guest', () => {
         hostFrame.postMessage,
         {
           type: 'hypothesisGuestUnloaded',
-          frameIdentifier: 'frame-id',
         },
-        '*'
+        '*',
+        sinon.match.array.contains([sidebarPort, hostPort])
       );
     });
   });
 
   it('notifies host frame when guest frame is unloaded', () => {
+    const sidebarPort = {};
+    const hostPort = {};
     createGuest({ subFrameIdentifier: 'frame-id' });
+    hostRPC().disconnect.returns(hostPort);
+    sidebarRPC().disconnect.returns(sidebarPort);
 
     window.dispatchEvent(new Event('unload'));
 
@@ -1271,9 +1281,9 @@ describe('Guest', () => {
       hostFrame.postMessage,
       {
         type: 'hypothesisGuestUnloaded',
-        frameIdentifier: 'frame-id',
       },
-      '*'
+      '*',
+      sinon.match.array.contains([sidebarPort, hostPort])
     );
   });
 

src/annotator/test/integration/anchoring-test.js

@@ -48,8 +48,11 @@ describe('anchoring', () => {
     container = document.createElement('div');
     container.innerHTML = testPageHTML;
     document.body.appendChild(container);
+
     const fakePortFinder = {
-      discover: sinon.stub().resolves(new MessageChannel().port1),
+      discover: sinon.stub().callsFake(async () => {
+        return new MessageChannel().port1;
+      }),
       destroy: sinon.stub(),
     };
     guestImports.$mock({
@@ -57,6 +60,7 @@ describe('anchoring', () => {
         PortFinder: sinon.stub().returns(fakePortFinder),
       },
     });
+
     guest = new Guest(container);
   });
 

src/annotator/test/sidebar-test.js

@@ -216,15 +216,26 @@ describe('Sidebar', () => {
     assert.calledWith(fakeSendErrorsTo, sidebar.iframe.contentWindow);
   });
 
-  it('notifies sidebar app when a guest frame is unloaded', () => {
+  it('notifies other frames when a guest is unloaded', () => {
+    const guestHostChannel = new MessageChannel();
+    const guestSidebarChannel = new MessageChannel();
+    const guestPorts = [guestHostChannel.port1, guestSidebarChannel.port1];
     createSidebar();
 
     const event = new MessageEvent('message', {
-      data: { type: 'hypothesisGuestUnloaded', frameIdentifier: 'frame-id' },
+      data: { type: 'hypothesisGuestUnloaded' },
+      ports: guestPorts,
     });
+
+    const prevPorts = fakePortRPCs.length;
     window.dispatchEvent(event);
 
-    assert.calledWith(sidebarRPC().call, 'frameDestroyed', 'frame-id');
+    const tempPortRPCs = fakePortRPCs.slice(prevPorts);
+    assert.equal(tempPortRPCs.length, guestPorts.length);
+    tempPortRPCs.forEach(rpc => {
+      assert.calledWith(rpc.call, 'frameDestroyed');
+      assert.called(rpc.destroy);
+    });
   });
 
   function getConfigString(sidebar) {

src/shared/messaging/port-rpc.js

@@ -123,13 +123,24 @@ export class PortRPC {
   }
 
   /**
-   * Disconnect the RPC channel. After this is invoked no further method calls
-   * will be received.
+   * Disconnect and return the current MessagePort.
+   *
+   * This does not close the port, so it can still be used afterwards.
+   */
+  disconnect() {
+    const port = this._port;
+    this._port = null;
+    this._listeners.removeAll();
+    return port;
+  }
+
+  /**
+   * Disconnect the RPC channel and close the MessagePort.
    */
   destroy() {
+    const port = this.disconnect();
+    port?.close();
     this._destroyed = true;
-    this._listeners.removeAll();
-    this._port?.close();
   }
 
   /**

src/shared/messaging/test/port-rpc-test.js

@@ -174,4 +174,24 @@ describe('PortRPC', () => {
     assert.calledWith(testMethod, 'first', 'call');
     assert.calledWith(testMethod, 'second', 'call');
   });
+
+  describe('#disconnect', () => {
+    it('disconnects and returns port', async () => {
+      const rpc = new PortRPC();
+      const testMethod = sinon.stub();
+      rpc.on('test', testMethod);
+
+      assert.isNull(rpc.disconnect());
+
+      const { port1 } = new MessageChannel();
+      rpc.connect(port1);
+
+      assert.equal(rpc.disconnect(), port1);
+      assert.isNull(rpc.disconnect());
+
+      rpc.call('test');
+      await waitForMessageDelivery();
+      assert.notCalled(testMethod);
+    });
+  });
 });

src/sidebar/services/frame-sync.js

@@ -179,12 +179,17 @@ export class FrameSyncService {
   _connectGuest(port) {
     /** @type {PortRPC<GuestToSidebarEvent, SidebarToGuestEvent>} */
     const guestRPC = new PortRPC();
+    this._guestRPC.push(guestRPC);
+
+    /** @type {string|null} */
+    let frameIdentifier;
 
     // Update document metadata for this guest. We currently assume that the
     // guest will make this call once after it connects. To handle updates
     // to the document, we'll need to change `connectFrame` to update rather than
     // add to the frame list.
     guestRPC.on('documentInfoChanged', info => {
+      frameIdentifier = info.frameIdentifier;
       this._store.connectFrame({
         id: info.frameIdentifier,
         metadata: info.metadata,
@@ -192,6 +197,15 @@ export class FrameSyncService {
       });
     });
 
+    guestRPC.on('frameDestroyed', () => {
+      const frame = this._store.frames().find(f => f.id === frameIdentifier);
+      if (frame) {
+        this._store.destroyFrame(frame);
+      }
+      guestRPC.destroy();
+      this._guestRPC = this._guestRPC.filter(rpc => rpc !== guestRPC);
+    });
+
     // A new annotation, note or highlight was created in the frame
     guestRPC.on(
       'createAnnotation',
@@ -272,7 +286,6 @@ export class FrameSyncService {
       this._hostRPC.call('closeSidebar');
     });
 
-    this._guestRPC.push(guestRPC);
     guestRPC.connect(port);
 
     // Synchronize highlight visibility in this guest with the sidebar's controls.
@@ -287,17 +300,6 @@ export class FrameSyncService {
       this._store.setSidebarOpened(true);
     });
 
-    // Listen for notifications of a guest being unloaded. This message is routed
-    // via the host frame rather than coming directly from the unloaded guest
-    // to work around https://bugs.webkit.org/show_bug.cgi?id=231167.
-    this._hostRPC.on('frameDestroyed', frameIdentifier => {
-      const frame = this._store.frames().find(f => f.id === frameIdentifier);
-      if (frame) {
-        this._store.destroyFrame(frame);
-      }
-      // TODO - Remove the guest connection associated with this frame identifier.
-    });
-
     // When user toggles the highlight visibility control in the sidebar container,
     // update the visibility in all the guest frames.
     this._hostRPC.on('setHighlightsVisible', visible => {

src/sidebar/services/test/frame-sync-test.js

@@ -472,12 +472,25 @@ describe('FrameSyncService', () => {
     });
   });
 
-  context('when a frame is destroyed', () => {
-    const frameId = fixtures.framesListEntry.id;
+  context('when a guest frame is destroyed', () => {
+    it('disconnects the guest', async () => {
+      frameSync.connect();
+      await connectGuest();
+
+      emitGuestEvent('frameDestroyed');
+
+      assert.called(guestRPC().destroy);
+    });
 
-    it('removes the frame from the frames list', () => {
+    it('removes the guest from the store', async () => {
       frameSync.connect();
-      emitHostEvent('frameDestroyed', frameId);
+      await connectGuest();
+
+      emitGuestEvent('documentInfoChanged', {
+        frameIdentifier: fixtures.framesListEntry.id,
+        uri: 'http://example.org',
+      });
+      emitGuestEvent('frameDestroyed');
 
       assert.calledWith(fakeStore.destroyFrame, fixtures.framesListEntry);
     });

src/types/port-rpc-events.d.ts

@@ -20,7 +20,10 @@ export type GuestToHostEvent =
   /**
    * The guest informs the host that the anchors have been changed in the main annotatable frame.
    */
-  | 'anchorsChanged';
+  | 'anchorsChanged'
+
+  /** The guest was unloaded. */
+  | 'frameDestroyed';
 
 /**
  * Events that the guest sends to the sidebar
@@ -62,7 +65,10 @@ export type GuestToSidebarEvent =
   /**
    * The guest is asking the sidebar to toggle some annotations.
    */
-  | 'toggleAnnotationSelection';
+  | 'toggleAnnotationSelection'
+
+  /** The guest frame was unloaded. */
+  | 'frameDestroyed';
 
 /**
  * Events that the host sends to the guest
@@ -102,11 +108,6 @@ export type HostToGuestEvent =
  * Events that the host sends to the sidebar
  */
 export type HostToSidebarEvent =
-  /**
-   * The host informs the sidebar that a guest frame has been destroyed
-   */
-  | 'frameDestroyed'
-
   /**
    * Highlights have been toggled on/off.
    */