Commit f1e042eb authored by Eduardo Sanz García's avatar Eduardo Sanz García Committed by Eduardo

Improve ergonomics of PortFinder and PortProvider

For PortFinder, I followed this advice:
https://github.com/hypothesis/client/pull/3881#discussion_r743030606,
except that I made the argument of `PorFinder#discover` and string
(instead of an object).

```
const portFinder = new PortFinder({ source: 'guest', hostFrame });
portFinder.discover('sidebar');
portFinder.discover('host');
```

For PortProvider, I followed this advice:
https://github.com/hypothesis/client/pull/3881#discussion_r743033013,
except that I used a getter.

```
cont portProvider = new PortProvider(...)
const bridge = new Bridge();
bridge.createChannel(portProvider.sidebarPort);
```

I have renamed the properties of `Message`:
- `source` becomes `authority`
- `channel` and `port` have been replaced by `frame1` and `frame2`

I did that to align `port1` and `frame1` and `port2` and `frame2`, while
also avoiding clashing with other names (`source`, `target`) which have
other meaning in the `window.postMessage` context.

I have removed one level of nesting in the `PortProvider#discover` that
make the method more readable.

I added some other suggestions from PR #3881.
parent 3d8982f8
......@@ -7,8 +7,7 @@ const POLLING_INTERVAL_FOR_PORT = 250;
/**
* @typedef {import('../types/annotator').Destroyable} Destroyable
* @typedef {import('./port-util').Message} Message
* @typedef {Message['channel']} Channel
* @typedef {Message['port']} Port
* @typedef {import('./port-util').Frame} Frame
*/
/**
......@@ -16,14 +15,18 @@ const POLLING_INTERVAL_FOR_PORT = 250;
* MessagePort-based connection to other frames. It is used together with
* PortProvider which runs in the host frame. See PortProvider for an overview.
*
* Channel nomenclature is `[frame1]-[frame2]` so that:
* - `port1` should be owned by/transferred to `frame1`, and
* - `port2` should be owned by/transferred to `frame2`
*
* @implements Destroyable
*/
export class PortFinder {
constructor() {
/**
* @param {object} options
* @param {Exclude<Frame, 'host'>} options.source - the role of this frame
* @param {Window} options.hostFrame - the frame where the `PortProvider` is
* listening for messages.
*/
constructor({ hostFrame, source }) {
this._hostFrame = hostFrame;
this._source = source;
this._listeners = new ListenerCollection();
}
......@@ -34,20 +37,16 @@ export class PortFinder {
/**
* Request a specific port from `hostFrame`
*
* @param {object} options
* @param {Channel} options.channel - requested channel
* @param {Window} options.hostFrame - frame where the hypothesis client is
* loaded and `PortProvider` is listening for messages
* @param {Port} options.port - requested port
* @param {Frame} target - the frame aiming to be discovered
* @return {Promise<MessagePort>}
*/
async discover({ channel, hostFrame, port }) {
async discover(target) {
let isValidRequest = false;
if (
(channel === 'guest-host' && port === 'guest') ||
(channel === 'guest-sidebar' && port === 'guest') ||
(channel === 'host-sidebar' && port === 'sidebar') ||
(channel === 'notebook-sidebar' && port === 'notebook')
(this._source === 'guest' && target === 'host') ||
(this._source === 'guest' && target === 'sidebar') ||
(this._source === 'sidebar' && target === 'host') ||
(this._source === 'notebook' && target === 'sidebar')
) {
isValidRequest = true;
}
......@@ -57,26 +56,34 @@ export class PortFinder {
}
return new Promise((resolve, reject) => {
function postRequest() {
hostFrame.postMessage(
{ channel, port, source: 'hypothesis', type: 'request' },
const postRequest = () => {
this._hostFrame.postMessage(
{
authority: 'hypothesis',
frame1: this._source,
frame2: target,
type: 'request',
},
'*'
);
}
};
// In some situations, because `guest` iframe/s load in parallel to the `host`
// frame, we can not assume that the code in the `host` frame is executed before
// the code in a `guest` frame. Hence, we can't assume that `PortProvider` (in
// the `host` frame) is initialized before `PortFinder` (in the non-host frames).
// Therefore, for the `PortFinder`, we implement a polling strategy (sending a
// message every N milliseconds) until a response is received.
// Because `guest` iframes load in parallel to the `host` frame, we can
// not assume that the code in the `host` frame is executed before the
// code in a `guest` frame. Hence, we can't assume that `PortProvider` (in
// the `host` frame) is initialized before `PortFinder` (in the non-host
// frames). Therefore, for the `PortFinder`, we implement a polling
// strategy (sending a message every N milliseconds) until a response is
// received.
const intervalId = setInterval(postRequest, POLLING_INTERVAL_FOR_PORT);
// The `host` frame maybe busy, that's why we should wait.
const timeoutId = setTimeout(() => {
clearInterval(intervalId);
reject(
new Error(`Unable to find '${port}' port on '${channel}' channel`)
new Error(
`Unable to establish ${this._source}-${target} communication channel`
)
);
}, MAX_WAIT_FOR_PORT);
......@@ -84,9 +91,9 @@ export class PortFinder {
const { data, ports } = /** @type {MessageEvent} */ (event);
if (
isMessageEqual(data, {
channel,
port,
source: 'hypothesis',
authority: 'hypothesis',
frame1: this._source,
frame2: target,
type: 'offer',
})
) {
......
This diff is collapsed.
......@@ -3,20 +3,19 @@
// message and avoid listening to messages that could have the same properties
// but different source. This is not a security feature but an
// anti-collision mechanism.
const SOURCE = 'hypothesis';
const AUTHORITY = 'hypothesis';
/**
* These types are the used in by `PortProvider` and `PortFinder` for the
* inter-frame discovery and communication processes.
*
* @typedef {'guest-host'|'guest-sidebar'|'host-sidebar'|'notebook-sidebar'} Channel
* @typedef {'guest'|'host'|'notebook'|'sidebar'} Port
* @typedef {'guest'|'host'|'notebook'|'sidebar'} Frame
*
* @typedef Message
* @prop {Channel} channel
* @prop {Port} port
* @prop {'offer'|'request'} type
* @prop {SOURCE} source -
* @prop {AUTHORITY} authority
* @prop {Frame} frame1
* @prop {Frame} frame2
* @prop {'offer'|'request'} type
*/
/**
......@@ -26,18 +25,18 @@ const SOURCE = 'hypothesis';
* @param {any} data
* @return {data is Message}
*/
function isMessageValid(data) {
function isMessage(data) {
if (data === null || typeof data !== 'object') {
return false;
}
for (let property of ['channel', 'port', 'source', 'type']) {
for (let property of ['frame1', 'frame2', 'type']) {
if (typeof data[property] !== 'string') {
return false;
}
}
return data.source === SOURCE;
return data.authority === AUTHORITY;
}
/**
......@@ -47,7 +46,7 @@ function isMessageValid(data) {
* @param {Message} message
*/
export function isMessageEqual(data, message) {
if (!isMessageValid(data)) {
if (!isMessage(data)) {
return false;
}
......
......@@ -4,9 +4,19 @@ import { PortFinder } from '../port-finder';
const MAX_WAIT_FOR_PORT = 1000 * 5;
describe('PortFinder', () => {
const authority = 'hypothesis';
const frame1 = 'guest';
const type = 'offer';
let portFinder;
let portFinders;
function portProviderOffer({ data, ports = [] }) {
function createPortFinder(source = frame1) {
const instance = new PortFinder({ hostFrame: window, source });
portFinders.push(instance);
return instance;
}
function sendPortProviderOffer({ data, ports = [] }) {
const event = new MessageEvent('message', {
data,
ports,
......@@ -18,34 +28,35 @@ describe('PortFinder', () => {
}
beforeEach(() => {
portFinders = [];
sinon.stub(window, 'postMessage');
portFinder = new PortFinder();
portFinder = createPortFinder();
});
afterEach(() => {
window.postMessage.restore();
portFinder.destroy();
portFinders.forEach(instance => instance.destroy());
});
describe('#destroy', () => {
it('ignores subsequent `offer` messages of ports', async () => {
let error;
const channel = 'host-sidebar';
const port = 'sidebar';
const target = 'host';
const { port1 } = new MessageChannel();
const clock = sinon.useFakeTimers();
try {
portFinder
.discover({
channel,
hostFrame: window,
port,
})
.catch(e => (error = e));
portFinder.discover(target).catch(e => (error = e));
portFinder.destroy();
portProviderOffer({
data: { channel, port, source: 'hypothesis', type: 'offer' },
sendPortProviderOffer({
data: {
authority,
frame1,
frame2: target,
type,
},
ports: [port1],
});
clock.tick(MAX_WAIT_FOR_PORT);
......@@ -57,25 +68,17 @@ describe('PortFinder', () => {
assert.equal(
error.message,
"Unable to find 'sidebar' port on 'host-sidebar' channel"
'Unable to establish guest-host communication channel'
);
});
});
describe('#discover', () => {
[
{ channel: 'invalid', port: 'guest' },
{ channel: 'guest-host', port: 'invalid' },
{ channel: 'guest-host', port: 'host' },
].forEach(({ channel, port }) =>
['guest', 'invalid'].forEach(target =>
it('rejects if requesting an invalid port', async () => {
let error;
try {
await portFinder.discover({
channel,
hostFrame: window,
port,
});
await portFinder.discover(target);
} catch (e) {
error = e;
}
......@@ -84,24 +87,24 @@ describe('PortFinder', () => {
);
[
{ channel: 'guest-host', port: 'guest' },
{ channel: 'guest-sidebar', port: 'guest' },
{ channel: 'host-sidebar', port: 'sidebar' },
{ channel: 'notebook-sidebar', port: 'notebook' },
].forEach(({ channel, port }) =>
{ source: 'guest', target: 'host' },
{ source: 'guest', target: 'sidebar' },
{ source: 'sidebar', target: 'host' },
{ source: 'notebook', target: 'sidebar' },
].forEach(({ source, target }) =>
it('resolves if requesting a valid port', async () => {
const { port1 } = new MessageChannel();
let resolvedPort;
portFinder
.discover({
channel,
hostFrame: window,
port,
})
.then(port => (resolvedPort = port));
portProviderOffer({
data: { channel, port, source: 'hypothesis', type: 'offer' },
portFinder = createPortFinder(source);
portFinder.discover(target).then(port => (resolvedPort = port));
sendPortProviderOffer({
data: {
authority,
frame1: source,
frame2: target,
type,
},
ports: [port1],
});
await delay(0);
......@@ -112,18 +115,11 @@ describe('PortFinder', () => {
it("times out if host doesn't respond", async () => {
let error;
const channel = 'host-sidebar';
const port = 'sidebar';
const target = 'host';
const clock = sinon.useFakeTimers();
try {
portFinder
.discover({
channel,
hostFrame: window,
port,
})
.catch(e => (error = e));
portFinder.discover(target).catch(e => (error = e));
clock.tick(MAX_WAIT_FOR_PORT);
} finally {
clock.restore();
......@@ -132,7 +128,7 @@ describe('PortFinder', () => {
assert.callCount(window.postMessage, 21);
assert.alwaysCalledWithExactly(
window.postMessage,
{ channel, port, source: 'hypothesis', type: 'request' },
{ authority, frame1, frame2: target, type: 'request' },
'*'
);
......@@ -140,7 +136,7 @@ describe('PortFinder', () => {
assert.equal(
error.message,
"Unable to find 'sidebar' port on 'host-sidebar' channel"
'Unable to establish guest-host communication channel'
);
});
});
......
import { delay } from '../../test-util/wait';
import { PortProvider } from '../port-provider';
const source = 'hypothesis';
const authority = 'hypothesis';
describe('PortProvider', () => {
let portProvider;
function portFinderRequest({
async function sendPortFinderRequest({
data,
origin = window.location.origin,
source = window,
......@@ -18,6 +18,7 @@ describe('PortProvider', () => {
});
window.dispatchEvent(event);
await delay(0);
return event;
}
......@@ -25,7 +26,6 @@ describe('PortProvider', () => {
beforeEach(() => {
sinon.stub(window, 'postMessage');
portProvider = new PortProvider(window.location.origin);
portProvider.listen();
});
afterEach(() => {
......@@ -36,97 +36,76 @@ describe('PortProvider', () => {
describe('#destroy', () => {
it('ignores valid port request if `PortFinder` has been destroyed', async () => {
portProvider.destroy();
portFinderRequest({
await sendPortFinderRequest({
data: {
channel: 'host-sidebar',
port: 'sidebar',
source,
authority,
frame1: 'sidebar',
frame2: 'host',
type: 'request',
},
});
await delay(0);
assert.notCalled(window.postMessage);
});
});
describe('#getPort', () => {
it('returns `null` if called with wrong arguments', () => {
let hostPort;
// Incorrect channel
hostPort = portProvider.getPort({
channel: 'notebook-sidebar',
port: 'host',
});
assert.isNull(hostPort);
// Incorrect port
hostPort = portProvider.getPort({
channel: 'host-sidebar',
port: 'sidebar',
});
assert.isNull(hostPort);
});
it('returns the `host` port of the `host-sidebar` channel if called with the right arguments', () => {
const hostPort = portProvider.getPort({
channel: 'host-sidebar',
port: 'host',
});
assert.exists(hostPort);
describe('#sidebarPort', () => {
it('returns the `host` port of the `sidebar-host` channel', () => {
assert.instanceOf(portProvider.sidebarPort, MessagePort);
});
});
describe('#listen', () => {
it('ignores all port requests before `listen` is called', async () => {
portProvider.listen();
portProvider.destroy();
portProvider = new PortProvider(window.location.origin);
const data = {
channel: 'host-sidebar',
port: 'sidebar',
source,
authority,
frame1: 'sidebar',
frame2: 'host',
type: 'request',
};
portFinderRequest({
await sendPortFinderRequest({
data,
});
await delay(0);
assert.notCalled(window.postMessage);
portProvider.listen();
portFinderRequest({
await sendPortFinderRequest({
data,
});
await delay(0);
assert.calledOnce(window.postMessage);
});
});
describe('listens for port requests', () => {
it('ignores port requests with invalid sources', async () => {
const data = {
channel: 'host-sidebar',
port: 'sidebar',
source,
type: 'request',
};
portFinderRequest({
data,
source: null,
});
portFinderRequest({
data,
[
{ source: null, reason: 'source is null' },
{
source: new MessageChannel().port1,
});
await delay(0);
reason: 'source is a MessageChannel',
},
].forEach(({ source, reason }) =>
it(`ignores port requests if ${reason}`, async () => {
portProvider.listen();
const data = {
authority,
frame1: 'sidebar',
frame2: 'host',
type: 'request',
};
assert.notCalled(window.postMessage);
});
await sendPortFinderRequest({
data,
source,
});
assert.notCalled(window.postMessage);
})
);
[
// Disabled this check because it make axes-core to crash
......@@ -134,71 +113,73 @@ describe('PortProvider', () => {
//{ data: null, reason: 'if message is null' },
{
data: {
channel: 'sidebar-host', // invalid channel (swapped words)
port: 'sidebar',
source,
authority: 'dummy', // invalid authority
frame1: 'sidebar',
frame2: 'host',
type: 'request',
},
reason: 'if message contains an invalid channel',
reason: 'contains an invalid authority',
},
{
data: {
channel: 'host-sidebar',
port: 'host', // invalid port
source,
authority,
frame1: 'host', // invalid source
frame2: 'host',
type: 'request',
},
reason: 'if message contains an invalid port',
reason: 'contains an invalid frame1',
},
{
data: {
channel: 'host-sidebar',
port: 'sidebar',
source: 'dummy',
authority,
frame1: 'sidebar',
frame2: 'dummy', // invalid target
type: 'request',
},
reason: 'if message contains an invalid source',
reason: 'contains an invalid frame2',
},
{
data: {
channel: 'host-sidebar',
port: 'dummy',
source,
type: 'offer', // invalid offer
authority,
frame1: 'sidebar',
frame2: 'host',
type: 'offer', // invalid type
},
reason: 'if message contains an invalid offer',
reason: 'contains an invalid type',
},
{
data: {
channel: 'host-sidebar',
port: 'sidebar',
source,
authority,
frame1: 'sidebar',
frame2: 'host',
type: 'request',
},
origin: 'https://dummy.com',
reason: 'if message comes from invalid origin',
reason: 'comes from invalid origin',
},
].forEach(({ data, reason, origin }) => {
it(`ignores port request ${reason}`, async () => {
portFinderRequest({ data, origin: origin ?? window.location.origin });
await delay(0);
it(`ignores port request if message ${reason}`, async () => {
portProvider.listen();
await sendPortFinderRequest({
data,
origin: origin ?? window.location.origin,
});
assert.notCalled(window.postMessage);
});
});
it('responds to a valid port request', async () => {
portProvider.listen();
const data = {
channel: 'host-sidebar',
port: 'sidebar',
source,
authority,
frame1: 'sidebar',
frame2: 'host',
type: 'request',
};
portFinderRequest({
await sendPortFinderRequest({
data,
});
await delay(0);
assert.calledWith(
window.postMessage,
......@@ -209,19 +190,19 @@ describe('PortProvider', () => {
});
it('responds to the first valid port request but ignores additional requests', async () => {
portProvider.listen();
const data = {
channel: 'guest-host',
port: 'guest',
source,
authority,
frame1: 'guest',
frame2: 'sidebar',
type: 'request',
};
for (let i = 0; i < 4; ++i) {
portFinderRequest({
await sendPortFinderRequest({
data,
});
}
await delay(0);
assert.calledOnceWithExactly(
window.postMessage,
......@@ -232,30 +213,29 @@ describe('PortProvider', () => {
});
it('sends the counterpart port via the sidebar port', async () => {
portFinderRequest({
portProvider.listen();
await sendPortFinderRequest({
data: {
channel: 'host-sidebar',
port: 'sidebar',
source,
authority,
frame1: 'sidebar',
frame2: 'host',
type: 'request',
},
});
await delay(0);
const [sidebarPort] = window.postMessage.getCall(0).args[2];
const handler = sinon.stub();
sidebarPort.onmessage = handler;
const data = {
channel: 'guest-sidebar',
port: 'guest',
source,
authority,
frame1: 'guest',
frame2: 'sidebar',
type: 'request',
};
portFinderRequest({
await sendPortFinderRequest({
data,
});
await delay(0);
assert.calledWith(
handler,
......@@ -266,18 +246,18 @@ describe('PortProvider', () => {
});
it('sends the counterpart port via the listener', async () => {
portProvider.listen();
const handler = sinon.stub();
portProvider.on('hostPortRequest', handler);
const data = {
channel: 'guest-host',
port: 'guest',
source,
authority,
frame1: 'guest',
frame2: 'host',
type: 'request',
};
portFinderRequest({
await sendPortFinderRequest({
data,
});
await delay(0);
assert.calledWith(handler, 'guest', sinon.match.instanceOf(MessagePort));
});
......
import { isMessageEqual } from '../port-util';
const source = 'hypothesis';
describe('port-util', () => {
describe('isMessageEqual', () => {
const authority = 'hypothesis';
const frame1 = 'guest';
const frame2 = 'sidebar';
const type = 'offer';
[
{
data: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
},
message: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
authority,
frame1,
frame2,
type,
},
expectedResult: true,
reason: 'data matches the message',
},
{
data: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
},
message: {
source,
type: 'offer',
channel: 'host-sidebar',
port: 'guest',
authority,
frame1,
frame2,
type,
},
expectedResult: true,
reason: 'data matches the message (properties in different order)',
},
{
data: null,
message: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
},
expectedResult: false,
reason: 'data is null',
},
{
data: {
channel: 'host-sidebar',
port: 9, // wrong type
source,
type: 'offer',
},
message: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
authority,
// frame1 property missing
frame2,
type,
},
expectedResult: false,
reason: 'data has a property with the wrong type',
reason: 'data has one property that is missing',
},
{
data: {
// channel property missing
port: 'guest',
source,
type: 'offer',
},
message: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
authority,
frame1,
frame2: 9, // wrong type
type,
},
expectedResult: false,
reason: 'data has one property that is missing',
reason: 'data has one property with a wrong type',
},
{
data: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
authority,
extra: 'dummy', // additional
},
message: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
frame1,
frame2,
type,
},
expectedResult: false,
reason: 'data has one additional property',
},
{
data: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
window, // not serializable
},
message: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
authority,
frame1: 'dummy', // different
frame2,
type,
},
expectedResult: false,
reason: "data has one property that can't be serialized",
reason: 'data has one property that is different',
},
{
data: {
channel: 'host-sidebar',
port: 'guest',
source,
type: 'offer',
},
message: {
channel: 'guest-sidebar', // different
port: 'guest',
source,
type: 'offer',
authority,
frame1,
frame2,
type,
window, // not serializable
},
expectedResult: false,
reason: 'data has one property that is different',
reason: "data has one property that can't be serialized",
},
].forEach(({ data, message, expectedResult, reason }) => {
].forEach(({ data, expectedResult, reason }) => {
it(`returns '${expectedResult}' because the ${reason}`, () => {
const result = isMessageEqual(data, message);
const result = isMessageEqual(data, {
authority,
frame1,
frame2,
type,
});
assert.equal(result, expectedResult);
});
});
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment