Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {COMPACT_VERSION_NAME} from 'react-devtools-extensions/src/utils';
import {getIsReloadAndProfileSupported} from 'react-devtools-shared/src/utils';

let welcomeHasInitialized = false;
const requiredBackends = new Set<string>();

function welcome(event: $FlowFixMe) {
if (
Expand Down Expand Up @@ -49,8 +50,6 @@ function welcome(event: $FlowFixMe) {
setup(window.__REACT_DEVTOOLS_GLOBAL_HOOK__);
}

window.addEventListener('message', welcome);

function setup(hook: ?DevToolsHook) {
// this should not happen, but Chrome can be weird sometimes
if (hook == null) {
Expand All @@ -71,20 +70,27 @@ function setup(hook: ?DevToolsHook) {
updateRequiredBackends();

// register renderers that inject themselves later.
hook.sub('renderer', ({renderer}) => {
const unsubscribeRendererListener = hook.sub('renderer', ({renderer}) => {
registerRenderer(renderer, hook);
updateRequiredBackends();
});

// listen for backend installations.
hook.sub('devtools-backend-installed', version => {
activateBackend(version, hook);
updateRequiredBackends();
const unsubscribeBackendInstallationListener = hook.sub(
'devtools-backend-installed',
version => {
activateBackend(version, hook);
updateRequiredBackends();
},
);

const unsubscribeShutdownListener: () => void = hook.sub('shutdown', () => {
unsubscribeRendererListener();
unsubscribeBackendInstallationListener();
unsubscribeShutdownListener();
});
}

const requiredBackends = new Set<string>();

function registerRenderer(renderer: ReactRenderer, hook: DevToolsHook) {
let version = renderer.reconcilerVersion || renderer.version;
if (!hasAssignedBackend(version)) {
Expand Down Expand Up @@ -139,6 +145,7 @@ function activateBackend(version: string, hook: DevToolsHook) {
// If we received 'shutdown' from `agent`, we assume the `bridge` is already shutting down,
// and that caused the 'shutdown' event on the `agent`, so we don't need to call `bridge.shutdown()` here.
hook.emit('shutdown');
delete window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__;
});

initBackend(hook, agent, window, getIsReloadAndProfileSupported());
Expand Down Expand Up @@ -178,3 +185,13 @@ function updateRequiredBackends() {
'*',
);
}

/*
* Make sure this is executed only once in case Frontend is reloaded multiple times while Backend is initializing
* We can't use `reactDevToolsAgent` field on a global Hook object, because it only cleaned up after both Frontend and Backend initialized
*/
if (!window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__) {
window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__ = true;

window.addEventListener('message', welcome);
}
3 changes: 3 additions & 0 deletions packages/react-devtools-shared/src/backend/agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,9 @@ export default class Agent extends EventEmitter<{
shutdown: () => void = () => {
// Clean up the overlay if visible, and associated events.
this.emit('shutdown');

this._bridge.removeAllListeners();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe? What happens if other objects are listening to the shutdown event too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is safe, because Agent is actually notified by Bridge here:

bridge.addListener('shutdown', this.shutdown);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked irl. What I was asking was

  1. ProfilerStore subscribes to bridge
  2. Agent subscribes to bridge
  3. Bridge emits shutdown event
  4. Agent remove all listeners here
  5. Would ProfileStore get the shutdown event?

We found that the current impl does account for this (removing all listeners won't prevent existing listeners from getting the currently-processed event):

const clonedListeners = Array.from(listeners);

We should prob still be a good neighbour and only unsubscribe our own listeners tho

this.removeAllListeners();
};

startProfiling: (recordChangeDescriptions: boolean) => void =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3472,7 +3472,7 @@ export function attach(
}

function cleanup() {
// We don't patch any methods so there is no cleanup.
isProfiling = false;
}

function rootSupportsProfiling(root: any) {
Expand Down
7 changes: 2 additions & 5 deletions packages/react-devtools-shared/src/backend/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,12 @@ export function initBackend(
});
hook.reactDevtoolsAgent = null;
};
agent.addListener('shutdown', onAgentShutdown);
subs.push(() => {
agent.removeListener('shutdown', onAgentShutdown);
});

// Agent's event listeners are cleaned up by Agent in `shutdown` implementation.
agent.addListener('shutdown', onAgentShutdown);
agent.addListener('updateHookSettings', settings => {
hook.settings = settings;
});

agent.addListener('getHookSettings', () => {
if (hook.settings != null) {
agent.onHookSettings(hook.settings);
Expand Down
Loading