Cross-origin error handling in DEV #10353

acdlite · 2017-08-02T14:25:30Z

Cross-origin errors aren't accessible by React in DEV mode because we catch errors using a global error handler, in order to preserve the "Pause on exceptions" behavior of the DevTools. When this happens, we should use a custom error object that explains what happened.

For uncaught errors, the actual error message is logged to the console by the browser, so React should skip logging the custom error message, to avoid confusion.

Includes a new DOM test case:

Closes #10321

Cross-origin errors aren't accessible by React in DEV mode because we catch errors using a global error handler, in order to preserve the "Pause on exceptions" behavior of the DevTools. When this happens, we should use a custom error object that explains what happened. For uncaught errors, the actual error message is logged to the console by the browser, so React should skip logging the message again.

gaearon · 2017-08-02T14:29:08Z

src/renderers/shared/utils/ReactErrorUtils.js

+              'behavior of the DevTools. This is only an issue in DEV-mode; ' +
+              'in production, React uses a normal try-catch statement.\n\n' +
+              "It's recommended to serve JavaScript files from the same " +
+              'origin as your application.',


This might be interpreted as advice against CDNs. Which I'm not sure we want to give? Do we have opinions on this?

In that case, you would use an Access Control Allow Origin header. Do you think we should add this detail? Might be better to link to a page with more details?

Let's keep it like this for now.

I think we should mention something about CORS headers and how to set them up on a script tag. CORS will always be required for modules so people might as well start getting familiar with configuring it for cross-origin. whatwg/html#2440 (comment)

The issue isn't that it is cross-origin but that it is cross-origin + CORS not being set up. CDNs should support it and users that rely on CDNs should configure their script tags accordingly. Or CRA and Webpack should do it automatically.

cc @mjackson, have you had a chance to look into this for unpkg?

unpkg currently sets Access-Control-Allow-Origin: * on responses which should be sufficient for all requests w/out credentials.

Do you have to change your script tag to set the crossorigin attribute to anonymous for this to be respected by the browser though?

<script src="https://example.com/example-framework.js" crossorigin="anonymous"></script>

I think that could be the issue.

EDIT: Or maybe it's enough to set it to anything? E.g.:

<script src="https://example.com/example-framework.js" crossorigin></script>

I don't think so, @sebmarkbage. I created a test page that loads 16.0.0-beta.2 from unpkg w/out using <script crossorigin> and it seems to work ok (i.e. I can use "Pause on exceptions" w/out any problem) and React is able to catch and report the error in the console.

Is that what I'm supposed to be testing?

I think it's only if the error is thrown from a script other than the origin of the page...?

Look how @acdlite used the external expect library here. https://github.com/facebook/react/pull/10353/files#diff-f2b0545941b151d6a1f95ada1639d6f2R147

Triggering an "invariant" inside React, if React is hosted on unpkg, should probably also work similarly.

I can confirm just crossorigin is enough.

gaearon · 2017-08-02T14:29:55Z

src/renderers/shared/fiber/ReactFiberScheduler.js

-        willRetry,
-      });
+
+      if (__DEV__) {


Probably worth leaving a comment explaining why we need these two paths.

gaearon · 2017-08-02T14:33:18Z

For uncaught errors, the actual error message is logged to the console by the browser, so React should skip logging the message again.

I don't quite get this. Aren't we only skipping logging for our own custom fallback error? That's what the logic reads like to me.

acdlite · 2017-08-02T14:35:59Z

@gaearon Yeah. I meant we should skip logging the custom error, since that would be confusing. I considered just always skipping the message in DEV. Wasn't sure if the current double-logging behavior was desired for some reason.

gaearon · 2017-08-02T14:53:26Z

Wasn't sure if the current double-logging behavior was desired for some reason.

It is useful in case your app swallows errors. Then you won’t see the message unless we also print it. IMO we should keep it, even if it’s a bit annoying.

acdlite · 2017-08-02T14:59:32Z

But in DEV, the error is logged regardless because it's considered unhandled by the browser.

gaearon · 2017-08-02T15:05:47Z

Hmm, I’m not sure what you mean.
I’m referring to a situation where an error is accidentally handled, but the developer does not intend to. Like

try {
  this.setState()
} catch (err) {
  this.setState()
}

which is usually found in async code. People think they’re handling network errors, but they also swallow JS errors.

I think it’s important we keep logging the error in this case. Even if the browser thinks it’s caught.

acdlite · 2017-08-02T15:09:49Z

Even in that case, I believe the error is treated as unhandled because of the dispatchEvent event trick we use in invokeGuardedCallback. Then we rethrow it and it's caught again by the user's catch block, but it was already logged by the browser by the time that happens.

When I get back to my computer I'll make a GIF

gaearon · 2017-08-02T15:20:22Z

Hmm. Was this the case before? I believe such errors don’t surface in 15.

acdlite · 2017-08-02T15:25:07Z

It's different in 16 because we wrap everything in one big invokeGuardedCallback at the top of the stack. Any error thrown inside happens on a different event than the surrounding user code, so the browser treats it as unhandled and logs it.

bvaughn · 2017-08-02T15:51:16Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+          willRetry,
+          shouldIgnoreErrorMessage: error != null &&
+            typeof error.__reactShouldIgnoreErrorMessage === 'boolean' &&
+            error.__reactShouldIgnoreErrorMessage,


Why typeof error.__reactShouldIgnoreErrorMessage === 'boolean' && error.__reactShouldIgnoreErrorMessage instead of error.__reactShouldIgnoreErrorMessage === true ?

Really 😐

bvaughn · 2017-08-02T15:53:06Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+          willRetry,
+          shouldIgnoreErrorMessage: false,
+        });
+      }


Might be easier to read if the if/else condition above were collapsed into:

capturedErrors.set(boundary, { componentName, componentStack, error, errorBoundary: errorBoundaryFound ? boundary.stateNode : null, errorBoundaryFound, errorBoundaryName, willRetry, shouldIgnoreErrorMessage: __DEV__ && error != null && error.__reactShouldIgnoreErrorMessage === true });

Will that be dead code eliminated?

No. During stripEnvironmentVariables, __DEV__ would be converted to true/false.

I want it to be DCE'd, but if we change this to always ignore the message in DEV then this doesn't matter. #10353 (comment)

I'm confused. You have it in an if(DEV)/else currently, which won't be DCEd.

In the production build, it will be DCE'd the same as the rest of the DEV blocks

I'm confused. This PR adds the following:

if (__DEV__) { capturedErrors.set(boundary, {...}); } else { capturedErrors.set(boundary, {...}); }

I suggested collapsing this if/else branch b'c I thought it was more readable. I don't understand how DCE applies here, since neither approach would actually get DCE'd.

What I want to DCE is the extra checks

error != null && error.__reactShouldIgnoreErrorMessage === true

that only happen in DEV.

But this convo is moot anyway, because I'm going to remove this branch and just always ignore the error message in DEV.

Ah, I misunderstood your concern then. So you could have done:

__DEV__ ? error != null && error.__reactShouldIgnoreErrorMessage === true : false

But yeah 😁

acdlite · 2017-08-02T16:03:18Z

@bvaughn Do you object to skipping the error message in the error logger in DEV mode? Given that it's always logged by the browser. #10353 (comment)

gaearon · 2017-08-02T16:04:33Z

I'd prefer to skip it if it really still captures those nasty cases. But then the message should make it clear the actual error is just above. Since in my experience people look for first error above and don't scroll further up.

acdlite · 2017-08-02T16:08:26Z

New fixture demonstrating that the error is logged even if the originating update is wrapped in try-catch:

gaearon · 2017-08-02T16:09:18Z

One potentially confusing wording issue: we say "React caught an error" but it says "Uncaught error" right above. Would be nice to somehow make this seem less contradictory for users.

acdlite · 2017-08-02T16:14:50Z

I'll let y'all come up with improved messaging, or I'll pick this up again when I get back from PTO.

gaearon · 2017-08-02T16:15:54Z

I don't think the messages should block landing this. Just an observation.

acdlite · 2017-08-02T16:39:47Z

Pushed a commit that removes the double logging in DEV. Unit tests for the error logger are now failing because messages don't match. I'll fix 'em later, gotta go to bed :D

gaearon · 2017-08-02T16:41:00Z

When are you going on PTO? Maybe I can take over this if you'd like.

acdlite · 2017-08-02T16:42:04Z

@gaearon I'll be back Monday. And sure, no problem!

acdlite · 2017-08-02T16:45:14Z

fixtures/dom/src/components/fixtures/error-handling/index.js

+  container = document.createElement('div');
+
+  triggerErrorAndCatch = () => {
+    // Use setImmediate so that the render is syncrhonous


"syncrhonous"

Gah, every time. I don't even know how this happens because 'c' and 'r' are both left-hand keys 🤦‍♂️

Also just realized this isn't an amazing explanation. Without the setImmediate, the update is batched because it's inside an event handler. With the setImmediate, the update is deferred until the next tick, but within that tick, the setState is synchronous.

setImmediate isn't sync in an async-by-default world. Why no ReactDOM.flushSync instead?

Because for some reason I was thinking that wasn't in master yet :D Good call

sebmarkbage

Need to tweak error message. Tests failing but I think direction is fine.

gaearon · 2017-08-02T23:13:38Z

@sebmarkbage Which tweak in particular? I can take over this tomorrow.

sebmarkbage · 2017-08-02T23:16:41Z

Something about CORS and the use of crossorigin attribute.

bvaughn · 2017-08-03T17:14:18Z

src/renderers/shared/fiber/ReactFiberErrorLogger.js

+    // In production, we print the error directly.
+    // This will include the message, the JS stack, and anything the browser wants to show.
+    // We pass the error object instead of custom message so that the browser displays the error natively.
+    console.error(error);


We should remove the message and name variables now. They don't seem to be used anymore.

gaearon · 2017-08-03T17:18:56Z

OK, final messages:

bvaughn

Nice improvements! 🎉

gaearon · 2017-08-03T17:25:58Z

Everything passed locally.

bvaughn · 2017-08-03T18:41:44Z

This is probably not super common, but I wonder if the updated wording makes this type of scenario more confusing?

gaearon · 2017-08-03T18:53:03Z

Seems like this would be fixed by @spicyj’s suggestion: log them here rather than here.

gaearon · 2017-08-03T18:54:42Z

We could also "sandwich" those errors. i.e. we could log the first message immediately and then if there were errors during teardown (between first log and commit) we could log another message saying "Scroll up to find the original error" or something.

bvaughn · 2017-08-03T21:29:02Z

Seems like this would be fixed by @spicyj’s suggestion: log them here rather than here.

Sounds good to me. Is @spicyj on this then? Or should I do it?

gaearon · 2017-08-03T21:45:27Z

I don't think anyone is on this yet. You can make it yours.

bvaughn · 2017-08-03T21:48:13Z

#10373

mjackson · 2017-08-05T00:08:24Z

src/renderers/shared/utils/ReactErrorUtils.js

+              'error handler, in order to preserve the "Pause on exceptions" ' +
+              'behavior of the DevTools. This is only an issue in DEV-mode; ' +
+              'in production, React uses a normal try-catch statement.\n\n' +
+              'If you are using React from a CDN, ensure that the <script> tag ' +


It might be nice to say "If you are using React from a CDN in development, ensure ..." just to re-emphasize that this is only an issue in dev.

~~Isn't it only a dev issue though? Since in production we use a regular try/catch instead of the funky global handler~~

Sorry. I read your statement twice as "to re-emphasize that this isn't only an issue in dev."

PR welcome :-)

leidegre · 2017-08-10T11:45:26Z

I'm awfully confused by this and the search for an answer lead me here.

I'm not getting the "correct" (as I would suspect) error information in my error boundary.

A cross-origin error was thrown. React doesn't have access to the actual error because it catches errors using a global error handler, in order to preserve the "Pause on exceptions" behavior of the DevTools. This is only an issue in DEV-mode; in production, React uses a normal try-catch statement.

I always get the above message even though everything is served from localhost.

The error information is there in the console log together with additional stuff which is fine but I was expecting the error boundary componentDidCatch(error, info) function arguments to reflect the actual error, they don't appear to do so. The info.componentStack is correct but the error.message and error.stack is actually that ... ...cross-origin error was thrown... message. Is it supposed to work like that, the only way to see the actual error is to check the console log? It can't be, then logging the error to some service wouldn't really be possible...

gaearon · 2017-08-10T11:46:54Z

Can you provide a project reproducing this, with browser information?

leidegre · 2017-08-10T11:48:07Z

@gaearon I'll try to whip something together. Chrome 60.

gaearon · 2017-08-10T11:59:26Z

Any chance you're serving something else from other origin? For example you'd see this if another library (not React) throws but it's on CDN.

leidegre · 2017-08-10T12:06:17Z

@gaearon I don't think so but of course, that thought has crossed my mind.

I managed to get a repo working, I'll keep investigating, you if wanna take a quick look you can https://github.com/tessin/tessin-mini

Launch dev yarn run watch
Goto localhost:3000
Click on dashboard, should give you the error

This is a very complicated webpack setup that I've been experimenting with but there should be no CORS involved, I also noted different behavior between beta 2 and 5.

Running Chrome Version 60.0.3112.90 (Official Build) (64-bit) on Windows 10

Please ask if I need to clarify anything.

leidegre · 2017-08-10T12:24:59Z

Also, it doesn't appear as if the server-side rendering is trapping the error, it's propagated and not caught by the error boundary component.

Works as expected in a production build, you can run the production version and test it if you do.

yarn run build:prod && node server-prod.js

Feel free to send me down various rabbit holes if it helps, I don't know what else to do about this ATM.

leidegre · 2017-08-10T12:45:39Z

Demo link -> http://i.imgur.com/O7M3O2V.gif

bvaughn · 2017-08-10T15:25:52Z

I also noted different behavior between beta 2 and 5.

Beta 3 changed our wording around the component stack stuff. We now log an additional message below the actual error containing the component stack info. Is this what you're referring to?

bvaughn · 2017-08-10T15:29:28Z

I just checked out and ran tessin-mini.

$ yarn install
$ yarn run watch

Chrome 60.0.3112.90, OS X

Safari 10.1.2

Firefox 54.0.1

Admittedly the Firefox formatting/ordering is a bit wonky, but otherwise these look reasonable?

leidegre · 2017-08-11T09:21:49Z

@bvaughn The issue isn't with the console output, the issue is with that get's passed to the componentDidCatch function. I see the correct error in the log but the I get this CORS informational error which is unexpected.

...or are you saying that you are not getting that CORS error? (you should be able to see it if you follow the link to my demo)

leidegre · 2017-08-11T09:28:47Z

@bvaughn You can see here that I get additional errors in the console relating to CORS (which should not occur) and that the error information is different. componentDidCatch is only called once but not with the correct/relevant error information.

componentDidCatch(error, info) {
  console.log("componentDidCatch", error, info);
  this.setState({
    hasError: true,
    error,
    info
  });
}

The behavior is different between development and production builds. You can see this in the demo I put together.

leidegre · 2017-08-11T10:48:01Z

Looking through the code trying to understand why this happens.

I only hit the onError handler once, and it's with the CORS error, don't understand how this is possible.

bvaughn · 2017-08-11T15:23:20Z

Thanks for clarifying. I was focused on the console output.

Yes, I see the cross-origin error in the body as well if I click on the "Dashboard" link. (Loading the dashboard page directly seems to display the correct error message FWIW.)

Looking at the network tab, it seems like everything is coming from localhost so this seems to be a bug. Let's take the discussion to a new GH issue! 😄

Edit I've filed #10441

acdlite added 2 commits August 1, 2017 11:09

Add DOM fixture for cross-origin errors

01e2470

acdlite requested review from sebmarkbage and gaearon August 2, 2017 14:25

facebook-github-bot added the CLA Signed label Aug 2, 2017

gaearon reviewed Aug 2, 2017

View reviewed changes

acdlite requested a review from bvaughn August 2, 2017 14:38

bvaughn reviewed Aug 2, 2017

View reviewed changes

acdlite force-pushed the crossoriginerror branch from 43793a1 to dc24f32 Compare August 2, 2017 16:16

acdlite commented Aug 2, 2017

View reviewed changes

sebmarkbage previously requested changes Aug 2, 2017

View reviewed changes

bvaughn reviewed Aug 3, 2017

View reviewed changes

gaearon added 2 commits August 3, 2017 18:16

Concise wording

39df724

Unused variables

2ea6063

bvaughn approved these changes Aug 3, 2017

View reviewed changes

gaearon merged commit a650699 into facebook:master Aug 3, 2017

bvaughn mentioned this pull request Aug 3, 2017

Log captured errors sooner #10373

Merged

bvaughn mentioned this pull request Aug 3, 2017

React 16 RC #10294

Closed

mjackson reviewed Aug 5, 2017

View reviewed changes

bvaughn mentioned this pull request Aug 11, 2017

Cross-origin error passed to componentDidCatch incorrectly #10441

Open

Cross-origin error handling in DEV #10353

Cross-origin error handling in DEV #10353

Conversation

acdlite commented Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Aug 2, 2017

acdlite commented Aug 2, 2017 • edited Loading

gaearon commented Aug 2, 2017

acdlite commented Aug 2, 2017

gaearon commented Aug 2, 2017

acdlite commented Aug 2, 2017

gaearon commented Aug 2, 2017

acdlite commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

acdlite Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

bvaughn Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

acdlite commented Aug 2, 2017 • edited Loading

gaearon commented Aug 2, 2017

acdlite commented Aug 2, 2017

gaearon commented Aug 2, 2017 • edited Loading

acdlite commented Aug 2, 2017

gaearon commented Aug 2, 2017

acdlite commented Aug 2, 2017

gaearon commented Aug 2, 2017

acdlite commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

gaearon commented Aug 2, 2017

sebmarkbage commented Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Aug 3, 2017

bvaughn left a comment

Choose a reason for hiding this comment

gaearon commented Aug 3, 2017

bvaughn commented Aug 3, 2017

gaearon commented Aug 3, 2017

gaearon commented Aug 3, 2017 • edited Loading

bvaughn commented Aug 3, 2017

gaearon commented Aug 3, 2017

bvaughn commented Aug 3, 2017

Choose a reason for hiding this comment

bvaughn Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leidegre commented Aug 10, 2017

gaearon commented Aug 10, 2017

leidegre commented Aug 10, 2017

gaearon commented Aug 10, 2017

leidegre commented Aug 10, 2017 • edited Loading

leidegre commented Aug 10, 2017

acdlite commented Aug 2, 2017 •

edited

Loading

sebmarkbage Aug 2, 2017 •

edited

Loading

sebmarkbage Aug 3, 2017 •

edited

Loading

acdlite commented Aug 2, 2017 •

edited

Loading

acdlite Aug 2, 2017 •

edited

Loading

bvaughn Aug 2, 2017 •

edited

Loading

acdlite Aug 2, 2017 •

edited

Loading

bvaughn Aug 2, 2017 •

edited

Loading

acdlite commented Aug 2, 2017 •

edited

Loading

gaearon commented Aug 2, 2017 •

edited

Loading

sebmarkbage commented Aug 2, 2017 •

edited

Loading

gaearon commented Aug 3, 2017 •

edited

Loading

bvaughn Aug 5, 2017 •

edited

Loading

leidegre commented Aug 10, 2017 •

edited

Loading

leidegre commented Aug 10, 2017 •

edited

Loading

bvaughn commented Aug 10, 2017 •

edited

Loading

bvaughn commented Aug 10, 2017 •

edited

Loading

leidegre commented Aug 11, 2017 •

edited

Loading

leidegre commented Aug 11, 2017 •

edited

Loading

bvaughn commented Aug 11, 2017 •

edited

Loading