From 9cc71f5149a242d55e3a80afe68d1b92c3fab561 Mon Sep 17 00:00:00 2001 From: Karim Piyar Ali Date: Wed, 12 Jul 2023 18:59:37 -0700 Subject: [PATCH 1/5] Update stack diffing algorithm in describeNativeComponentFrame - Attempt have a predefined "root" or common frame between sample and control frames --- packages/shared/ReactComponentStackFrame.js | 198 +++++++++++++------- 1 file changed, 134 insertions(+), 64 deletions(-) diff --git a/packages/shared/ReactComponentStackFrame.js b/packages/shared/ReactComponentStackFrame.js index 9df78e0fae465..634018b3a82b9 100644 --- a/packages/shared/ReactComponentStackFrame.js +++ b/packages/shared/ReactComponentStackFrame.js @@ -60,6 +60,17 @@ if (__DEV__) { componentFrameCache = new PossiblyWeakMap(); } +/** + * 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, @@ -76,13 +87,12 @@ 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 @@ -90,75 +100,135 @@ export function describeNativeComponentFrame( 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) + ?.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'), + ); + 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 From bafd70f3b2985526ba44bf66e4aaef5d8cafa254 Mon Sep 17 00:00:00 2001 From: Karim Piyar Ali Date: Wed, 19 Jul 2023 16:00:56 -0700 Subject: [PATCH 2/5] Fix typo in `Object.getOwnPropertyDescriptor` call --- packages/shared/ReactComponentStackFrame.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/shared/ReactComponentStackFrame.js b/packages/shared/ReactComponentStackFrame.js index 634018b3a82b9..f5981971bbfe7 100644 --- a/packages/shared/ReactComponentStackFrame.js +++ b/packages/shared/ReactComponentStackFrame.js @@ -125,9 +125,11 @@ export function describeNativeComponentFrame( // Before ES6, the `name` property was not configurable. if ( - // $FlowFixMe[method-unbinding] - Object.getOwnPropertyDescriptor(this.DetermineComponentFrameRoot) - ?.configurable + Object.getOwnPropertyDescriptor( + // $FlowFixMe[method-unbinding] + this.DetermineComponentFrameRoot, + 'name', + )?.configurable ) { // Configurable properties can be updated even if its writable // descriptor is set to `false`. V8 utilizes a function's `name` From 364938d474ef327ea92b7f298cb757ae28d34a07 Mon Sep 17 00:00:00 2001 From: Karim Piyar Ali Date: Wed, 19 Jul 2023 16:02:18 -0700 Subject: [PATCH 3/5] Switch to having `DetermineComponentFrameRoot` be an object property --- packages/shared/ReactComponentStackFrame.js | 61 +++++++++------------ 1 file changed, 26 insertions(+), 35 deletions(-) diff --git a/packages/shared/ReactComponentStackFrame.js b/packages/shared/ReactComponentStackFrame.js index f5981971bbfe7..7be997cb8ab4d 100644 --- a/packages/shared/ReactComponentStackFrame.js +++ b/packages/shared/ReactComponentStackFrame.js @@ -105,42 +105,14 @@ export function describeNativeComponentFrame( * 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 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. - * - * 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 ( - Object.getOwnPropertyDescriptor( - // $FlowFixMe[method-unbinding] - this.DetermineComponentFrameRoot, - 'name', - )?.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', - }); - } - } - + const RunInRootFrame = { DetermineComponentFrameRoot(): [?string, ?string] { let control; try { @@ -202,12 +174,31 @@ export function describeNativeComponentFrame( } } 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'}, + ); } try { const [sampleStack, controlStack] = - new RunInRootFrame().DetermineComponentFrameRoot(); + 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. From 8476ffa1b13dbbf4883758c9588c87766672b0b6 Mon Sep 17 00:00:00 2001 From: Karim Piyar Ali Date: Thu, 21 Sep 2023 14:40:09 -0700 Subject: [PATCH 4/5] Switch from `findIndex` to a while loop --- packages/shared/ReactComponentStackFrame.js | 25 ++++++++++++--------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/shared/ReactComponentStackFrame.js b/packages/shared/ReactComponentStackFrame.js index 7be997cb8ab4d..b76d0303b89ed 100644 --- a/packages/shared/ReactComponentStackFrame.js +++ b/packages/shared/ReactComponentStackFrame.js @@ -185,11 +185,11 @@ export function describeNativeComponentFrame( ); // 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. + // V8 utilizes a function's `name` property when generating a stack trace. Object.defineProperty( RunInRootFrame.DetermineComponentFrameRoot, + // Configurable properties can be updated even if its writable descriptor + // is set to `false`. // $FlowFixMe[cannot-write] 'name', {value: 'DetermineComponentFrameRoot'}, @@ -204,13 +204,18 @@ export function describeNativeComponentFrame( // Skipping one frame that we assume is the frame that calls the two. const sampleLines = sampleStack.split('\n'); const controlLines = controlStack.split('\n'); - let s = sampleLines.findIndex(line => - line.includes('DetermineComponentFrameRoot'), - ); - let c = controlLines.findIndex(line => - line.includes('DetermineComponentFrameRoot'), - ); - if (s === -1 || c === -1) { + let s = 0; + let c = 0; + while (s < sampleLines.length && !sampleLines[s].includes('DetermineComponentFrameRoot')) { + s++; + } + while (c < controlLines.length && !controlLines[c].includes('DetermineComponentFrameRoot')) { + c++; + } + // We couldn't find our intentionally injected common root frame, attempt + // to find another common root frame by search from the bottom of the + // control stack... + if (s === sampleLines.length || c === controlLines.length) { s = sampleLines.length - 1; c = controlLines.length - 1; while (s >= 1 && c >= 0 && sampleLines[s] !== controlLines[c]) { From 38abf48c9541efca320397469d9f834a89120a04 Mon Sep 17 00:00:00 2001 From: Karim Piyar Ali Date: Mon, 25 Sep 2023 15:32:38 -0400 Subject: [PATCH 5/5] Run prettier --- packages/shared/ReactComponentStackFrame.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/shared/ReactComponentStackFrame.js b/packages/shared/ReactComponentStackFrame.js index b76d0303b89ed..9738fcaa2d946 100644 --- a/packages/shared/ReactComponentStackFrame.js +++ b/packages/shared/ReactComponentStackFrame.js @@ -206,10 +206,16 @@ export function describeNativeComponentFrame( const controlLines = controlStack.split('\n'); let s = 0; let c = 0; - while (s < sampleLines.length && !sampleLines[s].includes('DetermineComponentFrameRoot')) { + while ( + s < sampleLines.length && + !sampleLines[s].includes('DetermineComponentFrameRoot') + ) { s++; } - while (c < controlLines.length && !controlLines[c].includes('DetermineComponentFrameRoot')) { + while ( + c < controlLines.length && + !controlLines[c].includes('DetermineComponentFrameRoot') + ) { c++; } // We couldn't find our intentionally injected common root frame, attempt