-
Notifications
You must be signed in to change notification settings - Fork 0
Fix stale executionContext after breakpoint/alert interruption (#17355) #585
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
base: main
Are you sure you want to change the base?
Changes from all commits
c116fb9
214288d
704dff1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1114,11 +1114,29 @@ export function isUnsafeClassRenderPhaseUpdate(fiber: Fiber): boolean { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| return (executionContext & RenderContext) !== NoContext; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // When execution is interrupted by a breakpoint, alert(), or a browser | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // debugging pause, the finally blocks that reset executionContext may not run. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // When React resumes, executionContext still has RenderContext or CommitContext | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // set even though we're not actually in the middle of work. Detect this by | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // checking whether workInProgress/workInProgressRoot are null and clear the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // stale flags so we don't throw "Should not already be working." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function fixStaleExecutionContext(): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (executionContext & (RenderContext | CommitContext)) !== NoContext && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| workInProgress === null && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| workInProgressRoot === null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| executionContext &= ~(RenderContext | CommitContext); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1123
to
+1131
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silent state reset could mask real bugs This function silently clears stale Consider adding a
Suggested change
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberWorkLoop.js
Line: 1123-1131
Comment:
**Silent state reset could mask real bugs**
This function silently clears stale `RenderContext`/`CommitContext` flags without any indication that it activated. If there were ever a real bug in React's state management that left `executionContext` inconsistent (not caused by a debugger interruption), this fix would hide it.
Consider adding a `__DEV__`-mode warning when the stale context is detected and cleared, so that internal developers can distinguish between a genuine debugger interruption and an actual bug:
```suggestion
function fixStaleExecutionContext(): void {
if (
(executionContext & (RenderContext | CommitContext)) !== NoContext &&
workInProgress === null &&
workInProgressRoot === null
) {
if (__DEV__) {
console.warn(
'Detected stale executionContext (%s) with no work in progress. ' +
'This is likely caused by a debugger breakpoint or alert() interrupting ' +
'a React render. Clearing stale flags.',
executionContext,
);
}
executionContext &= ~(RenderContext | CommitContext);
}
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function performWorkOnRoot( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| root: FiberRoot, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lanes: Lanes, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| forceSync: boolean, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fixStaleExecutionContext(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix doesn't cover the The stale context scenario can still crash before this fix runs. Both if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
throw new Error('Cannot flush passive effects while already rendering.');
}If Consider also guarding the assertion in Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberWorkLoop.js
Line: 1138
Comment:
**Fix doesn't cover the `flushPassiveEffectsImpl` assertion**
The stale context scenario can still crash before this fix runs. Both `performConcurrentWorkOnRoot` ([ReactFiberRootScheduler.js:546](packages/react-reconciler/src/ReactFiberRootScheduler.js)) and `performSyncWorkOnRoot` ([ReactFiberRootScheduler.js:611](packages/react-reconciler/src/ReactFiberRootScheduler.js)) call `flushPendingEffects()` **before** calling `performWorkOnRoot()`. That call chain reaches `flushPassiveEffectsImpl()` (line 4723), which has its own assertion:
```javascript
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
throw new Error('Cannot flush passive effects while already rendering.');
}
```
If `executionContext` is stale and there are pending passive effects, this assertion fires before `fixStaleExecutionContext()` at line 1138 ever executes. Similarly, the fix at line 3532 in `completeRoot` is placed **after** the `do { flushPendingEffects() } while (...)` loop, so the same `flushPassiveEffectsImpl` assertion would fire first there too.
Consider also guarding the assertion in `flushPassiveEffectsImpl` (line 4723), or moving the fix to a shared entry point earlier in the call chain (e.g., inside `flushPendingEffects` itself).
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error('Should not already be working.'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -3511,6 +3529,8 @@ function completeRoot( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| } while (pendingEffectsStatus !== NO_PENDING_EFFECTS); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flushRenderPhaseStrictModeWarningsInDEV(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fixStaleExecutionContext(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error('Should not already be working.'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading comment about
finallyblocksThe comment says "the finally blocks that reset executionContext may not run," but
renderRootSync(line 2736) andrenderRootConcurrent(line 3019) restoreexecutionContextvia direct assignment outside anyfinallyblock. TheRenderContextflag restoration is done linearly after the work loop, not in afinally. This means the stale context scenario forRenderContextisn't aboutfinallyblocks failing — it's about execution being interrupted before the cleanup line is reached.Only the
CommitContextrestorations (e.g., lines 3870, 4034) actually usetry/finally. Consider updating the comment to accurately describe the mechanism:Prompt To Fix With AI