Fix context handling for errors thrown in ClassComponent render #8604

bvaughn · 2016-12-19T21:26:26Z

Context providers are currently popped one too many times if an error is thrown by a context-provider's render method. In this case the begin phase is exited before context is pushed but we still pop it in.

This PR adds a test for this behavior (failing before) and fixes it by ensuring that we always push context before continuing (or unwinding). FWIW we plan to further clean-up and improve the handling of context-related code soon so this specific fix might be short-lived.

This PR relates to #8593.

Remaining work

Replace the try/finally approach with one of the ones mentioned in this comment.
Verify the changes mentioned this comment are fixed by the final solution

sophiebits · 2016-12-19T21:27:07Z

src/renderers/shared/fiber/__tests__/ReactIncremental-test.js

@@ -1975,4 +1975,46 @@ describe('ReactIncremental', () => {
    instance.setState({});
    ReactNoop.flush();
  });
+
+  fit('maintains the correct context index when unwinding due to an error ocurring during render', () => {


Context providers are currently popped one too many times if an error is thrown by a context-provider's render method. In this case, the begin phase is exited before context is pushed but we still pop it in. This PR adds a test for this behavior (failing before) and fixes it by ensuring that we always push context before continuing (or unwinding).

acdlite · 2016-12-20T18:10:52Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+      ReactCurrentOwner.current = workInProgress;
+      const nextChildren = instance.render();
+      reconcileChildren(current, workInProgress, nextChildren);
+    } finally {


try blocks prevent the containing function from being optimized. Can you find a way to solve this problem without adding a new try block? Maybe by fixing it in unwindContext?

I could move the call to instance.render into a separate function that is try/finally wrapped to minimize the impact. (I'm not an expert with the whole inlining area.) In its currently form I think it would be difficult to fix things in unwindContext.

Although I'm working today on an effort to combine ReactFiberContext and ReactFiberHostContext and add fiber-level tracking so that we won't pop a Fiber more than once, or pop one that wasn't pushed. It will hopefully ease the pressure here a bit as it would enable us to just dev-warn and ignore the bad pop rather than throw an error.

gaearon · 2016-12-21T21:55:08Z

I think this now already works on master.
Can you amend this to just include the test?

gaearon

Just leave the test.

bvaughn · 2016-12-21T23:17:13Z

I could, although I don't think Sebastian's recent change fixed this specific issue. It looks to me like we're still not properly handling an error thrown in render in master.

I just ran this test against my branch #8611 (which was rebased against master as of a few moments ago) and it still fails due to an "Unexpected Fiber popped" warning. Thoughts?

sebmarkbage · 2016-12-21T23:27:49Z

Two alternative solutions:

A) If something fails to call the constructor, then nothing will be set of stateNode which will cause isContextProvider to be false. All you have to do is make sure that we always push context right after stateNode is set, because that's the indicator whether this is a context provider or not when unwinding the context. This will always be way before render() btw. So it will already be set even if render is called.

B) We currently identify something as a context provider by what's on the instance. However, all context providers are also required to have a static childContextTypes. We could also switch our validation to a static one that checks for this. That way we can push at least null at the top. We'd have to mutate the current later once we get actual context in that case.

gaearon · 2016-12-22T00:26:03Z

Oops, I thought this was passing. I must have copy pasted the wrong test, sorry.

All you have to do is make sure that we always push context right after stateNode is set

I have a wip branch that tries to do this but it gets easy to miss because we have to do it from ClassComponent file and then we have the same did-I-forget-this-branch problem.

We could also switch our validation to a static one that checks for this. That way we can push at least null at the top. We'd have to mutate the current later once we get actual context in that case.

I prefer this solution. We'd also need to mutate current of didPerformWorkStack since we don't know that until later.

bvaughn · 2016-12-22T00:27:45Z

Mutating current seems kind of smelly/error-prone (eg what's the guarantee that something else hasn't pushed or popped in the meanwhile?)

gaearon · 2016-12-22T00:27:48Z

If you fix this could you please also check #8611 (comment) does not reproduce?

gaearon · 2016-12-22T00:28:25Z

(eg what's the guarantee that something else hasn't pushed or popped in the meanwhile?)

You can still pass fiber to replaceContext(). Check it matches top-level fiber in DEV.
(Assuming it's a thin wrapper over current assignment)

bvaughn · 2016-12-22T00:31:27Z

You can still pass fiber to replaceContext(). Check it matches top-level fiber.

Okay. I guess that's fair.

If you fix this could you please also check #8611 (comment) does not reproduce?

I'm not sure I have quite enough context on that comment. If I add the childContextTypes and getChildContext() attributes to all of the Broken* classes in ReactErrorBoundaries-test and all tests still pass for Fiber, does that indicate there's no longer a problem? (Because that's the case currently, in bvaughn:shared-context-stack)

gaearon · 2016-12-22T00:39:56Z

Because that's the case currently, in bvaughn:shared-context-stack

You need to temporarily flip useFiber in ReactDOMFeatureFlags to test this because that file uses ReactDOM which runs in non-Fiber mode by default. Make sure to revert it back before committing.

diff --git a/src/renderers/dom/shared/ReactDOMFeatureFlags.js b/src/renderers/dom/shared/ReactDOMFeatureFlags.js
index 5e9d932..17bae0a 100644
--- a/src/renderers/dom/shared/ReactDOMFeatureFlags.js
+++ b/src/renderers/dom/shared/ReactDOMFeatureFlags.js
@@ -13,7 +13,7 @@
 
 var ReactDOMFeatureFlags = {
   useCreateElement: true,
-  useFiber: false,
+  useFiber: true,
 };
 
 module.exports = ReactDOMFeatureFlags;
diff --git a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js
index 6ec4e36..23a5744 100644
--- a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js
+++ b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js
@@ -73,6 +73,12 @@ describe('ReactErrorBoundaries', () => {
     };
 
     BrokenComponentWillMount = class extends React.Component {
+      static childContextTypes = {
+        foo: React.PropTypes.number
+      };
+      getChildContext() {
+        return {foo: 42};
+      }
       constructor(props) {
         super(props);
         log.push('BrokenComponentWillMount constructor');

bvaughn · 2016-12-22T00:52:58Z

Yeah, I know to flip that flag. Maybe I forgot to flip it though when bouncing back and forth between branches today. 😄 My bad. I'll make sure to verify both fixes!

Also rename another test to have a shorter name.

gaearon · 2016-12-22T15:10:44Z

I'm not sure if you started working on this but I sent a PR to fix this: #8627.
Let me know what you think!

bvaughn · 2016-12-22T16:23:19Z

No, yesterday evening was eaten up by #8611 and #8622. Happy to review your fix. 😄

bvaughn · 2016-12-22T17:35:29Z

Closing this PR in favor of #8627

* Push class context providers early Previously we used to push them only after the instance was available. This caused issues in cases an error is thrown during componentWillMount(). In that case we never got to pushing the provider in the begin phase, but in complete phase the provider check returned true since the instance existed by that point. As a result we got mismatching context pops. We solve the issue by making the context check independent of whether the instance actually exists. Instead we're checking the type itself. This lets us push class context early. However there's another problem: we might not know the context value. If the instance is not yet created, we can't call getChildContext on it. To fix this, we are introducing a way to replace current value on the stack, and a way to read the previous value. This also helps remove some branching and split the memoized from invalidated code paths. * Add a now-passing test from #8604 Also rename another test to have a shorter name. * Move isContextProvider() checks into push() and pop() All uses of push() and pop() are guarded by it anyway. This makes it more similar to how we use host context. There is only one other place where isContextProvider() is used and that's legacy code needed for renderSubtree(). * Clarify why we read the previous context * Use invariant instead of throwing an error * Fix off-by-one in ReactFiberStack * Manually keep track of the last parent context The previous algorithm was flawed and worked by accident, as shown by the failing tests after an off-by-one was fixed. The implementation of getPrevious() was incorrect because the context stack currently has no notion of a previous value per cursor. Instead, we are caching the previous value directly in the ReactFiberContext in a local variable. Additionally, we are using push() and pop() instead of adding a new replace() method.

facebook-github-bot added the GH Review: review-needed label Dec 19, 2016

bvaughn requested a review from gaearon December 19, 2016 21:26

sophiebits reviewed Dec 19, 2016

View reviewed changes

bvaughn force-pushed the add-failing-test-context-pop-due-to-render-error branch from 5a914bd to 4227318 Compare December 19, 2016 21:29

bvaughn changed the title ~~Add failing test for context bug caused by errors thrown in render~~ Fix context handling for errors thrown in ClassComponent render Dec 19, 2016

bvaughn force-pushed the add-failing-test-context-pop-due-to-render-error branch 3 times, most recently from efad51d to 6c36cc1 Compare December 19, 2016 22:49

bvaughn assigned gaearon Dec 19, 2016

bvaughn force-pushed the add-failing-test-context-pop-due-to-render-error branch from 6c36cc1 to b86cdde Compare December 20, 2016 00:14

acdlite reviewed Dec 20, 2016

View reviewed changes

gaearon requested changes Dec 21, 2016

View reviewed changes

gaearon mentioned this pull request Dec 21, 2016

Added ReactFiberStack shared by ReactFiberContext and ReactFiberHostContext #8611

Merged

3 tasks

gaearon added a commit to gaearon/react that referenced this pull request Dec 22, 2016

Add a now-passing test from facebook#8604

d64c76c

Also rename another test to have a shorter name.

gaearon mentioned this pull request Dec 22, 2016

[Fiber] Push class context providers even if they crash #8627

Merged

bvaughn closed this Dec 22, 2016

bvaughn deleted the add-failing-test-context-pop-due-to-render-error branch December 22, 2016 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix context handling for errors thrown in ClassComponent render #8604

Fix context handling for errors thrown in ClassComponent render #8604

bvaughn commented Dec 19, 2016 •

edited

Loading

sophiebits Dec 19, 2016

bvaughn Dec 19, 2016

acdlite Dec 20, 2016

bvaughn Dec 20, 2016 •

edited

Loading

gaearon commented Dec 21, 2016

gaearon left a comment

bvaughn commented Dec 21, 2016 •

edited

Loading

sebmarkbage commented Dec 21, 2016

gaearon commented Dec 22, 2016

bvaughn commented Dec 22, 2016

gaearon commented Dec 22, 2016

gaearon commented Dec 22, 2016 •

edited

Loading

bvaughn commented Dec 22, 2016

gaearon commented Dec 22, 2016 •

edited

Loading

bvaughn commented Dec 22, 2016

gaearon commented Dec 22, 2016

bvaughn commented Dec 22, 2016

bvaughn commented Dec 22, 2016

Fix context handling for errors thrown in ClassComponent render #8604

Fix context handling for errors thrown in ClassComponent render #8604

Conversation

bvaughn commented Dec 19, 2016 • edited Loading

Remaining work

sophiebits Dec 19, 2016

Choose a reason for hiding this comment

bvaughn Dec 19, 2016

Choose a reason for hiding this comment

acdlite Dec 20, 2016

Choose a reason for hiding this comment

bvaughn Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

gaearon commented Dec 21, 2016

gaearon left a comment

Choose a reason for hiding this comment

bvaughn commented Dec 21, 2016 • edited Loading

sebmarkbage commented Dec 21, 2016

gaearon commented Dec 22, 2016

bvaughn commented Dec 22, 2016

gaearon commented Dec 22, 2016

gaearon commented Dec 22, 2016 • edited Loading

bvaughn commented Dec 22, 2016

gaearon commented Dec 22, 2016 • edited Loading

bvaughn commented Dec 22, 2016

gaearon commented Dec 22, 2016

bvaughn commented Dec 22, 2016

bvaughn commented Dec 22, 2016

bvaughn commented Dec 19, 2016 •

edited

Loading

bvaughn Dec 20, 2016 •

edited

Loading

bvaughn commented Dec 21, 2016 •

edited

Loading

gaearon commented Dec 22, 2016 •

edited

Loading

gaearon commented Dec 22, 2016 •

edited

Loading