[Fizz] Send errors down to client #24202
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
Wait.. There is life after React 18?! |
|
Comparing: 645ec5d...abc6156 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
| @@ -194,6 +196,8 @@ export opaque type Request = { | |||
| partialBoundaries: Array<SuspenseBoundary>, // Partially completed boundaries that can flush its segments early. | |||
| // onError is called when an error happens anywhere in the tree. It might recover. | |||
| onError: (error: mixed) => void, | |||
| // getErrorHash is used in production primarily to avoid leaking internals, secondarily to save bytes. In production we'll send the error hash. | |||
| getErrorHash: (error: mixed) => string, | |||
There was a problem hiding this comment.
Can we instead use the return value of onError and if it's undefined we blank it out in PROD or pass it through? That way we don't need to introduce another API and deal with conflicts, and you have the option to hash or encrypt it in DEV too.
There was a problem hiding this comment.
Yeah, I was thinking the same thing =)
salazarm
force-pushed
the
recoverableErrorServerToClient
branch
from
9672f26
to
b9d8d26
Compare
salazarm
force-pushed
the
recoverableErrorServerToClient
branch
from
b9d8d26
to
1b5ea1f
Compare
|
Hi @salazarm! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
There was a problem hiding this comment.
This PR only covers the case where the error happened after already having flushed the boundary. In the case the error happens early we'd write the boundary immediately into error state without going through any script tags using writeStartCompletedSuspenseBoundary(...). So the encoding mechanism needs to go through something serialized into the HTML.
This PR also has a XSS hole that needs to be fixed.
| @@ -1877,5 +1880,9 @@ export function writeClientRenderBoundaryInstruction( | |||
| } | |||
|
|
|||
| writeChunk(destination, boundaryID); | |||
| if (error) { | |||
| writeChunk(destination, clientRenderErrorScript1); | |||
| writeChunk(destination, stringToChunk(error)); | |||
There was a problem hiding this comment.
If the error message sneaks in a user space string, that can be used to insert tags like </script><script>alert('p0wned'). The approach chosen in #23063 won't work here by itself unless we also encode the whole script tag rather than just this string.
There was a problem hiding this comment.
Maybe if we assume that the rest of the script doesn't contain </script> or comments, it's ok to only escape script tags but it needs vetting.
There was a problem hiding this comment.
Hmm, fully escaping it should be fine since this should be a string anyways
| // If this callback errors, we intentionally let that error bubble up to become a fatal error | ||
| // so that someone fixes the error reporting instead of hiding it. | ||
| const onError = request.onError; | ||
| onError(error); | ||
| let errorMsg = onError(error); | ||
| if (__DEV__) { |
There was a problem hiding this comment.
If you provide a hash in DEV should we include that too?
There was a problem hiding this comment.
Hmm, I don't think it'd be that useful? Surely the hash would be used to get this original error message anyways?
| const msg = error && error.message ? error.message : error; | ||
| errorMsg = JSON.stringify({ | ||
| error: msg, | ||
| componentStack: getCurrentStackInDEV(), |
There was a problem hiding this comment.
This stack will no longer be correct by the time we get here because we've already popped the component stacks by the time we get here. So this might only tell you the nearest suspending component or nearest suspense boundary.
To do this accurately, we'd need to add a try/catch around deeper points in the tree and catch it + stash it at those places.
There was a problem hiding this comment.
Yeah I was thinking that the nearest suspend boundary might be fine but yeah I guess we should stash it and then have it be consumed here
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Closing to pick up in #24551 |
Summary
This sends down errors to the client to make debugging hydration errors in particular less mysterious.
There is an
onErrorcallback on the server side already for surfacing these issues, but frameworks need to surface that to users, for now this will surface it to users directly in the browser.We send the errors inline with the client rendered boundaries. In production we should only be sending hashes so there shouldn't be too much overhead in terms of bytes.
How did you test this change?
Jest