refactor[devtools/extension]: handle ports disconnection, instead of frequent reconnection (#27336)

- Instead of reconnecting ports from devtools page and proxy content
script, now handling their disconnection properly
- `proxy.js` is now dynamically registered as a content script, which
loaded for each page. This will probably not work well for Firefox,
since we are still on manifest v2, I will try to fix this in the next
few PRs.
- Handling the case when devtools page port was reconnected and bridge
is still present. This could happen if user switches the tab and Chrome
decides to kill service worker, devtools page port gets disconnected,
and then user returns back to the tab. When port is reconnected, we
check if bridge message listener is present, connecting them if so.
- Added simple debounce when evaluating if page has react application
running. We start this check in `chrome.network.onNavigated` listener,
which is asynchronous. Also, this check itself is asynchronous, so
previously we could mount React DevTools multiple times if navigates
multiple times while `chrome.devtools.inspectedWindow.eval` (which is
also asynchronous) can be executed.
https://github.com/hoxyq/react/blob/00b7c4331819289548b40714aea12335368e10f4/packages/react-devtools-extensions/src/main/index.js#L575-L583



https://github.com/facebook/react/assets/28902667/9d519a77-145e-413c-b142-b5063223d073

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