Send frame identifier for guests as part of connection setup

Guest frames pass an identifier for their associated browser frame to the
sidebar. The sidebar in turn uses this identifier as a key to refer to different
guests in various places. This identifier used to be sent as part of the
`documentInfoChanged` message after a guest <-> sidebar connection is
established, rather than as part of the initial connection metadata. As a result
the sidebar had to invent a temporary ID for the new guest, which was used until
it learned the real ID.

This commit simplifies the picture by adding a `sourceId` attribute to frame
connection setup messages. This replaces the `frameIdentifier` in the
`documentInfoChanged` message, and allows the sidebar to know the final ID of
the guest as soon as it connects.

src/annotator/guest.js

@@ -192,6 +192,7 @@ export class Guest {
     this._portFinder = new PortFinder({
       hostFrame: this._hostFrame,
       source: 'guest',
+      sourceId: this._frameIdentifier ?? undefined,
     });
 
     this.features = new FeatureFlags();
@@ -316,7 +317,6 @@ export class Guest {
     return {
       uri: normalizeURI(uri),
       metadata,
-      frameIdentifier: this._frameIdentifier,
     };
   }
 

src/annotator/test/guest-test.js

@@ -1354,7 +1354,6 @@ describe('Guest', () => {
         title: 'Test title',
         documentFingerprint: 'test-fingerprint',
       },
-      frameIdentifier: null,
     });
   });
 
@@ -1371,7 +1370,6 @@ describe('Guest', () => {
       metadata: {
         title: 'Page 1',
       },
-      frameIdentifier: null,
     });
 
     sidebarRPCCall.resetHistory();
@@ -1386,7 +1384,6 @@ describe('Guest', () => {
       metadata: {
         title: 'Page 2',
       },
-      frameIdentifier: null,
     });
   });
 

src/shared/messaging/index.js → src/shared/messaging/index.ts

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

src/shared/messaging/port-finder.js

@@ -25,12 +25,14 @@ export class PortFinder {
   /**
    * @param {object} options
    *   @param {Exclude<Frame, 'host'>} options.source - the role of this frame
+   *   @param {string} [options.sourceId] - Identifier for this frame
    *   @param {Window} options.hostFrame - the frame where the `PortProvider` is
    *     listening for messages.
    */
-  constructor({ hostFrame, source }) {
+  constructor({ hostFrame, source, sourceId }) {
     this._hostFrame = hostFrame;
     this._source = source;
+    this._sourceId = sourceId;
     this._listeners = new ListenerCollection();
   }
 
@@ -69,6 +71,7 @@ export class PortFinder {
             frame2: target,
             type: 'request',
             requestId,
+            sourceId: this._sourceId,
           },
           '*'
         );

src/shared/messaging/port-provider.js

@@ -144,7 +144,7 @@ export class PortProvider {
         return;
       }
 
-      const { frame1, frame2, requestId } = data;
+      const { frame1, frame2, requestId, sourceId } = data;
       const channel = /** @type {Channel} */ (`${frame1}-${frame2}`);
 
       if (!isSourceWindow(source)) {
@@ -183,7 +183,11 @@ export class PortProvider {
           ? this._sidebarHostChannel
           : new MessageChannel();
 
-      const message = { frame1, frame2, type: 'offer', requestId };
+      // 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`

src/shared/messaging/port-util.js

 /**
- * These types are the used in by `PortProvider` and `PortFinder` for the
- * inter-frame discovery and communication processes.
+ * Message sent by `PortProvider` and `PortFinder` to establish a
+ * MessageChannel-based connection between two frames.
  *
  * @typedef {'guest'|'host'|'notebook'|'sidebar'} Frame
  *
  * @typedef Message
- * @prop {Frame} frame1
- * @prop {Frame} frame2
- * @prop {'offer'|'request'} type
- * @prop {string} requestId - ID of the request. Used to associate `offer`
- *   responses with requests and enable PortProvider to ignore re-sent requests.
+ * @prop {Frame} frame1 - Role of the source frame
+ * @prop {Frame} frame2 - Role of the target frame
+ * @prop {'offer'|'request'} type - Message type. "request" messages are sent
+ *   by the source frame to the host frame to request a connection. "offer"
+ *   messages are sent from the host frame back to the source frame and also
+ *   to the target frame, accompanied by a MessagePort.
+ * @prop {string} requestId - ID of the request. Used to associate "offer"
+ *   messages with their corresponding "request" messages.
+ * @prop {string} [sourceId] - Identifier for the source frame. This is useful
+ *   in cases where multiple source frames with a given role may connect to
+ *   the same destination frame.
  */
 
 /**

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

@@ -16,8 +16,8 @@ describe('PortFinder', () => {
   let portFinder;
   let portFinders;
 
-  function createPortFinder(source = frame1) {
-    const instance = new PortFinder({ hostFrame: window, source });
+  function createPortFinder(source = frame1, sourceId) {
+    const instance = new PortFinder({ hostFrame: window, source, sourceId });
     portFinders.push(instance);
     return instance;
   }
@@ -99,6 +99,29 @@ describe('PortFinder', () => {
       })
     );
 
+    it('sends port request to host frame', async () => {
+      const clock = sinon.useFakeTimers();
+      try {
+        portFinder = createPortFinder('guest', 'guest-id');
+        portFinder.discover('sidebar');
+        clock.tick(POLLING_INTERVAL_FOR_PORT);
+
+        assert.calledWith(
+          window.postMessage,
+          {
+            frame1: 'guest',
+            frame2: 'sidebar',
+            type: 'request',
+            requestId,
+            sourceId: 'guest-id',
+          },
+          '*'
+        );
+      } finally {
+        clock.restore();
+      }
+    });
+
     [
       { source: 'guest', target: 'host' },
       { source: 'guest', target: 'sidebar' },
@@ -144,7 +167,13 @@ describe('PortFinder', () => {
       assert.callCount(window.postMessage, expectedCalls);
       assert.alwaysCalledWithExactly(
         window.postMessage,
-        { frame1, frame2: target, type: 'request', requestId },
+        {
+          frame1,
+          frame2: target,
+          type: 'request',
+          requestId,
+          sourceId: undefined,
+        },
         '*'
       );
 

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

@@ -158,6 +158,7 @@ describe('PortProvider', () => {
         frame2: 'host',
         type: 'request',
         requestId: 'abcdef',
+        sourceId: undefined,
       };
 
       await sendPortFinderRequest({
@@ -178,6 +179,7 @@ describe('PortProvider', () => {
         frame2: 'sidebar',
         type: 'request',
         requestId: 'abcdef',
+        sourceId: undefined,
       };
 
       await sendPortFinderRequest({ data, origin: 'null' });
@@ -193,6 +195,7 @@ describe('PortProvider', () => {
         frame2: 'sidebar',
         type: 'request',
         requestId: 'abcdef',
+        sourceId: undefined,
       };
 
       for (let i = 0; i < 4; ++i) {
@@ -228,6 +231,7 @@ describe('PortProvider', () => {
         frame2: 'sidebar',
         type: 'request',
         requestId: 'ghijkl',
+        sourceId: 'test-frame',
       };
       await sendPortFinderRequest({
         data,

src/sidebar/services/frame-sync.js

@@ -2,11 +2,17 @@ import debounce from 'lodash.debounce';
 import shallowEqual from 'shallowequal';
 
 import { ListenerCollection } from '../../shared/listener-collection';
-import { PortFinder, PortRPC, isMessageEqual } from '../../shared/messaging';
+import {
+  PortFinder,
+  PortRPC,
+  isMessage,
+  isMessageEqual,
+} from '../../shared/messaging';
 import { isReply, isPublic } from '../helpers/annotation-metadata';
 import { watch } from '../util/watch';
 
 /**
+ * @typedef {import('../../shared/messaging').Message} Message
  * @typedef {import('../../types/annotator').AnnotationData} AnnotationData
  * @typedef {import('../../types/annotator').DocumentMetadata} DocumentMetadata
  * @typedef {import('../../types/api').Annotation} Annotation
@@ -19,7 +25,6 @@ import { watch } from '../util/watch';
 
 /**
  * @typedef DocumentInfo
- * @prop {string|null} frameIdentifier
  * @prop {string} uri
  * @prop {DocumentMetadata} metadata
  */
@@ -120,7 +125,6 @@ export class FrameSyncService {
      * @type {Map<string|null, PortRPC<GuestToSidebarEvent, SidebarToGuestEvent>>}
      */
     this._guestRPC = new Map();
-    this._nextGuestId = 0;
 
     /**
      * Tags of annotations that are currently loaded into guest frames.
@@ -256,21 +260,13 @@ export class FrameSyncService {
    * Set up a connection to a new guest frame.
    *
    * @param {MessagePort} port - Port for communicating with the guest
+   * @param {string|null} sourceId - Identifier for the guest frame
    */
-  _connectGuest(port) {
+  _connectGuest(port, sourceId) {
     /** @type {PortRPC<GuestToSidebarEvent, SidebarToGuestEvent>} */
     const guestRPC = new PortRPC();
 
-    // Add guest RPC to map with a temporary ID until we learn the real ID.
-    //
-    // We need to add the guest to the map immediately so that any notifications
-    // sent from this service to all guests, before we learn the real frame ID,
-    // are sent to this new guest.
-    ++this._nextGuestId;
-    let frameIdentifier = /** @type {string|null} */ (
-      `temp-${this._nextGuestId}`
-    );
-    this._guestRPC.set(frameIdentifier, guestRPC);
+    this._guestRPC.set(sourceId, guestRPC);
 
     // Update document metadata for this guest. We currently assume that the
     // guest will make this call once after it connects. To handle updates
@@ -280,13 +276,8 @@ export class FrameSyncService {
       'documentInfoChanged',
       /** @param {DocumentInfo} info */
       info => {
-        this._guestRPC.delete(frameIdentifier);
-
-        frameIdentifier = info.frameIdentifier;
-        this._guestRPC.set(frameIdentifier, guestRPC);
-
         this._store.connectFrame({
-          id: info.frameIdentifier,
+          id: sourceId,
           metadata: info.metadata,
           uri: info.uri,
         });
@@ -297,12 +288,12 @@ export class FrameSyncService {
     // a certain time frame.
 
     guestRPC.on('close', () => {
-      const frame = this._store.frames().find(f => f.id === frameIdentifier);
+      const frame = this._store.frames().find(f => f.id === sourceId);
       if (frame) {
         this._store.destroyFrame(frame);
       }
       guestRPC.destroy();
-      this._guestRPC.delete(frameIdentifier);
+      this._guestRPC.delete(sourceId);
     });
 
     // A new annotation, note or highlight was created in the frame
@@ -487,14 +478,20 @@ export class FrameSyncService {
     // Listen for guests connecting to the sidebar.
     this._listeners.add(hostPort, 'message', event => {
       const { data, ports } = event;
+
+      const message = /** @type {unknown|Message} */ (data);
+      if (!isMessage(message)) {
+        return;
+      }
+
       if (
-        isMessageEqual(data, {
+        isMessageEqual(message, {
           frame1: 'guest',
           frame2: 'sidebar',
           type: 'offer',
         })
       ) {
-        this._connectGuest(ports[0]);
+        this._connectGuest(ports[0], message.sourceId ?? null);
       }
     });
   }

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

@@ -34,9 +34,6 @@ const fixtures = {
     metadata: {
       link: [],
     },
-
-    // This should match the guest frame ID from `framesListEntry`.
-    frameIdentifier: 'abc',
   },
 };
 
@@ -191,9 +188,10 @@ describe('FrameSyncService', () => {
   /**
    * Simulate a new guest frame connecting to the sidebar.
    *
+   * @param {string} [frameId] - Guest frame ID, or `undefined` for main frame
    * @return {MessagePort} - The port that was sent to the sidebar
    */
-  async function connectGuest() {
+  async function connectGuest(frameId) {
     const { port1 } = new MessageChannel();
     hostPort.postMessage(
       {
@@ -201,6 +199,7 @@ describe('FrameSyncService', () => {
         frame2: 'sidebar',
         type: 'offer',
         requestId: 'abc',
+        sourceId: frameId,
       },
       [port1]
     );
@@ -268,18 +267,16 @@ describe('FrameSyncService', () => {
       // Connect two guests, one representing the main frame and one representing
       // an iframe.
       await connectGuest();
-      await connectGuest();
+      await connectGuest('iframe');
 
       const mainGuestRPC = fakePortRPCs[1];
       const iframeGuestRPC = fakePortRPCs[2];
 
       mainGuestRPC.emit('documentInfoChanged', {
-        frameIdentifier: null,
         uri: mainFrameAnn.uri,
       });
 
       iframeGuestRPC.emit('documentInfoChanged', {
-        frameIdentifier: 'iframe',
         uri: iframeAnn.uri,
       });
 
@@ -310,8 +307,6 @@ describe('FrameSyncService', () => {
       // what happens in VitalSource for example.
       await connectGuest();
       emitGuestEvent('documentInfoChanged', {
-        frameIdentifier: 'iframe',
-
         // Note that URI does not match annotation URI. The backend can still return
         // the annotation for this frame based on URI equivalence information.
         uri: 'https://publisher.com/books/1234/chapter1.html',
@@ -573,14 +568,16 @@ describe('FrameSyncService', () => {
       frameSync.connect();
     });
 
-    it("adds the page's metadata to the frames list", async () => {
+    it('adds guest frame details to the store', async () => {
       const frameInfo = fixtures.htmlDocumentInfo;
-      await connectGuest();
+      const frameId = 'test-frame';
+
+      await connectGuest(frameId);
       emitGuestEvent('documentInfoChanged', frameInfo);
 
       assert.deepEqual(fakeStore.frames(), [
         {
-          id: frameInfo.frameIdentifier,
+          id: frameId,
           metadata: frameInfo.metadata,
           uri: frameInfo.uri,
 
@@ -642,7 +639,6 @@ describe('FrameSyncService', () => {
       await connectGuest();
 
       emitGuestEvent('documentInfoChanged', {
-        frameIdentifier: 'abc',
         uri: 'http://example.org',
       });
       emitGuestEvent('close');