From 1b5f043e015afae9e203bec0388805fa610ed917 Mon Sep 17 00:00:00 2001 From: Ivan Babak Date: Fri, 26 Apr 2019 04:30:04 -0700 Subject: [PATCH] Fix for 'Attempting to use a disconnected port object' Fixes https://github.com/bvaughn/react-devtools-experimental/issues/217 The error reproduces with any two React websites, e.g. `https://reactjs.org` and `https://nextjs.org`, by keeping the DevTools Components tab open and switching between these websites in the same browser tab. There are several issues with the code that contribute to this: 1. `Bridge` leaves behind a dangling timer that fires `_flush` after the bridge has been abandoned ("shutdown"). 2. `bridge.send('shutdown')` is asynchronous, so the event handlers do not get unsubscribed in time. 3. `port.onDisconnect` does not trigger on in-tab navigation like new URL or back/forward navigation. 4. State management design of the code that uses shared variables and callbacks makes it hard to handle race conditions originating from the browser. This commit cleans up some of the lacking symmetry when using `addListener`/`removeListener`, but the code in `shells/browser/shared/src/main.js` is hard to reason about with regards to race conditions, and there are many possible race conditions originating from the browser, so maybe there could be a better design paradigm (like a formal state machine) to manage the state changes in response to sequences of events than plain old event listeners, callbacks, and shared variables. Unrelated, but clicking Chrome Back/Forward/Back/Forward very fast makes the browser and the DevTools and the DevTools of DevTools stall and become unresponsive for some time, then recovers but the Back/Forward/Stop/Refresh button and favicon loading indicator may remain broken. Looks like a Chrome bug, some kind of a temporary deadlock in handling the browser history. --- flow.js | 1 + shells/browser/shared/src/backend.js | 12 +++---- shells/browser/shared/src/main.js | 21 ++++++------ shells/dev/src/backend.js | 8 +++-- shells/dev/src/devtools.js | 10 ++++-- src/__tests__/setupTests.js | 7 +++- src/bridge.js | 48 +++++++++++++++++++++++++++- src/types.js | 3 +- 8 files changed, 87 insertions(+), 23 deletions(-) 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 c86a1a5d4b..4745a15491 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( @@ -57,11 +57,9 @@ function setup(hook) { const agent = new Agent(); agent.addBridge(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 02ff78fd19..0d737ff7fd 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 2c22aade07..7da4eb6df8 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, |};