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.

Sign up for GitHub

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

Jump to bottom

Update stack diffing algorithm in describeNativeComponentFrame #27132

Merged
merged 6 commits into from
Nov 8, 2023
Merged

Update stack diffing algorithm in describeNativeComponentFrame #27132

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9cc71f5
Update stack diffing algorithm in describeNativeComponentFrame
KarimP Jul 13, 2023
bafd70f
Fix typo in `Object.getOwnPropertyDescriptor` call
KarimP Jul 19, 2023
364938d
Switch to having `DetermineComponentFrameRoot` be an object property
KarimP Jul 19, 2023
08883b3
Merge branch 'main' into component-stack-fix
KarimP Sep 25, 2023
8476ffa
Switch from `findIndex` to a while loop
KarimP Sep 21, 2023
38abf48
Run prettier
KarimP Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
198 changes: 134 additions & 64 deletions packages/shared/ReactComponentStackFrame.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ if (__DEV__) {
componentFrameCache = new PossiblyWeakMap<Function, string>();
}

/**
* Leverages native browser/VM stack frames to get proper details (e.g.
* filename, line + col number) for a single component in a component stack. We
* do this by:
* (1) throwing and catching an error in the function - this will be our
* control error.
* (2) calling the component which will eventually throw an error that we'll
* catch - this will be our sample error.
* (3) diffing the control and sample error stacks to find the stack frame
* which represents our component.
*/
export function describeNativeComponentFrame(
fn: Function,
construct: boolean,
Expand All @@ -76,89 +87,148 @@ export function describeNativeComponentFrame(
}
}

let control;

reentry = true;
const previousPrepareStackTrace = Error.prepareStackTrace;
// $FlowFixMe[incompatible-type] It does accept undefined.
Error.prepareStackTrace = undefined;
let previousDispatcher;

if (__DEV__) {
previousDispatcher = ReactCurrentDispatcher.current;
// Set the dispatcher in DEV because this might be call in the render function
// for warnings.
ReactCurrentDispatcher.current = null;
disableLogs();
}
try {
// This should throw.
if (construct) {
// Something should be setting the props in the constructor.
const Fake = function () {
throw Error();
};
// $FlowFixMe[prop-missing]
Object.defineProperty(Fake.prototype, 'props', {
set: function () {
// We use a throwing setter instead of frozen or non-writable props
// because that won't throw in a non-strict mode function.
throw Error();
},
});
if (typeof Reflect === 'object' && Reflect.construct) {
// We construct a different control for this case to include any extra
// frames added by the construct call.
try {
Reflect.construct(Fake, []);
} catch (x) {
control = x;
}
Reflect.construct(fn, [], Fake);
} else {
try {
Fake.call();
} catch (x) {
control = x;
}
// $FlowFixMe[prop-missing] found when upgrading Flow
fn.call(Fake.prototype);

/**
* Finding a common stack frame between sample and control errors can be
* tricky given the different types and levels of stack trace truncation from
* different JS VMs. So instead we'll attempt to control what that common
* frame should be through this class:
* Having both the sample and control errors be under the
* `DescribeNativeComponentFrameRoot` method call will ensure that a stack
* frame exists that has the method name `DescribeNativeComponentFrameRoot` in
* it for both control and sample stacks.
*
* Note that we're using a class method here instead of a plain object
* property to prevent Closure compiler from eliding away the object and the
* extra method call.
*/
class RunInRootFrame {
constructor() {
// Bun and Safari will require setting these properties should the class
// ever get transpiled into an ES2015 constructor function.
// $FlowFixMe[method-unbinding]
this.DetermineComponentFrameRoot.displayName =
'DetermineComponentFrameRoot';

// Before ES6, the `name` property was not configurable.
if (
// $FlowFixMe[method-unbinding]
Object.getOwnPropertyDescriptor(this.DetermineComponentFrameRoot)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm should this be Object.getOwnPropertyDescriptor(this.DetermineComponentFrameRoot, 'name')?

Also JS q -- if the name property is not configurable, does that mean it should already hold the value DetermineComponentFrameRoot?

Copy link
Contributor Author

@KarimP KarimP Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh yup, I should specify 'name' there!

the name property is not configurable, does that mean it should already hold the value DetermineComponentFrameRoot

I think it depends on the JS VM? But yes for most the name property should automatically be set to the function name defined in code. However setting both name and displayName dynamically in code can account for any post-processing that might get done to the code. For example, it's possible for:

RunInRootFrame.DetermineComponentFrameRoot =
  function DetermineComponentFrameRoot() {
    ...
  };

to get minified to:

a.DetermineComponentFrameRoot = function b() {
  ...
};

In this case, I think most VMs would set the name property here to "b". Pre-ES6 it wouldn't be possible to change that. ES6 onwards the name property is not writable (so we can't directly set it via property access), but is configurable, so we can change it via Object.defineProperty. Also some VMs might specify both the function name and method name in stack traces, so e.g. V8 would have a stack trace line like:

    at a.b [as DetermineComponentFrameRoot]

But it's not guaranteed for all VMs.

?.configurable
) {
// Configurable properties can be updated even if its writable
// descriptor is set to `false`. V8 utilizes a function's `name`
// property when generating a stack trace.
// $FlowFixMe[method-unbinding]
Object.defineProperty(this.DetermineComponentFrameRoot, 'name', {
value: 'DetermineComponentFrameRoot',
});
}
} else {
}

DetermineComponentFrameRoot(): [?string, ?string] {
let control;
try {
throw Error();
} catch (x) {
control = x;
}
// TODO(luna): This will currently only throw if the function component
// tries to access React/ReactDOM/props. We should probably make this throw
// in simple components too
const maybePromise = fn();
// This should throw.
if (construct) {
// Something should be setting the props in the constructor.
const Fake = function () {
throw Error();
};
// $FlowFixMe[prop-missing]
Object.defineProperty(Fake.prototype, 'props', {
set: function () {
// We use a throwing setter instead of frozen or non-writable props
// because that won't throw in a non-strict mode function.
throw Error();
},
});
if (typeof Reflect === 'object' && Reflect.construct) {
// We construct a different control for this case to include any extra
// frames added by the construct call.
try {
Reflect.construct(Fake, []);
} catch (x) {
control = x;
}
Reflect.construct(fn, [], Fake);
} else {
try {
Fake.call();
} catch (x) {
control = x;
}
// $FlowFixMe[prop-missing] found when upgrading Flow
fn.call(Fake.prototype);
}
} else {
try {
throw Error();
} catch (x) {
control = x;
}
// TODO(luna): This will currently only throw if the function component
// tries to access React/ReactDOM/props. We should probably make this throw
// in simple components too
const maybePromise = fn();

// If the function component returns a promise, it's likely an async
// component, which we don't yet support. Attach a noop catch handler to
// silence the error.
// TODO: Implement component stacks for async client components?
if (maybePromise && typeof maybePromise.catch === 'function') {
maybePromise.catch(() => {});
// If the function component returns a promise, it's likely an async
// component, which we don't yet support. Attach a noop catch handler to
// silence the error.
// TODO: Implement component stacks for async client components?
if (maybePromise && typeof maybePromise.catch === 'function') {
maybePromise.catch(() => {});
}
}
} catch (sample) {
// This is inlined manually because closure doesn't do it for us.
if (sample && control && typeof sample.stack === 'string') {
return [sample.stack, control.stack];
}
}
return [null, null];
}
} catch (sample) {
// This is inlined manually because closure doesn't do it for us.
if (sample && control && typeof sample.stack === 'string') {
}

try {
const [sampleStack, controlStack] =
new RunInRootFrame().DetermineComponentFrameRoot();
if (sampleStack && controlStack) {
// This extracts the first frame from the sample that isn't also in the control.
// Skipping one frame that we assume is the frame that calls the two.
const sampleLines = sample.stack.split('\n');
const controlLines = control.stack.split('\n');
let s = sampleLines.length - 1;
let c = controlLines.length - 1;
while (s >= 1 && c >= 0 && sampleLines[s] !== controlLines[c]) {
// We expect at least one stack frame to be shared.
// Typically this will be the root most one. However, stack frames may be
// cut off due to maximum stack limits. In this case, one maybe cut off
// earlier than the other. We assume that the sample is longer or the same
// and there for cut off earlier. So we should find the root most frame in
// the sample somewhere in the control.
c--;
const sampleLines = sampleStack.split('\n');
const controlLines = controlStack.split('\n');
let s = sampleLines.findIndex(line =>
line.includes('DetermineComponentFrameRoot'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I’d find some alternative methods to use here because newer methods tend to be slower. We don’t use the names elsewhere so compress worse. We also convert this away from arrow function so it’ll be longer than it looks.

There’s also the risk of older environments not supporting it.

Maybe just a plain loop.

);
let c = controlLines.findIndex(line =>
line.includes('DetermineComponentFrameRoot'),
);
if (s === -1 || c === -1) {
s = sampleLines.length - 1;
c = controlLines.length - 1;
while (s >= 1 && c >= 0 && sampleLines[s] !== controlLines[c]) {
// We expect at least one stack frame to be shared.
// Typically this will be the root most one. However, stack frames may be
// cut off due to maximum stack limits. In this case, one maybe cut off
// earlier than the other. We assume that the sample is longer or the same
// and there for cut off earlier. So we should find the root most frame in
// the sample somewhere in the control.
c--;
}
}
for (; s >= 1 && c >= 0; s--, c--) {
// Next we find the first one that isn't the same which should be the
Expand Down