[Fiber] Push class context providers even if they crash by gaearon · Pull Request #8627 · facebook/react

gaearon · 2016-12-22T15:09:38Z

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.

I also added a test from #8604 to verify this also solves the problem described in that PR.

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.

Also rename another test to have a shorter name.

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().

gaearon · 2016-12-22T15:54:08Z

I found one additional bug there but I can't figure out how to fix it.

When an error boundary itself is a context provider, beginFailedWork doesn't push it.
If I make it push it though, I get unexpected pops in unwinding.
I don't understand how this part of the code works but maybe @acdlite has ideas.

It's not blocking for this PR though.

bvaughn · 2016-12-22T16:28:49Z

src/renderers/shared/fiber/ReactFiberContext.js

-  const unmaskedContext = getUnmaskedContext();
-  const context = {};
+  const hasOwnContext = isContextProvider(workInProgress);
+  // If it is a context provider then use the previous context instead.


nit: This comment makes sense to me, with the context I have, but might not be sufficient explanation for someone seeing this part of the code for the first time. Maybe we should elaborate about why we use the previous one. Maybe even just a pointer to see the comments in pushContextProvider?

bvaughn · 2016-12-22T16:29:33Z

src/renderers/shared/fiber/ReactFiberContext.js

-    // Instance might be null, if the fiber errored during construction
-    fiber.stateNode &&
-    typeof fiber.stateNode.getChildContext === 'function'
+    fiber.type.childContextTypes != null


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

src/renderers/shared/fiber/ReactFiberContext.js

+exports.invalidateContextProvider = function(workInProgress : Fiber) : void {
+  const instance = workInProgress.stateNode;
+  if (instance == null) {
+    throw new Error('Expected to have an instance by this point.');


Curious why you're using throw here instead of invariant? (How do we determine when to do one vs the other?)

We should always use invariants but I didn't bother because we're going to do a separate pass to replace them all anyway. (It's in the umbrella.)

bvaughn · 2016-12-22T16:37:44Z

src/renderers/shared/fiber/ReactFiberStack.js

+  if (index === 0) {
+    return null;
+  }
+  return valueStack[index - 1];


Isn't previous actually the value at valueStack[index]?

Current is in cursor.current and previous is the top item in the stack (at index).

PS I was already thinking about doing this, but I believe I'll submit a follow-up PR that adds some unit tests for ReactFiberStack itself.

Ah, I think you're right. I wonder why tests didn't catch this.

No problem. I'll add some tests for this specific type of thing. 👍

bvaughn · 2016-12-22T16:48:17Z

src/renderers/shared/fiber/ReactFiberStack.js

+    }
+    return null;
+  }
+  if (index === 0) {


This check is unnecessary. We only care about < 0 which is already checked for above.

Rather than "unnecessary" I should have said "bad" because it would prevent the default value from being returned as previous if one was set. (eg like the default for didPerformWorkStackCursor)

gaearon · 2016-12-22T17:06:36Z

Addressed.

bvaughn · 2016-12-22T17:11:04Z

Nice clean up Dan!

gaearon · 2016-12-22T17:14:24Z

My only concern is whether c57cf3e is bad from optimization point of view. I.e. maybe it prevents inlining or something. What do you think?

gaearon · 2016-12-22T17:18:21Z

Hmm maybe off by one wasn't a bug after all..

bvaughn · 2016-12-22T17:19:50Z

My only concern if c57cf3e is bad from optimization point of view. I.e. maybe it prevents inlining or something. What do you think?

Unfortunately this is not something I'm that knowledgable about yet. I should defer to @sebmarkbage.

Hmm maybe off by one wasn't a bug after all..

You mean getPrevious? It definitely was.

gaearon · 2016-12-22T17:22:26Z

Then there must be another off-by-one there that cancels it out since the test started failing after I fixed it (it goes into an infinite loop and shows wrong context in some tests).

bvaughn · 2016-12-22T17:26:25Z

Hm. I'll finish up these ReactFiberStack tests and then I can help diagnose. Should only take a few minutes.

bvaughn · 2016-12-22T17:38:03Z

Tests added for ReactFiberStack ~> gaearon#1

gaearon · 2016-12-22T17:39:49Z

Hmm, I just realized valueStack and fiberStack aren't "tracking" each other despite both being indexed by the same index. fiberStack[i] corresponds to the current value whereas valueStack[i] is one value behind.

gaearon · 2016-12-22T17:41:55Z

This was a little confusing to me. While we're at it: why doesn't reset reset the current value (but resets its fiber)?

bvaughn · 2016-12-22T17:46:04Z

why doesn't reset reset the current value (but resets its fiber)

The stack doesn't track its allocated cursors so it has no way to reset current values. (ReactFiberContext and ReactFiberHostContext are responsible for doing this.) In the future- if we change the context API- we might be allocating a lot of cursors so it seemed best not to track them/hold onto them. (I was kind of on the fence about this to be honest.)

Once the stack has been reset, any cursor values should be ignored as they are no longer safe.

gaearon · 2016-12-22T17:48:07Z

I totally misunderstood how stack works.
I thought values were held separately. 😄

bvaughn · 2016-12-22T17:49:42Z

FYI in ReactIncremental test...

This test fails: merges and masks context
This test hangs: does not leak own context into context provider

gaearon · 2016-12-22T17:50:09Z

My approach worked by accident. By addressing index - 1 I just managed to get the right value (e.g. context rather than didWork) from the stack but it just happened to work.

bvaughn · 2016-12-22T17:50:43Z

Ha! That's funny 😁 Yeah I had a few moments of off-by-one confusion like that when working on the initial stack branch.

bvaughn · 2016-12-22T17:51:14Z

I totally should have spotted that. Sorry.

This type of stack-management stuff is still a bit foreign to my thinking.

gaearon · 2016-12-22T17:52:58Z

Do I understand correctly that order of push and pop operations must always be exact reverse or we risk popping the wrong thing corresponding to the right fiber?

bvaughn · 2016-12-22T17:54:02Z

Yes. That is correct. Mirror order.

gaearon · 2016-12-22T17:59:40Z

It might make sense to track cursor objects together with fibers in DEV for better checks.

bvaughn

We need an alternate approach to getPrevious. Sorry for not spotting this initially. ☹️

gaearon · 2016-12-22T18:04:44Z

I don't think replace can work on anything but innermost level, right? We can't replace two things one after another. We can only pop twice and push twice.

bvaughn · 2016-12-22T18:09:56Z

You can replace multiple cursors, so long as you're only concerned with updating the "current" value of each. (Or you could pop+push again yes.) replace checks for a Fiber match when but that's not technically needed. You could push another cursor/Fiber, and still safely replace a value from a previous cursor/Fiber's push, so long as it's the top-most value for that cursor.

The part about this that doesn't work is getting the previous value. Stack currently doesn't know or care about what's on it or in what order, so the previous value is not guaranteed to be for the same cursor. We'd have to track previous on the cursor itself if we wanted to be able to return it. Alternately we could mirror pop-then-repush but this assumes that nothing else has pushed to the stack in the meanwhile.

bvaughn · 2016-12-22T18:12:38Z

We'd have to track previous on the cursor itself if we wanted to be able to return it.

Actually even this would be problematic, because once we pop- we'd have the same problem, trying to figure what the previous was.

I think the only safe thing to do here is to unwind the stack a little, then push them back on the stack once we've read them.

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.

gaearon · 2016-12-22T18:25:13Z

OK I think that fixes it.

bvaughn

This looks good!

gaearon · 2016-12-22T18:36:39Z

I'll land since it fixes bugs, happy to follow up on perf improvements if @sebmarkbage sees any.

gaearon added 2 commits December 22, 2016 15:01

Add a now-passing test from facebook#8604

d64c76c

Also rename another test to have a shorter name.

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

gaearon requested review from bvaughn and sebmarkbage December 22, 2016 15:09

gaearon mentioned this pull request Dec 22, 2016

Fix context handling for errors thrown in ClassComponent render #8604

Closed

2 tasks

facebook-github-bot added the CLA Signed label Dec 22, 2016

Move isContextProvider() checks into push() and pop()

c57cf3e

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().

bvaughn suggested changes Dec 22, 2016

View reviewed changes

bvaughn reviewed Dec 22, 2016

View reviewed changes

gaearon added 3 commits December 22, 2016 17:02

Clarify why we read the previous context

81804e3

Use invariant instead of throwing an error

a6f5cf0

Fix off-by-one in ReactFiberStack

504a694

bvaughn approved these changes Dec 22, 2016

View reviewed changes

bvaughn mentioned this pull request Dec 22, 2016

Added unit tests for ReactFiberStack gaearon/react#1

Closed

bvaughn suggested changes Dec 22, 2016

View reviewed changes

bvaughn approved these changes Dec 22, 2016

View reviewed changes

gaearon merged commit c978f78 into facebook:master Dec 22, 2016

gaearon deleted the context-boundaries branch December 22, 2016 18:36

Conversation

gaearon commented Dec 22, 2016

Uh oh!

gaearon commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

bvaughn commented Dec 22, 2016

Uh oh!

gaearon commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

bvaughn commented Dec 22, 2016

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

bvaughn commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn commented Dec 22, 2016

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

bvaughn commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

bvaughn commented Dec 22, 2016

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

bvaughn commented Dec 22, 2016

Uh oh!

bvaughn commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

bvaughn commented Dec 22, 2016

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

bvaughn left a comment

Choose a reason for hiding this comment

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

bvaughn commented Dec 22, 2016

Uh oh!

bvaughn commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Dec 22, 2016

Uh oh!

gaearon commented Dec 22, 2016 •

edited

Loading

gaearon commented Dec 22, 2016 •

edited

Loading

bvaughn commented Dec 22, 2016 •

edited

Loading

bvaughn commented Dec 22, 2016 •

edited

Loading

bvaughn commented Dec 22, 2016 •

edited

Loading

bvaughn commented Dec 22, 2016 •

edited

Loading