Fix frame discoverability #3457

esanzgar · 2021-05-27T08:40:11Z

After one failed attempt to limit the frame discoverability (#3352), I
suggest here another temporary fix which involves:

Disable the top-down, breadth-first traversal of frames to enable
discoverability.
Replace it by a targeted discoverability: identify the sender of the
initial postMessage message and direct it to the right frame:
- if the sender is the sidebar iframe (server frame), the
  postMessage is sent only to the host frame using
  window.parent.
- if the sender is an annotatable iframe(s), send the postMessage
  to the sidebar iframe.

Pros:

resolves the broken ePub example
resolves the hyper-connectivity of frames

Cons:

the notebook iframe is still not able to be discovered
introduce an artificial delay on the discoverablity of annotatable
iframes that could potentially brake

Background

The client relies on an inter-frame communication system between the
host frame, where the client is initially loaded, and a number of
children iframes. For the communication to work, every frame needs to be
able to discover the iframe that acts as a server by sending a
frame.postMessage. Currently, the sidebar iframe is the server.

The following frames must establish communication with the server
sidebar iframe:

host frame (where the client is initially loaded)
notebook iframe
additional annotatable iframe(s) (each have an enable-annotation
attribute) where the another client instance is injected.

This layout represents the current arrangement of frames:

host frame (client)
|-> (generally, shadow DOMed) sidebar iframe (server)
|-> (generally, shadow DOMed) notebook iframe (client)
|-> [annotatable iframe/s] (client)
     |-> [annotatable iframe/s] (client)

There are two problems with the current discoverability algorithm:

It relies on window.frames to list all the other frames. Because
sidebar and notebook iframes are generally wrapped on a shadow
DOM they are not listed on window.frames.
It is very generic: the algorithm starts from the top-most frame in
the hierarchy (window.top) and send messages to all the frame
children recursively. If there are several clients initialised on
individual frames, this algorithm causes all the host frames to
be connected to all the sidebar iframes.

`externalContainerSelector` allows the placement of the sidebar inside the element specified by the CSS selector. See https://h.readthedocs.io/projects/client/en/latest/publishers/config/#cmdoption-arg-externalcontainerselector

After one failed attempt to limit the frame discoverability (#3352), I suggest here another temporary fix which involves: * Disable the top-down, breadth-first traversal of frames to enable discoverability. * Replace it by a targeted discoverability: identify the sender of the initial postMessage message and direct it to the right frame: - if the sender is the `sidebar` iframe (server frame), the postMessage is sent *only* to the `host` frame using `window.parent`. - if the sender is an annotatable iframe(s), send the postMessage to the `sidebar` iframe. Pros: - resolves the broken ePub example - resolves the hyper-connectivity of frames Cons: - the `notebook` iframe is still not able to be discovered - introduce an artificial delay on the discoverablity of annotatable iframes that could potentially brake Background ---------- The client relies on an inter-frame communication system between the `host` frame, where the client is initially loaded, and a number of children iframes. For the communication to work, every frame needs to be able to discover the iframe that acts as a server by sending a `frame.postMessage`. Currently, the `sidebar` iframe is the server. The following frames must establish communication with the server `sidebar` iframe: - `host` frame (where the client is initially loaded) - `notebook` iframe - additional annotatable iframe(s) (each have an `enable-annotation` attribute) where the another client instance is injected. This layout represents the current arrangement of frames: ``` host frame (client) |-> (generally, shadow DOMed) sidebar iframe (server) |-> (generally, shadow DOMed) notebook iframe (client) |-> [annotatable iframe/s] (client) |-> [annotatable iframe/s] (client) ``` There are two problems with the current discoverability algorithm: 1. It relies on `window.frames` to list all the other frames. Because `sidebar` and `notebook` iframes are generally wrapped on a shadow DOM they are not listed on `window.frames`. 2. It is very generic: the algorithm starts from the top-most frame in the hierarchy (`window.top`) and send messages to all the frame children *recursively*. If there are several clients initialised on individual frames, this algorithm causes *all* the `host` frames to be connected to all the `sidebar` iframes.

robertknight

Hi Eduardo,

The solution here relies on the guest and host frames being same-origin, so that the guest can directly query the DOM of the host. That's true in the common case of the guest/host being the same frame. I think it might also be true for all of the epub demos that we've built so far but I'm not certain about that. It might not be true for other use cases in future - especially when the content is put in an iframe for security reasons. I'm going to add some comments on how we might "fix up" the approach sketched in this PR, and then I'll say something about alternative communication mechanisms that avoid the guest <-> host same-origin constraint.

Regarding this PR:

I think it is important to avoid having knowledge about the DOM structure of the sidebar, notebook etc. spread across multiple places in the code. Otherwise someone else might change the code in the Sidebar class in future and not realize they are breaking something here.
It is also important to preserve the layering of the various modules that make up the client. src/shared/ is intended to be at the root of the dependency graph so it can be used by src/{annotator, sidebar} but not the other way around. Cyclic dependencies can cause various problems down the line.
Dealing with the different orderings in which cross-origin frames can load is one of the trickier aspects of setting up communication. I think there are some things you can rely on (eg. the host will always load before the sidebar), others that you can't (whether guest frames load before the sidebar) and some guarantees which we can probably make at present but may not always hold (whether we'll always be able to guarantee that the host code runs before any guest frames have loaded). In the case where we can't guarantee the order in which two frames load, having one party send repeated "hello" messages at an interval to the other until communication is established or a timeout elapses may actually be a reasonable thing to do.

If we were to rethink the frame communication mechanism to avoid the guest <-> host same-origin constraint then it could work something like:

The sidebar frame notifies the host when it is ready to communicate with guests
Guest frames notify the host when they are ready to communicate with the sidebar (in the common case where the host also contains a guest, this can happen without any messages being sent)
When each partner in a (sidebar, guest) pair are ready to communicate, the host frame creates a MessageChannel and sends one port to the guest and one port to the sidebar. Each side then calls start on the channel after receiving it to start receiving messages.

Here step (1) and (2) can happen in either order. Step (2) assumes that code will run in the host frame before guest frames. If we can't make that assumption then step (2) might have to involve some polling by the guest until the host responds.

robertknight · 2021-05-27T12:01:44Z

src/shared/discovery.js

+        let hostFrame;
+        do {
+          hostFrame = /** @type  {HypothesisWindow} */ (this.target.parent);
+        } while (hostFrame.__hypothesis_frame);


This assumes that all the annotateable (or "guest") frames are same-origin, since you can't access global JS properties on cross-origin iframes. I believe that is true for epub demos currently, but it seems quite plausible to me that we might have use cases in future where this is not the case.

What I think we do have stronger assurance over is that the "guest" frame will know how many levels up the tree the "host" frame is, because that information can be provided as part of the configuration for the guest frame.

robertknight · 2021-05-27T12:05:01Z

src/shared/discovery.js

+          // Sidebar iframe can be shadow DOMed
+          const shadowDomSidebar = /** @type {HTMLIFrameElement|null} */ (
+            hostFrame.document.querySelector('hypothesis-sidebar')
+          )?.shadowRoot?.querySelector('iframe')?.contentWindow;


There is a layering problem here: this code embeds deep knowledge of the annotator DOM structure in a part of the code that is far away from it and is also at a lower-level in the dependency structure. In other words, src/{annotator, sidebar} may have a dependency on src/shared but not vice-versa.

robertknight · 2021-05-27T12:09:35Z

src/shared/discovery.js

-        queue.push(parent.frames[i]);
+
+      // Notebook iframe
+      if (this.target.frameElement?.classList.contains('NotebookIframe')) {


This also embeds information about the DOM structure of the notebook far away from the source.

robertknight · 2021-05-27T12:25:48Z

src/shared/discovery.js

+          if (sidebar) {
+            setTimeout(
+              () => sidebar.postMessage(beaconMessage, this.origin),
+              1000 /* TODO: arbitrary delay as we can't check the readyState of the `sidebar iframe` because of cross-origins */


Knowing when the various frames are ready to receive messages is one of the trickier parts of setting up communication. If we had no guarantees about what order frames would be ready in (host, guest, sidebar) then sending messages at an interval until we get a response or some timeout has elapsed may be a reasonable strategy. I think it helps to explicitly state what guarantees you have (or assume you have). I think the current status is:

The host frame will always load before the sidebar frame, because the host code creates the sidebar

Guest frames may or may not finish loading before the "host" code runs. For guest frames where the parent injects code as a result of the enable-annotation attribute then the host will always load first. However we might have use cases in future where the guest is not the same origin as the host. I'm not sure in these cases whether it will be possible to ensure that the host code runs first in the ancestor frame before the guest frame is created.

esanzgar · 2021-05-28T13:01:17Z

Thank you very much for the advise and I agree with that.

I will close this draft PR and work on an alternative solution.

esanzgar added 2 commits May 27, 2021 09:32

Add test scenario for externalContainerSelector option

7815342

`externalContainerSelector` allows the placement of the sidebar inside the element specified by the CSS selector. See https://h.readthedocs.io/projects/client/en/latest/publishers/config/#cmdoption-arg-externalcontainerselector

esanzgar force-pushed the limit-frame-discoverability branch from 9e92a49 to d667662 Compare May 27, 2021 08:42

esanzgar requested a review from robertknight May 27, 2021 08:43

robertknight reviewed May 27, 2021

View reviewed changes

esanzgar closed this May 28, 2021

esanzgar mentioned this pull request Jun 7, 2021

Propose plan to improve the communication between frames #3403

Closed

esanzgar deleted the limit-frame-discoverability branch July 2, 2021 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix frame discoverability #3457

Fix frame discoverability #3457

esanzgar commented May 27, 2021 •

edited

Loading

robertknight left a comment

robertknight May 27, 2021

robertknight May 27, 2021

robertknight May 27, 2021

robertknight May 27, 2021

esanzgar commented May 28, 2021

Fix frame discoverability #3457

Fix frame discoverability #3457

Conversation

esanzgar commented May 27, 2021 • edited Loading

Background

robertknight left a comment

Choose a reason for hiding this comment

robertknight May 27, 2021

Choose a reason for hiding this comment

robertknight May 27, 2021

Choose a reason for hiding this comment

robertknight May 27, 2021

Choose a reason for hiding this comment

robertknight May 27, 2021

Choose a reason for hiding this comment

esanzgar commented May 28, 2021

esanzgar commented May 27, 2021 •

edited

Loading