Handle sidebar iframe reloads more gracefully

If the `<hypothesis-sidebar>` element is moved around in the DOM, this can cause
the sidebar to reload. There may also be other causes of the sidebar reloading
(eg. process crash for out-of-process iframe?). If the sidebar loads a second
time, it will fail to connect to the host frame since `PortProvider` will try to
re-use the `MessageChannel` it has already allocated, but sending that channels
ports will fail since they have already been transferred.

Recovering from this scenario fully involves a lot of changes since all the
places that have a connection to the sidebar would need to support replacing
that channel. Also various state in host/guest frames (eg. currently loaded
annotations) will be out of sync and need resetting.

What this commit does is just to handle the situation more gracefully, by
logging a meaningful error in the console and, after a delay, showing an error
message in the sidebar telling the user to reload the page. This also avoids
spamming Sentry [1] with errors about a situation that is out of our control. We
get a lot of error reports about this each month, mainly from certain
high-traffic pages that embed the client, but so far no actual complaints from
users about Hypothesis not working. Therefore it doesn't yet seem valuable
enough to do all the work to recover from a sidebar frame reload automatically.

[1] https://hypothesis.sentry.io/issues/2975780063/

src/shared/messaging/port-provider.ts

@@ -75,6 +75,7 @@ export class PortProvider implements Destroyable {
 
   private _listeners: ListenerCollection;
   private _sidebarHostChannel: MessageChannel;
+  private _sidebarConnected: boolean;
 
   /**
    * Begin listening to port requests from other frames.
@@ -91,6 +92,7 @@ export class PortProvider implements Destroyable {
     // Create the `sidebar-host` channel immediately, while other channels are
     // created on demand
     this._sidebarHostChannel = new MessageChannel();
+    this._sidebarConnected = false;
 
     this._listeners = new ListenerCollection();
 
@@ -180,6 +182,15 @@ export class PortProvider implements Destroyable {
       }
       this._handledRequests.add(requestId);
 
+      // If the source window has an opaque origin [1], `event.origin` will be
+      // the string "null". This is not a legal value for the `targetOrigin`
+      // parameter to `postMessage`, so remap it to "*".
+      //
+      // [1] https://html.spec.whatwg.org/multipage/origin.html#origin.
+      //     Documents with opaque origins include file:// URLs and
+      //     sandboxed iframes.
+      const targetOrigin = origin === 'null' ? '*' : origin;
+
       // Create the channel for these two frames to communicate and send the
       // corresponding ports to them.
       const messageChannel =
@@ -187,20 +198,27 @@ export class PortProvider implements Destroyable {
           ? this._sidebarHostChannel
           : new MessageChannel();
 
+      // The sidebar can only connect once. It might try to connect a second
+      // time if something causes the iframe to reload. We can't recover from
+      // this yet. Instead we just log a warning here. The port discovery
+      // protocol doesn't have a way to return errors, so the sidebar will only
+      // learn about this when it times out waiting for a response.
+      if (messageChannel === this._sidebarHostChannel) {
+        if (this._sidebarConnected) {
+          console.warn(
+            'Ignoring second request from Hypothesis sidebar to connect to host frame',
+          );
+          return;
+        }
+        this._sidebarConnected = true;
+      }
+
       // The message that is sent to the target frame that the source wants to
       // connect to, as well as the source frame requesting the connection.
       // Each message is accompanied by a port for the appropriate end of the
       // connection.
       const message = { frame1, frame2, type: 'offer', requestId, sourceId };
 
-      // If the source window has an opaque origin [1], `event.origin` will be
-      // the string "null". This is not a legal value for the `targetOrigin`
-      // parameter to `postMessage`, so remap it to "*".
-      //
-      // [1] https://html.spec.whatwg.org/multipage/origin.html#origin.
-      //     Documents with opaque origins include file:// URLs and
-      //     sandboxed iframes.
-      const targetOrigin = origin === 'null' ? '*' : origin;
       source.postMessage(message, targetOrigin, [messageChannel.port1]);
 
       if (frame2 === 'sidebar') {

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

@@ -173,6 +173,33 @@ describe('PortProvider', () => {
       );
     });
 
+    it('ignores a second request from sidebar frame for sidebar <-> host connection', async () => {
+      const warnStub = sinon.stub(console, 'warn');
+      try {
+        const data = {
+          frame1: 'sidebar',
+          frame2: 'host',
+          type: 'request',
+          sourceId: undefined,
+        };
+        await sendPortFinderRequest({
+          data: { ...data, requestId: 'first' },
+        });
+        window.postMessage.resetHistory();
+
+        await sendPortFinderRequest({
+          data: { ...data, requestId: 'second' },
+        });
+        assert.notCalled(window.postMessage);
+        assert.calledWith(
+          warnStub,
+          'Ignoring second request from Hypothesis sidebar to connect to host frame',
+        );
+      } finally {
+        warnStub.restore();
+      }
+    });
+
     it('responds to a valid port request from a source with an opaque origin', async () => {
       const data = {
         frame1: 'guest',

src/sidebar/index.tsx

@@ -103,11 +103,24 @@ function initServices(
 }
 
 /**
+ * Setup connection between sidebar and host page.
+ *
  * @inject
  */
-function setupFrameSync(frameSync: FrameSyncService, store: SidebarStore) {
+function setupFrameSync(
+  frameSync: FrameSyncService,
+  store: SidebarStore,
+  toastMessenger: ToastMessengerService,
+) {
   if (store.route() === 'sidebar') {
-    frameSync.connect();
+    frameSync.connect().catch(() => {
+      toastMessenger.error(
+        'Hypothesis failed to connect to the web page. Try reloading the page.',
+        {
+          autoDismiss: false,
+        },
+      );
+    });
   }
 }