Skip to content

Commit e869d7d

Browse files
hoxyqhoxyq
committed
fix[react-devtools]: remove all listeners when Agent is shutdown
1 parent 1460d67 commit e869d7d

4 files changed

Lines changed: 162 additions & 145 deletions

File tree

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

Lines changed: 156 additions & 139 deletions
Original file line numberDiff line numberDiff line change
@@ -15,166 +15,183 @@ import {hasAssignedBackend} from 'react-devtools-shared/src/backend/utils';
1515
import {COMPACT_VERSION_NAME} from 'react-devtools-extensions/src/utils';
1616
import {getIsReloadAndProfileSupported} from 'react-devtools-shared/src/utils';
1717

18-
let welcomeHasInitialized = false;
19-
20-
function welcome(event: $FlowFixMe) {
21-
if (
22-
event.source !== window ||
23-
event.data.source !== 'react-devtools-content-script'
24-
) {
25-
return;
26-
}
27-
28-
// In some circumstances, this method is called more than once for a single welcome message.
29-
// The exact circumstances of this are unclear, though it seems related to 3rd party event batching code.
30-
//
31-
// Regardless, call this method multiple times can cause DevTools to add duplicate elements to the Store
32-
// (and throw an error) or worse yet, choke up entirely and freeze the browser.
33-
//
34-
// The simplest solution is to ignore the duplicate events.
35-
// To be clear, this SHOULD NOT BE NECESSARY, since we remove the event handler below.
36-
//
37-
// See https://github.com/facebook/react/issues/24162
38-
if (welcomeHasInitialized) {
39-
console.warn(
40-
'React DevTools detected duplicate welcome "message" events from the content script.',
41-
);
42-
return;
43-
}
44-
45-
welcomeHasInitialized = true;
46-
47-
window.removeEventListener('message', welcome);
18+
/*
19+
* Make sure this is executed only once in case Frontend is reloaded multiple times while Backend is initializing
20+
* We can't use `reactDevToolsAgent` field on a global Hook object, because it only cleaned up after both Frontend and Backend initialized
21+
*/
22+
if (!window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__) {
23+
window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__ = true;
24+
25+
let welcomeHasInitialized = false;
26+
function welcome(event: $FlowFixMe) {
27+
if (
28+
event.source !== window ||
29+
event.data.source !== 'react-devtools-content-script'
30+
) {
31+
return;
32+
}
33+
34+
// In some circumstances, this method is called more than once for a single welcome message.
35+
// The exact circumstances of this are unclear, though it seems related to 3rd party event batching code.
36+
//
37+
// Regardless, call this method multiple times can cause DevTools to add duplicate elements to the Store
38+
// (and throw an error) or worse yet, choke up entirely and freeze the browser.
39+
//
40+
// The simplest solution is to ignore the duplicate events.
41+
// To be clear, this SHOULD NOT BE NECESSARY, since we remove the event handler below.
42+
//
43+
// See https://github.com/facebook/react/issues/24162
44+
if (welcomeHasInitialized) {
45+
console.warn(
46+
'React DevTools detected duplicate welcome "message" events from the content script.',
47+
);
48+
return;
49+
}
4850

49-
setup(window.__REACT_DEVTOOLS_GLOBAL_HOOK__);
50-
}
51+
welcomeHasInitialized = true;
5152

52-
window.addEventListener('message', welcome);
53+
window.removeEventListener('message', welcome);
5354

54-
function setup(hook: ?DevToolsHook) {
55-
// this should not happen, but Chrome can be weird sometimes
56-
if (hook == null) {
57-
return;
55+
setup(window.__REACT_DEVTOOLS_GLOBAL_HOOK__);
5856
}
5957

60-
// register renderers that have already injected themselves.
61-
hook.renderers.forEach(renderer => {
62-
registerRenderer(renderer, hook);
63-
});
58+
window.addEventListener('message', welcome);
6459

65-
// Activate and remove from required all present backends, registered within the hook
66-
hook.backends.forEach((_, backendVersion) => {
67-
requiredBackends.delete(backendVersion);
68-
activateBackend(backendVersion, hook);
69-
});
60+
function setup(hook: ?DevToolsHook) {
61+
// this should not happen, but Chrome can be weird sometimes
62+
if (hook == null) {
63+
return;
64+
}
7065

71-
updateRequiredBackends();
66+
// register renderers that have already injected themselves.
67+
hook.renderers.forEach(renderer => {
68+
registerRenderer(renderer, hook);
69+
});
7270

73-
// register renderers that inject themselves later.
74-
hook.sub('renderer', ({renderer}) => {
75-
registerRenderer(renderer, hook);
76-
updateRequiredBackends();
77-
});
71+
// Activate and remove from required all present backends, registered within the hook
72+
hook.backends.forEach((_, backendVersion) => {
73+
requiredBackends.delete(backendVersion);
74+
activateBackend(backendVersion, hook);
75+
});
7876

79-
// listen for backend installations.
80-
hook.sub('devtools-backend-installed', version => {
81-
activateBackend(version, hook);
8277
updateRequiredBackends();
83-
});
84-
}
8578

86-
const requiredBackends = new Set<string>();
79+
// register renderers that inject themselves later.
80+
const unsubscribeRendererListener = hook.sub('renderer', ({renderer}) => {
81+
registerRenderer(renderer, hook);
82+
updateRequiredBackends();
83+
});
84+
85+
// listen for backend installations.
86+
const unsubscribeBackendInstallationListener = hook.sub(
87+
'devtools-backend-installed',
88+
version => {
89+
activateBackend(version, hook);
90+
updateRequiredBackends();
91+
},
92+
);
8793

88-
function registerRenderer(renderer: ReactRenderer, hook: DevToolsHook) {
89-
let version = renderer.reconcilerVersion || renderer.version;
90-
if (!hasAssignedBackend(version)) {
91-
version = COMPACT_VERSION_NAME;
94+
const unsubscribeShutdownListener = hook.sub('shutdown', () => {
95+
unsubscribeRendererListener();
96+
unsubscribeBackendInstallationListener();
97+
unsubscribeShutdownListener();
98+
});
9299
}
93100

94-
// Check if required backend is already activated, no need to require again
95-
if (!hook.backends.has(version)) {
96-
requiredBackends.add(version);
97-
}
98-
}
101+
const requiredBackends = new Set<string>();
99102

100-
function activateBackend(version: string, hook: DevToolsHook) {
101-
const backend = hook.backends.get(version);
102-
if (!backend) {
103-
throw new Error(`Could not find backend for version "${version}"`);
104-
}
103+
function registerRenderer(renderer: ReactRenderer, hook: DevToolsHook) {
104+
let version = renderer.reconcilerVersion || renderer.version;
105+
if (!hasAssignedBackend(version)) {
106+
version = COMPACT_VERSION_NAME;
107+
}
105108

106-
const {Agent, Bridge, initBackend, setupNativeStyleEditor} = backend;
107-
const bridge = new Bridge({
108-
listen(fn) {
109-
const listener = (event: $FlowFixMe) => {
110-
if (
111-
event.source !== window ||
112-
!event.data ||
113-
event.data.source !== 'react-devtools-content-script' ||
114-
!event.data.payload
115-
) {
116-
return;
117-
}
118-
fn(event.data.payload);
119-
};
120-
window.addEventListener('message', listener);
121-
return () => {
122-
window.removeEventListener('message', listener);
123-
};
124-
},
125-
send(event: string, payload: any, transferable?: Array<any>) {
126-
window.postMessage(
127-
{
128-
source: 'react-devtools-bridge',
129-
payload: {event, payload},
130-
},
131-
'*',
132-
transferable,
133-
);
134-
},
135-
});
136-
137-
const agent = new Agent(bridge);
138-
agent.addListener('shutdown', () => {
139-
// If we received 'shutdown' from `agent`, we assume the `bridge` is already shutting down,
140-
// and that caused the 'shutdown' event on the `agent`, so we don't need to call `bridge.shutdown()` here.
141-
hook.emit('shutdown');
142-
});
143-
144-
initBackend(hook, agent, window, getIsReloadAndProfileSupported());
145-
146-
// Setup React Native style editor if a renderer like react-native-web has injected it.
147-
if (typeof setupNativeStyleEditor === 'function' && hook.resolveRNStyle) {
148-
setupNativeStyleEditor(
149-
bridge,
150-
agent,
151-
hook.resolveRNStyle,
152-
hook.nativeStyleEditorValidAttributes,
153-
);
109+
// Check if required backend is already activated, no need to require again
110+
if (!hook.backends.has(version)) {
111+
requiredBackends.add(version);
112+
}
154113
}
155114

156-
// Let the frontend know that the backend has attached listeners and is ready for messages.
157-
// This covers the case of syncing saved values after reloading/navigating while DevTools remain open.
158-
bridge.send('extensionBackendInitialized');
115+
function activateBackend(version: string, hook: DevToolsHook) {
116+
const backend = hook.backends.get(version);
117+
if (!backend) {
118+
throw new Error(`Could not find backend for version "${version}"`);
119+
}
120+
121+
const {Agent, Bridge, initBackend, setupNativeStyleEditor} = backend;
122+
const bridge = new Bridge({
123+
listen(fn) {
124+
const listener = (event: $FlowFixMe) => {
125+
if (
126+
event.source !== window ||
127+
!event.data ||
128+
event.data.source !== 'react-devtools-content-script' ||
129+
!event.data.payload
130+
) {
131+
return;
132+
}
133+
fn(event.data.payload);
134+
};
135+
window.addEventListener('message', listener);
136+
return () => {
137+
window.removeEventListener('message', listener);
138+
};
139+
},
140+
send(event: string, payload: any, transferable?: Array<any>) {
141+
window.postMessage(
142+
{
143+
source: 'react-devtools-bridge',
144+
payload: {event, payload},
145+
},
146+
'*',
147+
transferable,
148+
);
149+
},
150+
});
151+
152+
const agent = new Agent(bridge);
153+
agent.addListener('shutdown', () => {
154+
// If we received 'shutdown' from `agent`, we assume the `bridge` is already shutting down,
155+
// and that caused the 'shutdown' event on the `agent`, so we don't need to call `bridge.shutdown()` here.
156+
hook.emit('shutdown');
157+
delete window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__;
158+
});
159+
160+
initBackend(hook, agent, window, getIsReloadAndProfileSupported());
161+
162+
// Setup React Native style editor if a renderer like react-native-web has injected it.
163+
if (typeof setupNativeStyleEditor === 'function' && hook.resolveRNStyle) {
164+
setupNativeStyleEditor(
165+
bridge,
166+
agent,
167+
hook.resolveRNStyle,
168+
hook.nativeStyleEditorValidAttributes,
169+
);
170+
}
159171

160-
// this backend is activated
161-
requiredBackends.delete(version);
162-
}
172+
// Let the frontend know that the backend has attached listeners and is ready for messages.
173+
// This covers the case of syncing saved values after reloading/navigating while DevTools remain open.
174+
bridge.send('extensionBackendInitialized');
163175

164-
// tell the service worker which versions of backends are needed for the current page
165-
function updateRequiredBackends() {
166-
if (requiredBackends.size === 0) {
167-
return;
176+
// this backend is activated
177+
requiredBackends.delete(version);
168178
}
169179

170-
window.postMessage(
171-
{
172-
source: 'react-devtools-backend-manager',
173-
payload: {
174-
type: 'require-backends',
175-
versions: Array.from(requiredBackends),
180+
// tell the service worker which versions of backends are needed for the current page
181+
function updateRequiredBackends() {
182+
if (requiredBackends.size === 0) {
183+
return;
184+
}
185+
186+
window.postMessage(
187+
{
188+
source: 'react-devtools-backend-manager',
189+
payload: {
190+
type: 'require-backends',
191+
versions: Array.from(requiredBackends),
192+
},
176193
},
177-
},
178-
'*',
179-
);
194+
'*',
195+
);
196+
}
180197
}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -754,6 +754,9 @@ export default class Agent extends EventEmitter<{
754754
shutdown: () => void = () => {
755755
// Clean up the overlay if visible, and associated events.
756756
this.emit('shutdown');
757+
758+
this._bridge.removeAllListeners();
759+
this.removeAllListeners();
757760
};
758761

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

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3472,7 +3472,7 @@ export function attach(
34723472
}
34733473

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

34783478
function rootSupportsProfiling(root: any) {

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

Lines changed: 2 additions & 5 deletions
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)
⚡