diff --git a/flow.js b/flow.js index df570e1021..fbbc999554 100644 --- a/flow.js +++ b/flow.js @@ -5,6 +5,7 @@ declare module 'events' { addListener: (type: string, fn: Function) => void; emit: (type: string, data: any) => void; removeListener: (type: string, fn: Function) => void; + removeAllListeners: (type?: string) => void; } declare export default typeof EventEmitter; diff --git a/shells/browser/shared/src/backend.js b/shells/browser/shared/src/backend.js index 88723ff563..97f9e55d6e 100644 --- a/shells/browser/shared/src/backend.js +++ b/shells/browser/shared/src/backend.js @@ -24,8 +24,6 @@ function setup(hook) { const Bridge = require('src/bridge').default; const { initBackend } = require('src/backend'); - const listeners = []; - const bridge = new Bridge({ listen(fn) { const listener = event => { @@ -39,8 +37,10 @@ function setup(hook) { } fn(event.data.payload); }; - listeners.push(listener); window.addEventListener('message', listener); + return () => { + window.removeEventListener('message', listener); + }; }, send(event: string, payload: any, transferable?: Array) { window.postMessage( @@ -56,11 +56,9 @@ function setup(hook) { const agent = new Agent(bridge); agent.addListener('shutdown', () => { + // If we received 'shutdown' from `agent`, we assume the `bridge` is already shutting down, + // and that caused the 'shutdown' event on the `agent`, so we don't need to call `bridge.shutdown()` here. hook.emit('shutdown'); - listeners.forEach(fn => { - window.removeEventListener('message', fn); - }); - listeners.splice(0); }); initBackend(hook, agent, window); diff --git a/shells/browser/shared/src/main.js b/shells/browser/shared/src/main.js index 8f3ad6823d..400ffa9475 100644 --- a/shells/browser/shared/src/main.js +++ b/shells/browser/shared/src/main.js @@ -46,22 +46,24 @@ function createPanelIfReactLoaded() { let root = null; function initBridgeAndStore() { - let hasPortBeenDisconnected = false; const port = chrome.runtime.connect({ name: '' + chrome.devtools.inspectedWindow.tabId, }); - port.onDisconnect.addListener(() => { - hasPortBeenDisconnected = true; - }); + // Looks like `port.onDisconnect` does not trigger on in-tab navigation like new URL or back/forward navigation, + // so it makes no sense to handle it here. bridge = new Bridge({ listen(fn) { - port.onMessage.addListener(message => fn(message)); + const listener = message => fn(message); + // Store the reference so that we unsubscribe from the same object. + const portOnMessage = port.onMessage; + portOnMessage.addListener(listener); + return () => { + portOnMessage.removeListener(listener); + }; }, send(event: string, payload: any, transferable?: Array) { - if (!hasPortBeenDisconnected) { - port.postMessage({ event, payload }, transferable); - } + port.postMessage({ event, payload }, transferable); }, }); bridge.addListener('reloadAppForProfiling', () => { @@ -270,7 +272,8 @@ function createPanelIfReactLoaded() { // Shutdown bridge and re-initialize DevTools panel when a new page is loaded. chrome.devtools.network.onNavigated.addListener(function onNavigated() { - bridge.send('shutdown'); + // `bridge.shutdown()` will remove all listeners we added, so we don't have to. + bridge.shutdown(); // It's easiest to recreate the DevTools panel (to clean up potential stale state). // We can revisit this in the future as a small optimization. diff --git a/shells/dev/src/backend.js b/shells/dev/src/backend.js index bfc403bb6d..ac0799b97f 100644 --- a/shells/dev/src/backend.js +++ b/shells/dev/src/backend.js @@ -7,9 +7,13 @@ import { initBackend } from 'src/backend'; const bridge = new Bridge({ listen(fn) { - window.addEventListener('message', event => { + const listener = event => { fn(event.data); - }); + }; + window.addEventListener('message', listener); + return () => { + window.removeEventListener('message', listener); + }; }, send(event: string, payload: any, transferable?: Array) { window.parent.postMessage({ event, payload }, '*', transferable); diff --git a/shells/dev/src/devtools.js b/shells/dev/src/devtools.js index 3fb49174c5..50e6afd663 100644 --- a/shells/dev/src/devtools.js +++ b/shells/dev/src/devtools.js @@ -43,9 +43,15 @@ inject('./build/app.js', () => { connect(cb) { const bridge = new Bridge({ listen(fn) { - contentWindow.parent.addEventListener('message', ({ data }) => { + const listener = ({ data }) => { fn(data); - }); + }; + // Preserve the reference to the window we subscribe to, so we can unsubscribe from it when required. + const contentWindowParent = contentWindow.parent; + contentWindowParent.addEventListener('message', listener); + return () => { + contentWindowParent.removeEventListener('message', listener); + }; }, send(event: string, payload: any, transferable?: Array) { contentWindow.postMessage({ event, payload }, '*', transferable); diff --git a/src/__tests__/setupTests.js b/src/__tests__/setupTests.js index 33fa159c5e..ce96ed2b48 100644 --- a/src/__tests__/setupTests.js +++ b/src/__tests__/setupTests.js @@ -28,10 +28,15 @@ env.beforeEach(() => { installHook(global); const bridgeListeners = []; - const bridge = new Bridge({ listen(callback) { bridgeListeners.push(callback); + return () => { + const index = bridgeListeners.indexOf(callback); + if (index >= 0) { + bridgeListeners.splice(index, 1); + } + }; }, send(event: string, payload: any, transferable?: Array) { bridgeListeners.forEach(callback => callback({ event, payload })); diff --git a/src/bridge.js b/src/bridge.js index 2a9d53fb4f..eb93462074 100644 --- a/src/bridge.js +++ b/src/bridge.js @@ -14,19 +14,25 @@ type Message = {| export default class Bridge extends EventEmitter { _messageQueue: Array = []; _timeoutID: TimeoutID | null = null; + _destroyed: boolean = false; _wall: Wall; + _wallUnlisten: Function | null = null; constructor(wall: Wall) { super(); this._wall = wall; - wall.listen((message: Message) => { + this._wallUnlisten = wall.listen((message: Message) => { this.emit(message.event, message.payload); }); } send(event: string, payload: any, transferable?: Array) { + if (this._destroyed) { + return; + } + // When we receive a message: // - we add it to our queue of messages to be sent // - if there hasn't been a message recently, we set a timer for 0 ms in @@ -41,7 +47,47 @@ export default class Bridge extends EventEmitter { } } + shutdown() { + if (this._destroyed) { + return; + } + + // Mark this bridge as destroyed, i.e. disable its public API. + this._destroyed = true; + + // Disable the API inherited from EventEmitter that can add more listeners and send more messages. + this.addListener = function() {}; + this.emit = function() {}; + // NOTE: There's also EventEmitter API like `on` and `prependListener` that we didn't add to our Flow type of EventEmitter. + + // Unsubscribe this bridge incoming message listeners to be sure, and so they don't have to do that. + this.removeAllListeners(); + + // Stop accepting and emitting incoming messages from the wall. + const wallUnlisten = this._wallUnlisten; + if (wallUnlisten) { + wallUnlisten(); + } + + // Queue the shutdown outgoing message for subscribers. + this.send('shutdown'); + + // Synchronously flush all queued outgoing messages. + // At this step the subscribers' code may run in this call stack. + do { + this._flush(); + } while (this._messageQueue.length); + + // Make sure once again that there is no dangling timer. + clearTimeout(this._timeoutID); + this._timeoutID = null; + } + _flush = () => { + // This method is used after the bridge is marked as destroyed in shutdown sequence, + // so we do not bail out if the bridge marked as destroyed. + // It is a private method that the bridge ensures is only called at the right times. + clearTimeout(this._timeoutID); this._timeoutID = null; diff --git a/src/types.js b/src/types.js index 2bd576af9b..9e5a9de04e 100644 --- a/src/types.js +++ b/src/types.js @@ -7,6 +7,7 @@ export type Bridge = { }; export type Wall = {| - listen: (fn: Function) => void, + // `listen` returns the "unlisten" function. + listen: (fn: Function) => Function, send: (event: string, payload: any, transferable?: Array) => void, |};