From b4026e0b9f8f89fbaf694c24b8de4fcd8a1ba5f3 Mon Sep 17 00:00:00 2001
From: Christopher Garrett <me@pzuraq.com>
Date: Wed, 1 Aug 2018 16:53:56 -0700
Subject: [PATCH] [bugfix] Prevents the recursive redefinition of root chains

This fix prevents an edge case where recursive call to `writableChains`
causes the root `_chains` ChainNode on a meta to be overwritten,
resulting in a chains node that does not match up with the `watching`
state of the meta.

This can occur when:

1. A computed property has been setup on a class, which lazily delays
finalizing chains.
2. An observer is then added to the class, which eagerly forces its
dependencies to setup their chains.

In the case observed, it was only triggered because there were some
number of intermediate aliases between the `computed` and the
`observer`. Aliases are not volatile, but they also don't use the
computed cache for values so they cannot rely on `getCachedValueFor`
when setting up chains. This results in eager fetching of computed
values, which leads to the nested recursive call.

This PR solves the problem by ensuring that the root chain node is set
on the meta before we attempt to add any values to it (e.g. with
`copy`). The copy method has been renamed to `copyTo` to indicate that
it is not actually creating or returning a copy (open to suggestions
for a better name).
---
 packages/ember-meta/lib/meta.ts           | 10 ++---
 packages/ember-metal/lib/chains.ts        |  6 +--
 packages/ember-metal/tests/chains_test.js | 51 +++++++++++++++++++++++
 3 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/packages/ember-meta/lib/meta.ts b/packages/ember-meta/lib/meta.ts
index aad90b85a62..a2e707cc338 100644
--- a/packages/ember-meta/lib/meta.ts
+++ b/packages/ember-meta/lib/meta.ts
@@ -323,12 +323,12 @@ export class Meta {
     );
     let ret = this._chains;
     if (ret === undefined) {
-      if (this.parent === null) {
-        ret = create(this.source);
-      } else {
-        ret = this.parent.writableChains(create).copy(this.source);
+      this._chains = ret = create(this.source);
+
+      if (this.parent !== null) {
+        let parentChains = this.parent.writableChains(create);
+        parentChains.copyTo(ret);
       }
-      this._chains = ret;
     }
     return ret;
   }
diff --git a/packages/ember-metal/lib/chains.ts b/packages/ember-metal/lib/chains.ts
index d76e2065f5e..55a279f576d 100644
--- a/packages/ember-metal/lib/chains.ts
+++ b/packages/ember-metal/lib/chains.ts
@@ -208,18 +208,16 @@ class ChainNode {
   }
 
   // copies a top level object only
-  copy(obj: any) {
-    let ret = makeChainNode(obj);
+  copyTo(target: ChainNode) {
     let paths = this.paths;
     if (paths !== undefined) {
       let path;
       for (path in paths) {
         if (paths[path] > 0) {
-          ret.add(path);
+          target.add(path);
         }
       }
     }
-    return ret;
   }
 
   // called on the root node of a chain to setup watchers on the specified
diff --git a/packages/ember-metal/tests/chains_test.js b/packages/ember-metal/tests/chains_test.js
index 90bc2f3d040..91aa9bfc2ab 100644
--- a/packages/ember-metal/tests/chains_test.js
+++ b/packages/ember-metal/tests/chains_test.js
@@ -5,6 +5,7 @@ import {
   finishChains,
   defineProperty,
   computed,
+  alias,
   notifyPropertyChange,
   watch,
   unwatch,
@@ -152,5 +153,55 @@ moduleFor(
       assert.equal(watcherCount(obj.a, 'b.c'), 0);
       assert.equal(watcherCount(obj.a.b, 'c'), 0);
     }
+
+    ['@test writable chains is not defined more than once'](assert) {
+      assert.expect(0);
+      function didChange() {}
+
+      let obj = {
+        foo: {
+          bar: {
+            baz: {
+              value: 123,
+            },
+          },
+        },
+      };
+
+      // Setup object like a constructor, which delays initializing values in chains
+      let parentMeta = meta(obj);
+      parentMeta.proto = obj;
+
+      // Define a standard computed property, which will eventually setup dependencies
+      defineProperty(
+        obj,
+        'bar',
+        computed('foo.bar', {
+          get() {
+            return this.foo.bar;
+          },
+        })
+      );
+
+      // Define some aliases, which will proxy chains along
+      defineProperty(obj, 'baz', alias('bar.baz'));
+      defineProperty(obj, 'value', alias('baz.value'));
+
+      // Define an observer, which will eagerly attempt to setup chains and watch
+      // their values. This follows the aliases eagerly, and forces the first
+      // computed to actually set up its values/dependencies for chains. If
+      // writableChains was not already defined, this results in multiple root
+      // chain nodes being defined on the same object meta.
+      addObserver(obj, 'value', null, didChange);
+
+      let childObj = Object.create(obj);
+
+      let childMeta = meta(childObj);
+
+      finishChains(childMeta);
+
+      // If we can unwatch the root computed, then chains was not overwritten
+      unwatch(childObj, 'foo.bar');
+    }
   }
 );