Skip to content

Commit

Permalink
Limit and fix communication between frames
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
esanzgar committed Apr 29, 2021
1 parent e1eaca4 commit 3563f04
Show file tree
Hide file tree
Showing 6 changed files with 25 additions and 15 deletions.
3 changes: 2 additions & 1 deletion src/annotator/cross-frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,9 @@ export class CrossFrame {
};

// Initiate connection to the sidebar.
const onDiscoveryCallback = (source, origin, token) =>
const onDiscoveryCallback = (source, origin, token) => {
bridge.createChannel(source, origin, token);
};
discovery.startDiscovery(onDiscoveryCallback);
frameObserver.observe(injectIntoFrame, iframeUnloaded);

Expand Down
1 change: 1 addition & 0 deletions src/annotator/guest.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ export default class Guest {
// Setup connection to sidebar.
this.crossframe = new CrossFrame(this.element, {
config,
server: config.server,
on: (event, handler) => this._emitter.subscribe(event, handler),
emit: (event, ...args) => this._emitter.publish(event, ...args),
});
Expand Down
3 changes: 3 additions & 0 deletions src/annotator/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ function init() {
// frame context belongs to hypothesis.
// Needs to be a global property that's set.
window_.__hypothesis_frame = true;
config.server = false;
} else {
config.server = true;
}

// Load the PDF anchoring/metadata integration.
Expand Down
29 changes: 17 additions & 12 deletions src/shared/discovery.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

import { ListenerCollection } from '../annotator/util/listener-collection';

/** @typedef {import('../types/annotator').HypothesisWindow} HypothesisWindow */

/**
* Discovery finds frames in the current tab/window that can be annotated (the
* "clients") or can fetch annotations from the backend (the "server").
Expand All @@ -34,7 +36,7 @@ import { ListenerCollection } from '../annotator/util/listener-collection';
*/
export default class Discovery {
/**
* @param {Window} target
* @param {HypothesisWindow} target
* @param {object} [options]
* @param {boolean} [options.server]
* @param {string} [options.origin]
Expand Down Expand Up @@ -103,21 +105,24 @@ export default class Discovery {
_beacon() {
let beaconMessage;
if (this.server) {
// `host` frame (frame where the client is initially loaded)
beaconMessage = '__cross_frame_dhcp_offer';
} else {
beaconMessage = '__cross_frame_dhcp_discovery';
}
if (this.target.__hypothesis_frame) {
// Annotatable iframe(s) have a global `__hypothesis_frame` variable.

// Find the closest parent that is not an annotatable iframe.
/** @type {HypothesisWindow} */
let parent = this.target.parent;
while (parent.__hypothesis_frame) {
parent = this.target.parent;
}

// Perform a top-down, breadth-first traversal of frames in the current
// window and send messages to them.
const queue = [this.target.top];
while (queue.length > 0) {
const parent = /** @type {Window} */ (queue.shift());
if (parent !== this.target) {
parent.postMessage(beaconMessage, this.origin);
}
for (let i = 0; i < parent.frames.length; i++) {
queue.push(parent.frames[i]);
this.target.parent.postMessage(beaconMessage, this.origin);
} else {
// `sideframe` or `notebook`, iframes
this.target.parent.postMessage(beaconMessage, this.origin);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/sidebar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import * as rendererOptions from '../shared/renderer-options';
import {
startServer as startRPCServer,
preStartServer as preStartRPCServer,
} from './cross-origin-rpc.js';
} from './cross-origin-rpc';
import disableOpenerForExternalLinks from './util/disable-opener-for-external-links';
import { fetchConfig } from './config/fetch-config';
import * as sentry from './util/sentry';
Expand Down
2 changes: 1 addition & 1 deletion src/sidebar/services/frame-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ export class FrameSyncService {
});
};

const discovery = new Discovery(window, { server: true });
const discovery = new Discovery(window);
discovery.startDiscovery(this._bridge.createChannel.bind(this._bridge));
this._bridge.onConnect(addFrame);

Expand Down

0 comments on commit 3563f04

Please sign in to comment.