Prevent cloning objects into previousData that are not plain objects

src/core/component.js

@@ -336,21 +336,23 @@ Component.prototype = {
     var isSinglePropSchema = isSingleProp(schema);
     var mixinEls = this.el.mixinEls;
     var previousData;
-
     // 1. Default values (lowest precendence).
     if (isSinglePropSchema) {
-      // Clone default value if object so components don't share object.
-      data = typeof schema.default === 'object' ? utils.clone(schema.default) : schema.default;
+      // Clone default value if plain object so components don't share the same object
+      // that might be modified by the user.
+      data = schema.default.constructor === Object ? utils.clone(schema.default) : schema.default;
     } else {
       // Preserve previously set properties if clobber not enabled.
       previousData = !clobber && this.attrValue;
-      // Clone default value if object so components don't share object
-      data = typeof previousData === 'object' ? utils.clone(previousData) : {};
+      // Clone previous data to prevent sharing references with attrValue that might be
+      // modified by the user.
+      data = typeof previousData === 'object' ? cloneData(previousData) : {};
 
       // Apply defaults.
       for (i = 0, keys = Object.keys(schema), keysLength = keys.length; i < keysLength; i++) {
         defaultValue = schema[keys[i]].default;
         if (data[keys[i]] !== undefined) { continue; }
+        // Clone default value if object so components don't share object
         data[keys[i]] = defaultValue && defaultValue.constructor === Object
           ? utils.clone(defaultValue)
           : defaultValue;
@@ -468,6 +470,25 @@ module.exports.registerComponent = function (name, definition) {
   return NewComponent;
 };
 
+/**
+* Clones component data.
+* Clone only the properties that are plain objects while
+* keeping a reference for the rest.
+*
+* @param data - Component data to clone.
+* @returns Cloned data.
+*/
+function cloneData (data) {
+  var clone = {};
+  var parsedProperty;
+  var key;
+  for (key in data) {
+    parsedProperty = data[key];
+    clone[key] = parsedProperty.constructor === Object ? utils.clone(parsedProperty) : parsedProperty;
+  }
+  return clone;
+}
+
 /**
 * Object extending with checking for single-property schema.
 *

tests/core/component.test.js

@@ -1,4 +1,4 @@
-/* global AFRAME, assert, process, suite, teardown, test, setup, sinon, HTMLElement */
+/* global AFRAME, assert, process, suite, teardown, test, setup, sinon, HTMLElement, HTMLHeadElement */
 var Component = require('core/component');
 var components = require('index').components;
 
@@ -192,6 +192,7 @@ suite('Component', function () {
 
     setup(function () {
       el = entityFactory();
+      components.dummy = undefined;
     });
 
     test('emits componentchanged for multi-prop', function (done) {
@@ -356,6 +357,75 @@ suite('Component', function () {
       });
       el.setAttribute('material', '');
     });
+
+    test('a selector property default is not cloned into data', function () {
+      registerComponent('dummy', {
+        schema: {type: 'selector', default: document.body}
+      });
+      var el = document.createElement('a-entity');
+      el.hasLoaded = true;
+      el.setAttribute('dummy', 'head');
+      el.components.dummy.updateProperties('');
+      assert.equal(el.components.dummy.data, el.components.dummy.schema.default);
+    });
+
+    test('a plain object schema default is cloned into data', function () {
+      registerComponent('dummy', {
+        schema: {type: 'vec3', default: {x: 1, y: 1, z: 1}}
+      });
+      var el = document.createElement('a-entity');
+      el.hasLoaded = true;
+      el.setAttribute('dummy', '2 2 2');
+      el.components.dummy.updateProperties('');
+      assert.notEqual(el.components.dummy.data, el.components.dummy.schema.default);
+      assert.deepEqual(el.components.dummy.data, {x: 1, y: 1, z: 1});
+    });
+
+    test.only('do not clone properties from attrValue into data that are not plain objects', function () {
+      registerComponent('dummy', {
+        schema: {
+          color: {default: 'blue'},
+          direction: {type: 'vec3'},
+          el: {type: 'selector', default: 'body'}
+        }
+      });
+      var el = document.createElement('a-entity');
+      el.hasLoaded = true;
+      el.setAttribute('dummy', '');
+      assert.notOk(el.components.dummy.attrValue.el);
+      // The direction property will be preserved
+      // across updateProperties calls but cloned
+      // into a different object
+      el.components.dummy.updateProperties({
+        color: 'green',
+        direction: {x: 1, y: 1, z: 1},
+        el: document.head
+      });
+      el.components.dummy.updateProperties({
+        color: 'red',
+        el: document.head
+      });
+      var data = el.getAttribute('dummy');
+      var attrValue = el.components.dummy.attrValue;
+      assert.notEqual(data, attrValue);
+      assert.equal(data.color, attrValue.color);
+      // The HTMLElement is not cloned in attrValue
+      // a reference is shared instead.
+      assert.equal(data.el, attrValue.el);
+      assert.equal(data.el.constructor, HTMLHeadElement);
+      assert.notEqual(data.direction, attrValue.direction);
+      assert.deepEqual(data.direction, {x: 1, y: 1, z: 1});
+      assert.deepEqual(attrValue.direction, {x: 1, y: 1, z: 1});
+      el.components.dummy.updateProperties({
+        color: 'red',
+        direction: {x: 1, y: 1, z: 1}
+      });
+      data = el.getAttribute('dummy');
+      // The HTMLElement is not cloned in attrValue
+      // a reference is shared instead.
+      assert.equal(data.el.constructor, HTMLHeadElement);
+      assert.equal(data.el, el.components.dummy.attrValue.el);
+    });
   });
 
   suite('resetProperty', function () {