`bailoutOnLowPriority` pushes context to mirror complete phase context pop #8590

bvaughn · 2016-12-16T18:45:01Z

While testing the new ReactNativeFiber renderer on internal apps, I discovered that the Ads Manager app as well as the "App - UIExplorer Browser" component inside of Catalyst both threw the an "Unexpected context pop" error. I traced the root cause of this error to the fact that in the complete phase we always pop context for context providers but in the begin phase we don't always push. (Specially if we bailout on low priority we don't push.) This causes us to pop too many times in certain cases.

This PR corrects the behavior, making push/pop mirror each other, and adds a unit test that verifies the fix. Both Ads Manager and "App - UIExplorer Browser" function correctly once this fix is in place.

cc @acdlite to review since we spoke about this briefly and you have some context (heh) about the problem.
cc @gaearon as well since I think you implemented this stuff initially?

gaearon · 2016-12-16T19:02:27Z

It feels counter-intuitive to me that despite bailout in begin we still complete that fiber. Is this intentional @sebmarkbage? I would imagine it could cause other issues in complete phase.

Even if it works today, it's a bit hard to write complete code without knowing for sure if begin has run fully or bailed. We might fix this bug but later get others for the same underlying reason.

gaearon · 2016-12-16T19:03:57Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

-    if (workInProgress.tag === HostPortal) {
-      pushHostContainer(workInProgress.stateNode.containerInfo);
+    switch (workInProgress.tag) {
+      case ClassComponent:


If we do this I think we should handle HostComponent here too for consistency.
It also gets popped unconditionally in complete phase.
This defensive check is likely why it didn't break so far but we can replace it with an assertion.

+1 from me for replacing with an assertion.

Sorry. I missed this comment. Let me add an assertion, as you say! 😄

gaearon

Accepting if you:

Follow up with @sebmarkbage to ensure that running complete after begin bailed is intentional and there are good reasons for this in the first place.
If the above is true, please also fix HostComponent in the same way and try to remove the defensive check in host context code.
Try to figure out why we hit the bailout code in the first place in RN. We didn't enable deferred mode by default, did we? Then I don't understand why bailout happened.

acdlite · 2016-12-16T19:09:12Z

If I'm not mistaken, the conceptual model for bailing out is memoization, and the conceptual model for the complete phase is unwinding the stack to the return pointer. If that's so, then it makes sense to me that you'd complete a unit of work even when begin bails out.

acdlite · 2016-12-16T19:11:32Z

Relatedly, if I'm recalling a conversation with @sebmarkbage correctly, a lot of the stuff we're doing in complete right now should be moved to begin, like setting the memoizedProps and memoizedState. complete is there mostly for coroutines.

gaearon · 2016-12-16T19:21:01Z

I would like these branches to be unified:

It's too easy to mess them up right now.

acdlite · 2016-12-16T19:27:38Z

I think I asked this already, but is there any case where you don't push context for a context provider in the begin phase? If not, why not do a single check here? https://github.com/bvaughn/react/blob/ed3debfbe2a3d446ddc8755e6013238d4b812ca2/src/renderers/shared/fiber/ReactFiberBeginWork.js#L531 And a corresponding check for HostComponent.

bvaughn · 2016-12-16T19:59:15Z

If we do this I think we should handle HostComponent here too for consistency. It also gets popped unconditionally in complete phase.

I tried handling HostComponent in bailoutOnLowPriority at one point but IIR it caused an issue. I will revisit and look at the defensive check you mentioned.

I think I asked this already, but is there any case where you don't push a context for a context

As of this PR, we always push- even if we return early for bailoutOnLowPriority or bailoutOnAlreadyFinishedWork.

bvaughn · 2016-12-16T22:54:01Z

RE: @gaearon

Follow up with @sebmarkbage to ensure that running complete after being bailed is intentional and there are good reasons for this in the first place.

Done. Sebastian confirmed that this is intentional.

If the above is true, please also fix HostComponent in the same way and try to remove the defensive check in host context code.

Calling pushHostContext for HostComponent in bailoutOnLowPriority seems to be safe (in that it doesn't cause any new test errors). A quick smoke test of native apps seems okay also. I'm not sure currently how to create a test case that fails without this change though.

Try to figure out why we hit the bailout code in the first place in RN. We didn't enable deferred mode by default, did we? Then I don't understand why bailout happened.

If a ClassComponent's sCU returns false then updateClassComponent bails out early on it. The outer workLoop in the scheduler keeps running though so child component(s) are still processed but with a priority of NoWork. If a child component is a context-provider, it will trigger the asymmetric push/pop behavior that existed previously.

gaearon · 2016-12-16T22:56:13Z

Sounds good. Any objections to unifying this logic per #8590 (comment)? Like extract them in a single function since it looks like we need to do exact same thing in both cases.

acdlite · 2016-12-16T22:57:07Z

If we always push it should be moved to the switch statement here. https://github.com/bvaughn/react/blob/ed3debfbe2a3d446ddc8755e6013238d4b812ca2/src/renderers/shared/fiber/ReactFiberBeginWork.js#L531

gaearon · 2016-12-16T22:58:10Z

@acdlite

If we always push it should be moved to the switch statement here.

I don't get it, why? If we move it there it won't push on bailout. Don't we want to push on bailout per this thread?

bvaughn · 2016-12-16T22:58:26Z

Any objections to unifying this logic per #8590 (comment)?

No objections, really. I'm just less confident about the scope of such a change since I haven't tested it yet (and I'm still learning my way around this code). I can make a pass at doing that.

If we always push it should be moved to the switch statement here.

We can't because in some cases we bailout before that line is reached.

acdlite · 2016-12-16T23:00:21Z

Oh pssh don't mind me.

Still seems weird that we're pushing in different places. OTOH don't want to perform the check for all components, just ones that are possible context providers. So ok. I just wish the logic were more centralized.

bvaughn · 2016-12-16T23:00:31Z

Replacing the matching fiber check in popHostContext with an assertion causes 11 tests to fail. Adding the call to pushHostContext in bailoutOnLowPriority does not prevent the failures. I will need to dig more into that change before I'm comfortable applying it.

Should I block this PR on that? Or back out the pushHostContext call and just fix the ClassComponent case for now? (Preference for the latter).

At the moment, I've added a new TODO comment there linking to this PR thread.

…rror complete phase context pop

gaearon · 2016-12-16T23:17:59Z

Oh okay. It doesn't work because then we would have to check if something was a context provider before we pop. Currently we pop without checks because it's faster. And checking twice seems like useless work.

gaearon · 2016-12-16T23:19:48Z

Maybe we can add a __DEV__-only warning if, when we return early from popping, we made a mistake and it turned out to be a context provider.

bvaughn · 2016-12-17T00:00:28Z

@gaearon I think I added the correct warning check in b22c673. Let me know if not. I'm not as familiar with host component context yet. 😄

gaearon · 2016-12-17T00:24:55Z

Does this warning fail when you "forget" to push?

bvaughn · 2016-12-17T00:25:59Z

I think this warning triggers when you pop context for a HostComponent that you didn't push. To be honest, I haven't traced through the HostComponent flow as much so I'm not super confident.

gaearon · 2016-12-17T00:44:29Z

The warning appears working to me. It fails in some (new) cases—I'll send you a PR.

gaearon · 2016-12-17T00:54:54Z

Okay, I sent a PR against your branch: bvaughn#1.

gaearon · 2016-12-17T01:28:11Z

I don't want to block you on this but it would be great to fix all context bugs together. So if my fix in bvaughn#1 doesn't work I'd appreciate if you could take the test I added in there and fix it some other way.

bvaughn · 2016-12-17T01:33:21Z

I don't mind, so long as things like the Calendar tool bug Tom reported can wait on me to write a few more tests! 😁

gaearon · 2016-12-17T11:14:44Z

Let's merge just the change to how non-host context works to keep this focused then.

I have a work-in-progress branch trying to address other issues so I can submit it later against master.

bvaughn · 2016-12-17T15:34:52Z

Roger that. Let me know if you'd like any help writing test cases for your follow-up. I thought a little about how I might write some last night.

gaearon · 2016-12-17T18:24:41Z

These are a start: #8593

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

gaearon reviewed Dec 16, 2016

View reviewed changes

gaearon approved these changes Dec 16, 2016

View reviewed changes

bailoutOnLowPriority correct pushes context for ClassComponents to mi…

441cc5c

…rror complete phase context pop

bvaughn force-pushed the unexpected-context-pop branch from 4fcd840 to 441cc5c Compare December 16, 2016 23:04

bvaughn force-pushed the unexpected-context-pop branch from 4e31116 to b22c673 Compare December 17, 2016 00:01

bvaughn force-pushed the unexpected-context-pop branch from b22c673 to 441cc5c Compare December 17, 2016 15:37

bvaughn merged commit a5e7287 into facebook:master Dec 17, 2016

bvaughn deleted the unexpected-context-pop branch December 17, 2016 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`bailoutOnLowPriority` pushes context to mirror complete phase context pop #8590

`bailoutOnLowPriority` pushes context to mirror complete phase context pop #8590

bvaughn commented Dec 16, 2016

gaearon commented Dec 16, 2016

gaearon Dec 16, 2016

sophiebits Dec 16, 2016

bvaughn Dec 16, 2016

gaearon left a comment •

edited

Loading

acdlite commented Dec 16, 2016

acdlite commented Dec 16, 2016

gaearon commented Dec 16, 2016

acdlite commented Dec 16, 2016 •

edited

Loading

bvaughn commented Dec 16, 2016 •

edited

Loading

bvaughn commented Dec 16, 2016

gaearon commented Dec 16, 2016

acdlite commented Dec 16, 2016

gaearon commented Dec 16, 2016

bvaughn commented Dec 16, 2016

acdlite commented Dec 16, 2016 •

edited

Loading

bvaughn commented Dec 16, 2016 •

edited

Loading

gaearon commented Dec 16, 2016

gaearon commented Dec 16, 2016

bvaughn commented Dec 17, 2016 •

edited

Loading

gaearon commented Dec 17, 2016

bvaughn commented Dec 17, 2016

gaearon commented Dec 17, 2016 •

edited

Loading

gaearon commented Dec 17, 2016

gaearon commented Dec 17, 2016

bvaughn commented Dec 17, 2016 •

edited

Loading

gaearon commented Dec 17, 2016 •

edited

Loading

bvaughn commented Dec 17, 2016

gaearon commented Dec 17, 2016

bailoutOnLowPriority pushes context to mirror complete phase context pop #8590

bailoutOnLowPriority pushes context to mirror complete phase context pop #8590

Conversation

bvaughn commented Dec 16, 2016

gaearon commented Dec 16, 2016

gaearon Dec 16, 2016

Choose a reason for hiding this comment

sophiebits Dec 16, 2016

Choose a reason for hiding this comment

bvaughn Dec 16, 2016

Choose a reason for hiding this comment

gaearon left a comment • edited Loading

Choose a reason for hiding this comment

acdlite commented Dec 16, 2016

acdlite commented Dec 16, 2016

gaearon commented Dec 16, 2016

acdlite commented Dec 16, 2016 • edited Loading

bvaughn commented Dec 16, 2016 • edited Loading

bvaughn commented Dec 16, 2016

gaearon commented Dec 16, 2016

acdlite commented Dec 16, 2016

gaearon commented Dec 16, 2016

bvaughn commented Dec 16, 2016

acdlite commented Dec 16, 2016 • edited Loading

bvaughn commented Dec 16, 2016 • edited Loading

gaearon commented Dec 16, 2016

gaearon commented Dec 16, 2016

bvaughn commented Dec 17, 2016 • edited Loading

gaearon commented Dec 17, 2016

bvaughn commented Dec 17, 2016

gaearon commented Dec 17, 2016 • edited Loading

gaearon commented Dec 17, 2016

gaearon commented Dec 17, 2016

bvaughn commented Dec 17, 2016 • edited Loading

gaearon commented Dec 17, 2016 • edited Loading

bvaughn commented Dec 17, 2016

gaearon commented Dec 17, 2016

`bailoutOnLowPriority` pushes context to mirror complete phase context pop #8590

`bailoutOnLowPriority` pushes context to mirror complete phase context pop #8590

gaearon left a comment •

edited

Loading

acdlite commented Dec 16, 2016 •

edited

Loading

bvaughn commented Dec 16, 2016 •

edited

Loading

acdlite commented Dec 16, 2016 •

edited

Loading

bvaughn commented Dec 16, 2016 •

edited

Loading

bvaughn commented Dec 17, 2016 •

edited

Loading

gaearon commented Dec 17, 2016 •

edited

Loading

bvaughn commented Dec 17, 2016 •

edited

Loading

gaearon commented Dec 17, 2016 •

edited

Loading