Add built-in "connect" and "close" events to PortRPC

Add two built-in events to PortRPC which are dispatched when PortRPC connects to
a port and when it is destroyed or the containing frame is unloaded. These
events can be used in the counterpart PortRPC to confirm that the sender has
successfully received and connected to the port, and to get notified when the
port goes away.

Sending the "close" event in the context of a window unloading in Safari
<= 15 requires a workaround that involves registering a handler in the
parent frame. This handler is currently installed only in the host frame.

The new "close" event is used to replace the "frameDestroyed" message
that was used by guests to notify the sidebar when it went away. This solves
several problems:

 - It centralizes the workaround for https://bugs.webkit.org/show_bug.cgi?id=231167
   in `post-rpc.js`, instead of having it spread between several
   modules.

 - It provides a way to tear down the guest-host connection when the
   guest goes away.

 - It provides a way to handle the case where a guest is unloaded before it
   has received and connected to the port, by having the host/sidebar frames
   expect the "connect" call within a timeout. This is not yet implemented.

src/annotator/guest.js

@@ -216,8 +216,6 @@ export class Guest {
      * @type {Set<string>}
      */
     this._focusedAnnotations = new Set();
-
-    this._listeners.add(window, 'unload', () => this._sendUnloadNotification());
   }
 
   // Add DOM event listeners for clicks, taps etc. on the document and
@@ -435,8 +433,6 @@ export class Guest {
   }
 
   destroy() {
-    this._sendUnloadNotification();
-
     this._portFinder.destroy();
     this._hostRPC.destroy();
     this._sidebarRPC.destroy();
@@ -452,45 +448,6 @@ export class Guest {
     this._integration.destroy();
   }
 
-  /**
-   * 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.
-   *
-   * [1] https://bugs.webkit.org/show_bug.cgi?id=231167.
-   */
-  _sendUnloadNotification() {
-    const ports = [];
-
-    // There is currently a race condition where the guest can fail to notify
-    // the host and sidebar when it unloads if it has not yet received its ports
-    // for guest-host and guest-sidebar commmunication.
-    //
-    // For the moment this can be tolerated as guest frames are not displayed in
-    // the sidebar until the guest has received the port and sent a
-    // `documentInfoChanged` notification on it. Nevertheless the host / sidebar
-    // will be left with a disconnected PortRPC for the guest, so we should aim
-    // to fix this in future.
-
-    const sidebarPort = this._sidebarRPC.disconnect();
-    if (sidebarPort) {
-      ports.push(sidebarPort);
-    }
-
-    const hostPort = this._hostRPC.disconnect();
-    if (hostPort) {
-      ports.push(hostPort);
-    }
-
-    this._hostFrame.postMessage(
-      { type: 'hypothesisGuestUnloaded' },
-      '*',
-      ports
-    );
-  }
-
   /**
    * Anchor an annotation's selectors in the document.
    *

src/annotator/index.js

@@ -9,7 +9,10 @@ import { registerIcons } from '@hypothesis/frontend-shared';
 import { annotatorIcons } from './icons';
 registerIcons(annotatorIcons);
 
-import { PortProvider } from '../shared/messaging';
+import {
+  PortProvider,
+  installPortCloseWorkaroundForSafari,
+} from '../shared/messaging';
 import { getConfig } from './config/index';
 import { Guest } from './guest';
 import { HypothesisInjector } from './hypothesis-injector';
@@ -52,6 +55,9 @@ function init() {
   const destroyables = [];
 
   if (hostFrame === window) {
+    // Ensure port "close" notifications from eg. guest frames are delivered properly.
+    installPortCloseWorkaroundForSafari();
+
     const sidebarConfig = getConfig('sidebar');
 
     const hypothesisAppsOrigin = new URL(sidebarConfig.sidebarAppUrl).origin;

src/annotator/sidebar.js

@@ -218,21 +218,6 @@ export class Sidebar {
         this.open();
       }
     });
-
-    // 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, ports } = /** @type {MessageEvent} */ (event);
-      if (data?.type === 'hypothesisGuestUnloaded') {
-        for (let port of ports) {
-          const rpc = new PortRPC();
-          rpc.connect(port);
-          rpc.call('frameDestroyed');
-          rpc.destroy();
-        }
-      }
-    });
   }
 
   destroy() {
@@ -316,7 +301,7 @@ export class Sidebar {
       );
     }
 
-    guestRPC.on('frameDestroyed', () => {
+    guestRPC.on('close', () => {
       guestRPC.destroy();
       this._guestRPC = this._guestRPC.filter(rpc => rpc !== guestRPC);
     });

src/annotator/test/guest-test.js

@@ -1231,48 +1231,6 @@ describe('Guest', () => {
       guest.destroy();
       assert.called(sidebarRPC().destroy);
     });
-
-    it('notifies host frame that guest has been unloaded', () => {
-      const guest = createGuest({ subFrameIdentifier: 'frame-id' });
-
-      const hostPort = {};
-      hostRPC().disconnect.returns(hostPort);
-
-      const sidebarPort = {};
-      sidebarRPC().disconnect.returns(sidebarPort);
-
-      guest.destroy();
-
-      assert.calledWith(
-        hostFrame.postMessage,
-        {
-          type: 'hypothesisGuestUnloaded',
-        },
-        '*',
-        sinon.match.array.contains([sidebarPort, hostPort])
-      );
-    });
-  });
-
-  it('notifies host and sidebar frames when guest is unloaded', () => {
-    createGuest({ subFrameIdentifier: 'frame-id' });
-
-    const hostPort = {};
-    hostRPC().disconnect.returns(hostPort);
-
-    const sidebarPort = {};
-    sidebarRPC().disconnect.returns(sidebarPort);
-
-    window.dispatchEvent(new Event('unload'));
-
-    assert.calledWith(
-      hostFrame.postMessage,
-      {
-        type: 'hypothesisGuestUnloaded',
-      },
-      '*',
-      sinon.match.array.contains([sidebarPort, hostPort])
-    );
   });
 
   it('discovers and creates a channel for communication with the sidebar', async () => {

src/annotator/test/sidebar-test.js

@@ -216,28 +216,6 @@ describe('Sidebar', () => {
     assert.calledWith(fakeSendErrorsTo, sidebar.iframe.contentWindow);
   });
 
-  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' },
-      ports: guestPorts,
-    });
-
-    const prevPorts = fakePortRPCs.length;
-    window.dispatchEvent(event);
-
-    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) {
     return sidebar.iframe.src;
   }
@@ -508,13 +486,13 @@ describe('Sidebar', () => {
       });
     });
 
-    describe('on "frameDestroyed" event', () => {
+    describe('on "close" event', () => {
       it('disconnects the guest', () => {
         const sidebar = createSidebar();
         connectGuest(sidebar);
         guestRPC().call.resetHistory();
 
-        emitGuestEvent('frameDestroyed');
+        emitGuestEvent('close');
 
         assert.called(guestRPC().destroy);
 

src/shared/messaging/index.js

 export { PortFinder } from './port-finder';
 export { PortProvider } from './port-provider';
-export { PortRPC } from './port-rpc';
+export { PortRPC, installPortCloseWorkaroundForSafari } from './port-rpc';
 export { isMessageEqual } from './port-util';

src/shared/messaging/port-rpc.js

@@ -50,6 +50,77 @@ const PROTOCOL = 'frame-rpc';
  * @typedef {import('../../types/annotator').Destroyable} Destroyable
  */
 
+/**
+ * Send a PortRPC method call.
+ *
+ * @param {MessagePort} port
+ * @param {string} method
+ * @param {any[]} [arguments]
+ * @param {number} [sequence] - Sequence number used for replies
+ */
+function sendCall(port, method, args = [], sequence = -1) {
+  port.postMessage(
+    /** @type {RequestMessage} */ ({
+      protocol: PROTOCOL,
+      version: VERSION,
+      arguments: args,
+      method,
+      sequence,
+    })
+  );
+}
+
+/**
+ * Install a message listener that ensures proper delivery of "close" notifications
+ * from {@link PortRPC}s in Safari <= 15.
+ *
+ * This must be called in the _parent_ frame of the frame that owns the port.
+ * In Hypothesis this means for example that the workaround would be installed
+ * in the host frame to ensure delivery of "close" notifications from "guest"
+ * frames.
+ *
+ * @return {() => void} - Callback that removes the listener
+ */
+export function installPortCloseWorkaroundForSafari() {
+  if (!shouldUseSafariWorkaround()) {
+    return () => {};
+  }
+
+  /** @param {MessageEvent} event */
+  const handler = event => {
+    if (event.data?.type === 'hypothesisPortClosed' && event.ports[0]) {
+      const port = event.ports[0];
+      sendCall(port, 'close');
+      port.close();
+    }
+  };
+
+  window.addEventListener('message', handler);
+  return () => window.removeEventListener('message', handler);
+}
+
+/**
+ * Test whether this browser needs the workaround for https://bugs.webkit.org/show_bug.cgi?id=231167.
+ */
+function shouldUseSafariWorkaround() {
+  const webkitVersionMatch = navigator.userAgent.match(
+    /\bAppleWebKit\/([0-9]+)\b/
+  );
+  if (!webkitVersionMatch) {
+    return false;
+  }
+  const version = parseInt(webkitVersionMatch[1]);
+
+  // Chrome's user agent contains the token "AppleWebKit/537.36", where the
+  // version number is frozen. This corresponds to a version of Safari from 2013,
+  // which is older than all supported Safari versions.
+  if (version <= 537) {
+    return false;
+  }
+
+  return true;
+}
+
 /**
  * PortRPC provides remote procedure calls between frames or workers. It uses
  * the Channel Messaging API [1] as a transport.
@@ -60,6 +131,16 @@ const PROTOCOL = 'frame-rpc';
  * {@link connect} to make the PortRPC instance in each frame use the corresponding
  * port.
  *
+ * In addition to the custom methods that a PortRPC handles, there are several
+ * built-in events which are sent automatically:
+ *
+ * - "connect" is sent when a PortRPC connects to a port. This event can
+ *   be used to confirm that the sending frame has received the port and will
+ *   send a "close" event when it goes away.
+ * - "close" is sent when a PortRPC is destroyed or the owning frame is
+ *   unloaded. This event may not be dispatched if the guest frame terminates
+ *   abnormally (eg. due to an OS process crash).
+ *
  * [1] https://developer.mozilla.org/en-US/docs/Web/API/Channel_Messaging_API
  *
  * @template {string} OnMethod - Names of RPC methods this client responds to
@@ -80,6 +161,22 @@ export class PortRPC {
     this._callbacks = new Map();
 
     this._listeners = new ListenerCollection();
+    this._listeners.add(window, 'unload', () => {
+      if (this._port) {
+        // Send "close" notification directly. This works in Chrome, Firefox and
+        // Safari >= 16.
+        sendCall(this._port, 'close');
+
+        // To work around a bug in Safari <= 15 which prevents sending messages
+        // while a window is unloading, try transferring the port to the parent frame
+        // and re-sending the "close" event from there.
+        if (window !== window.parent && shouldUseSafariWorkaround()) {
+          window.parent.postMessage({ type: 'hypothesisPortClosed' }, '*', [
+            this._port,
+          ]);
+        }
+      }
+    });
 
     /**
      * Method and arguments of pending RPC calls made before a port was connected.
@@ -87,14 +184,19 @@ export class PortRPC {
      * @type {Array<[CallMethod, any[]]>}
      */
     this._pendingCalls = [];
+
+    this._receivedCloseEvent = false;
   }
 
   /**
    * Register a method handler for incoming RPC requests.
    *
+   * This can also be used to register a handler for the built-in "connect"
+   * and "close" events.
+   *
    * All handlers must be registered before {@link connect} is invoked.
    *
-   * @param {OnMethod} method
+   * @param {OnMethod|'close'|'connect'} method
    * @param {(...args: any[]) => void} handler
    */
   on(method, handler) {
@@ -115,6 +217,7 @@ export class PortRPC {
       this._handle(/** @type {MessageEvent} */ (event))
     );
     port.start();
+    sendCall(port, 'connect');
 
     for (let [method, args] of this._pendingCalls) {
       this.call(method, ...args);
@@ -122,25 +225,16 @@ export class PortRPC {
     this._pendingCalls = [];
   }
 
-  /**
-   * 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();
+    if (this._port) {
+      sendCall(this._port, 'close');
+      this._port.close();
+    }
     this._destroyed = true;
+    this._listeners.removeAll();
   }
 
   /**
@@ -171,16 +265,7 @@ export class PortRPC {
       args = args.slice(0, -1);
     }
 
-    /** @type {RequestMessage} */
-    const message = {
-      arguments: args,
-      method,
-      protocol: PROTOCOL,
-      sequence: seq,
-      version: VERSION,
-    };
-
-    this._port.postMessage(message);
+    sendCall(this._port, method, args, seq);
   }
 
   /**
@@ -218,6 +303,15 @@ export class PortRPC {
     }
 
     if ('method' in msg) {
+      // Only allow close event to fire once.
+      if (msg.method === 'close') {
+        if (this._receivedCloseEvent) {
+          return;
+        } else {
+          this._receivedCloseEvent = true;
+        }
+      }
+
       const handler = this._methods.get(msg.method);
       if (!handler) {
         return;

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

@@ -175,23 +175,54 @@ describe('PortRPC', () => {
     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);
+  it('should send "connect" event after connection', async () => {
+    const { port1, port2 } = new MessageChannel();
+    const sender = new PortRPC();
+    const receiver = new PortRPC();
+    const connectHandler = sinon.stub();
 
-      assert.isNull(rpc.disconnect());
+    receiver.on('connect', connectHandler);
+    receiver.connect(port2);
+    sender.connect(port1);
 
-      const { port1 } = new MessageChannel();
-      rpc.connect(port1);
+    await waitForMessageDelivery();
 
-      assert.equal(rpc.disconnect(), port1);
-      assert.isNull(rpc.disconnect());
+    assert.calledWith(connectHandler);
+  });
 
-      rpc.call('test');
-      await waitForMessageDelivery();
-      assert.notCalled(testMethod);
-    });
+  it('should send "close" event after port is destroyed', async () => {
+    const { port1, port2 } = new MessageChannel();
+    const sender = new PortRPC();
+    const receiver = new PortRPC();
+    const closeHandler = sinon.stub();
+
+    receiver.on('close', closeHandler);
+    receiver.connect(port2);
+    sender.connect(port1);
+    await waitForMessageDelivery();
+
+    closeHandler.resetHistory();
+    sender.destroy();
+    await waitForMessageDelivery();
+
+    assert.calledWith(closeHandler);
+  });
+
+  it('should send "close" event when window is unloaded', async () => {
+    const { port1, port2 } = new MessageChannel();
+    const sender = new PortRPC();
+    const receiver = new PortRPC();
+    const closeHandler = sinon.stub();
+
+    receiver.on('close', closeHandler);
+    receiver.connect(port2);
+    sender.connect(port1);
+    await waitForMessageDelivery();
+
+    closeHandler.resetHistory();
+    window.dispatchEvent(new Event('unload'));
+    await waitForMessageDelivery();
+
+    assert.calledWith(closeHandler);
   });
 });

src/sidebar/services/frame-sync.js

@@ -249,7 +249,10 @@ export class FrameSyncService {
       });
     });
 
-    guestRPC.on('frameDestroyed', () => {
+    // TODO - Close connection if we don't receive a "connect" message within
+    // a certain time frame.
+
+    guestRPC.on('close', () => {
       const frame = this._store.frames().find(f => f.id === frameIdentifier);
       if (frame) {
         this._store.destroyFrame(frame);

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

@@ -549,7 +549,7 @@ describe('FrameSyncService', () => {
       frameSync.connect();
       await connectGuest();
 
-      emitGuestEvent('frameDestroyed');
+      emitGuestEvent('close');
 
       assert.called(guestRPC().destroy);
     });
@@ -562,7 +562,7 @@ describe('FrameSyncService', () => {
         frameIdentifier: fixtures.framesListEntry.id,
         uri: 'http://example.org',
       });
-      emitGuestEvent('frameDestroyed');
+      emitGuestEvent('close');
 
       assert.calledWith(fakeStore.destroyFrame, fixtures.framesListEntry);
     });

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

@@ -20,10 +20,7 @@ export type GuestToHostEvent =
   /**
    * The guest informs the host that the anchors have been changed in the main annotatable frame.
    */
-  | 'anchorsChanged'
-
-  /** The guest was unloaded. */
-  | 'frameDestroyed';
+  | 'anchorsChanged';
 
 /**
  * Events that the guest sends to the sidebar
@@ -65,10 +62,7 @@ export type GuestToSidebarEvent =
   /**
    * The guest is asking the sidebar to toggle some annotations.
    */
-  | 'toggleAnnotationSelection'
-
-  /** The guest frame was unloaded. */
-  | 'frameDestroyed';
+  | 'toggleAnnotationSelection';
 
 /**
  * Events that the host sends to the guest