Fix stale executionContext after breakpoint/alert interruption (#17355)#585
Fix stale executionContext after breakpoint/alert interruption (#17355)#585everettbu wants to merge 3 commits into
Conversation
When execution is interrupted by a breakpoint, alert(), or browser debugging pause (particularly in Firefox), the finally blocks that normally reset executionContext may not run. This leaves RenderContext or CommitContext flags set even though workInProgress and workInProgressRoot are both null. On resume, performWorkOnRoot and completeRoot see the stale flags and throw "Should not already be working." Add fixStaleExecutionContext() which detects this mismatch and clears the stale render/commit flags before the assertion. Called at the top of performWorkOnRoot and after flushPendingEffects in completeRoot.
Greptile SummaryThis PR adds a defensive
Confidence Score: 2/5
Important Files Changed
Last reviewed commit: 704dff1 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function fixStaleExecutionContext(): void { | ||
| if ( | ||
| (executionContext & (RenderContext | CommitContext)) !== NoContext && | ||
| workInProgress === null && | ||
| workInProgressRoot === null | ||
| ) { | ||
| executionContext &= ~(RenderContext | CommitContext); | ||
| } | ||
| } |
There was a problem hiding this 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:
| function fixStaleExecutionContext(): void { | |
| if ( | |
| (executionContext & (RenderContext | CommitContext)) !== NoContext && | |
| workInProgress === null && | |
| workInProgressRoot === null | |
| ) { | |
| executionContext &= ~(RenderContext | CommitContext); | |
| } | |
| } | |
| 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); | |
| } | |
| } |
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 AI
This 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.When execution is interrupted by a breakpoint, alert(), or browser debugging pause (particularly in Firefox), the finally blocks that normally reset executionContext may not run. This leaves RenderContext or CommitContext flags set even though workInProgress and workInProgressRoot are both null. On resume, performWorkOnRoot and completeRoot see the stale flags and throw "Should not already be working." Add fixStaleExecutionContext() which detects this mismatch and clears the stale render/commit flags before the assertion. Called at the top of performWorkOnRoot and after flushPendingEffects in completeRoot.
c116fb9 to
214288d
Compare
| // When execution is interrupted by a breakpoint, alert(), or a browser | ||
| // debugging pause, the finally blocks that reset executionContext may not run. |
There was a problem hiding this comment.
Misleading comment about finally blocks
The comment says "the finally blocks that reset executionContext may not run," but renderRootSync (line 2736) and renderRootConcurrent (line 3019) restore executionContext via direct assignment outside any finally block. The RenderContext flag restoration is done linearly after the work loop, not in a finally. This means the stale context scenario for RenderContext isn't about finally blocks failing — it's about execution being interrupted before the cleanup line is reached.
Only the CommitContext restorations (e.g., lines 3870, 4034) actually use try/finally. Consider updating the comment to accurately describe the mechanism:
| // When execution is interrupted by a breakpoint, alert(), or a browser | |
| // debugging pause, the finally blocks that reset executionContext may not run. | |
| // When execution is interrupted by a breakpoint, alert(), or a browser | |
| // debugging pause, the code that restores executionContext after rendering | |
| // (which is not in a finally block) may not have run yet. When React | |
| // resumes, executionContext still has RenderContext or CommitContext |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberWorkLoop.js
Line: 1117-1118
Comment:
**Misleading comment about `finally` blocks**
The comment says "the finally blocks that reset executionContext may not run," but `renderRootSync` (line 2736) and `renderRootConcurrent` (line 3019) restore `executionContext` via direct assignment **outside** any `finally` block. The `RenderContext` flag restoration is done linearly after the work loop, not in a `finally`. This means the stale context scenario for `RenderContext` isn't about `finally` blocks failing — it's about execution being interrupted before the cleanup line is reached.
Only the `CommitContext` restorations (e.g., lines 3870, 4034) actually use `try`/`finally`. Consider updating the comment to accurately describe the mechanism:
```suggestion
// When execution is interrupted by a breakpoint, alert(), or a browser
// debugging pause, the code that restores executionContext after rendering
// (which is not in a finally block) may not have run yet. When React
// resumes, executionContext still has RenderContext or CommitContext
```
How can I resolve this? If you propose a fix, please make it concise.…m/gpardhivvarma/react into fix/stale-execution-context-17355
| lanes: Lanes, | ||
| forceSync: boolean, | ||
| ): void { | ||
| fixStaleExecutionContext(); |
There was a problem hiding this comment.
Fix doesn't cover the flushPassiveEffectsImpl assertion
The stale context scenario can still crash before this fix runs. Both performConcurrentWorkOnRoot (ReactFiberRootScheduler.js:546) and performSyncWorkOnRoot (ReactFiberRootScheduler.js:611) call flushPendingEffects() before calling performWorkOnRoot(). That call chain reaches flushPassiveEffectsImpl() (line 4723), which has its own assertion:
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).
Prompt To Fix With AI
This 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.2 participants
Mirror of facebook/react#35865
Original author: gpardhivvarma
Summary
Fixes #17355.
When execution is interrupted by a breakpoint,
alert(), or a browser debugging pause (particularly in Firefox), thefinallyblocks that normally resetexecutionContextmay not run. This leavesRenderContextorCommitContextflags set even thoughworkInProgressandworkInProgressRootare both null.On resume,
performWorkOnRootandcompleteRootsee the stale flags and throw "Should not already be working."This adds
fixStaleExecutionContext()which detects this mismatch — render/commit flags are set but nothing is actually in progress — and clears just those flags before the assertion. It's called at the top ofperformWorkOnRootand afterflushPendingEffectsincompleteRoot, the two places that guard with this check.The fix is defensive: it only activates when the context is genuinely stale, so normal execution paths are unaffected.
How did you test this change?
yarn prettier— no formatting issuesyarn linc— lint passes on changed filesyarn flow dom-browser— no type errorsyarn test packages/react-reconciler/src/__tests__/— all 76 suites pass (1132 tests)This edge case is inherently difficult to unit-test because it requires simulating an external interruption (breakpoint/alert) mid-render. The fix only runs when executionContext is clearly inconsistent with the actual work-in-progress state, and the existing test suite confirms it doesn't change behavior under normal conditions.