From 72d00ab623502983ebd7ac0756cf2787df109811 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 10 Apr 2020 19:39:02 -0700 Subject: [PATCH] Fix Component Stacks for IE and Native Classes in Safari (#18575) * Add more edge cases to fixture Also adjust some expectations. I think the column should ideally be 1 but varies. The Example row is one line off because it throws on the hook but should ideally be the component. Similarly class components with constructors may have the line in the constructor. * Account for the construct call taking a stack frame We do this by first searching for the first different frame, then find the same frames and then find the first different frame again. * Throw controls Otherwise they don't get a stack frame associated with them in IE. * Protect against generating stacks failing Errors while generating stacks will bubble to the root. Since this technique is a bit sketchy, we should probably protect against it. * Don't construct the thing that throws Instead, we pass the prototype as the "this". It's new every time anyway. --- fixtures/stacks/BabelClass-compiled.js | 25 ------ fixtures/stacks/BabelClass-compiled.js.map | 1 - fixtures/stacks/BabelClass.js | 8 -- fixtures/stacks/BabelClasses-compiled.js | 80 +++++++++++++++++++ fixtures/stacks/BabelClasses-compiled.js.map | 1 + fixtures/stacks/BabelClasses.js | 17 ++++ .../stacks/{Component.js => Components.js} | 10 +++ fixtures/stacks/Example.js | 14 +++- fixtures/stacks/babel.config.json | 1 + fixtures/stacks/index.html | 13 +-- .../ReactErrorBoundaries-test.internal.js | 6 +- .../src/ReactFiberComponentStack.js | 18 +++-- packages/shared/ReactComponentStackFrame.js | 64 +++++++++++---- 13 files changed, 187 insertions(+), 71 deletions(-) delete mode 100644 fixtures/stacks/BabelClass-compiled.js delete mode 100644 fixtures/stacks/BabelClass-compiled.js.map delete mode 100644 fixtures/stacks/BabelClass.js create mode 100644 fixtures/stacks/BabelClasses-compiled.js create mode 100644 fixtures/stacks/BabelClasses-compiled.js.map create mode 100644 fixtures/stacks/BabelClasses.js rename fixtures/stacks/{Component.js => Components.js} (67%) diff --git a/fixtures/stacks/BabelClass-compiled.js b/fixtures/stacks/BabelClass-compiled.js deleted file mode 100644 index dc1424e138e5..000000000000 --- a/fixtures/stacks/BabelClass-compiled.js +++ /dev/null @@ -1,25 +0,0 @@ -function _inheritsLoose(subClass, superClass) { - subClass.prototype = Object.create(superClass.prototype); - subClass.prototype.constructor = subClass; - subClass.__proto__ = superClass; -} - -// Compile this with Babel. -// babel --config-file ./babel.config.json BabelClass.js --out-file BabelClass-compiled.js --source-maps -let BabelClass = /*#__PURE__*/ (function(_React$Component) { - _inheritsLoose(BabelClass, _React$Component); - - function BabelClass() { - return _React$Component.apply(this, arguments) || this; - } - - var _proto = BabelClass.prototype; - - _proto.render = function render() { - return this.props.children; - }; - - return BabelClass; -})(React.Component); - -//# sourceMappingURL=BabelClass-compiled.js.map diff --git a/fixtures/stacks/BabelClass-compiled.js.map b/fixtures/stacks/BabelClass-compiled.js.map deleted file mode 100644 index b36937026212..000000000000 --- a/fixtures/stacks/BabelClass-compiled.js.map +++ /dev/null @@ -1 +0,0 @@ -{"version":3,"sources":["BabelClass.js"],"names":[],"mappings":";;AAAA;AACA;IAEM,U;;;;;;;;;SACJ,M,GAAA,kBAAS;AACP,WAAO,KAAK,KAAL,CAAW,QAAlB;AACD,G;;;EAHsB,KAAK,CAAC,S","file":"BabelClass-compiled.js","sourcesContent":["// Compile this with Babel.\n// babel --config-file ./babel.config.json BabelClass.js --out-file BabelClass-compiled.js --source-maps\n\nclass BabelClass extends React.Component {\n render() {\n return this.props.children;\n }\n}\n"]} \ No newline at end of file diff --git a/fixtures/stacks/BabelClass.js b/fixtures/stacks/BabelClass.js deleted file mode 100644 index d76637c1ae89..000000000000 --- a/fixtures/stacks/BabelClass.js +++ /dev/null @@ -1,8 +0,0 @@ -// Compile this with Babel. -// babel --config-file ./babel.config.json BabelClass.js --out-file BabelClass-compiled.js --source-maps - -class BabelClass extends React.Component { - render() { - return this.props.children; - } -} diff --git a/fixtures/stacks/BabelClasses-compiled.js b/fixtures/stacks/BabelClasses-compiled.js new file mode 100644 index 000000000000..7350dba8eff9 --- /dev/null +++ b/fixtures/stacks/BabelClasses-compiled.js @@ -0,0 +1,80 @@ +function _assertThisInitialized(self) { + if (self === void 0) { + throw new ReferenceError( + "this hasn't been initialised - super() hasn't been called" + ); + } + return self; +} + +function _defineProperty(obj, key, value) { + if (key in obj) { + Object.defineProperty(obj, key, { + value: value, + enumerable: true, + configurable: true, + writable: true, + }); + } else { + obj[key] = value; + } + return obj; +} + +function _inheritsLoose(subClass, superClass) { + subClass.prototype = Object.create(superClass.prototype); + subClass.prototype.constructor = subClass; + subClass.__proto__ = superClass; +} + +// Compile this with Babel. +// babel --config-file ./babel.config.json BabelClasses.js --out-file BabelClasses-compiled.js --source-maps +let BabelClass = /*#__PURE__*/ (function(_React$Component) { + _inheritsLoose(BabelClass, _React$Component); + + function BabelClass() { + return _React$Component.apply(this, arguments) || this; + } + + var _proto = BabelClass.prototype; + + _proto.render = function render() { + return this.props.children; + }; + + return BabelClass; +})(React.Component); + +let BabelClassWithFields = /*#__PURE__*/ (function(_React$Component2) { + _inheritsLoose(BabelClassWithFields, _React$Component2); + + function BabelClassWithFields(...args) { + var _this; + + _this = _React$Component2.call(this, ...args) || this; + + _defineProperty( + _assertThisInitialized(_assertThisInitialized(_this)), + 'props', + void 0 + ); + + _defineProperty( + _assertThisInitialized(_assertThisInitialized(_this)), + 'state', + {} + ); + + return _this; + } + + var _proto2 = BabelClassWithFields.prototype; + + _proto2.render = function render() { + return this.props.children; + }; + + return BabelClassWithFields; +})(React.Component); + +//# sourceMappingURL=BabelClasses-compiled.js.map diff --git a/fixtures/stacks/BabelClasses-compiled.js.map b/fixtures/stacks/BabelClasses-compiled.js.map new file mode 100644 index 000000000000..f80429608ee9 --- /dev/null +++ b/fixtures/stacks/BabelClasses-compiled.js.map @@ -0,0 +1 @@ +{"version":3,"sources":["BabelClasses.js"],"names":[],"mappings":";;;;;;AAAA;AACA;IAEM,U;;;;;;;;;SACJ,M,qBAAS;AACP,WAAO,KAAK,KAAL,CAAW,QAAlB;AACD,G;;;EAHsB,KAAK,CAAC,S;;IAMzB,oB;;;;;;;;;;oFAGI,E;;;;;;;UACR,M,qBAAS;AACP,WAAO,KAAK,KAAL,CAAW,QAAlB;AACD,G;;;EANgC,KAAK,CAAC,S","file":"BabelClasses-compiled.js","sourcesContent":["// Compile this with Babel.\n// babel --config-file ./babel.config.json BabelClasses.js --out-file BabelClasses-compiled.js --source-maps\n\nclass BabelClass extends React.Component {\n render() {\n return this.props.children;\n }\n}\n\nclass BabelClassWithFields extends React.Component {\n // These compile to defineProperty which can break some interception techniques.\n props;\n state = {};\n render() {\n return this.props.children;\n }\n}\n"]} \ No newline at end of file diff --git a/fixtures/stacks/BabelClasses.js b/fixtures/stacks/BabelClasses.js new file mode 100644 index 000000000000..be39c388c42d --- /dev/null +++ b/fixtures/stacks/BabelClasses.js @@ -0,0 +1,17 @@ +// Compile this with Babel. +// babel --config-file ./babel.config.json BabelClasses.js --out-file BabelClasses-compiled.js --source-maps + +class BabelClass extends React.Component { + render() { + return this.props.children; + } +} + +class BabelClassWithFields extends React.Component { + // These compile to defineProperty which can break some interception techniques. + props; + state = {}; + render() { + return this.props.children; + } +} diff --git a/fixtures/stacks/Component.js b/fixtures/stacks/Components.js similarity index 67% rename from fixtures/stacks/Component.js rename to fixtures/stacks/Components.js index b4cd8a00123a..1a2159e7e190 100644 --- a/fixtures/stacks/Component.js +++ b/fixtures/stacks/Components.js @@ -18,3 +18,13 @@ class NativeClass extends React.Component { return this.props.children; } } + +class FrozenClass extends React.Component { + constructor() { + super(); + } + render() { + return this.props.children; + } +} +Object.freeze(FrozenClass.prototype); diff --git a/fixtures/stacks/Example.js b/fixtures/stacks/Example.js index 086934c0d9e1..3ea0ba14c489 100644 --- a/fixtures/stacks/Example.js +++ b/fixtures/stacks/Example.js @@ -44,12 +44,20 @@ function Example() { NativeClass, null, x( - BabelClass, + FrozenClass, null, x( - React.Suspense, + BabelClass, null, - x('div', null, x(Component, null, x(Throw))) + x( + BabelClassWithFields, + null, + x( + React.Suspense, + null, + x('div', null, x(Component, null, x(Throw))) + ) + ) ) ) ) diff --git a/fixtures/stacks/babel.config.json b/fixtures/stacks/babel.config.json index 1b89e92c4b32..97daef5d20a1 100644 --- a/fixtures/stacks/babel.config.json +++ b/fixtures/stacks/babel.config.json @@ -1,5 +1,6 @@ { "plugins": [ + ["@babel/plugin-proposal-class-properties", {"loose": false}], ["@babel/plugin-transform-classes", {"loose": true}] ] } diff --git a/fixtures/stacks/index.html b/fixtures/stacks/index.html index f4ee24472a82..316ac32722f4 100644 --- a/fixtures/stacks/index.html +++ b/fixtures/stacks/index.html @@ -27,8 +27,8 @@ - - + +

The above stack should look something like this:

-
     at Lazy
-    at Component (/stacks/Component.js:7:50)
+    at Component (/stacks/Component.js:7:1)
     at div
     at Suspense
+    at BabelClassWithFields (/stacks/BabelClasses-compiled.js:31:31)
     at BabelClass (/stacks/BabelClass-compiled.js:13:29)
+    at FrozenClass (/stacks/Components.js:22:1)
     at NativeClass (/stacks/Component.js:16:1)
     at SuspenseList
-    at Custom Name (/stacks/Component.js:11:23)
+    at Custom Name (/stacks/Component.js:11:1)
     at ErrorBoundary (/stacks/Example.js:5:1)
-    at Example (/stacks/Example.js:33:21)
+ at Example (/stacks/Example.js:32:1) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 81427c97eb05..06f51b0f3974 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -2440,13 +2440,11 @@ describe('ReactErrorBoundaries', () => { ); }); - it('should catch errors from errors in the throw phase from errors', () => { + it('should protect errors from errors in the stack generation', () => { const container = document.createElement('div'); const evilError = { - get message() { - throw new Error('gotta catch em all'); - }, + message: 'gotta catch em all', get stack() { throw new Error('gotta catch em all'); }, diff --git a/packages/react-reconciler/src/ReactFiberComponentStack.js b/packages/react-reconciler/src/ReactFiberComponentStack.js index 5867d4e9be6b..b539ee795084 100644 --- a/packages/react-reconciler/src/ReactFiberComponentStack.js +++ b/packages/react-reconciler/src/ReactFiberComponentStack.js @@ -59,11 +59,15 @@ function describeFiber(fiber: Fiber): string { } export function getStackByFiberInDevAndProd(workInProgress: Fiber): string { - let info = ''; - let node = workInProgress; - do { - info += describeFiber(node); - node = node.return; - } while (node); - return info; + try { + let info = ''; + let node = workInProgress; + do { + info += describeFiber(node); + node = node.return; + } while (node); + return info; + } catch (x) { + return '\nError generating stack: ' + x.message + '\n' + x.stack; + } } diff --git a/packages/shared/ReactComponentStackFrame.js b/packages/shared/ReactComponentStackFrame.js index f3c80bc1ce0c..bc8a76fba236 100644 --- a/packages/shared/ReactComponentStackFrame.js +++ b/packages/shared/ReactComponentStackFrame.js @@ -36,10 +36,12 @@ export function describeBuiltInComponentFrame( if (enableComponentStackLocations) { if (prefix === undefined) { // Extract the VM specific prefix used by each line. - const match = Error() - .stack.trim() - .match(/\n( *(at )?)/); - prefix = (match && match[1]) || ''; + try { + throw Error(); + } catch (x) { + const match = x.stack.trim().match(/\n( *(at )?)/); + prefix = (match && match[1]) || ''; + } } // We use the prefix to ensure our stacks line up with native stack frames. return '\n' + prefix + name; @@ -75,7 +77,7 @@ export function describeNativeComponentFrame( } } - const control = Error(); + let control; reentry = true; let previousDispatcher; @@ -90,7 +92,9 @@ export function describeNativeComponentFrame( // This should throw. if (construct) { // Something should be setting the props in the constructor. - const Fake = function() {}; + const Fake = function() { + throw Error(); + }; // $FlowFixMe Object.defineProperty(Fake.prototype, 'props', { set: function() { @@ -100,16 +104,33 @@ export function describeNativeComponentFrame( }, }); 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 { - fn.call(new Fake()); + try { + Fake.call(); + } catch (x) { + control = x; + } + fn.call(Fake.prototype); } } else { + try { + throw Error(); + } catch (x) { + control = x; + } fn(); } } catch (sample) { // This is inlined manually because closure doesn't do it for us. - if (sample && typeof sample.stack === 'string') { + if (sample && control && typeof sample.stack === 'string') { // 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'); @@ -127,7 +148,7 @@ export function describeNativeComponentFrame( } for (; s >= 1 && c >= 0; s--, c--) { // Next we find the first one that isn't the same which should be the - // frame that called our sample function. + // frame that called our sample function and the control. if (sampleLines[s] !== controlLines[c]) { // In V8, the first line is describing the message but other VMs don't. // If we're about to return the first line, and the control is also on the same @@ -135,16 +156,25 @@ export function describeNativeComponentFrame( // the control. I.e. before we entered the sample frame. So we ignore this result. // This can happen if you passed a class to function component, or non-function. if (s !== 1 || c !== 1) { - // V8 adds a "new" prefix for native classes. Let's remove it to make it prettier. - const frame = '\n' + sampleLines[s - 1].replace(' at new ', ' at '); - if (__DEV__) { - if (typeof fn === 'function') { - componentFrameCache.set(fn, frame); + do { + s--; + c--; + // We may still have similar intermediate frames from the construct call. + // The next one that isn't the same should be our match though. + if (c < 0 || sampleLines[s] !== controlLines[c]) { + // V8 adds a "new" prefix for native classes. Let's remove it to make it prettier. + const frame = '\n' + sampleLines[s].replace(' at new ', ' at '); + if (__DEV__) { + if (typeof fn === 'function') { + componentFrameCache.set(fn, frame); + } + } + // Return the line we found. + return frame; } - } - // Return the line we found. - return frame; + } while (s >= 1 && c >= 0); } + break; } } }