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

Propose plan to improve the communication between frames #3403

Closed
esanzgar opened this issue May 11, 2021 · 5 comments
Closed

Propose plan to improve the communication between frames #3403

esanzgar opened this issue May 11, 2021 · 5 comments
Assignees

Comments

@esanzgar
Copy link
Contributor

esanzgar commented May 11, 2021

  1. The shadow DOMing of the sideframe has caused the ePub demo to stop working.
  2. The notebook and sideframe are not able to discover each other.
  3. Iframes are hyper-connected which is the cause (entirely or partially) of several issues:

Because of all the above, we should propose a plan to tackle the communication issues in the client.

@robertknight has suggested MessageChannel API as a mechanism to enable sending private messages between frames.

@esanzgar
Copy link
Contributor Author

esanzgar commented Jun 2, 2021

This could be related to: hypothesis/lms#433

@esanzgar
Copy link
Contributor Author

esanzgar commented Jun 7, 2021

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 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
  • guest frames: iframes with annotatable content

At some point, the client considered guest any iframe from the same domain.
The host frame injected the client to these iframes (the sidebar is not
initialised in these guests iframe/s). This was a very general mechanism that
lead to unwanted behaviours.
Later,
it was modified so that only iframes that contained the enable-annotation HTML
attribute were candidate for the injection of the client. This is the mechanism
used in the EPUB demo.

This layout represents the current arrangement of frames:

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

This layout could be recursive, for example the multi-frame scenario in
http://localhost:3000/document/multi-frames can be represented by:

host frame (client)
|-> (generally, shadow DOMed) sidebar iframe (server)
|-> (generally, shadow DOMed) notebook iframe (client)

|-> host frame (client)
    |-> (generally, shadow DOMed) sidebar iframe (server)
    |-> (generally, shadow DOMed) notebook iframe (client)

|-> host frame (client)
    |-> (generally, shadow DOMed) sidebar iframe (server)
    |-> (generally, shadow DOMed) notebook iframe (client)

In the above scenario, we would expect frames connected in this manner:

group 1 (all connected among themselves)
host frame (client)
|-> (generally, shadow DOMed) sidebar iframe (server)
|-> (generally, shadow DOMed) notebook iframe (client)

group 2 (all connected among themselves)
|-> host frame (client)
    |-> (generally, shadow DOMed) sidebar iframe (server)
    |-> (generally, shadow DOMed) notebook iframe (client)

group 3 (all connected among themselves)
|-> host frame (client)
    |-> (generally, shadow DOMed) sidebar iframe (server)
    |-> (generally, shadow DOMed) notebook iframe (client)

Frames from one group should not be connected to frames from another group.

In addition to the above, it has been recommended to support guest iframe/s
from a different origin of the host frame and maybe also to support guest
iframe/s not direct children of the host frame (embeded in a number of wrapped
iframes). For example:

host frame (client, www.example.com)
|-> (generally, shadow DOMed) sidebar iframe (server)
|-> (generally, shadow DOMed) notebook iframe (client)
|-> iframe/s (from same or other origin)
    |-> [guest iframe/s] (client, wwww.otherdomain.com with a client configured to behave like a `guest` iframe)

Current problems

  • The current frame discoverability algorithm (starting from the top-most
    window, window.top, and recursively discovering all the children frame) is
    too generic. In the multi-frame scenario above, it can lead to the host
    frame from group 3 to be connected to the sidebar from group 2.

  • When a iframe is embeded on a shadow DOM it is not listed in the
    window.frames. This leads to guest iframes that loads after the
    sidebar to not been able to connect with the server (sidebar). See
    this example.

Constrains and requirements

  • Include as little knowledge about the DOM structure as possible in the logic
    to discover the frames. See more about this point in this
    comment

  • Because the host frame creates the sidebar and notebook iframes, it is
    warranty that host frame is ready to listen to messages originating from the
    sidebar and notebook iframes.

  • However, the above is not true for host (and sidebar) and guest frames.
    Because iframes load in parallel, we can not assume that the code in the
    host is executed before the code in a guest frame. Therefore, it is
    necessary to either (1) send a message in both direction (sidebar - offers
    -> guest and guest - request -> sidebar) or (2) pulling for a certain
    period. Generally, pulling (2) is not as reliable as method (1). Apart from
    adding a delay, there is not certainty if the iframe been pulled is pulling
    acts as a mere wrapper. I see the pulling (2) mechanism only working reliably
    if the guest iframe/s were never wrapped on intermediate iframe/s.

  • Window references can not be passed as argument using postMessage, because
    it is not a tranferable object. This mean that the host frame can't be used
    as an intermediary to pass a reference of the shadow DOMed sidebar iframe to
    other frames.

  • Start using MessageChannel API for communication. MessageChannel API is well
    supported, and, after the initial handsake and port exchange, the
    communication through the ports is private. A MessageChannel port can (and
    must) be passed on a postMessage call. Hence, the host frame would be able
    to work as intemediary and pass the sidebar port to other frames that
    request it.

Plan 1: enable notebook and sidebar communication

  • host and sidebar discover each other as currently done
  • host uses the bridge system to request a MessageChannel port of the
    sidebar
  • host will received a request from the notebook, if the host already has
    the sidebar port pass it, otherwise after it gets it from sidebar pass it
  • notebook uses sidebar port to communicate. The

Potential option 1: sidebar holds MessageChannel ports from all the other frames

  • refactor of the bridge service to work with with MessageChannel ports
    instead of frame.postMessage
  • after initial handsake, the sidebar and the other frame exchange ports
Server (`sidebar`)                Clients
[array of ports] <--------------> `host`
                 <--------------> `notebook`
                 <--------------> `guest` frame/s

Potential option 2: host holds MessageChannel ports from all the other frames and relays messages to certain destination

  • similar to the above except that the host frame will hold a broadcaster
    service.
  • frames discover the broadcaster service using window.parent.postMessage
  • after initial handsake, frames and the broadcaster exchange ports
  • bridge incorporates directionality
    bridge.call('typeMessage', [data], {destination: ['sidebar|annotatableFrames|notebook|myself|all']})
  • the broadcaster service will be re-directing the message to the corresponding
    frame/s
  • this system introduce a latency because the message has to be rebroacasted:
    client --sent--> broadcaster --broadcast--> client
Broadcaster (`host`)              Clients
[array of ports] <--------------> `host`
                 <--------------> `notebook`
                 <--------------> `guest` frame/s

@robertknight
Copy link
Member

Hi @esanzgar - Thanks for the thoughtful write-up. I left some notes here: https://hyp.is/go?url=https%3A%2F%2Fgithub.com%2Fhypothesis%2Fclient%2Fissues%2F3403&group=wqvn7Lar

In summary:

  • I think we can simplify the guest <-> host communication requirements slightly by assuming that a guest will always know which of its ancestors is the host.
  • Furthermore I think we can support only scenarios where the guest is a direct child of the host initially, though it may have a different origin
  • The host frame should always be able to get a reference to both windows that need to communicate with each other: either directly or via the event.source property of a MessageEvent from the window to the host, so I think we can let it have the responsibility of creating the MessageChannel
  • As you noted, a relay introduces latency in message delivery, though I don't know how much. More importantly though it means that any messages sent between the two parties will be observable by the host. With the current messages we send that's not really a problem as guest/host frames are always considered untrusted. It could be a problem for notebook <-> sidebar communication though

@esanzgar
Copy link
Contributor Author

esanzgar commented Jun 9, 2021

Based on your comments I created this prototype:
#3494

Thank you very much!

@esanzgar esanzgar self-assigned this Jun 18, 2021
@esanzgar
Copy link
Contributor Author

On the prototype above in #3494, we have used Potential option 1 and it feels it is the right solution.

The current shortcomings with the inter-frame communication were described in detail. Several solutions were also proposed.

I will close this ticket.

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 a pull request may close this issue.

2 participants