@samitouri / QOS-React / commits / 1d8d12005f

fix[react-devtools]: remove all listeners when Agent is shutdown (#31151)

Based on https://github.com/facebook/react/pull/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.

Ruslan Lesiutin committed Oct 9, 2024 at 13:34 UTC 1d8d12005fc9d856c4c936b269adb4f52bf82e47

4 files changed +31 -14

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

+25 -8

@@ -16,6 +16,7 @@ import {COMPACT_VERSION_NAME} from 'react-devtools-extensions/src/utils';

16 import {getIsReloadAndProfileSupported} from 'react-devtools-shared/src/utils';

18 let welcomeHasInitialized = false;

19 +const requiredBackends = new Set<string>();

21 function welcome(event: $FlowFixMe) {

22 if (

@@ -49,8 +50,6 @@ function welcome(event: $FlowFixMe) {

50 setup(window.__REACT_DEVTOOLS_GLOBAL_HOOK__);

51 }

52 -window.addEventListener('message', welcome);

53 -

53 function setup(hook: ?DevToolsHook) {

54 // this should not happen, but Chrome can be weird sometimes

55 if (hook == null) {

@@ -71,20 +70,27 @@ function setup(hook: ?DevToolsHook) {

70 updateRequiredBackends();

72 // register renderers that inject themselves later.

74 - hook.sub('renderer', ({renderer}) => {

73 + const unsubscribeRendererListener = hook.sub('renderer', ({renderer}) => {

74 registerRenderer(renderer, hook);

75 updateRequiredBackends();

76 });

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

91 });

92 }

86 -const requiredBackends = new Set<string>();

87 -

94 function registerRenderer(renderer: ReactRenderer, hook: DevToolsHook) {

95 let version = renderer.reconcilerVersion || renderer.version;

96 if (!hasAssignedBackend(version)) {

@@ -139,6 +145,7 @@ function activateBackend(version: string, hook: DevToolsHook) {

145 // If we received 'shutdown' from `agent`, we assume the `bridge` is already shutting down,

146 // and that caused the 'shutdown' event on the `agent`, so we don't need to call `bridge.shutdown()` here.

147 hook.emit('shutdown');

148 + delete window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__;

149 });

150

151 initBackend(hook, agent, window, getIsReloadAndProfileSupported());

@@ -178,3 +185,13 @@ function updateRequiredBackends() {

185 '*',

186 );

187 }

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

@@ -750,6 +750,9 @@ export default class Agent extends EventEmitter<{

750 shutdown: () => void = () => {

751 // Clean up the overlay if visible, and associated events.

752 this.emit('shutdown');

753 +

754 + this._bridge.removeAllListeners();

755 + this.removeAllListeners();

756 };

757

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

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

+1 -1

@@ -3473,7 +3473,7 @@ export function attach(

3473 }

3474

3475 function cleanup() {

3476 - // We don't patch any methods so there is no cleanup.

3476 + isProfiling = false;

3477 }

3478

3479 function rootSupportsProfiling(root: any) {

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

+2 -5

@@ -80,15 +80,12 @@ export function initBackend(

80 });

81 hook.reactDevtoolsAgent = null;

82 };

83 - agent.addListener('shutdown', onAgentShutdown);

84 - subs.push(() => {

85 - agent.removeListener('shutdown', onAgentShutdown);

86 - });

84 + // Agent's event listeners are cleaned up by Agent in `shutdown` implementation.

85 + agent.addListener('shutdown', onAgentShutdown);

86 agent.addListener('updateHookSettings', settings => {

87 hook.settings = settings;

88 });

91 -

89 agent.addListener('getHookSettings', () => {

90 if (hook.settings != null) {

91 agent.onHookSettings(hook.settings);