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

@@ -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;

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<any>) {
       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);

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<any>) {
-            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.

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<any>) {
     window.parent.postMessage({ event, payload }, '*', transferable);

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<any>) {
           contentWindow.postMessage({ event, payload }, '*', transferable);

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<any>) {
       bridgeListeners.forEach(callback => callback({ event, payload }));

src/bridge.js

@@ -14,19 +14,25 @@ type Message = {|
 export default class Bridge extends EventEmitter {
   _messageQueue: Array<any> = [];
   _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<any>) {
+    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;
 

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<any>) => void,
 |};