Skip to content

Commit b8ae38f

Browse files
sebmarkbagesebmarkbage
authored
Audit try/finally around console patching (facebook#31286)
Otherwise if something errors they can be left patched. [Review without whitespace](https://github.com/facebook/react/pull/31286/files?w=1)
1 parent 1ce58dd commit b8ae38f

File tree

4 files changed

+197
-184
lines changed

4 files changed

+197
-184
lines changed

‎packages/react-devtools-shared/src/backend/shared/DevToolsComponentStackFrame.js‎

Lines changed: 83 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -82,105 +82,104 @@ export function describeNativeComponentFrame(
8282
const previousDispatcher = currentDispatcherRef.H;
8383
currentDispatcherRef.H = null;
8484
disableLogs();
85+
try {
86+
// NOTE: keep in sync with the implementation in ReactComponentStackFrame
8587

86-
// NOTE: keep in sync with the implementation in ReactComponentStackFrame
87-
88-
/**
89-
* Finding a common stack frame between sample and control errors can be
90-
* tricky given the different types and levels of stack trace truncation from
91-
* different JS VMs. So instead we'll attempt to control what that common
92-
* frame should be through this object method:
93-
* Having both the sample and control errors be in the function under the
94-
* `DescribeNativeComponentFrameRoot` property, + setting the `name` and
95-
* `displayName` properties of the function ensures that a stack
96-
* frame exists that has the method name `DescribeNativeComponentFrameRoot` in
97-
* it for both control and sample stacks.
98-
*/
99-
const RunInRootFrame = {
100-
DetermineComponentFrameRoot(): [?string, ?string] {
101-
let control;
102-
try {
103-
// This should throw.
104-
if (construct) {
105-
// Something should be setting the props in the constructor.
106-
const Fake = function () {
107-
throw Error();
108-
};
109-
// $FlowFixMe[prop-missing]
110-
Object.defineProperty(Fake.prototype, 'props', {
111-
set: function () {
112-
// We use a throwing setter instead of frozen or non-writable props
113-
// because that won't throw in a non-strict mode function.
88+
/**
89+
* Finding a common stack frame between sample and control errors can be
90+
* tricky given the different types and levels of stack trace truncation from
91+
* different JS VMs. So instead we'll attempt to control what that common
92+
* frame should be through this object method:
93+
* Having both the sample and control errors be in the function under the
94+
* `DescribeNativeComponentFrameRoot` property, + setting the `name` and
95+
* `displayName` properties of the function ensures that a stack
96+
* frame exists that has the method name `DescribeNativeComponentFrameRoot` in
97+
* it for both control and sample stacks.
98+
*/
99+
const RunInRootFrame = {
100+
DetermineComponentFrameRoot(): [?string, ?string] {
101+
let control;
102+
try {
103+
// This should throw.
104+
if (construct) {
105+
// Something should be setting the props in the constructor.
106+
const Fake = function () {
114107
throw Error();
115-
},
116-
});
117-
if (typeof Reflect === 'object' && Reflect.construct) {
118-
// We construct a different control for this case to include any extra
119-
// frames added by the construct call.
120-
try {
121-
Reflect.construct(Fake, []);
122-
} catch (x) {
123-
control = x;
108+
};
109+
// $FlowFixMe[prop-missing]
110+
Object.defineProperty(Fake.prototype, 'props', {
111+
set: function () {
112+
// We use a throwing setter instead of frozen or non-writable props
113+
// because that won't throw in a non-strict mode function.
114+
throw Error();
115+
},
116+
});
117+
if (typeof Reflect === 'object' && Reflect.construct) {
118+
// We construct a different control for this case to include any extra
119+
// frames added by the construct call.
120+
try {
121+
Reflect.construct(Fake, []);
122+
} catch (x) {
123+
control = x;
124+
}
125+
Reflect.construct(fn, [], Fake);
126+
} else {
127+
try {
128+
Fake.call();
129+
} catch (x) {
130+
control = x;
131+
}
132+
// $FlowFixMe[prop-missing] found when upgrading Flow
133+
fn.call(Fake.prototype);
124134
}
125-
Reflect.construct(fn, [], Fake);
126135
} else {
127136
try {
128-
Fake.call();
137+
throw Error();
129138
} catch (x) {
130139
control = x;
131140
}
132-
// $FlowFixMe[prop-missing] found when upgrading Flow
133-
fn.call(Fake.prototype);
134-
}
135-
} else {
136-
try {
137-
throw Error();
138-
} catch (x) {
139-
control = x;
140-
}
141-
// TODO(luna): This will currently only throw if the function component
142-
// tries to access React/ReactDOM/props. We should probably make this throw
143-
// in simple components too
144-
const maybePromise = fn();
141+
// TODO(luna): This will currently only throw if the function component
142+
// tries to access React/ReactDOM/props. We should probably make this throw
143+
// in simple components too
144+
const maybePromise = fn();
145145

146-
// If the function component returns a promise, it's likely an async
147-
// component, which we don't yet support. Attach a noop catch handler to
148-
// silence the error.
149-
// TODO: Implement component stacks for async client components?
150-
if (maybePromise && typeof maybePromise.catch === 'function') {
151-
maybePromise.catch(() => {});
146+
// If the function component returns a promise, it's likely an async
147+
// component, which we don't yet support. Attach a noop catch handler to
148+
// silence the error.
149+
// TODO: Implement component stacks for async client components?
150+
if (maybePromise && typeof maybePromise.catch === 'function') {
151+
maybePromise.catch(() => {});
152+
}
153+
}
154+
} catch (sample) {
155+
// This is inlined manually because closure doesn't do it for us.
156+
if (sample && control && typeof sample.stack === 'string') {
157+
return [sample.stack, control.stack];
152158
}
153159
}
154-
} catch (sample) {
155-
// This is inlined manually because closure doesn't do it for us.
156-
if (sample && control && typeof sample.stack === 'string') {
157-
return [sample.stack, control.stack];
158-
}
159-
}
160-
return [null, null];
161-
},
162-
};
163-
// $FlowFixMe[prop-missing]
164-
RunInRootFrame.DetermineComponentFrameRoot.displayName =
165-
'DetermineComponentFrameRoot';
166-
const namePropDescriptor = Object.getOwnPropertyDescriptor(
167-
RunInRootFrame.DetermineComponentFrameRoot,
168-
'name',
169-
);
170-
// Before ES6, the `name` property was not configurable.
171-
if (namePropDescriptor && namePropDescriptor.configurable) {
172-
// V8 utilizes a function's `name` property when generating a stack trace.
173-
Object.defineProperty(
160+
return [null, null];
161+
},
162+
};
163+
// $FlowFixMe[prop-missing]
164+
RunInRootFrame.DetermineComponentFrameRoot.displayName =
165+
'DetermineComponentFrameRoot';
166+
const namePropDescriptor = Object.getOwnPropertyDescriptor(
174167
RunInRootFrame.DetermineComponentFrameRoot,
175-
// Configurable properties can be updated even if its writable descriptor
176-
// is set to `false`.
177-
// $FlowFixMe[cannot-write]
178168
'name',
179-
{value: 'DetermineComponentFrameRoot'},
180169
);
181-
}
170+
// Before ES6, the `name` property was not configurable.
171+
if (namePropDescriptor && namePropDescriptor.configurable) {
172+
// V8 utilizes a function's `name` property when generating a stack trace.
173+
Object.defineProperty(
174+
RunInRootFrame.DetermineComponentFrameRoot,
175+
// Configurable properties can be updated even if its writable descriptor
176+
// is set to `false`.
177+
// $FlowFixMe[cannot-write]
178+
'name',
179+
{value: 'DetermineComponentFrameRoot'},
180+
);
181+
}
182182

183-
try {
184183
const [sampleStack, controlStack] =
185184
RunInRootFrame.DetermineComponentFrameRoot();
186185
if (sampleStack && controlStack) {

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

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1299,8 +1299,11 @@ function mountReducer<S, I, A>(
12991299
initialState = init(initialArg);
13001300
if (shouldDoubleInvokeUserFnsInHooksDEV) {
13011301
setIsStrictModeForDevtools(true);
1302-
init(initialArg);
1303-
setIsStrictModeForDevtools(false);
1302+
try {
1303+
init(initialArg);
1304+
} finally {
1305+
setIsStrictModeForDevtools(false);
1306+
}
13041307
}
13051308
} else {
13061309
initialState = ((initialArg: any): S);
@@ -1900,9 +1903,12 @@ function mountStateImpl<S>(initialState: (() => S) | S): Hook {
19001903
initialState = initialStateInitializer();
19011904
if (shouldDoubleInvokeUserFnsInHooksDEV) {
19021905
setIsStrictModeForDevtools(true);
1903-
// $FlowFixMe[incompatible-use]: Flow doesn't like mixed types
1904-
initialStateInitializer();
1905-
setIsStrictModeForDevtools(false);
1906+
try {
1907+
// $FlowFixMe[incompatible-use]: Flow doesn't like mixed types
1908+
initialStateInitializer();
1909+
} finally {
1910+
setIsStrictModeForDevtools(false);
1911+
}
19061912
}
19071913
}
19081914
hook.memoizedState = hook.baseState = initialState;
@@ -2856,8 +2862,11 @@ function mountMemo<T>(
28562862
const nextValue = nextCreate();
28572863
if (shouldDoubleInvokeUserFnsInHooksDEV) {
28582864
setIsStrictModeForDevtools(true);
2859-
nextCreate();
2860-
setIsStrictModeForDevtools(false);
2865+
try {
2866+
nextCreate();
2867+
} finally {
2868+
setIsStrictModeForDevtools(false);
2869+
}
28612870
}
28622871
hook.memoizedState = [nextValue, nextDeps];
28632872
return nextValue;
@@ -2880,8 +2889,11 @@ function updateMemo<T>(
28802889
const nextValue = nextCreate();
28812890
if (shouldDoubleInvokeUserFnsInHooksDEV) {
28822891
setIsStrictModeForDevtools(true);
2883-
nextCreate();
2884-
setIsStrictModeForDevtools(false);
2892+
try {
2893+
nextCreate();
2894+
} finally {
2895+
setIsStrictModeForDevtools(false);
2896+
}
28852897
}
28862898
hook.memoizedState = [nextValue, nextDeps];
28872899
return nextValue;

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

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3987,15 +3987,18 @@ function doubleInvokeEffectsOnFiber(
39873987
shouldDoubleInvokePassiveEffects: boolean = true,
39883988
) {
39893989
setIsStrictModeForDevtools(true);
3990-
disappearLayoutEffects(fiber);
3991-
if (shouldDoubleInvokePassiveEffects) {
3992-
disconnectPassiveEffect(fiber);
3993-
}
3994-
reappearLayoutEffects(root, fiber.alternate, fiber, false);
3995-
if (shouldDoubleInvokePassiveEffects) {
3996-
reconnectPassiveEffects(root, fiber, NoLanes, null, false);
3990+
try {
3991+
disappearLayoutEffects(fiber);
3992+
if (shouldDoubleInvokePassiveEffects) {
3993+
disconnectPassiveEffect(fiber);
3994+
}
3995+
reappearLayoutEffects(root, fiber.alternate, fiber, false);
3996+
if (shouldDoubleInvokePassiveEffects) {
3997+
reconnectPassiveEffects(root, fiber, NoLanes, null, false);
3998+
}
3999+
} finally {
4000+
setIsStrictModeForDevtools(false);
39974001
}
3998-
setIsStrictModeForDevtools(false);
39994002
}
40004003

40014004
function doubleInvokeEffectsInDEVIfNecessary(

0 commit comments

Comments
 (0)