Merge pull request #229 from sompylasar/217-workaround-disconnected-port-error

Fix for 'Attempting to use a disconnected port object'

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

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,
 |};