Fix port discovery after a guest iframe is navigated

PortProvider previously used a `WeakMap<Window, Set<Channel>>` to keep
track of which ports had been requested from a particular source frame,
where the map keys came from `MessageEvent.source`. A problem with this
approach is that for messages from an iframe, `MessageEvent.source` refers to a
`WindowProxy` which retains the same identity across page navigations.
As a result if a guest iframe was navigated to a new page which also
embedded the client as a guest (or into which the client was
subsequently injected as a guest), the guest in the new document would
not be able to retrieve any ports.

Implement a fix whereby each call to `PortFinder.discover` generates a
random request ID which is sent with the initial port request plus any
re-sent requests. PortProvider then keeps track of the request IDs it
has seen and ignores messages with a previously seen request ID.

This change would also fix port discovery if the guest in a frame was
unloaded and later re-loaded.

src/shared/messaging/port-finder.js

 import { ListenerCollection } from '../listener-collection';
+import { generateHexString } from '../random';
 import { isMessageEqual } from './port-util';
 
 /** Timeout for waiting for the host frame to respond to a port request. */
@@ -58,6 +59,8 @@ export class PortFinder {
       throw new Error('Invalid request of channel/port');
     }
 
+    const requestId = generateHexString(6);
+
     return new Promise((resolve, reject) => {
       const postRequest = () => {
         this._hostFrame.postMessage(
@@ -65,6 +68,7 @@ export class PortFinder {
             frame1: this._source,
             frame2: target,
             type: 'request',
+            requestId,
           },
           '*'
         );
@@ -96,6 +100,7 @@ export class PortFinder {
             frame1: this._source,
             frame2: target,
             type: 'offer',
+            requestId,
           })
         ) {
           clearInterval(intervalId);

src/shared/messaging/port-provider.js

@@ -71,15 +71,15 @@ export class PortProvider {
     this._emitter = new TinyEmitter();
 
     /**
-     * Map of source frame to channels that have been created for this frame.
+     * IDs of port requests that have been handled.
      *
      * This is used to avoid responding to the same request multiple times.
      * Guest frames in particular may send duplicate requests (see comments in
      * PortFinder).
      *
-     * @type {WeakMap<Window, Set<Channel>>}
+     * @type {Set<string>}
      */
-    this._sentChannels = new WeakMap();
+    this._handledRequests = new Set();
 
     // Create the `sidebar-host` channel immediately, while other channels are
     // created on demand
@@ -87,7 +87,7 @@ export class PortProvider {
 
     this._listeners = new ListenerCollection();
 
-    /** @type {Array<Message & {allowedOrigin: string}>} */
+    /** @type {Array<Partial<Message> & {allowedOrigin: string}>} */
     this._allowedMessages = [
       {
         allowedOrigin: '*',
@@ -139,12 +139,12 @@ export class PortProvider {
     const handleRequest = event => {
       const { data, origin, source } = /** @type {MessageEvent} */ (event);
 
-      if (!isMessage(data) || data.type !== 'request') {
+      if (!isMessage(data) || data.type !== 'request' || !data.requestId) {
         // If this does not look like a message intended for us, ignore it.
         return;
       }
 
-      const { frame1, frame2 } = data;
+      const { frame1, frame2, requestId } = data;
       const channel = /** @type {Channel} */ (`${frame1}-${frame2}`);
 
       if (!isSourceWindow(source)) {
@@ -171,17 +171,10 @@ export class PortProvider {
         return;
       }
 
-      // Check if this request has already been received from this frame and
-      // ignore it if so.
-      let sentChannels = this._sentChannels.get(source);
-      if (!sentChannels) {
-        sentChannels = new Set();
-        this._sentChannels.set(source, sentChannels);
-      }
-      if (sentChannels.has(channel)) {
+      if (this._handledRequests.has(requestId)) {
         return;
       }
-      sentChannels.add(channel);
+      this._handledRequests.add(requestId);
 
       // Create the channel for these two frames to communicate and send the
       // corresponding ports to them.
@@ -190,7 +183,7 @@ export class PortProvider {
           ? this._sidebarHostChannel
           : new MessageChannel();
 
-      const message = { frame1, frame2, type: 'offer' };
+      const message = { frame1, frame2, type: 'offer', 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`
@@ -220,7 +213,7 @@ export class PortProvider {
 
   /**
    * @param {object} options
-   *   @param {Message} options.allowedMessage - the `data` must match this
+   *   @param {Partial<Message>} options.allowedMessage - the `data` must match this
    *     `Message`.
    *   @param {string} options.allowedOrigin - the `origin` must match this
    *     value. If `allowedOrigin` is '*', the origin is ignored.

src/shared/messaging/port-util.js

@@ -8,6 +8,8 @@
  * @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.
  */
 
 /**
@@ -35,7 +37,7 @@ export function isMessage(data) {
  * Return true if the data payload from a MessageEvent matches `message`.
  *
  * @param {any} data
- * @param {Message} message
+ * @param {Partial<Message>} message
  */
 export function isMessageEqual(data, message) {
   if (!isMessage(data)) {

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

@@ -3,9 +3,14 @@ import {
   MAX_WAIT_FOR_PORT,
   POLLING_INTERVAL_FOR_PORT,
   PortFinder,
+  $imports,
 } from '../port-finder';
 
+const requestId = 'abcdef';
+
 describe('PortFinder', () => {
+  let fakeGenerateHexString;
+
   const frame1 = 'guest';
   const type = 'offer';
   let portFinder;
@@ -28,13 +33,21 @@ describe('PortFinder', () => {
     return event;
   }
 
+  // Generate predictable IDs for port requests
+  fakeGenerateHexString = sinon.stub().returns(requestId);
+
   beforeEach(() => {
     portFinders = [];
     sinon.stub(window, 'postMessage');
     portFinder = createPortFinder();
+
+    $imports.$mock({
+      '../random': { generateHexString: fakeGenerateHexString },
+    });
   });
 
   afterEach(() => {
+    $imports.$restore();
     window.postMessage.restore();
     portFinders.forEach(instance => instance.destroy());
   });
@@ -103,6 +116,7 @@ describe('PortFinder', () => {
             frame1: source,
             frame2: target,
             type,
+            requestId,
           },
           ports: [port1],
         });
@@ -130,7 +144,7 @@ describe('PortFinder', () => {
       assert.callCount(window.postMessage, expectedCalls);
       assert.alwaysCalledWithExactly(
         window.postMessage,
-        { frame1, frame2: target, type: 'request' },
+        { frame1, frame2: target, type: 'request', requestId },
         '*'
       );
 

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

@@ -40,6 +40,7 @@ describe('PortProvider', () => {
           frame1: 'sidebar',
           frame2: 'host',
           type: 'request',
+          requestId: 'abcdef',
         },
       });
 
@@ -60,14 +61,24 @@ describe('PortProvider', () => {
 
     it('reports errors validating messages', async () => {
       await sendPortFinderRequest({
-        data: { frame1: 'sidebar', frame2: 'invalid', type: 'request' },
+        data: {
+          frame1: 'sidebar',
+          frame2: 'invalid',
+          type: 'request',
+          requestId: 'abcdef',
+        },
       });
       assert.called(fakeSendError);
     });
 
     it('only reports errors once', async () => {
       const request = {
-        data: { frame1: 'sidebar', frame2: 'invalid', type: 'request' },
+        data: {
+          frame1: 'sidebar',
+          frame2: 'invalid',
+          type: 'request',
+          requestId: 'abcdef',
+        },
       };
       await sendPortFinderRequest(request);
       await sendPortFinderRequest(request);
@@ -87,6 +98,7 @@ describe('PortProvider', () => {
           frame1: 'dummy', // invalid source
           frame2: 'host',
           type: 'request',
+          requestId: 'abcdef',
         },
         reason: 'contains an invalid frame1',
       },
@@ -95,6 +107,7 @@ describe('PortProvider', () => {
           frame1: 'sidebar',
           frame2: 'dummy', // invalid target
           type: 'request',
+          requestId: 'abcdef',
         },
         reason: 'contains an invalid frame2',
       },
@@ -103,6 +116,7 @@ describe('PortProvider', () => {
           frame1: 'sidebar',
           frame2: 'host',
           type: 'dummy', // invalid type
+          requestId: 'abcdef',
         },
         reason: 'contains an invalid type',
       },
@@ -111,6 +125,7 @@ describe('PortProvider', () => {
           frame1: 'sidebar',
           frame2: 'host',
           type: 'request',
+          requestId: 'abcdef',
         },
         source: null,
         reason: 'comes from invalid source',
@@ -120,6 +135,7 @@ describe('PortProvider', () => {
           frame1: 'sidebar',
           frame2: 'host',
           type: 'request',
+          requestId: 'abcdef',
         },
         origin: 'https://dummy.com',
         reason: 'comes from invalid origin',
@@ -141,6 +157,7 @@ describe('PortProvider', () => {
         frame1: 'sidebar',
         frame2: 'host',
         type: 'request',
+        requestId: 'abcdef',
       };
 
       await sendPortFinderRequest({
@@ -160,6 +177,7 @@ describe('PortProvider', () => {
         frame1: 'guest',
         frame2: 'sidebar',
         type: 'request',
+        requestId: 'abcdef',
       };
 
       await sendPortFinderRequest({ data, origin: 'null' });
@@ -174,6 +192,7 @@ describe('PortProvider', () => {
         frame1: 'guest',
         frame2: 'sidebar',
         type: 'request',
+        requestId: 'abcdef',
       };
 
       for (let i = 0; i < 4; ++i) {
@@ -196,6 +215,7 @@ describe('PortProvider', () => {
           frame1: 'sidebar',
           frame2: 'host',
           type: 'request',
+          requestId: 'abcdef',
         },
       });
 
@@ -207,6 +227,7 @@ describe('PortProvider', () => {
         frame1: 'guest',
         frame2: 'sidebar',
         type: 'request',
+        requestId: 'ghijkl',
       };
       await sendPortFinderRequest({
         data,
@@ -228,6 +249,7 @@ describe('PortProvider', () => {
           frame1: 'sidebar',
           frame2: 'host',
           type: 'request',
+          requestId: 'abcdef',
         },
       });
 
@@ -243,6 +265,7 @@ describe('PortProvider', () => {
           frame1: 'guest',
           frame2: 'host',
           type: 'request',
+          requestId: 'ghijkl',
         },
       });
 

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

@@ -7,6 +7,7 @@ describe('port-util', () => {
         frame1: 'guest',
         frame2: 'sidebar',
         type: 'request',
+        requestId: 'abcdef',
       },
 
       {
@@ -38,6 +39,7 @@ describe('port-util', () => {
     const frame1 = 'guest';
     const frame2 = 'sidebar';
     const type = 'offer';
+    const requestId = 'abcdef';
 
     [
       {
@@ -45,6 +47,7 @@ describe('port-util', () => {
           frame1,
           frame2,
           type,
+          requestId,
         },
         expectedResult: true,
         reason: 'data matches the message',
@@ -54,6 +57,7 @@ describe('port-util', () => {
           frame1,
           frame2,
           type,
+          requestId,
         },
         expectedResult: true,
         reason: 'data matches the message (properties in different order)',
@@ -73,6 +77,7 @@ describe('port-util', () => {
           // frame1 property missing
           frame2,
           type,
+          requestId,
         },
         expectedResult: false,
         reason: 'data has one property that is missing',
@@ -82,6 +87,7 @@ describe('port-util', () => {
           frame1,
           frame2: 9, // wrong type
           type,
+          requestId,
         },
         expectedResult: false,
         reason: 'data has one property with a wrong type',
@@ -91,6 +97,7 @@ describe('port-util', () => {
           frame1: 'dummy', // different
           frame2,
           type,
+          requestId,
         },
         expectedResult: false,
         reason: 'data has one property that is different',