Skip to content

Commit 72d00ab

Browse files
sebmarkbagesebmarkbage
authored
Fix Component Stacks for IE and Native Classes in Safari (facebook#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.
1 parent 98d410f commit 72d00ab

13 files changed

+187
-71
lines changed

‎fixtures/stacks/BabelClass-compiled.js

Lines changed: 0 additions & 25 deletions
This file was deleted.

‎fixtures/stacks/BabelClass-compiled.js.map

Lines changed: 0 additions & 1 deletion
This file was deleted.

‎fixtures/stacks/BabelClass.js

Lines changed: 0 additions & 8 deletions
This file was deleted.

‎fixtures/stacks/BabelClasses-compiled.js

Lines changed: 80 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎fixtures/stacks/BabelClasses-compiled.js.map

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎fixtures/stacks/BabelClasses.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// Compile this with Babel.
2+
// babel --config-file ./babel.config.json BabelClasses.js --out-file BabelClasses-compiled.js --source-maps
3+
4+
class BabelClass extends React.Component {
5+
render() {
6+
return this.props.children;
7+
}
8+
}
9+
10+
class BabelClassWithFields extends React.Component {
11+
// These compile to defineProperty which can break some interception techniques.
12+
props;
13+
state = {};
14+
render() {
15+
return this.props.children;
16+
}
17+
}

‎fixtures/stacks/Component.js renamed to ‎fixtures/stacks/Components.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,13 @@ class NativeClass extends React.Component {
1818
return this.props.children;
1919
}
2020
}
21+
22+
class FrozenClass extends React.Component {
23+
constructor() {
24+
super();
25+
}
26+
render() {
27+
return this.props.children;
28+
}
29+
}
30+
Object.freeze(FrozenClass.prototype);

‎fixtures/stacks/Example.js

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,20 @@ function Example() {
4444
NativeClass,
4545
null,
4646
x(
47-
BabelClass,
47+
FrozenClass,
4848
null,
4949
x(
50-
React.Suspense,
50+
BabelClass,
5151
null,
52-
x('div', null, x(Component, null, x(Throw)))
52+
x(
53+
BabelClassWithFields,
54+
null,
55+
x(
56+
React.Suspense,
57+
null,
58+
x('div', null, x(Component, null, x(Throw)))
59+
)
60+
)
5361
)
5462
)
5563
)

‎fixtures/stacks/babel.config.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
{
22
"plugins": [
3+
["@babel/plugin-proposal-class-properties", {"loose": false}],
34
["@babel/plugin-transform-classes", {"loose": true}]
45
]
56
}

‎fixtures/stacks/index.html

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,25 +27,26 @@
2727
</div>
2828
<script src="../../build/node_modules/react/umd/react.production.min.js"></script>
2929
<script src="../../build/node_modules/react-dom/umd/react-dom.production.min.js"></script>
30-
<script src="./Component.js"></script>
31-
<script src="./BabelClass-compiled.js"></script>
30+
<script src="./Components.js"></script>
31+
<script src="./BabelClasses-compiled.js"></script>
3232
<script src="./Example.js"></script>
3333
<script>
3434
const container = document.getElementById("container");
3535
ReactDOM.render(React.createElement(Example), container);
3636
</script>
3737
<h3>The above stack should look something like this:</h3>
3838
<pre>
39-
4039
at Lazy
41-
at Component (/stacks/Component.js:7:50)
40+
at Component (/stacks/Component.js:7:1)
4241
at div
4342
at Suspense
43+
at BabelClassWithFields (/stacks/BabelClasses-compiled.js:31:31)
4444
at BabelClass (/stacks/BabelClass-compiled.js:13:29)
45+
at FrozenClass (/stacks/Components.js:22:1)
4546
at NativeClass (/stacks/Component.js:16:1)
4647
at SuspenseList
47-
at Custom Name (/stacks/Component.js:11:23)
48+
at Custom Name (/stacks/Component.js:11:1)
4849
at ErrorBoundary (/stacks/Example.js:5:1)
49-
at Example (/stacks/Example.js:33:21)</pre>
50+
at Example (/stacks/Example.js:32:1)</pre>
5051
</body>
5152
</html>

‎packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2440,13 +2440,11 @@ describe('ReactErrorBoundaries', () => {
24402440
);
24412441
});
24422442

2443-
it('should catch errors from errors in the throw phase from errors', () => {
2443+
it('should protect errors from errors in the stack generation', () => {
24442444
const container = document.createElement('div');
24452445

24462446
const evilError = {
2447-
get message() {
2448-
throw new Error('gotta catch em all');
2449-
},
2447+
message: 'gotta catch em all',
24502448
get stack() {
24512449
throw new Error('gotta catch em all');
24522450
},

‎packages/react-reconciler/src/ReactFiberComponentStack.js

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,15 @@ function describeFiber(fiber: Fiber): string {
5959
}
6060

6161
export function getStackByFiberInDevAndProd(workInProgress: Fiber): string {
62-
let info = '';
63-
let node = workInProgress;
64-
do {
65-
info += describeFiber(node);
66-
node = node.return;
67-
} while (node);
68-
return info;
62+
try {
63+
let info = '';
64+
let node = workInProgress;
65+
do {
66+
info += describeFiber(node);
67+
node = node.return;
68+
} while (node);
69+
return info;
70+
} catch (x) {
71+
return '\nError generating stack: ' + x.message + '\n' + x.stack;
72+
}
6973
}

‎packages/shared/ReactComponentStackFrame.js

Lines changed: 47 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,12 @@ export function describeBuiltInComponentFrame(
3636
if (enableComponentStackLocations) {
3737
if (prefix === undefined) {
3838
// Extract the VM specific prefix used by each line.
39-
const match = Error()
40-
.stack.trim()
41-
.match(/\n( *(at )?)/);
42-
prefix = (match && match[1]) || '';
39+
try {
40+
throw Error();
41+
} catch (x) {
42+
const match = x.stack.trim().match(/\n( *(at )?)/);
43+
prefix = (match && match[1]) || '';
44+
}
4345
}
4446
// We use the prefix to ensure our stacks line up with native stack frames.
4547
return '\n' + prefix + name;
@@ -75,7 +77,7 @@ export function describeNativeComponentFrame(
7577
}
7678
}
7779

78-
const control = Error();
80+
let control;
7981

8082
reentry = true;
8183
let previousDispatcher;
@@ -90,7 +92,9 @@ export function describeNativeComponentFrame(
9092
// This should throw.
9193
if (construct) {
9294
// Something should be setting the props in the constructor.
93-
const Fake = function() {};
95+
const Fake = function() {
96+
throw Error();
97+
};
9498
// $FlowFixMe
9599
Object.defineProperty(Fake.prototype, 'props', {
96100
set: function() {
@@ -100,16 +104,33 @@ export function describeNativeComponentFrame(
100104
},
101105
});
102106
if (typeof Reflect === 'object' && Reflect.construct) {
107+
// We construct a different control for this case to include any extra
108+
// frames added by the construct call.
109+
try {
110+
Reflect.construct(Fake, []);
111+
} catch (x) {
112+
control = x;
113+
}
103114
Reflect.construct(fn, [], Fake);
104115
} else {
105-
fn.call(new Fake());
116+
try {
117+
Fake.call();
118+
} catch (x) {
119+
control = x;
120+
}
121+
fn.call(Fake.prototype);
106122
}
107123
} else {
124+
try {
125+
throw Error();
126+
} catch (x) {
127+
control = x;
128+
}
108129
fn();
109130
}
110131
} catch (sample) {
111132
// This is inlined manually because closure doesn't do it for us.
112-
if (sample && typeof sample.stack === 'string') {
133+
if (sample && control && typeof sample.stack === 'string') {
113134
// This extracts the first frame from the sample that isn't also in the control.
114135
// Skipping one frame that we assume is the frame that calls the two.
115136
const sampleLines = sample.stack.split('\n');
@@ -127,24 +148,33 @@ export function describeNativeComponentFrame(
127148
}
128149
for (; s >= 1 && c >= 0; s--, c--) {
129150
// Next we find the first one that isn't the same which should be the
130-
// frame that called our sample function.
151+
// frame that called our sample function and the control.
131152
if (sampleLines[s] !== controlLines[c]) {
132153
// In V8, the first line is describing the message but other VMs don't.
133154
// If we're about to return the first line, and the control is also on the same
134155
// line, that's a pretty good indicator that our sample threw at same line as
135156
// the control. I.e. before we entered the sample frame. So we ignore this result.
136157
// This can happen if you passed a class to function component, or non-function.
137158
if (s !== 1 || c !== 1) {
138-
// V8 adds a "new" prefix for native classes. Let's remove it to make it prettier.
139-
const frame = '\n' + sampleLines[s - 1].replace(' at new ', ' at ');
140-
if (__DEV__) {
141-
if (typeof fn === 'function') {
142-
componentFrameCache.set(fn, frame);
159+
do {
160+
s--;
161+
c--;
162+
// We may still have similar intermediate frames from the construct call.
163+
// The next one that isn't the same should be our match though.
164+
if (c < 0 || sampleLines[s] !== controlLines[c]) {
165+
// V8 adds a "new" prefix for native classes. Let's remove it to make it prettier.
166+
const frame = '\n' + sampleLines[s].replace(' at new ', ' at ');
167+
if (__DEV__) {
168+
if (typeof fn === 'function') {
169+
componentFrameCache.set(fn, frame);
170+
}
171+
}
172+
// Return the line we found.
173+
return frame;
143174
}
144-
}
145-
// Return the line we found.
146-
return frame;
175+
} while (s >= 1 && c >= 0);
147176
}
177+
break;
148178
}
149179
}
150180
}

0 commit comments

Comments
 (0)