Convert host <-> guest communication to use MessageChannel #3611

robertknight · 2021-07-26T11:34:51Z

This commit implements the initial transition of communication between
guest/host frames and the sidebar to use MessageChannel as the
underlying transport instead of window.postMessage, building on
previous changes to the Bridge and RPC classes.

This change aligns with an ongoing to plan to use MessageChannel
instead of window.postMessage, and also resolves an issue where guest
frames may fail to connect to the sidebar if the annotator code loads in
the guest before the sidebar application has finished loading.

This initial version has several limitations compared to what is planned
for the final version of the new inter-frame communication system:

It only supports guest frames which are the same frame as the host or
a direct, same-origin child.
Sidebar <-> host communication relies on the host frame also being a
guest frame. In order for the host frame to receive messages from the
sidebar, it must run the logic to establish sidebar <-> guest communication.
In other words, it is not possible to have a host frame which cannot
also be annotated.
The only supported roles for frames are sidebar and host/guest. There
is no separate role for notebook frames for use in notebook <->
sidebar communication.

Making this change involved replacing the protocol used by guest frames
and the sidebar to discover each other. The new one is a minimal protocol
which is intended to be temporary and replaced with a different one in future
that addresses the above limitations (see shared/communicator.js in this draft).

When the sidebar application starts up it notifies the parent frame that
it is ready to connect to guests via a hypothesisSidebarReady
message.
Guest frames wait for this notification to be received before
connecting to the sidebar. When they connect, they create a
MessageChannel and send one port to the sidebar via a
hypothesisGuestReady message and use the other port locally.
When the sidebar receives a hypothesisGuestReady message it creates
a channel to communicate with the guest.

This commit implements the initial transition of communication between guest/host frames and the sidebar to use `MessageChannel` as the underlying transport instead of `window.postMessage`, building on previous changes to the `Bridge` and `RPC` classes. This change aligns with an ongoing to plan to use `MessageChannel` instead of `window.postMessage`, and also resolves an issue where guest frames may fail to connect to the sidebar if the annotator code loads in the guest before the sidebar application has finished loading. This initial version has several limitations compared to what is planned for the final version of the new inter-frame communication system: - It only supports guest frames which are the same frame as the host or a direct, same-origin child. - Sidebar <-> host communication relies on the host frame also being a guest frame. In order for the host frame to receive messages from the sidebar, it must run the logic to establish sidebar <-> guest communication. In other words, it is not possible to have a host frame which cannot also be annotated. - The only supported roles for frames are sidebar and host/guest. There is no separate role for notebook frames for use in notebook <-> sidebar communication. Making this change involved replacing the protocol used by guest frames and the sidebar to discover each other. The new one is simpler than the previous one and works as follows: 1. When the sidebar application starts up it notifies the parent frame that it is ready to connect to guests via a `hypothesisSidebarReady` message. 2. Guest frames wait for this notification to be received before connecting to the sidebar. When they connect, they create a `MessageChannel` and send one port to the sidebar via a `hypothesisGuestReady` message and use the other port locally. 3. When the sidebar receives a `hypothesisGuestReady` message it creates a channel to communicate with the guest.

src/sidebar/services/frame-sync.js

esanzgar

The approach suggested here is sensible transition. @robertknight has well identified all the limitations

A left a few comments.

src/annotator/cross-frame.js

src/annotator/index.js

src/annotator/cross-frame.js

src/sidebar/services/frame-sync.js

Use the existing `MessageEvent.ports` property to access transferred ports instead of adding an extra property to the message itself.

codecov · 2021-07-27T15:40:16Z

Codecov Report

Merging #3611 (6f482fd) into master (4305f25) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3611   +/-   ##
=======================================
  Coverage   98.61%   98.61%           
=======================================
  Files         211      211           
  Lines        7735     7743    +8     
  Branches     1758     1761    +3     
=======================================
+ Hits         7628     7636    +8     
  Misses        107      107

Impacted Files	Coverage Δ
src/annotator/cross-frame.js	`100.00% <100.00%> (ø)`
src/annotator/sidebar.js	`98.09% <100.00%> (+0.04%)`	⬆️
src/sidebar/services/frame-sync.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4305f25...6f482fd. Read the comment docs.

Without this call the test doesn't execute the code paths it intends to test.

robertknight · 2021-07-27T15:50:42Z

Thanks for the feedback @esanzgar. I removed the port property from the hypothesisGuestReady message in favor of using MessageEvent.ports, per your suggestion. I also fixed an issue with a test that was causing code coverage misses.

This is now ready for review.

The previous code assumed that the `window.parent.__hypothesis` access would return undefined. This assumption was wrong. Instead it triggers an error. Handle this error and display a more informative message if a guest cannot connect to a sidebar.

robertknight · 2021-07-28T10:58:06Z

I pushed a commit to improve the handling of the situation where a guest-only frame has a cross-origin parent. Before it triggered an error. Now it outputs a useful console warning:

esanzgar

It looks good.

I left a couple of very minor suggestions.

esanzgar · 2021-07-28T12:00:00Z

src/annotator/cross-frame.js

@@ -71,26 +69,25 @@ export class CrossFrame {
     * Returns a promise that resolves once the connection has been established.
     *
     * @param {Window} frame - The window containing the sidebar application


Optionally:

Suggested change

* @param {Window} frame - The window containing the sidebar application

* @param {Window} sidebarFrame - The window containing the sidebar application

esanzgar · 2021-07-28T12:05:53Z

src/annotator/index.js

+  try {
+    sidebarWindow = /** @type {HypothesisWindow} */ (window.parent).__hypothesis
+      ?.sidebarWindow;


Suggested change

try {

sidebarWindow = /** @type {HypothesisWindow} */ (window.parent).__hypothesis

?.sidebarWindow;

try {

// Only `guest` frame/s that are direct children of the `host` frame are currently supported

sidebarWindow = /** @type {HypothesisWindow} */ (window.parent).__hypothesis

?.sidebarWindow;

I've added a comment here. More importantly looking at this again made be realize there was a bug where the assignment in the try block did not check that sidebarWindow was not already assigned.

esanzgar · 2021-07-28T12:22:11Z

src/annotator/test/cross-frame-test.js

-        origin: 'ORIGIN',
-        token: 'TOKEN',
-      });
+      cf.connectToSidebar(sidebarFrame);


I would suggest to add the origin parameter as well for consistency.

esanzgar · 2021-07-28T13:50:45Z

src/annotator/test/sidebar-test.js

+      const sidebar = createSidebar();
+
+      // Check `sidebar.ready` is not resolved before `hypothesisSidebarReady`
+      // message is received.
+      assert.equal(
+        await Promise.race([sidebar.ready, Promise.resolve('not-ready')]),
+        'not-ready'
+      );
+
+      window.dispatchEvent(
+        new MessageEvent('message', {
+          data: { type: 'hypothesisSidebarReady' },
+        })
+      );
+
+      return sidebar.ready;


I found this test a bit contrive. What about this alternative?

Suggested change

const sidebar = createSidebar();

// Check `sidebar.ready` is not resolved before `hypothesisSidebarReady`

// message is received.

assert.equal(

await Promise.race([sidebar.ready, Promise.resolve('not-ready')]),

'not-ready'

);

window.dispatchEvent(

new MessageEvent('message', {

data: { type: 'hypothesisSidebarReady' },

})

);

return sidebar.ready;

const sidebar = createSidebar();

window.dispatchEvent(

new MessageEvent('message', {

data: { type: 'hypothesisSidebarReady' },

})

);

await sidebar.ready;

assert(true);

Or maybe:

Suggested change

const sidebar = createSidebar();

// Check `sidebar.ready` is not resolved before `hypothesisSidebarReady`

// message is received.

assert.equal(

await Promise.race([sidebar.ready, Promise.resolve('not-ready')]),

'not-ready'

);

window.dispatchEvent(

new MessageEvent('message', {

data: { type: 'hypothesisSidebarReady' },

})

);

return sidebar.ready;

const sidebar = createSidebar();

let counter = 0;

const promise = sidebar.ready.then(() => ++counter);

window.dispatchEvent(

new MessageEvent('message', {

data: { type: 'hypothesisSidebarReady' },

})

);

await promise;

assert.equal(counter, 1);

The Promise.race part is an attempt to check that the sidebar.ready Promise is not already resolved. JavaScript intentionally doesn't provide APIs to inspect Promise state, so the best we can do is race it against a Promise that is immediately resolved.

const promise = sidebar.ready.then(() => ++counter);

Promises can only be resolved once, so unless sidebar.ready were re-assigned to a different Promise, counter can never have a value that is more than one here.

We don't want to connect to the sidebar in the parent frame if the current frame has one.

The test happened to work without it, but the parameter is ostensibly required.

After #3611 there is no more need to support `window.postMessage` on `RPC` and `Bridge` classes.

The current inter-frame communication doesn't work if an annotatable (guest) iframe is from a different origin than the host frame (see #3611 (comment)). This will be fixed in a more comprehensive overhaul of the inter-face communication (see #3533). Meanwhile, I add a scenario into the local dev server where the annotatable iframe is from a different origin than the host frame. For this, I needed to spawn an additional dev server at port 3002): ``` [11:32:50] Dev web server started at http://localhost:3000/ [11:32:50] Dev web server started at http://localhost:3002/ ``` Close #3629

The current inter-frame communication doesn't work if an annotatable (guest) iframe is from a different origin than the host frame (see #3611 (comment)). This will be fixed in a more comprehensive overhaul of the inter-face communication (see #3533). Meanwhile, I add a scenario into the local dev server where the annotatable iframe is from an origin different than the host frame. For this, I needed to spawn an additional dev server at port 3002): ``` [11:32:50] Dev web server started at http://localhost:3000/ [11:32:50] Dev web server started at http://localhost:3002/ ``` Close #3629

esanzgar reviewed Jul 27, 2021

View reviewed changes

src/sidebar/services/frame-sync.js Outdated Show resolved Hide resolved

esanzgar reviewed Jul 27, 2021

View reviewed changes

src/sidebar/services/frame-sync.js Show resolved Hide resolved

esanzgar reviewed Jul 27, 2021

View reviewed changes

Remove port property from hypothesisGuestReady message

fdfb22f

Use the existing `MessageEvent.ports` property to access transferred ports instead of adding an extra property to the message itself.

Add missing FrameSyncService.connect call in test

e80beb5

Without this call the test doesn't execute the code paths it intends to test.

robertknight marked this pull request as ready for review July 27, 2021 15:46

robertknight requested a review from esanzgar July 27, 2021 15:49

robertknight mentioned this pull request Jul 28, 2021

Remove Discovery class #3618

Merged

esanzgar approved these changes Jul 28, 2021

View reviewed changes

esanzgar mentioned this pull request Jul 28, 2021

Use ES2015 class methods #3617

Merged

robertknight added 3 commits July 29, 2021 09:09

Add missing if check around assignment of sidebarWindow

a037456

We don't want to connect to the sidebar in the parent frame if the current frame has one.

Add missing origin parameter in connectToSidebar call

2c9a094

The test happened to work without it, but the parameter is ostensibly required.

Reword a comment to clarify what is going on in a test

6f482fd

robertknight merged commit f144f6d into master Jul 29, 2021

robertknight deleted the messagechannel-guest-communication branch July 29, 2021 08:16

This was referenced Jul 29, 2021

Multiple unrelated Hypothesis frames on one page causes havoc #249

Closed

Error when top level frame and iframe on a page embed the Hypothesis client #187

Closed

Get Hypothesis client working in new VitalSource reader #3590

Closed

esanzgar mentioned this pull request Jul 30, 2021

Remove support for window.postMessage from RPC and Bridge #3628

Closed

esanzgar added a commit that referenced this pull request Aug 5, 2021

Remove deprecated uses of window.postMessage

c63ee46

After #3611 there is no more need to support `window.postMessage` on `RPC` and `Bridge` classes.

esanzgar mentioned this pull request Aug 5, 2021

Remove deprecated uses of window.postMessage #3649

Merged

esanzgar added a commit that referenced this pull request Aug 6, 2021

Remove deprecated uses of window.postMessage

2e65209

After #3611 there is no more need to support `window.postMessage` on `RPC` and `Bridge` classes.

esanzgar added a commit that referenced this pull request Aug 6, 2021

Remove deprecated uses of window.postMessage

9330488

After #3611 there is no more need to support `window.postMessage` on `RPC` and `Bridge` classes.

esanzgar mentioned this pull request Sep 6, 2021

Add cross-origin annotatable iframe dev server scenario #3732

Merged

robertknight mentioned this pull request Jul 29, 2021

Epub.js demo does not work #4283

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert host <-> guest communication to use MessageChannel #3611

Convert host <-> guest communication to use MessageChannel #3611

robertknight commented Jul 26, 2021 •

edited

Loading

esanzgar left a comment

codecov bot commented Jul 27, 2021 •

edited

Loading

robertknight commented Jul 27, 2021

robertknight commented Jul 28, 2021

esanzgar left a comment

esanzgar Jul 28, 2021

esanzgar Jul 28, 2021

robertknight Jul 29, 2021

esanzgar Jul 28, 2021

robertknight Jul 29, 2021

esanzgar Jul 28, 2021

robertknight Jul 29, 2021

	* @param {Window} frame - The window containing the sidebar application
	* @param {Window} sidebarFrame - The window containing the sidebar application

Convert host <-> guest communication to use MessageChannel #3611

Convert host <-> guest communication to use MessageChannel #3611

Conversation

robertknight commented Jul 26, 2021 • edited Loading

esanzgar left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 27, 2021 • edited Loading

Codecov Report

robertknight commented Jul 27, 2021

robertknight commented Jul 28, 2021

esanzgar left a comment

Choose a reason for hiding this comment

esanzgar Jul 28, 2021

Choose a reason for hiding this comment

esanzgar Jul 28, 2021

Choose a reason for hiding this comment

robertknight Jul 29, 2021

Choose a reason for hiding this comment

esanzgar Jul 28, 2021

Choose a reason for hiding this comment

robertknight Jul 29, 2021

Choose a reason for hiding this comment

esanzgar Jul 28, 2021

Choose a reason for hiding this comment

robertknight Jul 29, 2021

Choose a reason for hiding this comment

robertknight commented Jul 26, 2021 •

edited

Loading

codecov bot commented Jul 27, 2021 •

edited

Loading