Route guest frame unload notifications via host frame

Change how the sidebar is notified of guest frames being unloaded to
support guest frames where the client has been loaded via means other
than `HypothesisInjector` or where the guest is cross-origin.

Instead of listening for the guest frame's 'unload' event from the
parent frame in `HypothesisInjector`, the guest frame instead listens
for this event itself and sends a `hypothesisGuestUnloaded` message to
the host frame via `window.postMessage`, which in turn is handled in the
`Sidebar` class to relay it to the sidebar app via a `destroyFrame` RPC
call. This indirect route works around a bug in Safari (see code
comments).

As well as supporting future use cases, this also simplifies the
`HypothesisInjector` class as it no longer needs access to the `Bridge`.

src/annotator/cross-frame.js

@@ -29,11 +29,7 @@ export class CrossFrame {
   constructor(element, eventBus, config) {
     this._bridge = new Bridge();
     this._annotationSync = new AnnotationSync(eventBus, this._bridge);
-    this._hypothesisInjector = new HypothesisInjector(
-      element,
-      this._bridge,
-      config
-    );
+    this._hypothesisInjector = new HypothesisInjector(element, config);
   }
 
   /**

src/annotator/guest.js

@@ -113,8 +113,11 @@ export default class Guest {
    * @param {EventBus} eventBus -
    *   Enables communication between components sharing the same eventBus
    * @param {Record<string, any>} [config]
+   * @param {Window} [hostFrame] -
+   *   Host frame which this guest is associated with. This is expected to be
+   *   an ancestor of the guest frame. It may be same or cross origin.
    */
-  constructor(element, eventBus, config = {}) {
+  constructor(element, eventBus, config = {}, hostFrame = window) {
     this.element = element;
     this._emitter = eventBus.createEmitter();
     this._visibleHighlights = false;
@@ -186,6 +189,9 @@ export default class Guest {
      * @type {Set<string>}
      */
     this._focusedAnnotations = new Set();
+
+    this._hostFrame = hostFrame;
+    this._listeners.add(window, 'unload', () => this._notifyGuestUnload());
   }
 
   // Add DOM event listeners for clicks, taps etc. on the document and
@@ -351,6 +357,7 @@ export default class Guest {
   }
 
   destroy() {
+    this._notifyGuestUnload();
     this._listeners.removeAll();
 
     this._selectionObserver.disconnect();
@@ -363,6 +370,19 @@ export default class Guest {
     this.crossframe.destroy();
   }
 
+  /**
+   * Notify the host frame that the guest is being unloaded.
+   *
+   * The host frame in turn notifies the sidebar app that the guest has gone away.
+   */
+  _notifyGuestUnload() {
+    const message = {
+      type: 'hypothesisGuestUnloaded',
+      frameIdentifier: this._frameIdentifier,
+    };
+    this._hostFrame.postMessage(message, '*');
+  }
+
   /**
    * Anchor an annotation's selectors in the document.
    *

src/annotator/hypothesis-injector.js

 import { FrameObserver } from './frame-observer';
 
 /**
- * @typedef {import('../shared/bridge').Bridge} Bridge
  * @typedef {import('../types/annotator').Destroyable} Destroyable
  */
 
@@ -16,19 +15,15 @@ export class HypothesisInjector {
   /**
    * @param {Element} element - root of the DOM subtree to watch for the
    *   addition and removal of annotatable iframes
-   * @param {Bridge} bridge - Channel for communicating with the sidebar
    * @param {Record<string, any>} config - Annotator configuration that is
    *   injected, along with the Hypothesis client, into the child iframes
    */
-  constructor(element, bridge, config) {
-    this._bridge = bridge;
+  constructor(element, config) {
     this._config = config;
-    /** @type {Map<HTMLIFrameElement, string>} */
-    this._frameIdentifiers = new Map();
     this._frameObserver = new FrameObserver(
       element,
       frame => this._addHypothesis(frame),
-      frame => this._removeHypothesis(frame)
+      () => {}
     );
   }
 
@@ -55,7 +50,6 @@ export class HypothesisInjector {
 
     // Generate a random string to use as a frame ID. The format is not important.
     const subFrameIdentifier = Math.random().toString().replace(/\D/g, '');
-    this._frameIdentifiers.set(frame, subFrameIdentifier);
     const injectedConfig = {
       ...this._config,
       subFrameIdentifier,
@@ -64,14 +58,6 @@ export class HypothesisInjector {
     const { clientUrl } = this._config;
     injectHypothesis(frame, clientUrl, injectedConfig);
   }
-
-  /**
-   * @param {HTMLIFrameElement} frame
-   */
-  _removeHypothesis(frame) {
-    this._bridge.call('destroyFrame', this._frameIdentifiers.get(frame));
-    this._frameIdentifiers.delete(frame);
-  }
 }
 
 /**

src/annotator/index.js

@@ -51,15 +51,14 @@ function init() {
   window_.__hypothesis = {};
 
   const annotatorConfig = getConfig('annotator');
+  const hostFrame = annotatorConfig.subFrameIdentifier ? window.parent : window;
 
   // Create the guest that handles creating annotations and displaying highlights.
   const eventBus = new EventBus();
-  const guest = new Guest(document.body, eventBus, annotatorConfig);
+  const guest = new Guest(document.body, eventBus, annotatorConfig, hostFrame);
 
-  // Create the sidebar if this is the host frame. The `subFrameIdentifier`
-  // config option indicates a non-host/guest-only frame.
   let sidebar;
-  if (!annotatorConfig.subFrameIdentifier) {
+  if (window === hostFrame) {
     sidebar = new Sidebar(document.body, eventBus, guest, getConfig('sidebar'));
 
     // Expose sidebar window reference for use by same-origin guest frames.

src/annotator/sidebar.js

@@ -209,6 +209,17 @@ export default 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.
+    this._listeners.add(window, 'message', event => {
+      const messageData = /** @type {MessageEvent} */ (event).data;
+      if (messageData?.type === 'hypothesisGuestUnloaded') {
+        this._sidebarRPC.call('destroyFrame', messageData.frameIdentifier);
+      }
+    });
   }
 
   destroy() {

src/annotator/test/guest-test.js

@@ -35,6 +35,8 @@ describe('Guest', () => {
   let rangeUtil;
   let notifySelectionChanged;
 
+  let hostFrame;
+
   let CrossFrame;
   let fakeCrossFrame;
 
@@ -46,7 +48,7 @@ describe('Guest', () => {
   const createGuest = (config = {}) => {
     const element = document.createElement('div');
     eventBus = new EventBus();
-    const guest = new Guest(element, eventBus, config);
+    const guest = new Guest(element, eventBus, config, hostFrame);
     guests.push(guest);
     return guest;
   };
@@ -95,6 +97,10 @@ describe('Guest', () => {
 
     fakeCreateIntegration = sinon.stub().returns(fakeIntegration);
 
+    hostFrame = {
+      postMessage: sinon.stub(),
+    };
+
     class FakeSelectionObserver {
       constructor(callback) {
         notifySelectionChanged = callback;
@@ -1121,6 +1127,36 @@ describe('Guest', () => {
       guest.destroy();
       assert.calledWith(highlighter.removeAllHighlights, guest.element);
     });
+
+    it('notifies host frame that guest has been unloaded', () => {
+      const guest = createGuest({ subFrameIdentifier: 'frame-id' });
+
+      guest.destroy();
+
+      assert.calledWith(
+        hostFrame.postMessage,
+        {
+          type: 'hypothesisGuestUnloaded',
+          frameIdentifier: 'frame-id',
+        },
+        '*'
+      );
+    });
+  });
+
+  it('notifies host frame when guest frame is unloaded', () => {
+    createGuest({ subFrameIdentifier: 'frame-id' });
+
+    window.dispatchEvent(new Event('unload'));
+
+    assert.calledWith(
+      hostFrame.postMessage,
+      {
+        type: 'hypothesisGuestUnloaded',
+        frameIdentifier: 'frame-id',
+      },
+      '*'
+    );
   });
 
   describe('#contentContainer', () => {

src/annotator/test/integration/hypothesis-injector-test.js

@@ -3,7 +3,6 @@ import { HypothesisInjector } from '../../hypothesis-injector';
 
 describe('HypothesisInjector integration test', () => {
   let container;
-  let fakeBridge;
   let hypothesisInjectors;
 
   const sandbox = sinon.createSandbox();
@@ -22,7 +21,7 @@ describe('HypothesisInjector integration test', () => {
   }
 
   function createHypothesisInjector() {
-    const injector = new HypothesisInjector(container, fakeBridge, config);
+    const injector = new HypothesisInjector(container, config);
     hypothesisInjectors.push(injector);
     return injector;
   }
@@ -35,11 +34,6 @@ describe('HypothesisInjector integration test', () => {
   }
 
   beforeEach(() => {
-    fakeBridge = {
-      createChannel: sandbox.stub(),
-      call: sandbox.stub(),
-      destroy: sandbox.stub(),
-    };
     hypothesisInjectors = [];
 
     container = document.createElement('div');
@@ -74,21 +68,6 @@ describe('HypothesisInjector integration test', () => {
     );
   });
 
-  it('detects removed iframes', async () => {
-    // Create a iframe before initializing
-    const iframe = createAnnotatableIFrame();
-
-    // Now initialize
-    createHypothesisInjector();
-    await onDocumentReady(iframe);
-
-    // Remove the iframe
-    iframe.remove();
-    await waitForFrameObserver();
-
-    assert.calledWith(fakeBridge.call, 'destroyFrame');
-  });
-
   it('injects embed script in iframe', async () => {
     const iframe = createAnnotatableIFrame();
 
@@ -135,21 +114,6 @@ describe('HypothesisInjector integration test', () => {
     );
   });
 
-  it('detects dynamically removed iframes', async () => {
-    // Create a iframe before initializing
-    const iframe = createAnnotatableIFrame();
-
-    // Now initialize
-    createHypothesisInjector();
-    await waitForFrameObserver();
-    await onDocumentReady(iframe);
-
-    iframe.remove();
-    await waitForFrameObserver();
-
-    assert.calledWith(fakeBridge.call, 'destroyFrame');
-  });
-
   it('detects an iframe dynamically removed, and added again', async () => {
     const iframe = createAnnotatableIFrame();
 

src/annotator/test/sidebar-test.js

@@ -196,6 +196,17 @@ describe('Sidebar', () => {
     });
   });
 
+  it('notifies sidebar app when a guest frame is unloaded', () => {
+    createSidebar();
+
+    const event = new MessageEvent('message', {
+      data: { type: 'hypothesisGuestUnloaded', frameIdentifier: 'frame-id' },
+    });
+    window.dispatchEvent(event);
+
+    assert.calledWith(fakeBridge.call, 'destroyFrame', 'frame-id');
+  });
+
   function getConfigString(sidebar) {
     return sidebar.iframe.src;
   }

src/sidebar/services/frame-sync.js

@@ -130,15 +130,6 @@ export class FrameSyncService {
       );
     };
 
-    /** @param {string|null} frameIdentifier */
-    const destroyFrame = frameIdentifier => {
-      const frames = store.frames();
-      const frameToDestroy = frames.find(frame => frame.id === frameIdentifier);
-      if (frameToDestroy) {
-        store.destroyFrame(frameToDestroy);
-      }
-    };
-
     /**
      * Listen for messages coming in from connected guest frames and add new annotations
      * to the sidebar.
@@ -163,13 +154,6 @@ export class FrameSyncService {
         annotationsService.create(annot);
       });
 
-      // The `destroyFrame` message currently comes from the guests, but we'll
-      // likely need to route it via the host <-> sidebar channel to work around
-      // a Safari bug (https://bugs.webkit.org/show_bug.cgi?id=231167).
-      this._guestRPC.on('destroyFrame', frameIdentifier =>
-        destroyFrame(frameIdentifier)
-      );
-
       // Map of annotation tag to anchoring status
       // ('anchored'|'orphan'|'timeout').
       //
@@ -269,6 +253,16 @@ 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('destroyFrame', frameIdentifier => {
+      const frame = this._store.frames().find(f => f.id === frameIdentifier);
+      if (frame) {
+        this._store.destroyFrame(frame);
+      }
+    });
+
     // When user toggles the highlight visibility control in the sidebar container,
     // update the visibility in all the guest frames.
     this._hostRPC.on('setVisibleHighlights', state => {

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

@@ -444,7 +444,7 @@ describe('FrameSyncService', () => {
 
     it('removes the frame from the frames list', () => {
       frameSync.connect();
-      guestBridge().emit('destroyFrame', frameId);
+      hostBridge().emit('destroyFrame', frameId);
 
       assert.calledWith(fakeStore.destroyFrame, fixtures.framesListEntry);
     });