From 93675f290b28ff8764f99adc5da236be4351f3ca Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Tue, 5 Sep 2023 14:42:45 +0100 Subject: [PATCH] refactor[devtools/extension]: handle ports disconnection, instead of frequent reconnections --- .../dynamicallyInjectContentScripts.js | 62 +++++++++------ .../src/background/index.js | 79 +++++-------------- .../src/contentScripts/proxy.js | 12 --- .../src/main/debounce.js | 10 +++ .../src/main/index.js | 60 ++++++++++---- 5 files changed, 112 insertions(+), 111 deletions(-) create mode 100644 packages/react-devtools-extensions/src/main/debounce.js diff --git a/packages/react-devtools-extensions/src/background/dynamicallyInjectContentScripts.js b/packages/react-devtools-extensions/src/background/dynamicallyInjectContentScripts.js index 4f22b70bcfe9f..4e8811fddb847 100644 --- a/packages/react-devtools-extensions/src/background/dynamicallyInjectContentScripts.js +++ b/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: [''], - persistAcrossSessions: true, - runAt: 'document_start', - world: chrome.scripting.ExecutionWorld.MAIN, - }, - { - id: '@react-devtools/renderer', - js: ['build/renderer.js'], - matches: [''], - 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: [''], + persistAcrossSessions: true, + runAt: 'document_idle', + }, + ] + : [ + { + id: '@react-devtools/proxy', + js: ['build/proxy.js'], + matches: [''], + persistAcrossSessions: true, + runAt: 'document_end', + world: chrome.scripting.ExecutionWorld.ISOLATED, + }, + { + id: '@react-devtools/hook', + js: ['build/installHook.js'], + matches: [''], + persistAcrossSessions: true, + runAt: 'document_start', + world: chrome.scripting.ExecutionWorld.MAIN, + }, + { + id: '@react-devtools/renderer', + js: ['build/renderer.js'], + matches: [''], + 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(); diff --git a/packages/react-devtools-extensions/src/background/index.js b/packages/react-devtools-extensions/src/background/index.js index 86000f73adf63..c4aa1c677e86a 100644 --- a/packages/react-devtools-extensions/src/background/index.js +++ b/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,14 +63,21 @@ 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; } @@ -115,27 +85,16 @@ chrome.runtime.onConnect.addListener(port => { 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; diff --git a/packages/react-devtools-extensions/src/contentScripts/proxy.js b/packages/react-devtools-extensions/src/contentScripts/proxy.js index 76c915dca4189..f21e2ee968b2f 100644 --- a/packages/react-devtools-extensions/src/contentScripts/proxy.js +++ b/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', diff --git a/packages/react-devtools-extensions/src/main/debounce.js b/packages/react-devtools-extensions/src/main/debounce.js new file mode 100644 index 0000000000000..5ca765002f2fa --- /dev/null +++ b/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; diff --git a/packages/react-devtools-extensions/src/main/index.js b/packages/react-devtools-extensions/src/main/index.js index 92f77da4a3e73..19a086f1ea7a4 100644 --- a/packages/react-devtools-extensions/src/main/index.js +++ b/packages/react-devtools-extensions/src/main/index.js @@ -27,6 +27,7 @@ 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 @@ -34,13 +35,13 @@ 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) { - 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();