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

Recover from errors with a boundary in completion phase #14104

Merged
merged 7 commits into from
Nov 6, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 5, 2018

Fixes #13820.

Dunno if this is an "idiomatic" fix. The issue is that we null out nextUnitOfWork as we traverse up during completion, but we also don't attempt to find an error boundary if nextUnitOfWork is null:

if (nextUnitOfWork === null) {
// This is a fatal error.
didFatal = true;
onUncaughtError(thrownValue);
} else {

So errors in completion phase don't look for boundaries at all right now.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 5, 2018

Hmm. I was too confident. This causes some infinite loops.

@sizebot
Copy link

sizebot commented Nov 5, 2018

ReactDOM: size: 0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: b305c4e...22158a5

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 704.94 KB 705.83 KB 163.39 KB 163.56 KB UMD_DEV
react-dom.production.min.js 0.0% 0.0% 102.68 KB 102.69 KB 33.58 KB 33.59 KB UMD_PROD
react-dom.development.js +0.1% +0.1% 700.25 KB 701.14 KB 162.01 KB 162.2 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 102.77 KB 102.78 KB 33.12 KB 33.13 KB NODE_PROD
ReactDOM-dev.js +0.1% +0.1% 718.73 KB 719.62 KB 162.87 KB 163.06 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 0.0% 310.78 KB 310.82 KB 57.4 KB 57.41 KB FB_WWW_PROD
react-dom.profiling.min.js 0.0% 0.0% 105.13 KB 105.14 KB 33.43 KB 33.43 KB NODE_PROFILING
ReactDOM-profiling.js 0.0% 0.0% 315.06 KB 315.1 KB 58.29 KB 58.29 KB FB_WWW_PROFILING
react-dom.profiling.min.js 0.0% 0.0% 105 KB 105.01 KB 33.93 KB 33.93 KB UMD_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% +0.1% 491.55 KB 492.43 KB 108.95 KB 109.11 KB UMD_DEV
react-art.production.min.js 0.0% 0.0% 94.46 KB 94.47 KB 29.16 KB 29.17 KB UMD_PROD
react-art.development.js +0.2% +0.2% 423.34 KB 424.22 KB 91.92 KB 92.09 KB NODE_DEV
react-art.production.min.js 0.0% 0.0% 59.5 KB 59.51 KB 18.49 KB 18.49 KB NODE_PROD
ReactART-dev.js +0.2% +0.2% 428.16 KB 429.04 KB 90.48 KB 90.66 KB FB_WWW_DEV
ReactART-prod.js 0.0% 0.0% 183.64 KB 183.68 KB 31.51 KB 31.51 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.2% +0.2% 435.98 KB 436.87 KB 94.59 KB 94.77 KB UMD_DEV
react-test-renderer.production.min.js 0.0% 🔺+0.1% 60.69 KB 60.7 KB 18.87 KB 18.88 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.2% 431.2 KB 432.08 KB 93.44 KB 93.62 KB NODE_DEV
react-test-renderer.production.min.js 0.0% 0.0% 60.38 KB 60.38 KB 18.64 KB 18.64 KB NODE_PROD
ReactTestRenderer-dev.js +0.2% +0.2% 436.2 KB 437.09 KB 92.34 KB 92.52 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.2% +0.2% 421.18 KB 422.07 KB 90.35 KB 90.54 KB NODE_DEV
react-reconciler.production.min.js 0.0% 0.0% 60.72 KB 60.72 KB 18.28 KB 18.29 KB NODE_PROD
react-reconciler-persistent.development.js +0.2% +0.2% 419.63 KB 420.52 KB 89.73 KB 89.92 KB NODE_DEV
react-reconciler-persistent.production.min.js 0.0% 0.0% 60.73 KB 60.73 KB 18.29 KB 18.29 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.2% 554.44 KB 555.33 KB 121.35 KB 121.55 KB RN_FB_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 238.53 KB 238.57 KB 42.02 KB 42.03 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.2% +0.2% 554.11 KB 555 KB 121.25 KB 121.46 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 238.55 KB 238.59 KB 42.02 KB 42.03 KB RN_OSS_PROD
ReactFabric-dev.js +0.2% +0.2% 544.63 KB 545.51 KB 118.86 KB 119.06 KB RN_FB_DEV
ReactFabric-prod.js 0.0% 0.0% 233.42 KB 233.46 KB 40.72 KB 40.72 KB RN_FB_PROD
ReactFabric-dev.js +0.2% +0.2% 544.66 KB 545.55 KB 118.88 KB 119.08 KB RN_OSS_DEV
ReactFabric-prod.js 0.0% 0.0% 233.45 KB 233.49 KB 40.74 KB 40.74 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js 0.0% 0.0% 244.24 KB 244.28 KB 43.23 KB 43.24 KB RN_OSS_PROFILING
ReactFabric-profiling.js 0.0% 0.0% 237.99 KB 238.03 KB 41.99 KB 41.99 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js 0.0% 0.0% 244.22 KB 244.26 KB 43.23 KB 43.24 KB RN_FB_PROFILING
ReactFabric-profiling.js 0.0% 0.0% 237.95 KB 237.99 KB 41.97 KB 41.98 KB RN_FB_PROFILING

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 5, 2018

Pushed a different fix. This doesn't fail any tests but is awkward.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 5, 2018

This is less awkward. Ready for review. Still don't know if it's right approach.

const failedUnitOfWork: Fiber = nextUnitOfWork;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy);
}
Copy link

Choose a reason for hiding this comment

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

Does this mean that the bug only happens when in DEV?

Also, couldn't this code be combined into the following block to avoid implying that the conditions/variables have any effect in production?

        if (__DEV__) {
          // Reset global debug state
          // We assume this is defined in DEV
          (resetCurrentlyProcessingQueue: any)();

          if (!wasCompleting && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
            const failedUnitOfWork: Fiber = nextUnitOfWork;
            replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy);
          }
        }

Copy link
Collaborator Author

@gaearon gaearon Nov 6, 2018

Choose a reason for hiding this comment

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

Thanks. I pushed another commit that clarifies which parts of the fix are DEV-only.

@acdlite
Copy link
Collaborator

acdlite commented Nov 6, 2018

The issue is that we null out nextUnitOfWork as we traverse up during completion

Maybe we should fix this part instead? completeUnitOfWork has a workInProgress variable that is supposed to mirror nextUnitOfWork. If I recall correctly, the only reason they are different is because of Flow typing.

@acdlite
Copy link
Collaborator

acdlite commented Nov 6, 2018

Rephrased, here's the alternate fix I'm suggesting. Either:

  • Inside completeUnitOfWork, replace all occurrences of workInProgress with nextUnitOfWork.
  • Or, always set nextUnitOfWork = workInProgress before calling completeWork.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 6, 2018

Or, always set nextUnitOfWork = workInProgress before calling completeWork.

Isn’t that what I’m doing?

@acdlite
Copy link
Collaborator

acdlite commented Nov 6, 2018

Oh right, I read the first few commits and then didn't realize the subsequent ones changed it. :D

// Reset in case completion throws.
// This is only used in DEV and when replaying is on.
const mayReplay = mayReplayFailedUnitOfWork;
mayReplayFailedUnitOfWork = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love this boolean, especially because it's not dev only. But I'm more concerned that this replay logic is getting really convoluted and it seems there must be a better way to structure it.

Maybe we can move where replayUnitOfWork is called instead. If replayUnitOfWork is only called after a failed begin phase, we could fork beginWork to beginWorkInDEV which has a local try/catch.

function beginWorkInDEV(current, workInProgress, renderExpirationTime) {
  try {
    beginWork(current, workInProgress, renderExpirationTime);
  } catch (e) {
    resetContextDependences();
    resetHooks();

    // Either call replayUnitOfWork or inline the whole thing here

    // Then throw to outer catch block
    throw e;
  }
}

There might be other implications that I'm forgetting, but I think we should give this a shot before piling more complexity onto this code path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

especially because it's not dev only

It is though. I think it would get DCE’d because we never read it outside DEV. The only reason I write to it outside a flag is just because it seems awkward to do extra wrapping because of scoping.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just checked — this is what I originally did, but @sebmarkbage asked that I move the extra try out of the hot path: #12201 (comment)

I feel we should reconsider given the additional complexity of distinguishing between the begin and complete phases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pushed another commit that makes it clearer that part is DEV-only too. I'll look at your suggestion tomorrow/

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

Ok let's land this now so we can include the bugfix in the next patch release. Since this isn't the first time we've had a bug related to this part of the error-handling path, let's revisit soon to see if there's a better way factor it.

@sebmarkbage
Copy link
Collaborator

let's revisit soon to see if there's a better way factor it.

let's

jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
* Recover from errors with a boundary in completion phase

* Use a separate field for completing unit of work

* Use a simpler fix with one boolean

* Reoder conditions

* Clarify which paths are DEV-only

* Move duplicated line out

* Make it clearer this code is DEV-only
n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
* Recover from errors with a boundary in completion phase

* Use a separate field for completing unit of work

* Use a simpler fix with one boolean

* Reoder conditions

* Clarify which paths are DEV-only

* Move duplicated line out

* Make it clearer this code is DEV-only
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

componentDidCatch doesn't catch some invariants
6 participants