Fix: profiling crashes #30661 #28838

[EdmondChuiHW]

Summary

Profiling fails sometimes because onProfilingStatus is called repeatedly on some occasions, e.g. multiple calls to getProfilingStatus.

Subsequent calls should be a no-op if the profiling status hasn't changed.

Reported via #30661 #28838.

Tip

Hide whitespace changes on this PR

How did you test this change?

Tested as part of Fusebox implementation of reload-to-profile.

#31021

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 23, 2024 0:16am

[hoxyq]

onProfilingStatus is called repeatedly on some occasions, e.g. multiple calls to getProfilingStatus.

This looks like some race condition, because for the browser extension it is guaranteed that Store (and consequently ProfilerStore) are initialized before Agent is created. See #30945. This means that this getProfilingStatus should not be intercepted by Agent, because it doesn't exist yet:

react/packages/react-devtools-shared/src/devtools/ProfilerStore.js

Lines 97 to 99 in e7d213d

	// It's possible that profiling has already started (e.g. "reload and start profiling")
	// so the frontend needs to ask the backend for its status after mounting.
	bridge.send('getProfilingStatus');

Would be nice to understand what causes this race condition, this is probably a symptom of another issue, like not unsubscribing correctly some event listeners, when Backend or Frontend are reloaded.

[EdmondChuiHW]

@hoxyq

I think there are legit cases where it's called multiple times (separately from store and agent itself), e.g.

react/packages/react-devtools-shared/src/devtools/ProfilerStore.js

Lines 97 to 99 in 4708fb9

	// It's possible that profiling has already started (e.g. "reload and start profiling")
	// so the frontend needs to ask the backend for its status after mounting.
	bridge.send('getProfilingStatus');

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

Lines 242 to 244 in 4708fb9

	if (this._isProfiling) {
	bridge.send('profilingStatus', true);
	}

[hoxyq]

@hoxyq

I think there are legit cases where it's called multiple times (separately from store and agent itself), e.g.

react/packages/react-devtools-shared/src/devtools/ProfilerStore.js

Lines 97 to 99 in 4708fb9

	// It's possible that profiling has already started (e.g. "reload and start profiling")
	// so the frontend needs to ask the backend for its status after mounting.
	bridge.send('getProfilingStatus');

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

Lines 242 to 244 in 4708fb9

	if (this._isProfiling) {
	bridge.send('profilingStatus', true);
	}

ProfilerStore is created only from Store, and this event should not be intercepted, because Agent was not created yet, I've mentioned this in my comment.

The average lifecycle looks like this:
... -> new Store() -> ... -> new ProfilerStore() -> ... -> new Agent()

As per profilingStatus event from Agent - this is fine. We only send it from a constructor, if profiling has started, so it should be sent once.

After some thinking about it, this may be happening at a reload, when Frontend Frame (Chrome DevTools panel) is not reloaded, so the Store is still there, but the Backend Frame (Application Frame in a Tab) is actually reloaded, so we end up in some race condition between them - depending on what initializes first. This is the same as what I've mentioned before - we probably don't unsubscribe event listeners correctly, and end up emitting profilingStatus twice.

[EdmondChuiHW]

The average lifecycle looks like this:
... -> new Store() -> ... -> new ProfilerStore() -> ... -> new Agent()

@hoxyq thanks for breaking it down! Makes more sense now.

we probably don't unsubscribe event listeners correctly, and end up emitting profilingStatus twice.

It looks like we don't listen to the "shutdown" event from the bridge in Agent. ProfilerStore and Store both remove listeners upon "shutdown".

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

Line 206 in 4708fb9

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

I've added a draft PR to remove listeners upon shutdown: #31049. Let me know if this is the race condition you're thinking of