Skip to content

Commit 1d8d120

Browse files
authored
fix[react-devtools]: remove all listeners when Agent is shutdown (#31151)
Based on #31049, credits to @EdmondChuiHW. What is happening here: 1. Once Agent is destroyed, unsubscribe own listeners and bridge listeners. 2. [Browser extension only] Once Agent is destroyed, unsubscribe listeners from BackendManager. 3. [Browser extension only] I've discovered that `backendManager.js` content script can get injected multiple times by the browser. When Frontend is initializing, it will create Store first, and then execute a content script for bootstraping backend manager. If Frontend was destroyed somewhere between these 2 steps, Backend won't be notified, because it is not initialized yet, so it will not unsubscribe listeners correctly. We might end up duplicating listeners, and the next time Frontend is launched, it will report an issues "Cannot add / remove node ...", because same operations are emitted twice. To reproduce 3 you can do the following: 1. Click reload-to-profile 2. Right after when both app and Chrome DevTools panel are reloaded, close Chrome DevTools. 3. Open Chrome DevTools again, open Profiler panel and observe "Cannot add / remove node ..." error in the UI.
1 parent 4a86ec5 commit 1d8d120

File tree

4 files changed

+31
-14
lines changed

4 files changed

+31
-14
lines changed

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

+25-8
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {COMPACT_VERSION_NAME} from 'react-devtools-extensions/src/utils';
1616
import {getIsReloadAndProfileSupported} from 'react-devtools-shared/src/utils';
1717

1818
let welcomeHasInitialized = false;
19+
const requiredBackends = new Set<string>();
1920

2021
function welcome(event: $FlowFixMe) {
2122
if (
@@ -49,8 +50,6 @@ function welcome(event: $FlowFixMe) {
4950
setup(window.__REACT_DEVTOOLS_GLOBAL_HOOK__);
5051
}
5152

52-
window.addEventListener('message', welcome);
53-
5453
function setup(hook: ?DevToolsHook) {
5554
// this should not happen, but Chrome can be weird sometimes
5655
if (hook == null) {
@@ -71,20 +70,27 @@ function setup(hook: ?DevToolsHook) {
7170
updateRequiredBackends();
7271

7372
// register renderers that inject themselves later.
74-
hook.sub('renderer', ({renderer}) => {
73+
const unsubscribeRendererListener = hook.sub('renderer', ({renderer}) => {
7574
registerRenderer(renderer, hook);
7675
updateRequiredBackends();
7776
});
7877

7978
// listen for backend installations.
80-
hook.sub('devtools-backend-installed', version => {
81-
activateBackend(version, hook);
82-
updateRequiredBackends();
79+
const unsubscribeBackendInstallationListener = hook.sub(
80+
'devtools-backend-installed',
81+
version => {
82+
activateBackend(version, hook);
83+
updateRequiredBackends();
84+
},
85+
);
86+
87+
const unsubscribeShutdownListener: () => void = hook.sub('shutdown', () => {
88+
unsubscribeRendererListener();
89+
unsubscribeBackendInstallationListener();
90+
unsubscribeShutdownListener();
8391
});
8492
}
8593

86-
const requiredBackends = new Set<string>();
87-
8894
function registerRenderer(renderer: ReactRenderer, hook: DevToolsHook) {
8995
let version = renderer.reconcilerVersion || renderer.version;
9096
if (!hasAssignedBackend(version)) {
@@ -139,6 +145,7 @@ function activateBackend(version: string, hook: DevToolsHook) {
139145
// If we received 'shutdown' from `agent`, we assume the `bridge` is already shutting down,
140146
// and that caused the 'shutdown' event on the `agent`, so we don't need to call `bridge.shutdown()` here.
141147
hook.emit('shutdown');
148+
delete window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__;
142149
});
143150

144151
initBackend(hook, agent, window, getIsReloadAndProfileSupported());
@@ -178,3 +185,13 @@ function updateRequiredBackends() {
178185
'*',
179186
);
180187
}
188+
189+
/*
190+
* Make sure this is executed only once in case Frontend is reloaded multiple times while Backend is initializing
191+
* We can't use `reactDevToolsAgent` field on a global Hook object, because it only cleaned up after both Frontend and Backend initialized
192+
*/
193+
if (!window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__) {
194+
window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__ = true;
195+
196+
window.addEventListener('message', welcome);
197+
}

packages/react-devtools-shared/src/backend/agent.js

+3
Original file line numberDiff line numberDiff line change
@@ -750,6 +750,9 @@ export default class Agent extends EventEmitter<{
750750
shutdown: () => void = () => {
751751
// Clean up the overlay if visible, and associated events.
752752
this.emit('shutdown');
753+
754+
this._bridge.removeAllListeners();
755+
this.removeAllListeners();
753756
};
754757

755758
startProfiling: (recordChangeDescriptions: boolean) => void =

packages/react-devtools-shared/src/backend/fiber/renderer.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -3473,7 +3473,7 @@ export function attach(
34733473
}
34743474

34753475
function cleanup() {
3476-
// We don't patch any methods so there is no cleanup.
3476+
isProfiling = false;
34773477
}
34783478

34793479
function rootSupportsProfiling(root: any) {

packages/react-devtools-shared/src/backend/index.js

+2-5
Original file line numberDiff line numberDiff line change
@@ -80,15 +80,12 @@ export function initBackend(
8080
});
8181
hook.reactDevtoolsAgent = null;
8282
};
83-
agent.addListener('shutdown', onAgentShutdown);
84-
subs.push(() => {
85-
agent.removeListener('shutdown', onAgentShutdown);
86-
});
8783

84+
// Agent's event listeners are cleaned up by Agent in `shutdown` implementation.
85+
agent.addListener('shutdown', onAgentShutdown);
8886
agent.addListener('updateHookSettings', settings => {
8987
hook.settings = settings;
9088
});
91-
9289
agent.addListener('getHookSettings', () => {
9390
if (hook.settings != null) {
9491
agent.onHookSettings(hook.settings);

0 commit comments

Comments
 (0)