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

[Fizz] Send errors down to client #24202

Closed

Conversation

salazarm
Copy link
Contributor

Summary

This sends down errors to the client to make debugging hydration errors in particular less mysterious.
There is an onError callback 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

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 29, 2022
@salazarm salazarm requested a review from acdlite March 29, 2022 21:33
@gaearon
Copy link
Collaborator

gaearon commented Mar 29, 2022

Wait.. There is life after React 18?!

@sizebot
Copy link

sizebot commented Mar 29, 2022

Comparing: 645ec5d...abc6156

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.11% 130.96 kB 131.11 kB +0.16% 41.93 kB 41.99 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.11% 136.02 kB 136.17 kB +0.15% 43.41 kB 43.47 kB
facebook-www/ReactDOM-prod.classic.js +0.05% 435.63 kB 435.85 kB +0.11% 79.79 kB 79.88 kB
facebook-www/ReactDOM-prod.modern.js +0.05% 422.05 kB 422.27 kB +0.11% 77.78 kB 77.87 kB
facebook-www/ReactDOMForked-prod.classic.js +0.05% 435.63 kB 435.85 kB +0.11% 79.80 kB 79.89 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-server/cjs/react-server.production.min.js +0.65% 19.90 kB 20.03 kB +0.64% 6.85 kB 6.90 kB
oss-stable/react-server/cjs/react-server.production.min.js +0.65% 19.90 kB 20.03 kB +0.64% 6.85 kB 6.90 kB
oss-experimental/react-server/cjs/react-server.production.min.js +0.64% 20.23 kB 20.36 kB +0.71% 6.95 kB 7.00 kB
facebook-www/ReactDOMServer-prod.modern.js +0.63% 75.61 kB 76.09 kB +0.75% 15.62 kB 15.74 kB
facebook-www/ReactDOMServerStreaming-prod.modern.js +0.58% 77.43 kB 77.88 kB +0.70% 16.42 kB 16.54 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.production.min.js +0.55% 33.53 kB 33.71 kB +0.80% 11.44 kB 11.53 kB
oss-stable/react-dom/cjs/react-dom-server.browser.production.min.js +0.55% 33.53 kB 33.71 kB +0.80% 11.44 kB 11.53 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.production.min.js +0.55% 33.93 kB 34.11 kB +0.79% 11.58 kB 11.67 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +0.54% 32.37 kB 32.54 kB +0.74% 10.79 kB 10.87 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +0.54% 32.37 kB 32.54 kB +0.74% 10.79 kB 10.87 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +0.53% 32.77 kB 32.94 kB +0.74% 10.93 kB 11.01 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.production.min.js +0.53% 36.28 kB 36.48 kB +0.61% 12.19 kB 12.26 kB
oss-stable/react-dom/cjs/react-dom-server.node.production.min.js +0.53% 36.28 kB 36.48 kB +0.61% 12.19 kB 12.26 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.production.min.js +0.52% 33.66 kB 33.83 kB +0.81% 11.57 kB 11.66 kB
oss-stable/react-dom/umd/react-dom-server.browser.production.min.js +0.52% 33.66 kB 33.83 kB +0.81% 11.57 kB 11.66 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.min.js +0.52% 36.74 kB 36.93 kB +0.61% 12.34 kB 12.42 kB
oss-experimental/react-dom/umd/react-dom-server.browser.production.min.js +0.51% 34.06 kB 34.23 kB +0.86% 11.70 kB 11.80 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.production.min.js +0.50% 32.52 kB 32.68 kB +0.82% 10.92 kB 11.01 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.production.min.js +0.50% 32.52 kB 32.68 kB +0.82% 10.92 kB 11.01 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.production.min.js +0.50% 32.92 kB 33.08 kB +0.68% 11.06 kB 11.14 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.production.min.js +0.48% 36.06 kB 36.23 kB +0.58% 12.02 kB 12.09 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.production.min.js +0.48% 36.06 kB 36.23 kB +0.58% 12.02 kB 12.09 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.production.min.js +0.47% 36.51 kB 36.69 kB +0.53% 12.17 kB 12.24 kB
oss-stable-semver/react-server/cjs/react-server.development.js +0.32% 124.81 kB 125.21 kB +0.34% 31.27 kB 31.38 kB
oss-stable/react-server/cjs/react-server.development.js +0.32% 124.81 kB 125.21 kB +0.34% 31.27 kB 31.38 kB
oss-experimental/react-server/cjs/react-server.development.js +0.32% 125.68 kB 126.08 kB +0.36% 31.48 kB 31.60 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.development.js +0.29% 228.11 kB 228.76 kB +0.31% 55.43 kB 55.60 kB
oss-stable/react-dom/cjs/react-dom-server.node.development.js +0.29% 228.11 kB 228.76 kB +0.31% 55.43 kB 55.60 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.development.js +0.29% 241.51 kB 242.20 kB +0.33% 56.74 kB 56.93 kB
oss-stable/react-dom/umd/react-dom-server.browser.development.js +0.29% 241.51 kB 242.20 kB +0.33% 56.74 kB 56.93 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.development.js +0.29% 241.91 kB 242.60 kB +0.35% 56.53 kB 56.73 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.development.js +0.29% 241.91 kB 242.60 kB +0.35% 56.53 kB 56.73 kB
facebook-www/ReactDOMServerStreaming-dev.modern.js +0.29% 231.58 kB 232.24 kB +0.32% 55.37 kB 55.55 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js +0.29% 229.53 kB 230.19 kB +0.33% 55.80 kB 55.98 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.development.js +0.29% 230.11 kB 230.77 kB +0.31% 56.15 kB 56.32 kB
oss-stable/react-dom/cjs/react-dom-server.browser.development.js +0.29% 230.11 kB 230.77 kB +0.31% 56.15 kB 56.32 kB
oss-experimental/react-dom/umd/react-dom-server.browser.development.js +0.29% 243.01 kB 243.70 kB +0.32% 57.10 kB 57.28 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.29% 230.47 kB 231.13 kB +0.31% 55.90 kB 56.07 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.29% 230.47 kB 231.13 kB +0.31% 55.90 kB 56.07 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.development.js +0.28% 243.41 kB 244.10 kB +0.33% 56.90 kB 57.09 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js +0.28% 231.53 kB 232.19 kB +0.32% 56.52 kB 56.70 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.28% 231.90 kB 232.55 kB +0.33% 56.30 kB 56.48 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.development.js +0.28% 232.18 kB 232.84 kB +0.31% 56.37 kB 56.55 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.development.js +0.28% 232.18 kB 232.84 kB +0.31% 56.37 kB 56.55 kB
facebook-www/ReactDOMServer-dev.modern.js +0.28% 234.99 kB 235.65 kB +0.31% 56.17 kB 56.35 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.development.js +0.28% 233.60 kB 234.26 kB +0.32% 56.78 kB 56.96 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.production.min.js +0.22% 92.74 kB 92.95 kB +0.33% 28.51 kB 28.60 kB
oss-stable/react-reconciler/cjs/react-reconciler.production.min.js +0.22% 92.74 kB 92.95 kB +0.33% 28.51 kB 28.60 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.min.js +0.21% 97.41 kB 97.61 kB +0.24% 29.77 kB 29.84 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.node.development.server.js +0.20% 59.74 kB 59.86 kB +0.22% 15.42 kB 15.46 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.node.development.server.js +0.20% 59.74 kB 59.86 kB +0.22% 15.42 kB 15.46 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.node.development.server.js +0.20% 59.79 kB 59.91 kB +0.23% 15.44 kB 15.48 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.profiling.min.js +0.20% 101.64 kB 101.85 kB +0.32% 30.70 kB 30.80 kB
oss-stable/react-reconciler/cjs/react-reconciler.profiling.min.js +0.20% 101.64 kB 101.85 kB +0.32% 30.70 kB 30.80 kB

Generated by 🚫 dangerJS against abc6156

@@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was thinking the same thing =)

@salazarm salazarm force-pushed the recoverableErrorServerToClient branch from 9672f26 to b9d8d26 Compare March 31, 2022 15:12
@salazarm salazarm force-pushed the recoverableErrorServerToClient branch from b9d8d26 to 1b5ea1f Compare March 31, 2022 15:13
@facebook-github-bot
Copy link

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.

Process

In 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 CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

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));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage Apr 20, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, fully escaping it should be fine since this should be a string anyways

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

I wonder if we should encode the error message and the hash separately and provide them as separate arguments on the client.

// 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__) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you provide a hash in DEV should we include that too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@gnoff
Copy link
Collaborator

gnoff commented May 12, 2022

Closing to pick up in #24551

@gnoff gnoff closed this May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants