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

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 3 commits
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
193 changes: 128 additions & 65 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,141 @@ 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;

/**
* 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 object method:
* Having both the sample and control errors be in the function under the
* `DescribeNativeComponentFrameRoot` property, + setting the `name` and
* `displayName` properties of the function ensures that a stack
* frame exists that has the method name `DescribeNativeComponentFrameRoot` in
* it for both control and sample stacks.
*/
const RunInRootFrame = {
DetermineComponentFrameRoot(): [?string, ?string] {
let control;
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);
}
} 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(() => {});
}
}
Reflect.construct(fn, [], Fake);
} else {
try {
Fake.call();
} catch (x) {
control = x;
} 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];
}
// $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();
return [null, null];
},
};
// $FlowFixMe[prop-missing]
RunInRootFrame.DetermineComponentFrameRoot.displayName =
'DetermineComponentFrameRoot';
const namePropDescriptor = Object.getOwnPropertyDescriptor(
RunInRootFrame.DetermineComponentFrameRoot,
'name',
);
// Before ES6, the `name` property was not configurable.
if (namePropDescriptor && namePropDescriptor.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.
Object.defineProperty(
RunInRootFrame.DetermineComponentFrameRoot,
// $FlowFixMe[cannot-write]
'name',
{value: 'DetermineComponentFrameRoot'},
);
}

// 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') {
try {
const [sampleStack, controlStack] =
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