Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit and fix communication between frames #3352

Closed
wants to merge 1 commit into from
Closed

Conversation

esanzgar
Copy link
Contributor

The client relies on an inter-frame communication between the host
frame, where the client was initially loaded, and a number of iframe
children. 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 was initially loaded)
  • notebook iframe
  • additional annotatable iframe(s) (each have an enable-annotation
    attribute) where the guest instance is injected.

This layout represents the current arrangement of frames:

host frame (client)
  |-> sidebar iframe (server)
  |-> notebook iframe (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 now wrapped on a shadow DOM they
    are not listed on window.frames.

  2. It is very generic: the algorithm starts from the topmost 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.

To solve both problems, I investigated swapping the server from the
sidebar to the host frame. All iframe children are able to find the
host frame (window.parent). In addition, this approach would have
the advantage that the server will be initialised first and ready to
listen for message before any of the iframe children is initialised.
This layout describes the intended arrangement:

host frame (server)
  |-> sidebar iframe (client)
  |-> notebook iframe (client)
  |-> annotatable iframe/s (client)

While this would have been a clean solution, I found that the server is
coupled to the store functionality in the sidebar. Decoupling both would
take some efforts.

Instead, in this PR I am suggesting a pragmatic approach based on (1)
saving a reference to the sidebar iframe on a global window variable
in the host frame and (2) the current parent-children frame
arrangement. While not as clean as the first approach, it would resolve
both problems:

  1. No dependent on window.frames: when the host frame successfully
    establishes a communication with the sidebar iframe, it saves the
    reference to the sidebar iframe on a global reference.

  2. 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 the notebook or an annotatable iframe(s), we
      try send the postMessage to the sibling sidebar iframe, if a
      reference is known, otherwise we retry later.

@esanzgar esanzgar requested a review from robertknight April 28, 2021 13:16
@esanzgar esanzgar changed the base branch from master to multi-frame-test-case April 28, 2021 13:54
@esanzgar esanzgar force-pushed the multi-frame-test-case branch from 16e0643 to 8eba421 Compare April 28, 2021 13:59
@esanzgar esanzgar force-pushed the communication branch 3 times, most recently from 2cf6ffa to 8276921 Compare April 28, 2021 14:58
@esanzgar esanzgar force-pushed the multi-frame-test-case branch from 8eba421 to 8d67188 Compare April 28, 2021 18:25
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach implemented here relies on the child frame of the host frame being able to read properties on the Window global of the parent frame. This is possible if the host-frame is same origin but that is not true for the notebook or for guest frames in general. I mentioned a way that you can simulate this different-originness by using the sandbox attribute on an iframe.

I'm not aware of a way that a frame (eg. the host) can directly send a child frame a reference to another child so that they can directly communicate via postMessage. If you can find such a mechanism, let me know! A mechanism that I do know of which should work, is for a parent frame which has a reference to two other frames (eg. host having a reference to a child guest and the sidebar), to create a MessageChannel and send the two ends ("ports") of that channel to the respective child frames via the transfer argument to window.postMessage. The frames can then communicate directly with one another via the ports.

) {
const sendPostMessage = () => {
const sidebarFrame = /** @type {import('../types/annotator').HypothesisWindow} */ (this
.target.parent).__sidebar_frame;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Properties added to Window objects are not accessible from different-origin iframes, because the JavaScript worlds are completely separated and all communication has to be done via message ports. To simulate this locally in a development server where all the iframes are from the same origin, you can apply the sandbox attribute to the iframe without the allow-same-origin token. See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe. A sandboxed iframe without this token behaves as if it is from a different origin.

// Both the `notebook` iframe and annotatable iframe(s) have as parent the `host` frame.
// - the `notebook` iframe is identified by having an `hypothesis-app` element
// - annotatable iframe(s) are identified by having a global `window.__hypothesis_frame` variable
this.target.document.querySelector('hypothesis-app') ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing this code path is never invoked for the notebook app because the notebook never invokes the discovery process by calling Discovery#connect.

The client relies on an inter-frame communication between the host
frame, where the client was initially loaded, and a number of iframe
children. 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 was initially loaded)
- `notebook` iframe
- additional annotatable iframe(s) (each have an `enable-annotation`
  attribute) where the `guest` instance is injected.

This layout represents the current arrangement of frames:

```
host frame (client)
  |-> sidebar iframe (server)
  |-> notebook iframe (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 now wrapped on a shadow DOM they
   are not listed on `window.frames`.

2. It is very generic: the algorithm starts from the topmost 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.

To solve both problems, I have swapped the server from the `sidebar` to
the `host` frame. All iframe children are able to find the `host` frame
(`window.parent`). In addition, this approach has the the advantage that
the server is initialised first and ready to listen for message before
any of the iframe children are created.  This layout describes the
changes introduced:

```
host frame (server)
  |-> sidebar iframe (client)
  |-> notebook iframe (client)
  |-> annotatable iframe/s (client)
```

This PR resolves both both problems mentioned earlier:

1. No dependent on `window.frames`: all children iframe have access to
   `window.parent` no matter if the iframe is from another origin.

2. Targeted discoverability:
     - if the sender is the `sidebar` or `notebook` iframes, the
       postMessage is *only* sent to the `host` frame (server) using
       `window.parent`.
     - if the sender is an `annotatable` iframe(s), we find the closest
       parent that is not an `annotatable` iframe and send a
       postMessage. We assume that parent to be the `host` frame
       (server).
@esanzgar esanzgar force-pushed the multi-frame-test-case branch from 5d6d6b6 to 82f3f9b Compare April 29, 2021 12:35
Base automatically changed from multi-frame-test-case to master April 29, 2021 12:36
@robertknight
Copy link
Member

Hi @esanzgar - I think we can close this PR now, since we've determined that we'll need to take a different approach to setting up inter-frame communication?

@esanzgar
Copy link
Contributor Author

I will proposed an intermediate step to partially remediate the current situation elsewhere. Then, I will come out with a plan to readdress the current communication issues between the frames.

@esanzgar esanzgar closed this May 11, 2021
esanzgar added a commit that referenced this pull request May 27, 2021
After one failed attempt to solve the inter-frame discoverability issue
(#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

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.
esanzgar added a commit that referenced this pull request May 27, 2021
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

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.
esanzgar added a commit that referenced this pull request May 27, 2021
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.
@esanzgar esanzgar deleted the communication branch November 23, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants