Fix crash on master introduced during the radio bugfix #10207
Conversation
The failure happens because trackValueOnNode() can exit early if it detects an existing descriptor on node, or if it sees a "broken" Safari descriptor (which is how we encountered this bug in the wild). As a result, the tracker field was not set, and subsequent updateValueIfChanged() call went into the branch that initializes the tracker lazily. That branch has a bug in Fiber mode where it passes the wrong type. We did not see this issue before because that branch is relatively hard to hit (you have to either run it in Safari or define a custom DOM value descriptor which is what I did in the test). In the future, we will likely remove the lazy branch altogether since if we bailed out of setting up the tracker once, we will likely bail out every time. But for now I'm just focused on a minimal fix to unbreak master.
| // Mount | ||
| var node = ReactDOM.render(<input type="text" />, div); | ||
| // Update | ||
| ReactDOM.render(<input type="text" />, div); |
There was a problem hiding this comment.
do we also want to exercise the change plugin route? e.g. trigger a change event on the input.
There was a problem hiding this comment.
SimulateNative.change(node) should work, if the event triggered it passed through the function. It isn't specifically gonna catch a failure in Fiber but it covers the all paths through updateValueIfChanged.
There was a problem hiding this comment.
That was a great idea, thanks.
This makes it go through the Fiber argument code path.
|
Addressed. |
| // DOM node | ||
| node = subject; | ||
| } else { | ||
| // Fiber and Stack | ||
| node = ReactDOMComponentTree.getNodeFromInstance(subject); |
There was a problem hiding this comment.
Just to be clear for myself here, getNodeFromInstance works on Fibers?
There was a problem hiding this comment.
It does, yeah. See ReactDOMComponentTree.js#L190-L194
|
I think the biggest reason it’s so complicated is because we don’t just support Stack and Fiber, but also that they hold wrapper state in different places. So passing a DOM node is not “enough” because only Fiber keeps wrapper state on the DOM node. I wonder if the easiest solution to this is just to move wrapper state to DOM node in Stack. So that it works the same way as Fiber, and we can always pass the DOM node. |
|
If it’s too hard to simplify now we can also wait until Stack is gone. It shouldn’t take long. |
that is interesting...Then you in both cases you could use |
|
We could just store the value tracker on the DOM node, and leave the wrapperState out of it. Originally (way back) I believe I did that, but it made less sense from a correctness standpoint. In this case tho it leads to a significant simplification... |
|
Let’s try that? |
This moves the storage of the input value tracker to the DOM node instead of the wrapperState. This makes it easier to support both Stack and Fiber without out a lot of ugly type casting and logic branches. related: facebook#10207
* Move input valueTracker to DOM nodes This moves the storage of the input value tracker to the DOM node instead of the wrapperState. This makes it easier to support both Stack and Fiber without out a lot of ugly type casting and logic branches. related: #10207 * run prettier * remove instance accepting methods * rm unused trst method * address feedback * fix naming * fix lint
Fixes #10196.
See individual commits for descriptions of how this got broken, and why this fix works.
This is a minimal fix to unbreak master.
Follow-up items (not part of this PR) that @jquense and I will look into:
Remove lazy tracker path from
updateValueIfChanged. It isn’t useful, and makes code more complicated. It is easier if we can assume that tracker is always exists while component is mounted.Simplify the wild polymorphism. This method accepts a fiber, a Stack instance, and a DOM node. Stack will be gone soon, so we should look into either making it always accept a DOM node, or maybe making it always accept a fiber.
Related: reduce the abstraction in naming. I think Fix uncontrolled radios #10156 made things slightly more confusing by calling argument a “subject” and we lost track of what it means. Let’s be explicit and precise about what exactly it is, and try to use Flow with less
anys here.