Skip to content

Commit

Permalink
[bugfix] Prevents the recursive redefinition of root chains
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
pzuraq committed Aug 2, 2018
1 parent 24ce7de commit b4026e0
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 9 deletions.
10 changes: 5 additions & 5 deletions packages/ember-meta/lib/meta.ts
Expand Up @@ -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;
}
Expand Down
6 changes: 2 additions & 4 deletions packages/ember-metal/lib/chains.ts
Expand Up @@ -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
Expand Down
51 changes: 51 additions & 0 deletions packages/ember-metal/tests/chains_test.js
Expand Up @@ -5,6 +5,7 @@ import {
finishChains,
defineProperty,
computed,
alias,
notifyPropertyChange,
watch,
unwatch,
Expand Down Expand Up @@ -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');
}
}
);

0 comments on commit b4026e0

Please sign in to comment.