refactor[devtools/extension]: handle ports disconnection, instead of frequent reconnections

Author: hoxyq

packages/react-devtools-extensions/src/background/dynamicallyInjectContentScripts.js

@@ -2,26 +2,46 @@
 
 import {IS_FIREFOX} from '../utils';
 
-async function dynamicallyInjectContentScripts() {
-  const contentScriptsToInject = [
-    {
-      id: '@react-devtools/hook',
-      js: ['build/installHook.js'],
-      matches: ['<all_urls>'],
-      persistAcrossSessions: true,
-      runAt: 'document_start',
-      world: chrome.scripting.ExecutionWorld.MAIN,
-    },
-    {
-      id: '@react-devtools/renderer',
-      js: ['build/renderer.js'],
-      matches: ['<all_urls>'],
-      persistAcrossSessions: true,
-      runAt: 'document_start',
-      world: chrome.scripting.ExecutionWorld.MAIN,
-    },
-  ];
+// Firefox doesn't support ExecutionWorld.MAIN yet
+// equivalent logic for Firefox is in prepareInjection.js
+const contentScriptsToInject = IS_FIREFOX
+  ? [
+      {
+        id: '@react-devtools/proxy',
+        js: ['build/proxy.js'],
+        matches: ['<all_urls>'],
+        persistAcrossSessions: true,
+        runAt: 'document_idle',
+      },
+    ]
+  : [
+      {
+        id: '@react-devtools/proxy',
+        js: ['build/proxy.js'],
+        matches: ['<all_urls>'],
+        persistAcrossSessions: true,
+        runAt: 'document_end',
+        world: chrome.scripting.ExecutionWorld.ISOLATED,
+      },
+      {
+        id: '@react-devtools/hook',
+        js: ['build/installHook.js'],
+        matches: ['<all_urls>'],
+        persistAcrossSessions: true,
+        runAt: 'document_start',
+        world: chrome.scripting.ExecutionWorld.MAIN,
+      },
+      {
+        id: '@react-devtools/renderer',
+        js: ['build/renderer.js'],
+        matches: ['<all_urls>'],
+        persistAcrossSessions: true,
+        runAt: 'document_start',
+        world: chrome.scripting.ExecutionWorld.MAIN,
+      },
+    ];
 
+async function dynamicallyInjectContentScripts() {
   try {
     const alreadyRegisteredContentScripts =
       await chrome.scripting.getRegisteredContentScripts();
@@ -48,6 +68,4 @@ async function dynamicallyInjectContentScripts() {
   }
 }
 
-if (!IS_FIREFOX) {
-  dynamicallyInjectContentScripts();
-}
+dynamicallyInjectContentScripts();

packages/react-devtools-extensions/src/background/index.js

@@ -7,7 +7,6 @@ import {IS_FIREFOX, EXTENSION_CONTAINED_VERSIONS} from '../utils';
 import './dynamicallyInjectContentScripts';
 import './tabsManager';
 import setExtensionIconAndPopup from './setExtensionIconAndPopup';
-import injectProxy from './injectProxy';
 
 /*
   {
@@ -39,18 +38,6 @@ function registerExtensionPort(port, tabId) {
     ports[tabId].disconnectPipe?.();
 
     delete ports[tabId].extension;
-
-    const proxyPort = ports[tabId].proxy;
-    if (proxyPort) {
-      // Do not disconnect proxy port, we will inject this content script again
-      // If extension port has disconnected, it probably means that user did in-tab navigation
-      clearReconnectionTimeout(proxyPort);
-
-      proxyPort.postMessage({
-        source: 'react-devtools-service-worker',
-        stop: true,
-      });
-    }
   });
 }
 
@@ -59,36 +46,12 @@ function registerProxyPort(port, tabId) {
 
   // In case proxy port was disconnected from the other end, from content script
   // This can happen if content script was detached, when user does in-tab navigation
-  // Or if when we notify proxy port to stop reconnecting, when extension port dies
-  // This listener should never be called when we call port.shutdown() from this (background/index.js) script
+  // This listener should never be called when we call port.disconnect() from this (background/index.js) script
   port.onDisconnect.addListener(() => {
     ports[tabId].disconnectPipe?.();
 
     delete ports[tabId].proxy;
   });
-
-  port._reconnectionTimeoutId = setTimeout(
-    reconnectProxyPort,
-    25_000,
-    port,
-    tabId,
-  );
-}
-
-function clearReconnectionTimeout(port) {
-  if (port._reconnectionTimeoutId) {
-    clearTimeout(port._reconnectionTimeoutId);
-    delete port._reconnectionTimeoutId;
-  }
-}
-
-function reconnectProxyPort(port, tabId) {
-  // IMPORTANT: port.onDisconnect will only be emitted if disconnect() was called from the other end
-  // We need to do it manually here if we disconnect proxy port from service worker
-  ports[tabId].disconnectPipe?.();
-
-  // It should be reconnected automatically by proxy content script, look at proxy.js
-  port.disconnect();
 }
 
 function isNumeric(str: string): boolean {
@@ -100,42 +63,38 @@ chrome.runtime.onConnect.addListener(port => {
     // Proxy content script is executed in tab, so it should have it specified.
     const tabId = port.sender.tab.id;
 
+    if (ports[tabId]?.proxy) {
+      port.disconnect();
+      return;
+    }
+
     registerTab(tabId);
     registerProxyPort(port, tabId);
 
-    connectExtensionAndProxyPorts(
-      ports[tabId].extension,
-      ports[tabId].proxy,
-      tabId,
-    );
+    if (ports[tabId].extension) {
+      connectExtensionAndProxyPorts(
+        ports[tabId].extension,
+        ports[tabId].proxy,
+        tabId,
+      );
+    }
 
     return;
   }
 
   if (isNumeric(port.name)) {
     // Extension port doesn't have tab id specified, because its sender is the extension.
     const tabId = +port.name;
-    const extensionPortAlreadyConnected = ports[tabId]?.extension != null;
-
-    // Handle the case when extension port was disconnected and we were not notified
-    if (extensionPortAlreadyConnected) {
-      ports[tabId].disconnectPipe?.();
-    }
 
     registerTab(tabId);
     registerExtensionPort(port, tabId);
 
-    if (extensionPortAlreadyConnected) {
-      const proxyPort = ports[tabId].proxy;
-
-      // Avoid re-injecting the content script, we might end up in a situation
-      // where we would have multiple proxy ports opened and trying to reconnect
-      if (proxyPort) {
-        clearReconnectionTimeout(proxyPort);
-        reconnectProxyPort(proxyPort, tabId);
-      }
-    } else {
-      injectProxy(tabId);
+    if (ports[tabId].proxy) {
+      connectExtensionAndProxyPorts(
+        ports[tabId].extension,
+        ports[tabId].proxy,
+        tabId,
+      );
     }
 
     return;

packages/react-devtools-extensions/src/contentScripts/proxy.js

@@ -30,18 +30,6 @@ function sayHelloToBackendManager() {
 }
 
 function handleMessageFromDevtools(message) {
-  if (message.source === 'react-devtools-service-worker' && message.stop) {
-    window.removeEventListener('message', handleMessageFromPage);
-
-    // Calling disconnect here should not emit onDisconnect event inside this script
-    // This port will not attempt to reconnect again
-    // It will connect only once this content script will be injected again
-    port?.disconnect();
-    port = null;
-
-    return;
-  }
-
   window.postMessage(
     {
       source: 'react-devtools-content-script',

packages/react-devtools-extensions/src/main/debounce.js

@@ -0,0 +1,10 @@
+function debounce(fn, timeout) {
+  let executionTimeoutId = null;
+
+  return (...args) => {
+    clearTimeout(executionTimeoutId);
+    executionTimeoutId = setTimeout(fn, timeout, ...args);
+  };
+}
+
+export default debounce;

packages/react-devtools-extensions/src/main/index.js

@@ -27,20 +27,21 @@ import injectBackendManager from './injectBackendManager';
 import syncSavedPreferences from './syncSavedPreferences';
 import registerEventsLogger from './registerEventsLogger';
 import getProfilingFlags from './getProfilingFlags';
+import debounce from './debounce';
 import './requestAnimationFramePolyfill';
 
 // Try polling for at least 5 seconds, in case if it takes too long to load react
 const REACT_POLLING_TICK_COOLDOWN = 250;
 const REACT_POLLING_ATTEMPTS_THRESHOLD = 20;
 
 let reactPollingTimeoutId = null;
-function clearReactPollingTimeout() {
+export function clearReactPollingTimeout() {
   clearTimeout(reactPollingTimeoutId);
   reactPollingTimeoutId = null;
 }
 
-function executeIfReactHasLoaded(callback, attempt = 1) {
-  reactPollingTimeoutId = null;
+export function executeIfReactHasLoaded(callback, attempt = 1) {
+  clearReactPollingTimeout();
 
   if (attempt > REACT_POLLING_ATTEMPTS_THRESHOLD) {
     return;
@@ -81,21 +82,26 @@ function executeIfReactHasLoaded(callback, attempt = 1) {
   );
 }
 
+let lastSubscribedBridgeListener = null;
+
 function createBridge() {
   bridge = new Bridge({
     listen(fn) {
-      const listener = message => fn(message);
+      const bridgeListener = message => fn(message);
       // Store the reference so that we unsubscribe from the same object.
       const portOnMessage = port.onMessage;
-      portOnMessage.addListener(listener);
+      portOnMessage.addListener(bridgeListener);
+
+      lastSubscribedBridgeListener = bridgeListener;
 
       return () => {
-        portOnMessage.removeListener(listener);
+        port?.onMessage.removeListener(bridgeListener);
+        lastSubscribedBridgeListener = null;
       };
     },
 
     send(event: string, payload: any, transferable?: Array<any>) {
-      port.postMessage({event, payload}, transferable);
+      port?.postMessage({event, payload}, transferable);
     },
   });
 
@@ -469,9 +475,6 @@ function performInTabNavigationCleanup() {
   bridge = null;
   render = null;
   root = null;
-
-  port?.disconnect();
-  port = null;
 }
 
 function performFullCleanup() {
@@ -499,23 +502,37 @@ function performFullCleanup() {
 }
 
 function connectExtensionPort() {
+  if (port) {
+    throw new Error('DevTools port was already connected');
+  }
+
   const tabId = chrome.devtools.inspectedWindow.tabId;
   port = chrome.runtime.connect({
     name: String(tabId),
   });
 
+  // If DevTools port was reconnected and Bridge was already created
+  // We should subscribe bridge to this port events
+  // This could happen if service worker dies and all ports are disconnected,
+  // but later user continues the session and Chrome reconnects all ports
+  // Bridge object is still in-memory, though
+  if (lastSubscribedBridgeListener) {
+    port.onMessage.addListener(lastSubscribedBridgeListener);
+  }
+
   // This port may be disconnected by Chrome at some point, this callback
   // will be executed only if this port was disconnected from the other end
   // so, when we call `port.disconnect()` from this script,
   // this should not trigger this callback and port reconnection
-  port.onDisconnect.addListener(connectExtensionPort);
+  port.onDisconnect.addListener(() => {
+    port = null;
+    connectExtensionPort();
+  });
 }
 
 function mountReactDevTools() {
   registerEventsLogger();
 
-  connectExtensionPort();
-
   createBridgeAndStore();
 
   setReactSelectionFromBrowser(bridge);
@@ -532,7 +549,7 @@ function mountReactDevToolsWhenReactHasLoaded() {
     mountReactDevTools();
   }
 
-  executeIfReactHasLoaded(onReactReady);
+  executeIfReactHasLoaded(onReactReady, 1);
 }
 
 let bridge = null;
@@ -555,11 +572,18 @@ let port = null;
 // since global values stored on window get reset in this case.
 chrome.devtools.network.onNavigated.addListener(syncSavedPreferences);
 
-// Cleanup previous page state and remount everything
-chrome.devtools.network.onNavigated.addListener(() => {
+// In case when multiple navigation events emitted in a short period of time
+// This debounced callback primarily used to avoid mounting React DevTools multiple times, which results
+// into subscribing to the same events from Bridge and window multiple times
+// In this case, we will handle `operations` event twice or more and user will see
+// `Cannot add node "1" because a node with that id is already in the Store.`
+const debouncedOnNavigatedListener = debounce(() => {
   performInTabNavigationCleanup();
   mountReactDevToolsWhenReactHasLoaded();
-});
+}, 500);
+
+// Cleanup previous page state and remount everything
+chrome.devtools.network.onNavigated.addListener(debouncedOnNavigatedListener);
 
 // Should be emitted when browser DevTools are closed
 if (IS_FIREFOX) {
@@ -569,5 +593,7 @@ if (IS_FIREFOX) {
   window.addEventListener('beforeunload', performFullCleanup);
 }
 
+connectExtensionPort();
+
 syncSavedPreferences();
 mountReactDevToolsWhenReactHasLoaded();