Tag all user space call sites with the "react-stack-bottom-frame" name #30369
Conversation
This file contains 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
We can use this trick to avoid minifying these functions and ensure they get a unique name added to them in all browsers. This lets us find the bottom stack easily from stack traces.
|
I noticed that, in JSC, if the function body is too small/simple it can get inlined and the stack frame excluded. That could maybe also be a JIT level specific - like only if it gets to the highest optimization tier does it kick in. So it's not just minifiers that might inline but the VM as well. That's something we'll have to be careful about but that also applies to both the previous technique and this new one. |
eps1lon
reviewed
Jul 19, 2024
|
It's just an internal refactor that then becomes the basis for a stack of other PRs. |
This was referenced Jul 19, 2024
eps1lon
approved these changes
Jul 22, 2024
github-actions bot
pushed a commit
that referenced
this pull request
Jul 22, 2024
#30369) Ideally we wouldn't need to filter out React internals and it'd just be covered by ignore listing by any downstream tool. E.g. a framework using captureOwnerStack could have its own ignore listing. Printed owner stacks would get browser source map ignore-listing. React DevTools could have its own ignore list for internals. However, it's nice to be able to provide nice owner stacks without a bunch of noise by default. Especially on the server since they have to be serialized. We currently call each function that calls into user space and track its stack frame. However, this needs code for checking each one and doesn't let us work across bundles. Instead, we can name each of these frame something predictable by giving the function a name. Unfortunately, it's a common practice to rename functions or inline them in compilers. Even if we didn't, others downstream from us or a dev-mode minifier could. I use this `.bind()` trick to avoid minifying these functions and ensure they get a unique name added to them in all browsers. It's not 100% fool proof since a smart enough compiler could also discover that the `this` value is not used and strip out the function and then inline it but nobody does this yet at least. This lets us find the bottom stack easily from stack traces just by looking for the name. DiffTrain build for commit 792f192.
github-actions bot
pushed a commit
that referenced
this pull request
Jul 22, 2024
#30369) Ideally we wouldn't need to filter out React internals and it'd just be covered by ignore listing by any downstream tool. E.g. a framework using captureOwnerStack could have its own ignore listing. Printed owner stacks would get browser source map ignore-listing. React DevTools could have its own ignore list for internals. However, it's nice to be able to provide nice owner stacks without a bunch of noise by default. Especially on the server since they have to be serialized. We currently call each function that calls into user space and track its stack frame. However, this needs code for checking each one and doesn't let us work across bundles. Instead, we can name each of these frame something predictable by giving the function a name. Unfortunately, it's a common practice to rename functions or inline them in compilers. Even if we didn't, others downstream from us or a dev-mode minifier could. I use this `.bind()` trick to avoid minifying these functions and ensure they get a unique name added to them in all browsers. It's not 100% fool proof since a smart enough compiler could also discover that the `this` value is not used and strip out the function and then inline it but nobody does this yet at least. This lets us find the bottom stack easily from stack traces just by looking for the name. DiffTrain build for [792f192](792f192)
sebmarkbage
added a commit
that referenced
this pull request
Jul 22, 2024
Stacked on #30400 and #30369 Previously we were using fake evals to recreate a stack for console replaying and thrown errors. However, for owner stacks we just used the raw string that came from the server. This means that the format of the owner stack could include different formats. Like Spidermonkey format for the client components and V8 for the server components. This means that this stack can't be parsed natively by the browser like when printing them as error like in #30289. Additionally, since there's no source file registered with that name and no source mapping url, it can't be source mapped. Before: <img width="1329" alt="before-firefox" src="https://github.com/user-attachments/assets/cbe03f9c-96ac-48fb-b58f-f3a224a774f4"> Instead, we need to create a fake stack like we do for the other things. That way when it's printed as an Error it gets source mapped. It also means that the format is consistently in the native format of the current browser. After: <img width="753" alt="after-firefox" src="https://github.com/user-attachments/assets/b436f1f5-ca37-4203-b29f-df9828c9fad3"> So this is nice because you can just take the result from `captureOwnerStack()` and append it to an `Error` stack and print it natively. E.g. this is what React DevTools will do. If you want to parse and present it yourself though it's a bit awkward though. The `captureOwnerStack()` API now includes a bunch of `rsc://React/` URLs. These don't really have any direct connection to the source map. Only the browser knows this connection from the eval. You basically have to strip the prefix and then manually pass the remainder to your own `findSourceMapURL`. Another awkward part is that since Safari doesn't support eval sourceURL exposed into `error.stack` - it means that `captureOwnerStack()` get an empty location for server components since the fake eval doesn't work there. That's not a big deal since these stacks are already broken even for client modules for many because the `eval-source-map` strategy in Webpack doesn't work in Safari for this same reason. A lot of this refactoring is just clarifying that there's three kind of ReactComponentInfo fields: - `stack` - The raw stack as described on the original server. - `debugStack` - The Error object containing the stack as represented in the current client as fake evals. - `debugTask` - The same thing as `debugStack` but described in terms of a native `console.createTask`.
github-actions bot
pushed a commit
that referenced
this pull request
Jul 22, 2024
Stacked on #30400 and #30369 Previously we were using fake evals to recreate a stack for console replaying and thrown errors. However, for owner stacks we just used the raw string that came from the server. This means that the format of the owner stack could include different formats. Like Spidermonkey format for the client components and V8 for the server components. This means that this stack can't be parsed natively by the browser like when printing them as error like in #30289. Additionally, since there's no source file registered with that name and no source mapping url, it can't be source mapped. Before: <img width="1329" alt="before-firefox" src="https://github.com/user-attachments/assets/cbe03f9c-96ac-48fb-b58f-f3a224a774f4"> Instead, we need to create a fake stack like we do for the other things. That way when it's printed as an Error it gets source mapped. It also means that the format is consistently in the native format of the current browser. After: <img width="753" alt="after-firefox" src="https://github.com/user-attachments/assets/b436f1f5-ca37-4203-b29f-df9828c9fad3"> So this is nice because you can just take the result from `captureOwnerStack()` and append it to an `Error` stack and print it natively. E.g. this is what React DevTools will do. If you want to parse and present it yourself though it's a bit awkward though. The `captureOwnerStack()` API now includes a bunch of `rsc://React/` URLs. These don't really have any direct connection to the source map. Only the browser knows this connection from the eval. You basically have to strip the prefix and then manually pass the remainder to your own `findSourceMapURL`. Another awkward part is that since Safari doesn't support eval sourceURL exposed into `error.stack` - it means that `captureOwnerStack()` get an empty location for server components since the fake eval doesn't work there. That's not a big deal since these stacks are already broken even for client modules for many because the `eval-source-map` strategy in Webpack doesn't work in Safari for this same reason. A lot of this refactoring is just clarifying that there's three kind of ReactComponentInfo fields: - `stack` - The raw stack as described on the original server. - `debugStack` - The Error object containing the stack as represented in the current client as fake evals. - `debugTask` - The same thing as `debugStack` but described in terms of a native `console.createTask`. DiffTrain build for [b15c198](b15c198)
sebmarkbage
added a commit
that referenced
this pull request
Jul 22, 2024
Stacked on #30410. Use "owner stacks" as the appended component stack if it is available on the Fiber. This will only be available if the enableOwnerStacks flag is on. Otherwise it fallback to parent stacks. In prod, there's no owner so it's never added there. I was going back and forth on whether to inject essentially `captureOwnerStack` as part of the DevTools hooks or replicate the implementation but decided to replicate the implementation. The DevTools needs all the same information from internals to implement owner views elsewhere in the UI anyway so we're not saving anything in terms of the scope of internals. Additionally, we really need this information for non-current components as well like "rendered by" views of the currently selected component. It can also be useful if we need to change the format after the fact like we did for parent stacks in: #30289 Injecting the implementation would lock us into specifics both in terms of what the core needs to provide and what the DevTools can use. The implementation depends on the technique used in #30369 which tags frames to strip out with `react-stack-bottom-frame`. That's how the implementation knows how to materialize the error if it hasn't already. Firefox: <img width="487" alt="Screenshot 2024-07-21 at 11 33 37 PM" src="https://github.com/user-attachments/assets/d3539b53-4578-4fdd-af25-25698b2bcc7d"> Follow up: One thing about this view is that it doesn't include the current actual synchronous stack. When I used to append these I would include both the real current stack and the owner stack. That's because the owner stack doesn't include the name of the currently executing component. I'll probably inject the current stack too in addition to the owner stack. This is similar to how native Async Stacks are basically just appended onto the current stack rather than its own.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
Stacked on facebook#30410. Use "owner stacks" as the appended component stack if it is available on the Fiber. This will only be available if the enableOwnerStacks flag is on. Otherwise it fallback to parent stacks. In prod, there's no owner so it's never added there. I was going back and forth on whether to inject essentially `captureOwnerStack` as part of the DevTools hooks or replicate the implementation but decided to replicate the implementation. The DevTools needs all the same information from internals to implement owner views elsewhere in the UI anyway so we're not saving anything in terms of the scope of internals. Additionally, we really need this information for non-current components as well like "rendered by" views of the currently selected component. It can also be useful if we need to change the format after the fact like we did for parent stacks in: facebook#30289 Injecting the implementation would lock us into specifics both in terms of what the core needs to provide and what the DevTools can use. The implementation depends on the technique used in facebook#30369 which tags frames to strip out with `react-stack-bottom-frame`. That's how the implementation knows how to materialize the error if it hasn't already. Firefox: <img width="487" alt="Screenshot 2024-07-21 at 11 33 37 PM" src="https://github.com/user-attachments/assets/d3539b53-4578-4fdd-af25-25698b2bcc7d"> Follow up: One thing about this view is that it doesn't include the current actual synchronous stack. When I used to append these I would include both the real current stack and the owner stack. That's because the owner stack doesn't include the name of the currently executing component. I'll probably inject the current stack too in addition to the owner stack. This is similar to how native Async Stacks are basically just appended onto the current stack rather than its own.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
Stacked on facebook#30400 and facebook#30369 Previously we were using fake evals to recreate a stack for console replaying and thrown errors. However, for owner stacks we just used the raw string that came from the server. This means that the format of the owner stack could include different formats. Like Spidermonkey format for the client components and V8 for the server components. This means that this stack can't be parsed natively by the browser like when printing them as error like in facebook#30289. Additionally, since there's no source file registered with that name and no source mapping url, it can't be source mapped. Before: <img width="1329" alt="before-firefox" src="https://github.com/user-attachments/assets/cbe03f9c-96ac-48fb-b58f-f3a224a774f4"> Instead, we need to create a fake stack like we do for the other things. That way when it's printed as an Error it gets source mapped. It also means that the format is consistently in the native format of the current browser. After: <img width="753" alt="after-firefox" src="https://github.com/user-attachments/assets/b436f1f5-ca37-4203-b29f-df9828c9fad3"> So this is nice because you can just take the result from `captureOwnerStack()` and append it to an `Error` stack and print it natively. E.g. this is what React DevTools will do. If you want to parse and present it yourself though it's a bit awkward though. The `captureOwnerStack()` API now includes a bunch of `rsc://React/` URLs. These don't really have any direct connection to the source map. Only the browser knows this connection from the eval. You basically have to strip the prefix and then manually pass the remainder to your own `findSourceMapURL`. Another awkward part is that since Safari doesn't support eval sourceURL exposed into `error.stack` - it means that `captureOwnerStack()` get an empty location for server components since the fake eval doesn't work there. That's not a big deal since these stacks are already broken even for client modules for many because the `eval-source-map` strategy in Webpack doesn't work in Safari for this same reason. A lot of this refactoring is just clarifying that there's three kind of ReactComponentInfo fields: - `stack` - The raw stack as described on the original server. - `debugStack` - The Error object containing the stack as represented in the current client as fake evals. - `debugTask` - The same thing as `debugStack` but described in terms of a native `console.createTask`.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
facebook#30369) Ideally we wouldn't need to filter out React internals and it'd just be covered by ignore listing by any downstream tool. E.g. a framework using captureOwnerStack could have its own ignore listing. Printed owner stacks would get browser source map ignore-listing. React DevTools could have its own ignore list for internals. However, it's nice to be able to provide nice owner stacks without a bunch of noise by default. Especially on the server since they have to be serialized. We currently call each function that calls into user space and track its stack frame. However, this needs code for checking each one and doesn't let us work across bundles. Instead, we can name each of these frame something predictable by giving the function a name. Unfortunately, it's a common practice to rename functions or inline them in compilers. Even if we didn't, others downstream from us or a dev-mode minifier could. I use this `.bind()` trick to avoid minifying these functions and ensure they get a unique name added to them in all browsers. It's not 100% fool proof since a smart enough compiler could also discover that the `this` value is not used and strip out the function and then inline it but nobody does this yet at least. This lets us find the bottom stack easily from stack traces just by looking for the name.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
Stacked on facebook#30410. Use "owner stacks" as the appended component stack if it is available on the Fiber. This will only be available if the enableOwnerStacks flag is on. Otherwise it fallback to parent stacks. In prod, there's no owner so it's never added there. I was going back and forth on whether to inject essentially `captureOwnerStack` as part of the DevTools hooks or replicate the implementation but decided to replicate the implementation. The DevTools needs all the same information from internals to implement owner views elsewhere in the UI anyway so we're not saving anything in terms of the scope of internals. Additionally, we really need this information for non-current components as well like "rendered by" views of the currently selected component. It can also be useful if we need to change the format after the fact like we did for parent stacks in: facebook#30289 Injecting the implementation would lock us into specifics both in terms of what the core needs to provide and what the DevTools can use. The implementation depends on the technique used in facebook#30369 which tags frames to strip out with `react-stack-bottom-frame`. That's how the implementation knows how to materialize the error if it hasn't already. Firefox: <img width="487" alt="Screenshot 2024-07-21 at 11 33 37 PM" src="https://github.com/user-attachments/assets/d3539b53-4578-4fdd-af25-25698b2bcc7d"> Follow up: One thing about this view is that it doesn't include the current actual synchronous stack. When I used to append these I would include both the real current stack and the owner stack. That's because the owner stack doesn't include the name of the currently executing component. I'll probably inject the current stack too in addition to the owner stack. This is similar to how native Async Stacks are basically just appended onto the current stack rather than its own.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
Stacked on facebook#30400 and facebook#30369 Previously we were using fake evals to recreate a stack for console replaying and thrown errors. However, for owner stacks we just used the raw string that came from the server. This means that the format of the owner stack could include different formats. Like Spidermonkey format for the client components and V8 for the server components. This means that this stack can't be parsed natively by the browser like when printing them as error like in facebook#30289. Additionally, since there's no source file registered with that name and no source mapping url, it can't be source mapped. Before: <img width="1329" alt="before-firefox" src="https://github.com/user-attachments/assets/cbe03f9c-96ac-48fb-b58f-f3a224a774f4"> Instead, we need to create a fake stack like we do for the other things. That way when it's printed as an Error it gets source mapped. It also means that the format is consistently in the native format of the current browser. After: <img width="753" alt="after-firefox" src="https://github.com/user-attachments/assets/b436f1f5-ca37-4203-b29f-df9828c9fad3"> So this is nice because you can just take the result from `captureOwnerStack()` and append it to an `Error` stack and print it natively. E.g. this is what React DevTools will do. If you want to parse and present it yourself though it's a bit awkward though. The `captureOwnerStack()` API now includes a bunch of `rsc://React/` URLs. These don't really have any direct connection to the source map. Only the browser knows this connection from the eval. You basically have to strip the prefix and then manually pass the remainder to your own `findSourceMapURL`. Another awkward part is that since Safari doesn't support eval sourceURL exposed into `error.stack` - it means that `captureOwnerStack()` get an empty location for server components since the fake eval doesn't work there. That's not a big deal since these stacks are already broken even for client modules for many because the `eval-source-map` strategy in Webpack doesn't work in Safari for this same reason. A lot of this refactoring is just clarifying that there's three kind of ReactComponentInfo fields: - `stack` - The raw stack as described on the original server. - `debugStack` - The Error object containing the stack as represented in the current client as fake evals. - `debugTask` - The same thing as `debugStack` but described in terms of a native `console.createTask`.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
facebook#30369) Ideally we wouldn't need to filter out React internals and it'd just be covered by ignore listing by any downstream tool. E.g. a framework using captureOwnerStack could have its own ignore listing. Printed owner stacks would get browser source map ignore-listing. React DevTools could have its own ignore list for internals. However, it's nice to be able to provide nice owner stacks without a bunch of noise by default. Especially on the server since they have to be serialized. We currently call each function that calls into user space and track its stack frame. However, this needs code for checking each one and doesn't let us work across bundles. Instead, we can name each of these frame something predictable by giving the function a name. Unfortunately, it's a common practice to rename functions or inline them in compilers. Even if we didn't, others downstream from us or a dev-mode minifier could. I use this `.bind()` trick to avoid minifying these functions and ensure they get a unique name added to them in all browsers. It's not 100% fool proof since a smart enough compiler could also discover that the `this` value is not used and strip out the function and then inline it but nobody does this yet at least. This lets us find the bottom stack easily from stack traces just by looking for the name.
This was referenced Jul 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Ideally we wouldn't need to filter out React internals and it'd just be covered by ignore listing by any downstream tool. E.g. a framework using captureOwnerStack could have its own ignore listing. Printed owner stacks would get browser source map ignore-listing. React DevTools could have its own ignore list for internals. However, it's nice to be able to provide nice owner stacks without a bunch of noise by default. Especially on the server since they have to be serialized.
We currently call each function that calls into user space and track its stack frame. However, this needs code for checking each one and doesn't let us work across bundles.
Instead, we can name each of these frame something predictable by giving the function a name.
Unfortunately, it's a common practice to rename functions or inline them in compilers. Even if we didn't, others downstream from us or a dev-mode minifier could. I use this
.bind()trick to avoid minifying these functions and ensure they get a unique name added to them in all browsers. It's not 100% fool proof since a smart enough compiler could also discover that thethisvalue is not used and strip out the function and then inline it but nobody does this yet at least.This lets us find the bottom stack easily from stack traces just by looking for the name.