Apply suggestions from second code review

Co-authored-by: Robert Knight <robertknight@gmail.com>

src/shared/port-finder.js

 import { ListenerCollection } from './listener-collection';
 import { isMessageEqual } from './port-util';
 
-const MAX_WAIT_FOR_PORT = 1000 * 30;
+const MAX_WAIT_FOR_PORT = 1000 * 5;
 const POLLING_INTERVAL_FOR_PORT = 250;
 
 /**
@@ -12,14 +12,14 @@ const POLLING_INTERVAL_FOR_PORT = 250;
  */
 
 /**
- * PortFinder helps to discover `MessagePort` on a specific channel.
+ * PortFinder is used by non-host frames in the client to establish a
+ * MessagePort-based connection to other frames. It is used together with
+ * PortProvider which runs in the host frame. See PortProvider for an overview.
  *
  * Channel nomenclature is `[frame1]-[frame2]` so that:
  *   - `port1` should be owned by/transferred to `frame1`, and
  *   - `port2` should be owned by/transferred to `frame2`
  *
- * There should be the same amount of listener in this class as in PortProvider.
- *
  * @implements Destroyable
  */
 export class PortFinder {
@@ -32,7 +32,7 @@ export class PortFinder {
   }
 
   /**
-   * Polls the hostFrame for a specific port and returns a Promise of the port.
+   * Request a specific port from `hostFrame`
    *
    * @param {object} options
    *   @param {Channel} options.channel - requested channel
@@ -41,7 +41,7 @@ export class PortFinder {
    *   @param {Port} options.port - requested port
    * @return {Promise<MessagePort>}
    */
-  discover({ channel, hostFrame, port }) {
+  async discover({ channel, hostFrame, port }) {
     let isValidRequest = false;
     if (
       (channel === 'guest-host' && port === 'guest') ||
@@ -52,12 +52,11 @@ export class PortFinder {
       isValidRequest = true;
     }
 
-    return new Promise((resolve, reject) => {
-      if (!isValidRequest) {
-        reject(new Error('Invalid request of channel/port'));
-        return;
-      }
+    if (!isValidRequest) {
+      throw new Error('Invalid request of channel/port');
+    }
 
+    return new Promise((resolve, reject) => {
       function postRequest() {
         hostFrame.postMessage(
           { channel, port, source: 'hypothesis', type: 'request' },
@@ -65,13 +64,16 @@ export class PortFinder {
         );
       }
 
-      const intervalId = setInterval(
-        () => postRequest(),
-        POLLING_INTERVAL_FOR_PORT
-      );
+      // In some situations, because `guest` iframe/s load in parallel to the `host`
+      // frame, we can not assume that the code in the `host` frame is executed before
+      // the code in a `guest` frame. Hence, we can't assume that `PortProvider` (in
+      // the `host` frame) is initialized before `PortFinder` (in the non-host frames).
+      // Therefore, for the `PortFinder`, we implement a polling strategy (sending a
+      // message every N milliseconds) until a response is received.
+      const intervalId = setInterval(postRequest, POLLING_INTERVAL_FOR_PORT);
 
       // The `host` frame maybe busy, that's why we should wait.
-      const timeoutId = window.setTimeout(() => {
+      const timeoutId = setTimeout(() => {
         clearInterval(intervalId);
         reject(
           new Error(`Unable to find '${port}' port on '${channel}' channel`)

src/shared/port-provider.js

@@ -4,7 +4,6 @@ import { ListenerCollection } from './listener-collection';
 import { isMessageEqual } from './port-util';
 
 /**
- * @typedef {import('../types/config').SidebarConfig} SidebarConfig
  * @typedef {import('../types/annotator').Destroyable} Destroyable
  * @typedef {import('./port-util').Message} Message
  * @typedef {Message['channel']} Channel
@@ -12,18 +11,18 @@ import { isMessageEqual } from './port-util';
  */
 
 /**
- * PortProvider creates a `MessageChannel` for the communication between two
+ * PortProvider creates a `MessageChannel` for communication between two
  * frames.
  *
  * There are 4 types of frames:
  * - `host`: frame where the hypothesis client is initially loaded.
- * - `guest/s`: frame/s with annotatable content. In some instances the `guest`
+ * - `guest`: frames with annotatable content. In some instances a `guest`
  *    frame can be the same as the `host` frame, in other cases, it is an iframe
  *    where either (1) the hypothesis client has been injected or (2) the
  *    hypothesis client has been configured to act exclusively as a `guest` (not
  *    showing the sidebar).
- * - `notebook`: it is an another hypothesis app that runs on a separate iframe.
- * - `sidebar`: the main hypothesis app. It runs on an iframe on a different
+ * - `notebook`: another hypothesis app that runs in a separate iframe.
+ * - `sidebar`: the main hypothesis app. It runs in an iframe on a different
  *    origin than the host and is responsible for the communication with the
  *    backend (fetching and saving annotations).
  *
@@ -32,9 +31,9 @@ import { isMessageEqual } from './port-util';
  * `host` frame
  * |-> `sidebar` iframe
  * |-> `notebook` iframe
- * |-> [`guest` iframe/s]
+ * |-> [`guest` iframes]
  *
- * Currently, we support the communication between the following pair of frames:
+ * Currently, we support the communication between the following pairs of frames:
  * - `guest-host`
  * - `guest-sidebar`
  * - `host-sidebar`
@@ -46,16 +45,15 @@ import { isMessageEqual } from './port-util';
  * particular `MessagePort` and dispatches the corresponding `MessagePort`.
  *
  *
- *                 PortProvider                                       |                PortFinder
- * -------------------------------------------------------------------|------------------------------------------------------
- *
- * 2. listens to requests of `MessagePort` <---------------------------- 1. request a `MessagePort` using `window#postMessage`
- *                 |
- *                 V
- * 3. sends offers of `MessagePort` using `event#source#postMessage` ---> 4. listens to offers of `MessagePort`
- *                 |
- *                 V
- * 5. send reciprocal port to the `sidebar` frame using the `host-sidebar`
+ *        PortFinder (non-host frame)                 |       PortProvider (host frame)
+ * -----------------------------------------------------------------------------------------------
+ * 1. Request `MessagePort` via `window.postMessage` ---> 2. Receive request and create port pair
+ *                                                                          |
+ *                                                                          V
+ * 4. Receive requested port      <---------------------- 3. Send first port to requesting frame
+ *                                                        5. Send second port to other frame
+ *                                                           (eg. via MessageChannel connection
+ *                                                           between host and other frame)
  *
  * Channel nomenclature is `[frame1]-[frame2]` so that:
  *   - `port1` should be owned by/transferred to `frame1`, and
@@ -63,20 +61,6 @@ import { isMessageEqual } from './port-util';
  *
  * @implements Destroyable
  */
-
-/*
- * In some situations, because `guest` iframe/s load in parallel to the `host`
- * frame, we can not assume that the code in the `host` frame is executed before
- * the code in a `guest` frame. Hence, we can't assume that `PortProvider` (in
- * the `host` frame) is initialized before `PortFinder` (in the `guest` frame).
- * Therefore, for the `PortFinder`, we implement a polling strategy (sending a
- * message every N milliseconds) until a response is received.
- *
- * Two important characteristics of `MessagePort`:
- * - it can only be used by one frame: in Chrome the port is neutered if transferred twice
- * - messages are queued until the other port is ready to listen (`port.start()`)
- */
-
 export class PortProvider {
   /**
    * @param {string} hypothesisAppsOrigin - the origin of the hypothesis apps
@@ -96,6 +80,12 @@ export class PortProvider {
     /** @type {Map<Channel, Map<Window, MessageChannel>>} */
     this._channels = new Map();
 
+    // Two important characteristics of `MessagePort`:
+    // - Once created, a MessagePort can only be transferred to a different
+    //   frame once, and if any frame attempts to transfer it again it gets
+    //   neutered.
+    // - Messages are queued until the other port is ready to listen (`port.start()`)
+
     // Create the `host-sidebar` channel immediately, while other channels are
     // created on demand
     this._hostSidebarChannel = new MessageChannel();
@@ -129,11 +119,7 @@ export class PortProvider {
       return false;
     }
 
-    if (!isMessageEqual(data, allowedMessage)) {
-      return false;
-    }
-
-    return true;
+    return isMessageEqual(data, allowedMessage);
   }
 
   /**
@@ -141,30 +127,32 @@ export class PortProvider {
    *
    * @param {MessageEvent} event
    * @param {Message} message - the message to be sent.
-   * @param {MessagePort} port - the port to be sent via `window#postMessage`
-   *   (the origin is set to match the MessageEvent's origin)frame that sends the initial request th
-   * @param {MessagePort} [reciprocalPort] - if a reciprocal port is provided,
-   *   send this port (1) to the `sidebar` frame using the `host-sidebar`
+   * @param {MessagePort} port - the port requested.
+   * @param {MessagePort} [counterpartPort] - if a counterpart port is provided,
+   *   send this port either, (1) to the `sidebar` frame using the `host-sidebar`
    *   channel or (2) through the `onHostPortRequest` event listener.
    */
-  _sendPort(event, message, port, reciprocalPort) {
+  _sendPort(event, message, port, counterpartPort) {
     const source = /** @type {Window} */ (event.source);
 
     source.postMessage(message, event.origin, [port]);
 
-    if (reciprocalPort) {
-      if (['notebook-sidebar', 'guest-sidebar'].includes(message.channel)) {
-        this._hostSidebarChannel.port1.postMessage(message, [reciprocalPort]);
-      }
-      if (message.channel === 'guest-host' && message.port === 'guest') {
-        this._emitter.emit('hostPortRequest', reciprocalPort, message.port);
-      }
+    if (!counterpartPort) {
+      return;
+    }
+
+    if (['notebook-sidebar', 'guest-sidebar'].includes(message.channel)) {
+      this._hostSidebarChannel.port1.postMessage(message, [counterpartPort]);
+    }
+
+    if (message.channel === 'guest-host' && message.port === 'guest') {
+      this._emitter.emit('hostPortRequest', message.port, counterpartPort);
     }
   }
 
   /**
    * @param {'hostPortRequest'} eventName
-   * @param {(MessagePort, channel: 'guest') => void} handler - this handler
+   * @param {(source: 'guest', port: MessagePort) => void} handler - this handler
    *   fires when a request for the 'guest-host' channel is listened.
    */
   on(eventName, handler) {
@@ -173,7 +161,7 @@ export class PortProvider {
 
   /**
    * Returns a port from a channel. Currently, only returns the `host` port from
-   * the `host-Sidebar` channel. Otherwise, it returns `null`.
+   * the `host-sidebar` channel. Otherwise, it returns `null`.
    *
    * @param {object} options
    *   @param {'host-sidebar'} options.channel
@@ -238,7 +226,7 @@ export class PortProvider {
         let messageChannel = windowChannelMap.get(eventSource);
 
         // Ignore the port request if the channel for the specified window has
-        // already been created. This is to avoid transfer the port more than once.
+        // already been created. This is to avoid transfering the port more than once.
         if (messageChannel) {
           return;
         }

src/shared/port-util.js

@@ -8,6 +8,7 @@ const SOURCE = 'hypothesis';
 /**
  * These types are the used in by `PortProvider` and `PortFinder` for the
  * inter-frame discovery and communication processes.
+ *
  * @typedef {'guest-host'|'guest-sidebar'|'host-sidebar'|'notebook-sidebar'} Channel
  * @typedef {'guest'|'host'|'notebook'|'sidebar'} Port
  *
@@ -19,23 +20,19 @@ const SOURCE = 'hypothesis';
  */
 
 /**
- * The function checks if the data conforms to the expected format. It returns
- * `true` if all the properties are including the correct value in the `source`
- *  property, otherwise it returns `false`.
+ * Return true if an object, eg. from the data field of a `MessageEvent`, is a
+ * valid `Message`.
  *
  * @param {any} data
  * @return {data is Message}
  */
 function isMessageValid(data) {
-  if (!data || typeof data !== 'object') {
+  if (data === null || typeof data !== 'object') {
     return false;
   }
 
   for (let property of ['channel', 'port', 'source', 'type']) {
-    if (
-      data.hasOwnProperty(property) === false ||
-      typeof data[property] !== 'string'
-    ) {
+    if (typeof data[property] !== 'string') {
       return false;
     }
   }
@@ -59,7 +56,7 @@ export function isMessageEqual(data, message) {
       JSON.stringify(data, Object.keys(data).sort()) ===
       JSON.stringify(message, Object.keys(message).sort())
     );
-  } catch (error) {
+  } catch {
     return false;
   }
 }

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

 import { delay } from '../../test-util/wait';
 import { PortFinder } from '../port-finder';
 
+const MAX_WAIT_FOR_PORT = 1000 * 5;
+
 describe('PortFinder', () => {
   let portFinder;
 
-  function sendMessage({ data, ports = [] }) {
+  function portProviderOffer({ data, ports = [] }) {
     const event = new MessageEvent('message', {
       data,
       ports,
@@ -42,11 +44,11 @@ describe('PortFinder', () => {
           })
           .catch(e => (error = e));
         portFinder.destroy();
-        sendMessage({
+        portProviderOffer({
           data: { channel, port, source: 'hypothesis', type: 'offer' },
           ports: [port1],
         });
-        clock.tick(30000);
+        clock.tick(MAX_WAIT_FOR_PORT);
       } finally {
         clock.restore();
       }
@@ -98,7 +100,7 @@ describe('PortFinder', () => {
             port,
           })
           .then(port => (resolvedPort = port));
-        sendMessage({
+        portProviderOffer({
           data: { channel, port, source: 'hypothesis', type: 'offer' },
           ports: [port1],
         });
@@ -108,7 +110,7 @@ describe('PortFinder', () => {
       })
     );
 
-    it("timeouts if host doesn't respond", async () => {
+    it("times out if host doesn't respond", async () => {
       let error;
       const channel = 'host-sidebar';
       const port = 'sidebar';
@@ -122,12 +124,12 @@ describe('PortFinder', () => {
             port,
           })
           .catch(e => (error = e));
-        clock.tick(30000);
+        clock.tick(MAX_WAIT_FOR_PORT);
       } finally {
         clock.restore();
       }
 
-      assert.callCount(window.postMessage, 121);
+      assert.callCount(window.postMessage, 21);
       assert.alwaysCalledWithExactly(
         window.postMessage,
         { channel, port, source: 'hypothesis', type: 'request' },

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

@@ -6,7 +6,7 @@ const source = 'hypothesis';
 describe('PortProvider', () => {
   let portProvider;
 
-  function sendMessage({
+  function portFinderRequest({
     data,
     origin = window.location.origin,
     source = window,
@@ -36,7 +36,7 @@ describe('PortProvider', () => {
   describe('#destroy', () => {
     it('ignores valid port request if `PortFinder` has been destroyed', async () => {
       portProvider.destroy();
-      sendMessage({
+      portFinderRequest({
         data: {
           channel: 'host-sidebar',
           port: 'sidebar',
@@ -79,7 +79,7 @@ describe('PortProvider', () => {
   });
 
   describe('#listen', () => {
-    it('ignores all port request until start listening', async () => {
+    it('ignores all port requests before `listen` is called', async () => {
       portProvider.destroy();
       portProvider = new PortProvider(window.location.origin);
       const data = {
@@ -88,7 +88,7 @@ describe('PortProvider', () => {
         source,
         type: 'request',
       };
-      sendMessage({
+      portFinderRequest({
         data,
       });
       await delay(0);
@@ -96,7 +96,7 @@ describe('PortProvider', () => {
       assert.notCalled(window.postMessage);
 
       portProvider.listen();
-      sendMessage({
+      portFinderRequest({
         data,
       });
       await delay(0);
@@ -114,12 +114,12 @@ describe('PortProvider', () => {
         type: 'request',
       };
 
-      sendMessage({
+      portFinderRequest({
         data,
         source: null,
       });
 
-      sendMessage({
+      portFinderRequest({
         data,
         source: new MessageChannel().port1,
       });
@@ -128,41 +128,64 @@ describe('PortProvider', () => {
       assert.notCalled(window.postMessage);
     });
 
-    it('ignores port request with invalid message data', async () => {
-      sendMessage({
+    [
+      // Disabled this check because it make axes-core to crash
+      // Reported: https://github.com/dequelabs/axe-core/pull/3249
+      //{ data: null, reason: 'if message is null' },
+      {
         data: {
-          channel: 'dummy1-dummy2', // invalid channel
+          channel: 'sidebar-host', // invalid channel (swapped words)
           port: 'sidebar',
           source,
           type: 'request',
         },
-      });
-
-      // Disabled this check because it make axes-core to crash
-      // Reported: https://github.com/dequelabs/axe-core/pull/3249
-      // sendMessage({
-      //   data: null,
-      // });
-
-      await delay(0);
+        reason: 'if message contains an invalid channel',
+      },
+      {
+        data: {
+          channel: 'host-sidebar',
+          port: 'host', // invalid port
+          source,
+          type: 'request',
+        },
+        reason: 'if message contains an invalid port',
+      },
+      {
+        data: {
+          channel: 'host-sidebar',
+          port: 'sidebar',
+          source: 'dummy',
+          type: 'request',
+        },
+        reason: 'if message contains an invalid source',
+      },
+      {
+        data: {
+          channel: 'host-sidebar',
+          port: 'dummy',
+          source,
+          type: 'offer', // invalid offer
+        },
+        reason: 'if message contains an invalid offer',
+      },
+      {
+        data: {
+          channel: 'host-sidebar',
+          port: 'sidebar',
+          source,
+          type: 'request',
+        },
+        origin: 'https://dummy.com',
+        reason: 'if message comes from invalid origin',
+      },
+    ].forEach(({ data, reason, origin }) => {
+      it(`ignores port request ${reason}`, async () => {
+        portFinderRequest({ data, origin: origin ?? window.location.origin });
 
-      assert.notCalled(window.postMessage);
-    });
+        await delay(0);
 
-    it('ignores port request with invalid message origin', async () => {
-      const data = {
-        channel: 'host-sidebar',
-        port: 'sidebar',
-        source,
-        type: 'request',
-      };
-      sendMessage({
-        data,
-        origin: 'https://dummy.com',
+        assert.notCalled(window.postMessage);
       });
-      await delay(0);
-
-      assert.notCalled(window.postMessage);
     });
 
     it('responds to a valid port request', async () => {
@@ -172,7 +195,7 @@ describe('PortProvider', () => {
         source,
         type: 'request',
       };
-      sendMessage({
+      portFinderRequest({
         data,
       });
       await delay(0);
@@ -187,14 +210,14 @@ describe('PortProvider', () => {
 
     it('responds to the first valid port request, ignore additional requests', async () => {
       const data = {
-        channel: 'host-sidebar',
-        port: 'sidebar',
+        channel: 'guest-host',
+        port: 'guest',
         source,
         type: 'request',
       };
 
       for (let i = 0; i < 4; ++i) {
-        sendMessage({
+        portFinderRequest({
           data,
         });
       }
@@ -208,8 +231,8 @@ describe('PortProvider', () => {
       );
     });
 
-    it('sends the reciprocal port of the `guest-sidebar` channel (via the sidebar port)', async () => {
-      sendMessage({
+    it('sends the counterpart port via the sidebar port', async () => {
+      portFinderRequest({
         data: {
           channel: 'host-sidebar',
           port: 'sidebar',
@@ -222,13 +245,14 @@ describe('PortProvider', () => {
       const [sidebarPort] = window.postMessage.getCall(0).args[2];
       const handler = sinon.stub();
       sidebarPort.onmessage = handler;
+
       const data = {
         channel: 'guest-sidebar',
         port: 'guest',
         source,
         type: 'request',
       };
-      sendMessage({
+      portFinderRequest({
         data,
       });
       await delay(0);
@@ -241,7 +265,7 @@ describe('PortProvider', () => {
       );
     });
 
-    it('sends the reciprocal port of the `guest-host` channel (via listener)', async () => {
+    it('sends the counterpart port via the listener', async () => {
       const handler = sinon.stub();
       portProvider.on('hostPortRequest', handler);
       const data = {
@@ -250,12 +274,12 @@ describe('PortProvider', () => {
         source,
         type: 'request',
       };
-      sendMessage({
+      portFinderRequest({
         data,
       });
       await delay(0);
 
-      assert.calledWith(handler, sinon.match.instanceOf(MessagePort), 'guest');
+      assert.calledWith(handler, 'guest', sinon.match.instanceOf(MessagePort));
     });
   });
 });

src/shared/test/port-util-test.js

@@ -103,7 +103,7 @@ describe('port-util', () => {
           port: 'guest',
           source,
           type: 'offer',
-          window, // no serializable
+          window, // not serializable
         },
         message: {
           channel: 'host-sidebar',
@@ -128,7 +128,7 @@ describe('port-util', () => {
           type: 'offer',
         },
         expectedResult: false,
-        reason: 'data has one property that is different ',
+        reason: 'data has one property that is different',
       },
     ].forEach(({ data, message, expectedResult, reason }) => {
       it(`returns '${expectedResult}' because the ${reason}`, () => {