From 9043ad6b9dbc293b9cca33516bdcb026fbeee4b2 Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Tue, 18 Jul 2017 16:31:04 +0100
Subject: [PATCH] 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.
---
 scripts/fiber/tests-passing.txt               |  1 +
 .../__tests__/inputValueTracking-test.js      | 32 +++++++++++++++++--
 .../dom/shared/inputValueTracking.js          | 27 ++++++++++------
 3 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt
index 10ca5eaaff..b6ad6c5863 100644
--- a/scripts/fiber/tests-passing.txt
+++ b/scripts/fiber/tests-passing.txt
@@ -1658,6 +1658,7 @@ src/renderers/dom/shared/__tests__/inputValueTracking-test.js
 * should track value and return true when updating untracked instance
 * should return tracker from node
 * should stop tracking
+* does not crash for nodes with custom value property
 
 src/renderers/dom/shared/__tests__/quoteAttributeValueForBrowser-test.js
 * should escape boolean to string
diff --git a/src/renderers/dom/shared/__tests__/inputValueTracking-test.js b/src/renderers/dom/shared/__tests__/inputValueTracking-test.js
index dbec64a156..f6e461cff4 100644
--- a/src/renderers/dom/shared/__tests__/inputValueTracking-test.js
+++ b/src/renderers/dom/shared/__tests__/inputValueTracking-test.js
@@ -12,6 +12,7 @@
 'use strict';
 
 var React = require('react');
+var ReactDOM = require('react-dom');
 var ReactTestUtils = require('react-dom/test-utils');
 // TODO: can we express this test with only public API?
 var inputValueTracking = require('inputValueTracking');
@@ -135,9 +136,8 @@ describe('inputValueTracking', () => {
   });
 
   it('should return tracker from node', () => {
-    var node = ReactTestUtils.renderIntoDocument(
-      <input type="text" defaultValue="foo" />,
-    );
+    var div = document.createElement('div');
+    var node = ReactDOM.render(<input type="text" defaultValue="foo" />, div);
     var tracker = inputValueTracking._getTrackerFromNode(node);
     expect(tracker.getValue()).toEqual('foo');
   });
@@ -153,4 +153,30 @@ describe('inputValueTracking', () => {
 
     expect(input.hasOwnProperty('value')).toBe(false);
   });
+
+  it('does not crash for nodes with custom value property', () => {
+    // https://github.com/facebook/react/issues/10196
+    try {
+      var originalCreateElement = document.createElement;
+      document.createElement = function() {
+        var node = originalCreateElement.apply(this, arguments);
+        Object.defineProperty(node, 'value', {
+          get() {},
+          set() {},
+        });
+        return node;
+      };
+      var div = document.createElement('div');
+      // Mount
+      var node = ReactDOM.render(<input type="text" />, div);
+      // Update
+      ReactDOM.render(<input type="text" />, div);
+      // Change
+      ReactTestUtils.SimulateNative.change(node);
+      // Unmount
+      ReactDOM.unmountComponentAtNode(div);
+    } finally {
+      document.createElement = originalCreateElement;
+    }
+  });
 });
diff --git a/src/renderers/dom/shared/inputValueTracking.js b/src/renderers/dom/shared/inputValueTracking.js
index 05b5b5de36..f82518d13b 100644
--- a/src/renderers/dom/shared/inputValueTracking.js
+++ b/src/renderers/dom/shared/inputValueTracking.js
@@ -128,32 +128,41 @@ var inputValueTracking = {
     inst._wrapperState.valueTracker = trackValueOnNode(node, inst);
   },
 
+  // TODO: in practice "subject" can currently be Stack instance,
+  // Fiber, or DOM node. This is hard to understand. We should either
+  // make it accept only DOM nodes, or only Fiber/Stack instances.
   updateValueIfChanged(subject: SubjectWithWrapperState | Fiber) {
     if (!subject) {
       return false;
     }
-    var tracker = getTracker(subject);
 
+    var isNode = (subject: any).nodeType === ELEMENT_NODE;
+    var tracker = getTracker(subject);
     if (!tracker) {
-      if (typeof (subject: any).tag === 'number') {
+      if (isNode) {
+        // DOM node
+        inputValueTracking.trackNode((subject: any));
+      } else if (typeof (subject: any).tag === 'number') {
+        // Fiber
         inputValueTracking.trackNode((subject: any).stateNode);
       } else {
+        // Stack
         inputValueTracking.track((subject: any));
       }
       return true;
     }
 
-    var lastValue = tracker.getValue();
-
-    var node = subject;
-
-    // TODO: remove check when the Stack renderer is retired
-    if ((subject: any).nodeType !== ELEMENT_NODE) {
+    var node;
+    if (isNode) {
+      // DOM node
+      node = subject;
+    } else {
+      // Fiber and Stack
       node = ReactDOMComponentTree.getNodeFromInstance(subject);
     }
 
+    var lastValue = tracker.getValue();
     var nextValue = getValueFromNode(node);
-
     if (nextValue !== lastValue) {
       tracker.setValue(nextValue);
       return true;