@samitouri / QOS-React / commits / 9043ad6b9d

Fix crash on master introduced during the radio bugfix (#10207)

* Add a regression test that passes in Stack but fails in Fiber 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. * Fix updateValueIfChanged() lazy path to work with DOM argument * Slightly reorder lines for clarity and record tests * Also test the change event code path This makes it go through the Fiber argument code path.

Dan Abramov committed Jul 18, 2017 at 16:31 UTC 9043ad6b9dbc293b9cca33516bdcb026fbeee4b2
3 files changed +48 -12
scripts/fiber/tests-passing.txt
+1
@@ -1658,6 +1658,7 @@ src/renderers/dom/shared/__tests__/inputValueTracking-test.js
1658 * should track value and return true when updating untracked instance
1659 * should return tracker from node
1660 * should stop tracking
1661 +* does not crash for nodes with custom value property
1662
1663 src/renderers/dom/shared/__tests__/quoteAttributeValueForBrowser-test.js
1664 * should escape boolean to string
src/renderers/dom/shared/__tests__/inputValueTracking-test.js
+29 -3
@@ -12,6 +12,7 @@
12 'use strict';
13
14 var React = require('react');
15 +var ReactDOM = require('react-dom');
16 var ReactTestUtils = require('react-dom/test-utils');
17 // TODO: can we express this test with only public API?
18 var inputValueTracking = require('inputValueTracking');
@@ -135,9 +136,8 @@ describe('inputValueTracking', () => {
136 });
137
138 it('should return tracker from node', () => {
138 - var node = ReactTestUtils.renderIntoDocument(
139 - <input type="text" defaultValue="foo" />,
140 - );
139 + var div = document.createElement('div');
140 + var node = ReactDOM.render(<input type="text" defaultValue="foo" />, div);
141 var tracker = inputValueTracking._getTrackerFromNode(node);
142 expect(tracker.getValue()).toEqual('foo');
143 });
@@ -153,4 +153,30 @@ describe('inputValueTracking', () => {
153
154 expect(input.hasOwnProperty('value')).toBe(false);
155 });
156 +
157 + it('does not crash for nodes with custom value property', () => {
158 + // https://github.com/facebook/react/issues/10196
159 + try {
160 + var originalCreateElement = document.createElement;
161 + document.createElement = function() {
162 + var node = originalCreateElement.apply(this, arguments);
163 + Object.defineProperty(node, 'value', {
164 + get() {},
165 + set() {},
166 + });
167 + return node;
168 + };
169 + var div = document.createElement('div');
170 + // Mount
171 + var node = ReactDOM.render(<input type="text" />, div);
172 + // Update
173 + ReactDOM.render(<input type="text" />, div);
174 + // Change
175 + ReactTestUtils.SimulateNative.change(node);
176 + // Unmount
177 + ReactDOM.unmountComponentAtNode(div);
178 + } finally {
179 + document.createElement = originalCreateElement;
180 + }
181 + });
182 });
src/renderers/dom/shared/inputValueTracking.js
+18 -9
@@ -128,32 +128,41 @@ var inputValueTracking = {
128 inst._wrapperState.valueTracker = trackValueOnNode(node, inst);
129 },
130
131 + // TODO: in practice "subject" can currently be Stack instance,
132 + // Fiber, or DOM node. This is hard to understand. We should either
133 + // make it accept only DOM nodes, or only Fiber/Stack instances.
134 updateValueIfChanged(subject: SubjectWithWrapperState | Fiber) {
135 if (!subject) {
136 return false;
137 }
135 - var tracker = getTracker(subject);
138
139 + var isNode = (subject: any).nodeType === ELEMENT_NODE;
140 + var tracker = getTracker(subject);
141 if (!tracker) {
138 - if (typeof (subject: any).tag === 'number') {
142 + if (isNode) {
143 + // DOM node
144 + inputValueTracking.trackNode((subject: any));
145 + } else if (typeof (subject: any).tag === 'number') {
146 + // Fiber
147 inputValueTracking.trackNode((subject: any).stateNode);
148 } else {
149 + // Stack
150 inputValueTracking.track((subject: any));
151 }
152 return true;
153 }
154
146 - var lastValue = tracker.getValue();
147 -
148 - var node = subject;
149 -
150 - // TODO: remove check when the Stack renderer is retired
151 - if ((subject: any).nodeType !== ELEMENT_NODE) {
155 + var node;
156 + if (isNode) {
157 + // DOM node
158 + node = subject;
159 + } else {
160 + // Fiber and Stack
161 node = ReactDOMComponentTree.getNodeFromInstance(subject);
162 }
163
164 + var lastValue = tracker.getValue();
165 var nextValue = getValueFromNode(node);
156 -
166 if (nextValue !== lastValue) {
167 tracker.setValue(nextValue);
168 return true;