From 15e4fa03929ab4bb0501d4e533f72ea667ea72fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Mu=C3=B1oz?= Date: Sun, 31 Jan 2016 22:12:55 -0500 Subject: [PATCH 1/6] Assert that a property is watched before trying to unwatch it --- packages/ember-metal/lib/watch_key.js | 8 +++++--- packages/ember-metal/lib/watch_path.js | 7 ++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/ember-metal/lib/watch_key.js b/packages/ember-metal/lib/watch_key.js index 2247f8c78e2..f5fc66bf24c 100644 --- a/packages/ember-metal/lib/watch_key.js +++ b/packages/ember-metal/lib/watch_key.js @@ -1,7 +1,6 @@ import isEnabled from 'ember-metal/features'; -import { - meta as metaFor -} from 'ember-metal/meta'; +import { assert } from 'ember-metal/debug'; +import { meta as metaFor } from 'ember-metal/meta'; import { MANDATORY_SETTER_FUNCTION, DEFAULT_GETTER_FUNCTION, @@ -81,6 +80,9 @@ if (isEnabled('mandatory-setter')) { export function unwatchKey(obj, keyName, meta) { var m = meta || metaFor(obj); let count = m.peekWatching(keyName); + + assert(`Tried to unwatch '${keyName}' on object but no one was watching`, count > 0); + if (count === 1) { m.writeWatching(keyName, 0); diff --git a/packages/ember-metal/lib/watch_path.js b/packages/ember-metal/lib/watch_path.js index 17af226969c..37d11c8310c 100644 --- a/packages/ember-metal/lib/watch_path.js +++ b/packages/ember-metal/lib/watch_path.js @@ -1,6 +1,5 @@ -import { - meta as metaFor -} from 'ember-metal/meta'; +import { assert } from 'ember-metal/debug'; +import { meta as metaFor } from 'ember-metal/meta'; import { ChainNode } from 'ember-metal/chains'; // get the chains for the current object. If the current object has @@ -32,6 +31,8 @@ export function unwatchPath(obj, keyPath, meta) { var m = meta || metaFor(obj); let counter = m.peekWatching(keyPath) || 0; + assert(`Tried to unwatch '${keyPath}' on object but no one was watching`, counter > 0); + if (counter === 1) { m.writeWatching(keyPath, 0); chainsFor(obj, m).remove(keyPath); From f0608dcbf36f10def9041a364c2486aefa924e38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Mu=C3=B1oz?= Date: Mon, 1 Feb 2016 01:05:16 -0500 Subject: [PATCH 2/6] Add assertions for adding and removing observers --- packages/ember-metal/lib/meta_listeners.js | 27 ++++++++++++++++++++- packages/ember-metal/tests/observer_test.js | 26 ++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/packages/ember-metal/lib/meta_listeners.js b/packages/ember-metal/lib/meta_listeners.js index 736dda52585..034fec3486c 100644 --- a/packages/ember-metal/lib/meta_listeners.js +++ b/packages/ember-metal/lib/meta_listeners.js @@ -1,3 +1,5 @@ +import { assert, runInDebug } from 'ember-metal/debug'; + /* When we render a rich template hierarchy, the set of events that *might* happen tends to be much larger than the set of events that @@ -20,6 +22,24 @@ export var protoMethods = { if (!this._listeners) { this._listeners = []; } + + runInDebug(() => { + // Assert that an observer is only used once. + let listeners = this._listeners; + let matchFound = false; + + for (let i = listeners.length - 4; i >= 0; i -= 4) { + if (listeners[i+1] === target && + listeners[i+2] === method && + listeners[i] === eventName) { + matchFound = true; + break; + } + } + + assert(`Tried to add a duplicate listener for ${eventName}`, !matchFound); + }); + this._listeners.push(eventName, target, method, flags); }, @@ -39,6 +59,7 @@ export var protoMethods = { }, removeFromListeners(eventName, target, method, didRemove) { + let matchFound = false; let pointer = this; while (pointer) { let listeners = pointer._listeners; @@ -51,12 +72,14 @@ export var protoMethods = { didRemove(eventName, target, listeners[index + 2]); } listeners.splice(index, 4); + matchFound = true; } else { // we are trying to remove an inherited listener, so we do // just-in-time copying to detach our own listeners from // our inheritance chain. this._finalizeListeners(); - return this.removeFromListeners(eventName, target, method); + this.removeFromListeners(eventName, target, method); + return; } } } @@ -64,6 +87,8 @@ export var protoMethods = { if (pointer._listenersFinalized) { break; } pointer = pointer.parent; } + + assert(`Tried to remove a non-existant listener for ${eventName}`, matchFound); }, matchingListeners(eventName) { diff --git a/packages/ember-metal/tests/observer_test.js b/packages/ember-metal/tests/observer_test.js index 0d845807034..a1ad64e7f99 100644 --- a/packages/ember-metal/tests/observer_test.js +++ b/packages/ember-metal/tests/observer_test.js @@ -1166,3 +1166,29 @@ testBoth('observer switched on and off and then setter', function (get, set) { deepEqual(Object.keys(beer), ['type']); }); + +testBoth('adding the same observer multiple times throws an error', function (get, set) { + function Beer() { } + Beer.prototype.type = 'ipa'; + + let beer = new Beer(); + + addObserver(beer, 'type', K); + + throws(() => { + addObserver(beer, 'type', K); + }, /Tried to add.*type/); +}); + +testBoth('removing a non-existant observer throws an error', function (get, set) { + function Beer() { } + Beer.prototype.type = 'ipa'; + + let beer = new Beer(); + + addObserver(beer, 'type', function() {}); + + throws(() => { + removeObserver(beer, 'type', K); + }, /Tried to remove.*type/); +}); From ff235f3af570063fd2dbf526c2fd32a393b5bfe5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Mu=C3=B1oz?= Date: Mon, 1 Feb 2016 21:25:18 -0500 Subject: [PATCH 3/6] [BUGFIX] Fix/remove several invalid tests --- .../ember-metal/tests/binding/connect_test.js | 6 ++++-- packages/ember-metal/tests/events_test.js | 12 +++++++----- packages/ember-metal/tests/keys_test.js | 14 ++------------ packages/ember-metal/tests/meta_test.js | 17 ++++++++++++----- .../tests/watching/is_watching_test.js | 14 -------------- 5 files changed, 25 insertions(+), 38 deletions(-) diff --git a/packages/ember-metal/tests/binding/connect_test.js b/packages/ember-metal/tests/binding/connect_test.js index 4ec8384a72b..d5e9b3db2a5 100644 --- a/packages/ember-metal/tests/binding/connect_test.js +++ b/packages/ember-metal/tests/binding/connect_test.js @@ -90,7 +90,7 @@ testBoth('Connecting a binding to path', function(get, set) { equal(get(a, 'foo'), 'BIFF', 'a should have changed'); }); -testBoth('Calling connect more than once', function(get, set) { +testBoth('Calling connect more than once throws an error', function(get, set) { var b = { bar: 'BAR' }; var a = { foo: 'FOO', b: b }; @@ -100,7 +100,9 @@ testBoth('Calling connect more than once', function(get, set) { performTest(binding, a, b, get, set, function () { binding.connect(a); - binding.connect(a); + throws(() => { + binding.connect(a); + }, /Tried to add.*b.bar/); }); }); diff --git a/packages/ember-metal/tests/events_test.js b/packages/ember-metal/tests/events_test.js index 68e507b8ffc..7e074e4088a 100644 --- a/packages/ember-metal/tests/events_test.js +++ b/packages/ember-metal/tests/events_test.js @@ -56,15 +56,15 @@ QUnit.test('listeners should be inherited', function() { }); -QUnit.test('adding a listener more than once should only invoke once', function() { +QUnit.test('adding a listener more than once throws an error', function() { var obj = {}; var count = 0; var F = function() { count++; }; addListener(obj, 'event!', F); - addListener(obj, 'event!', F); - sendEvent(obj, 'event!'); - equal(count, 1, 'should only invoke once'); + throws(function() { + addListener(obj, 'event!', F); + }, /Tried to add a duplicate listener for event!/); }); QUnit.test('adding a listener with a target should invoke with target', function() { @@ -196,7 +196,9 @@ QUnit.test('while suspended, it should not be possible to add a duplicate listen addListener(obj, 'event!', target, target.method); function callback() { - addListener(obj, 'event!', target, target.method); + throws(function() { + addListener(obj, 'event!', target, target.method); + }, /Tried to add a duplicate listener for event!/); } sendEvent(obj, 'event!'); diff --git a/packages/ember-metal/tests/keys_test.js b/packages/ember-metal/tests/keys_test.js index 17f0a996ed1..2dbb9914d24 100644 --- a/packages/ember-metal/tests/keys_test.js +++ b/packages/ember-metal/tests/keys_test.js @@ -107,13 +107,8 @@ QUnit.test('observers switched on and off with setter in between (observed prope var yetAnotherBeer = new Beer(); addObserver(yetAnotherBeer, 'type', K); set(yetAnotherBeer, 'type', 'ale'); - removeObserver(beer, 'type', K); + removeObserver(yetAnotherBeer, 'type', K); deepEqual(Object.keys(yetAnotherBeer), ['type'], 'addObserver -> set -> removeOjbserver'); - - var itsMyLastBeer = new Beer(); - set(itsMyLastBeer, 'type', 'ale'); - removeObserver(beer, 'type', K); - deepEqual(Object.keys(itsMyLastBeer), ['type'], 'set -> removeObserver'); }); QUnit.test('observers switched on and off with setter in between (observed property is shadowing one on the prototype)', function () { @@ -132,13 +127,8 @@ QUnit.test('observers switched on and off with setter in between (observed prope var yetAnotherBeer = new Beer(); addObserver(yetAnotherBeer, 'type', K); set(yetAnotherBeer, 'type', 'ale'); - removeObserver(beer, 'type', K); + removeObserver(yetAnotherBeer, 'type', K); deepEqual(Object.keys(yetAnotherBeer), ['type'], 'addObserver -> set -> removeObserver'); - - var itsMyLastBeer = new Beer(); - set(itsMyLastBeer, 'type', 'ale'); - removeObserver(beer, 'type', K); - deepEqual(Object.keys(itsMyLastBeer), ['type'], 'set -> removeObserver'); }); QUnit.test('observers switched on and off with setter in between', function () { diff --git a/packages/ember-metal/tests/meta_test.js b/packages/ember-metal/tests/meta_test.js index a047c4dbcb2..55fdff6a583 100644 --- a/packages/ember-metal/tests/meta_test.js +++ b/packages/ember-metal/tests/meta_test.js @@ -66,12 +66,19 @@ QUnit.test('meta.listeners inheritance', function(assert) { assert.equal(matching.length, 3); }); -QUnit.test('meta.listeners deduplication', function(assert) { +QUnit.test('meta.listeners adding an existing listener throws an error', function(assert) { let t = {}; let m = meta({}); m.addToListeners('hello', t, 'm', 0); - m.addToListeners('hello', t, 'm', 0); - let matching = m.matchingListeners('hello'); - assert.equal(matching.length, 3); - assert.equal(matching[0], t); + throws(() => { + m.addToListeners('hello', t, 'm', 0); + }, /Tried to add a duplicate listener for hello/); +}); + +QUnit.test('meta.listeners removing a non-existing listener throws an error', function(assert) { + let t = {}; + let m = meta({}); + throws(() => { + m.removeFromListeners('hello', t, 'm'); + }, /Tried to remove a non-existant listener for hello/); }); diff --git a/packages/ember-metal/tests/watching/is_watching_test.js b/packages/ember-metal/tests/watching/is_watching_test.js index e49ce7c7373..b8fcc7a3cda 100644 --- a/packages/ember-metal/tests/watching/is_watching_test.js +++ b/packages/ember-metal/tests/watching/is_watching_test.js @@ -1,10 +1,6 @@ import { computed } from 'ember-metal/computed'; import { get } from 'ember-metal/property_get'; import { defineProperty } from 'ember-metal/properties'; -import { - Mixin, - observer -} from 'ember-metal/mixin'; import { addObserver, removeObserver @@ -25,16 +21,6 @@ function testObserver(setup, teardown, key) { equal(isWatching(obj, key), false, 'isWatching is false after observers are removed'); } -QUnit.test('isWatching is true for regular local observers', function() { - testObserver(function(obj, key, fn) { - Mixin.create({ - didChange: observer(key, fn) - }).apply(obj); - }, function(obj, key, fn) { - removeObserver(obj, key, obj, fn); - }); -}); - QUnit.test('isWatching is true for nonlocal observers', function() { testObserver(function(obj, key, fn) { addObserver(obj, key, obj, fn); From 63d38c6d1723eb12e16170f8df260cec9c158201 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Mu=C3=B1oz?= Date: Mon, 1 Feb 2016 23:21:37 -0500 Subject: [PATCH 4/6] [BUGFIX] Fix how KeyStream deals with destroyed objects --- packages/ember-metal/lib/streams/key-stream.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/ember-metal/lib/streams/key-stream.js b/packages/ember-metal/lib/streams/key-stream.js index 56b65a09c22..0266332de4e 100644 --- a/packages/ember-metal/lib/streams/key-stream.js +++ b/packages/ember-metal/lib/streams/key-stream.js @@ -63,7 +63,7 @@ export default BasicStream.extend({ if (object !== this.observedObject) { this._clearObservedObject(); - if (object && typeof object === 'object') { + if (typeof object === 'object' && object !== null && !object.isDestroyed) { addObserver(object, this.key, this, this.notify); this.observedObject = object; } @@ -73,10 +73,11 @@ export default BasicStream.extend({ _super$deactivate: BasicStream.prototype.deactivate, _clearObservedObject() { - if (this.observedObject) { - removeObserver(this.observedObject, this.key, this, this.notify); - this.observedObject = null; + let observedObject = this.observedObject; + if (observedObject !== null && !observedObject.isDestroyed) { + removeObserver(observedObject, this.key, this, this.notify); } + this.observedObject = null; }, deactivate() { From 2cc7dbcd678b2bc70f0727447616946ca5d9a490 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Mu=C3=B1oz?= Date: Mon, 1 Feb 2016 23:39:22 -0500 Subject: [PATCH 5/6] [BUGFIX] Don't try to unwatch the length property of an array --- packages/ember-metal/lib/watch_key.js | 3 +++ packages/ember-metal/lib/watch_path.js | 3 +++ 2 files changed, 6 insertions(+) diff --git a/packages/ember-metal/lib/watch_key.js b/packages/ember-metal/lib/watch_key.js index f5fc66bf24c..5c78bdfd4c5 100644 --- a/packages/ember-metal/lib/watch_key.js +++ b/packages/ember-metal/lib/watch_key.js @@ -78,6 +78,9 @@ if (isEnabled('mandatory-setter')) { } export function unwatchKey(obj, keyName, meta) { + // can't unwatch length on Array - it is special... + if (keyName === 'length' && Array.isArray(obj)) { return; } + var m = meta || metaFor(obj); let count = m.peekWatching(keyName); diff --git a/packages/ember-metal/lib/watch_path.js b/packages/ember-metal/lib/watch_path.js index 37d11c8310c..d963f293569 100644 --- a/packages/ember-metal/lib/watch_path.js +++ b/packages/ember-metal/lib/watch_path.js @@ -28,6 +28,9 @@ export function watchPath(obj, keyPath, meta) { } export function unwatchPath(obj, keyPath, meta) { + // can't unwatch length on Array - it is special... + if (keyPath === 'length' && Array.isArray(obj)) { return; } + var m = meta || metaFor(obj); let counter = m.peekWatching(keyPath) || 0; From 7552f6fb0bed81d14e8c3f6958a832dc0625cfa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Mu=C3=B1oz?= Date: Tue, 2 Feb 2016 00:35:15 -0500 Subject: [PATCH 6/6] [BUGFIX] Fix arrangedContent observer lifecycle --- .../ember-runtime/lib/system/array_proxy.js | 62 ++++++++++--------- .../system/array_proxy/content_update_test.js | 2 +- .../tests/system/array_proxy/length_test.js | 2 +- 3 files changed, 36 insertions(+), 30 deletions(-) diff --git a/packages/ember-runtime/lib/system/array_proxy.js b/packages/ember-runtime/lib/system/array_proxy.js index 931f73b804b..ddd7c345b1a 100644 --- a/packages/ember-runtime/lib/system/array_proxy.js +++ b/packages/ember-runtime/lib/system/array_proxy.js @@ -23,6 +23,11 @@ import alias from 'ember-metal/alias'; var OUT_OF_RANGE_EXCEPTION = 'Index out of range'; var EMPTY = []; +const ARRANGED_CONTENT_ARRAY_OBSERVER_EVENTS = { + willChange: 'arrangedContentArrayWillChange', + didChange: 'arrangedContentArrayDidChange' +}; + function K() { return this; } /** @@ -192,7 +197,10 @@ var ArrayProxy = EmberObject.extend(MutableArray, { this._prevContent = content; if (content) { - assert(`ArrayProxy expects an Array or Ember.ArrayProxy, but you passed ${typeof content}`, isArray(content) || content.isDestroyed); + assert( + `ArrayProxy expects an Array or Ember.ArrayProxy, but you passed ${typeof content}`, + isArray(content) || content.isDestroyed + ); content.addArrayObserver(this, { willChange: 'contentArrayWillChange', @@ -202,41 +210,34 @@ var ArrayProxy = EmberObject.extend(MutableArray, { }, _arrangedContentDidChange: observer('arrangedContent', function() { - this._teardownArrangedContent(this._prevArrangedContent); - var arrangedContent = get(this, 'arrangedContent'); - var len = arrangedContent ? get(arrangedContent, 'length') : 0; - - assert('Can\'t set ArrayProxy\'s content to itself', arrangedContent !== this); + let arrangedContent = get(this, 'arrangedContent'); - this._setupArrangedContent(); + this._updateArrangedContent(arrangedContent); + var len = arrangedContent ? get(arrangedContent, 'length') : 0; this.arrangedContentDidChange(this); this.arrangedContentArrayDidChange(this, 0, undefined, len); }), - _setupArrangedContent() { - var arrangedContent = get(this, 'arrangedContent'); - this._prevArrangedContent = arrangedContent; + _updateArrangedContent(newArrangedContent) { + let observedArrangedContent = this._observedArrangedContent; + if (observedArrangedContent !== newArrangedContent) { + if (observedArrangedContent) { + observedArrangedContent.removeArrayObserver(this, ARRANGED_CONTENT_ARRAY_OBSERVER_EVENTS); + } - if (arrangedContent) { - assert(`ArrayProxy expects an Array or Ember.ArrayProxy, but you passed ${typeof arrangedContent}`, - isArray(arrangedContent) || arrangedContent.isDestroyed); + if (newArrangedContent) { + assert(`Can't set ArrayProxy's content to itself`, newArrangedContent !== this); - arrangedContent.addArrayObserver(this, { - willChange: 'arrangedContentArrayWillChange', - didChange: 'arrangedContentArrayDidChange' - }); - } - }, + assert( + `ArrayProxy expects an Array or Ember.ArrayProxy, but you passed ${typeof newArrangedContent}`, + isArray(newArrangedContent) || newArrangedContent.isDestroyed + ); - _teardownArrangedContent() { - var arrangedContent = get(this, 'arrangedContent'); + newArrangedContent.addArrayObserver(this, ARRANGED_CONTENT_ARRAY_OBSERVER_EVENTS); + } - if (arrangedContent) { - arrangedContent.removeArrayObserver(this, { - willChange: 'arrangedContentArrayWillChange', - didChange: 'arrangedContentArrayDidChange' - }); + this._observedArrangedContent = newArrangedContent; } }, @@ -369,14 +370,19 @@ var ArrayProxy = EmberObject.extend(MutableArray, { }, init() { + this.__each = undefined; this._didInitArrayProxy = true; + this._content = undefined; + this._prevContent = undefined; + this._observedArrangedContent = undefined; this._super(...arguments); this._setupContent(); - this._setupArrangedContent(); + + this._updateArrangedContent(get(this, 'arrangedContent')); }, willDestroy() { - this._teardownArrangedContent(); + this._updateArrangedContent(undefined); this._teardownContent(this.get('content')); } }); diff --git a/packages/ember-runtime/tests/system/array_proxy/content_update_test.js b/packages/ember-runtime/tests/system/array_proxy/content_update_test.js index c8a269bbde6..016ad37d161 100644 --- a/packages/ember-runtime/tests/system/array_proxy/content_update_test.js +++ b/packages/ember-runtime/tests/system/array_proxy/content_update_test.js @@ -2,7 +2,7 @@ import { computed } from 'ember-metal/computed'; import ArrayProxy from 'ember-runtime/system/array_proxy'; import { A as emberA } from 'ember-runtime/system/native_array'; -QUnit.module('Ember.ArrayProxy - content update'); +QUnit.module('ArrayProxy - content update'); QUnit.test('The `contentArrayDidChange` method is invoked after `content` is updated.', function() { var proxy; diff --git a/packages/ember-runtime/tests/system/array_proxy/length_test.js b/packages/ember-runtime/tests/system/array_proxy/length_test.js index 669769cbb1e..9d8af6f3410 100644 --- a/packages/ember-runtime/tests/system/array_proxy/length_test.js +++ b/packages/ember-runtime/tests/system/array_proxy/length_test.js @@ -4,7 +4,7 @@ import { observer } from 'ember-metal/mixin'; import { computed } from 'ember-metal/computed'; import { A as a } from 'ember-runtime/system/native_array'; -QUnit.module('Ember.ArrayProxy - content change (length)'); +QUnit.module('ArrayProxy - content change (length)'); QUnit.test('array proxy + aliasedProperty complex test', function() { var aCalled, bCalled, cCalled, dCalled, eCalled;