Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unwinding starting with a wrong Fiber on error in the complete phase #13237

Merged
merged 4 commits into from Jul 19, 2018

Conversation

@gaearon
Copy link
Member

@gaearon gaearon commented Jul 18, 2018

Adds two regression tests (one for profiler stack and one for context stack). Also fixes the bug.

The bug was caused by a structure like this:

    </Provider>
  </div>
</errorInCompletePhase>

We forgot to update nextUnitOfWork (see the fix in the last commit). So it was still pointing at Provider when errorInCompletePhase threw. Normally we would update it right after so it wouldn't be noticeable, but in case of exception its value at the moment of throwing matters.

As a result, we would try to unwind from Provider (rather than from errorInCompletePhase), and thus pop the Provider twice. It has already been popped at the beginning of the complete phase.

This was easy to miss because a throwing complete phase is uncommon. But it can happen in host configs (e.g. RN nesting or DOM invalid style invariants).

I verified this fixes the original internal issue too.

This currently fails the tests due to an unexpected warning.
gaearon added 3 commits Jul 19, 2018
The bug was caused by a structure like this:

    </Provider>
  </div>
</errorInCompletePhase>

We forgot to update nextUnitOfWork so it was still pointing at Provider when errorInCompletePhase threw. As a result, we would try to unwind from Provider (rather than from errorInCompletePhase), and thus pop the Provider twice.
@gaearon gaearon changed the title Reproduce unwinding bug Fix unwinding starting with a wrong Fiber on error in the complete phase Jul 19, 2018
Copy link
Member

@acdlite acdlite left a comment

How about we remove the workInProgress parameter from completeUnitOfWork entirely? Since it's always supposed to equal to nextUnitOfWork.

Loading

current,
workInProgress,
nextRenderExpirationTime,
);
let next = nextUnitOfWork;
Copy link
Member

@acdlite acdlite Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next is always null ever since we deleted call/return. Let's just remove the block below.

Loading

@acdlite
Copy link
Member

@acdlite acdlite commented Jul 19, 2018

Oh I guess we don't because Flow :) Ok.

Loading

@gaearon gaearon merged commit 2c560cb into facebook:master Jul 19, 2018
1 check passed
Loading
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
…ase (facebook#13237)

* Add a repro case for profiler unwinding

This currently fails the tests due to an unexpected warning.

* Add a regression test for context stack

* Simplify the first test case

* Update nextUnitOfWork inside completeUnitOfWork()

The bug was caused by a structure like this:

    </Provider>
  </div>
</errorInCompletePhase>

We forgot to update nextUnitOfWork so it was still pointing at Provider when errorInCompletePhase threw. As a result, we would try to unwind from Provider (rather than from errorInCompletePhase), and thus pop the Provider twice.
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants