[Bugfix] Incorrect return pointer on deleted fiber #20942
Conversation
Based on facebook#20932 Co-Authored-By: Dan Abramov <dan.abramov@gmail.com>
facebook-github-bot
added
CLA Signed
React Core Team
When deleting a host node, we follow the return path to find the nearest mounted host parent. Which means the return pointer of the deleted tree must be correct in the mutation phase.
| @@ -1964,6 +1964,10 @@ function commitMutationEffects_begin( | |||
| if (deletions !== null) { | |||
| for (let i = 0; i < deletions.length; i++) { | |||
| const childToDelete = deletions[i]; | |||
| // When deleting a host node, we follow the return path to find the | |||
| // neartest mounted host parent. Which means the return pointer of | |||
| // the deleted tree must be correct. | |||
There was a problem hiding this comment.
Instead of doing that on-demand, we could do that eagerly here and then pass in the parent.
It's likely to be at least one DOM node deleted. If it's more than one, then it's unnecessary to backtrack more than once. We meant to do that optimization for that reason at some point anyway.
Or better yet, keep track of it on the stack since we're traversing through the parent anyway.
There was a problem hiding this comment.
We don't use recursion for this function yet (was reverted because of slow down in open source build) so it'd have to be a virtual stack.
So I'll go with option 1 (eagerly find the current parent) for now and leave a TODO for when this recursive traversal is converted to use the JS stack.
|
Comparing: ee43263...01ece92 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
| // When deleting a host node, we follow the return path to find the | ||
| // neartest mounted host parent. Which means the return pointer of | ||
| // the deleted tree must be correct. | ||
| childToDelete.return = fiber; |
There was a problem hiding this comment.
In my testing, the return pointer was not null the first time something was deleted. It ended up being null later b'c we tried to delete the same thing twice.
I'm wondering if the repro I had (the larger one that involved Feed and was more touch and go) is a different problem.
There was a problem hiding this comment.
It’s always non-null at this particular location, but before the fix it would sometimes point to the wrong alternate. Does that describe what you were seeing?
There was a problem hiding this comment.
Hm, pretty sure I was seeing a double deletion, and the pointer was cleared in detach mutation.
There was a problem hiding this comment.
You're right, I checked again and it was a double deletion.
Fix here: #20942
Alternate fix to facebook#20942 There was already a TODO to make this change, but at the time I left it, I couldn't think of a way that it would actually cause a bug, and I was hesistant to change something without fully understanding the ramifications. This was during a time when we were hunting down a different bug, so we were especially risk averse. What I should have done in retrospect is put the change behind a flag and tried rolling it out once the other bug had been flushed out. OTOH, now we have a regression test, which wouldn't have otherwise, and the bug it caused rarely fired in production.
|
Switched back to draft status because I found the "real" fix in #20948 I still want to land the change suggested here, though: #20942 (comment) |
* Add failing regression test Based on #20932 Co-Authored-By: Dan Abramov <dan.abramov@gmail.com> * Reset `subtreeFlags` in `resetWorkInProgress` Alternate fix to #20942 There was already a TODO to make this change, but at the time I left it, I couldn't think of a way that it would actually cause a bug, and I was hesistant to change something without fully understanding the ramifications. This was during a time when we were hunting down a different bug, so we were especially risk averse. What I should have done in retrospect is put the change behind a flag and tried rolling it out once the other bug had been flushed out. OTOH, now we have a regression test, which wouldn't have otherwise, and the bug it caused rarely fired in production. Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
* Add failing regression test Based on facebook#20932 Co-Authored-By: Dan Abramov <dan.abramov@gmail.com> * Reset `subtreeFlags` in `resetWorkInProgress` Alternate fix to facebook#20942 There was already a TODO to make this change, but at the time I left it, I couldn't think of a way that it would actually cause a bug, and I was hesistant to change something without fully understanding the ramifications. This was during a time when we were hunting down a different bug, so we were especially risk averse. What I should have done in retrospect is put the change behind a flag and tried rolling it out once the other bug had been flushed out. OTOH, now we have a regression test, which wouldn't have otherwise, and the bug it caused rarely fired in production. Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
Regression test ported from #20932.
I think now that I know the bug I can write a more focused test, but I'll keep this one too.