Fix: handle Firefox event loop bug causing 'Should not already be working' error#703
Open
Fix: handle Firefox event loop bug causing 'Should not already be working' error#703
Conversation
…king' error This fix addresses a long-standing issue where Firefox's event loop behavior causes React to throw "Should not already be working" errors when using alert()/debugger breakpoints in componentDidMount. Root cause: Firefox does not block MessageChannel events during alert()/debugger calls, causing React's scheduler to re-enter performWorkOnRoot while already in a render context. Solution: Instead of throwing an error when re-entry is detected, schedule the work to run on the next event loop iteration using Scheduler_scheduleCallback. Fixes #17355
Greptile SummaryThis PR attempts to fix a Firefox-specific bug where Key issues found:
Confidence Score: 1/5
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberWorkLoop.js
Line: 3522-3524
Comment:
**`forceSync` is not in scope — will throw `ReferenceError` at runtime**
`forceSync` is a parameter of `performWorkOnRoot`, not of `completeRoot`. Referencing it inside this closure in `completeRoot` will cause a `ReferenceError: forceSync is not defined` whenever this code path is triggered. This appears to be a direct copy-paste from the `performWorkOnRoot` change without adapting the argument list.
`completeRoot` does not take a `forceSync` parameter, so there is no obvious single value to pass here. The caller of `completeRoot` is what knows whether the work is forced-sync. One option is to thread `forceSync` down as an additional parameter to `completeRoot`, or alternatively to always pass `false` (concurrent) and accept a possible downgrade in urgency. Either way, the current code cannot execute correctly.
```suggestion
Scheduler_scheduleCallback(NormalSchedulerPriority, () => {
performWorkOnRoot(root, lanes, false);
});
```
_(Note: passing `false` is a conservative placeholder — the correct value of `forceSync` needs to be determined from the call-site context.)_
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberWorkLoop.js
Line: 1122-1129
Comment:
**Potential infinite re-scheduling loop**
If the `executionContext` is stuck with `RenderContext | CommitContext` set (e.g., due to a genuine bug rather than the Firefox MessageChannel race), this callback will reschedule itself on every tick indefinitely. The original `throw` was a hard stop that surfaced this invariant violation immediately; the new approach silently swallows it and spins forever.
Consider adding a retry counter or a flag to bail out after a reasonable number of retries:
```
let reentryRetries = 0;
const MAX_REENTRY_RETRIES = 3;
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
if (reentryRetries++ < MAX_REENTRY_RETRIES) {
Scheduler_scheduleCallback(NormalSchedulerPriority, () => {
reentryRetries = 0;
performWorkOnRoot(root, lanes, forceSync);
});
} else {
reentryRetries = 0;
throw new Error('Should not already be working.');
}
return;
}
```
This preserves the Firefox workaround for genuine scheduler race conditions while still failing loudly if the re-entry is a real bug. The same concern applies to the corresponding change in `completeRoot` (line 3519).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberWorkLoop.js
Line: 1125-1127
Comment:
**Original work priority is silently downgraded to `NormalSchedulerPriority`**
`performWorkOnRoot` is called with whatever lane priorities the scheduler originally assigned (potentially `SyncLane`, `InputContinuousLane`, etc.), but this rescheduled retry always uses `NormalSchedulerPriority`. If the work was synchronous (e.g., `forceSync === true` or includes a blocking lane), running it at normal priority after the current work finishes could violate React's priority ordering guarantees — for example, a user-initiated synchronous update could be deferred behind lower-priority idle work that is already queued.
The same concern applies to the `completeRoot` reschedule at line 3522.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: b28909b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+3522
to
+3524
| Scheduler_scheduleCallback(NormalSchedulerPriority, () => { | ||
| performWorkOnRoot(root, lanes, forceSync); | ||
| }); |
There was a problem hiding this comment.
forceSync is not in scope — will throw ReferenceError at runtime
forceSync is a parameter of performWorkOnRoot, not of completeRoot. Referencing it inside this closure in completeRoot will cause a ReferenceError: forceSync is not defined whenever this code path is triggered. This appears to be a direct copy-paste from the performWorkOnRoot change without adapting the argument list.
completeRoot does not take a forceSync parameter, so there is no obvious single value to pass here. The caller of completeRoot is what knows whether the work is forced-sync. One option is to thread forceSync down as an additional parameter to completeRoot, or alternatively to always pass false (concurrent) and accept a possible downgrade in urgency. Either way, the current code cannot execute correctly.
Suggested change
| Scheduler_scheduleCallback(NormalSchedulerPriority, () => { | |
| performWorkOnRoot(root, lanes, forceSync); | |
| }); | |
| Scheduler_scheduleCallback(NormalSchedulerPriority, () => { | |
| performWorkOnRoot(root, lanes, false); | |
| }); |
(Note: passing false is a conservative placeholder — the correct value of forceSync needs to be determined from the call-site context.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberWorkLoop.js
Line: 3522-3524
Comment:
**`forceSync` is not in scope — will throw `ReferenceError` at runtime**
`forceSync` is a parameter of `performWorkOnRoot`, not of `completeRoot`. Referencing it inside this closure in `completeRoot` will cause a `ReferenceError: forceSync is not defined` whenever this code path is triggered. This appears to be a direct copy-paste from the `performWorkOnRoot` change without adapting the argument list.
`completeRoot` does not take a `forceSync` parameter, so there is no obvious single value to pass here. The caller of `completeRoot` is what knows whether the work is forced-sync. One option is to thread `forceSync` down as an additional parameter to `completeRoot`, or alternatively to always pass `false` (concurrent) and accept a possible downgrade in urgency. Either way, the current code cannot execute correctly.
```suggestion
Scheduler_scheduleCallback(NormalSchedulerPriority, () => {
performWorkOnRoot(root, lanes, false);
});
```
_(Note: passing `false` is a conservative placeholder — the correct value of `forceSync` needs to be determined from the call-site context.)_
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
1122
to
1129
| if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { | ||
| throw new Error('Should not already be working.'); | ||
| // Fix for Firefox event loop bug where alert/debugger don't block MessageChannel events | ||
| // Schedule the work to run on the next event loop iteration instead of throwing | ||
| Scheduler_scheduleCallback(NormalSchedulerPriority, () => { | ||
| performWorkOnRoot(root, lanes, forceSync); | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Potential infinite re-scheduling loop
If the executionContext is stuck with RenderContext | CommitContext set (e.g., due to a genuine bug rather than the Firefox MessageChannel race), this callback will reschedule itself on every tick indefinitely. The original throw was a hard stop that surfaced this invariant violation immediately; the new approach silently swallows it and spins forever.
Consider adding a retry counter or a flag to bail out after a reasonable number of retries:
let reentryRetries = 0;
const MAX_REENTRY_RETRIES = 3;
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
if (reentryRetries++ < MAX_REENTRY_RETRIES) {
Scheduler_scheduleCallback(NormalSchedulerPriority, () => {
reentryRetries = 0;
performWorkOnRoot(root, lanes, forceSync);
});
} else {
reentryRetries = 0;
throw new Error('Should not already be working.');
}
return;
}
This preserves the Firefox workaround for genuine scheduler race conditions while still failing loudly if the re-entry is a real bug. The same concern applies to the corresponding change in completeRoot (line 3519).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberWorkLoop.js
Line: 1122-1129
Comment:
**Potential infinite re-scheduling loop**
If the `executionContext` is stuck with `RenderContext | CommitContext` set (e.g., due to a genuine bug rather than the Firefox MessageChannel race), this callback will reschedule itself on every tick indefinitely. The original `throw` was a hard stop that surfaced this invariant violation immediately; the new approach silently swallows it and spins forever.
Consider adding a retry counter or a flag to bail out after a reasonable number of retries:
```
let reentryRetries = 0;
const MAX_REENTRY_RETRIES = 3;
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
if (reentryRetries++ < MAX_REENTRY_RETRIES) {
Scheduler_scheduleCallback(NormalSchedulerPriority, () => {
reentryRetries = 0;
performWorkOnRoot(root, lanes, forceSync);
});
} else {
reentryRetries = 0;
throw new Error('Should not already be working.');
}
return;
}
```
This preserves the Firefox workaround for genuine scheduler race conditions while still failing loudly if the re-entry is a real bug. The same concern applies to the corresponding change in `completeRoot` (line 3519).
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1125
to
+1127
| Scheduler_scheduleCallback(NormalSchedulerPriority, () => { | ||
| performWorkOnRoot(root, lanes, forceSync); | ||
| }); |
There was a problem hiding this comment.
Original work priority is silently downgraded to NormalSchedulerPriority
performWorkOnRoot is called with whatever lane priorities the scheduler originally assigned (potentially SyncLane, InputContinuousLane, etc.), but this rescheduled retry always uses NormalSchedulerPriority. If the work was synchronous (e.g., forceSync === true or includes a blocking lane), running it at normal priority after the current work finishes could violate React's priority ordering guarantees — for example, a user-initiated synchronous update could be deferred behind lower-priority idle work that is already queued.
The same concern applies to the completeRoot reschedule at line 3522.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberWorkLoop.js
Line: 1125-1127
Comment:
**Original work priority is silently downgraded to `NormalSchedulerPriority`**
`performWorkOnRoot` is called with whatever lane priorities the scheduler originally assigned (potentially `SyncLane`, `InputContinuousLane`, etc.), but this rescheduled retry always uses `NormalSchedulerPriority`. If the work was synchronous (e.g., `forceSync === true` or includes a blocking lane), running it at normal priority after the current work finishes could violate React's priority ordering guarantees — for example, a user-initiated synchronous update could be deferred behind lower-priority idle work that is already queued.
The same concern applies to the `completeRoot` reschedule at line 3522.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mirror of facebook/react#36027
Original author: MorikawaSouma
Fixes #17355
Problem
React throws 'Should not already be working' error in Firefox when using alert()/debugger breakpoints in componentDidMount after setState.
Root Cause
Firefox does not block MessageChannel events during alert()/debugger calls, causing React's scheduler to re-enter performWorkOnRoot while already in a render context.
Solution
Instead of throwing an error when re-entry is detected, schedule the work to run on the next event loop iteration using Scheduler_scheduleCallback.
Testing
Fixes #17355