From 6b3c1c000a2179ef65687a117d39f3049e558503 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Tue, 25 Aug 2020 12:36:07 -0400 Subject: [PATCH] [BUGFIX lts] Ensure `destroy` methods on `CoreObject` are invoked. When an instance of `CoreObject` is destroyed with `@ember/destroyable` (or the polyfill when using Ember 3.20+), the instances own `destroy` method is never called. In _general_ "this is fine" because the majority of consumers are using `willDestroy` for their custom object teardown needs. Unfortunately, this isn't _quite_ good enough and any consumers that are directly relying on extending from `CoreObject` and having a custom `destroy` method called will no longer function. The prime example of this is the `Owner` instance itself. Currently, running `destroy(owner)` **does not** actually destroy any instantiated objects in the container. This is because `ContainerProxyMixin` implements `destroy` in order to call `this.__container__.destroy()` which is where the actual cleanup happens. That whole system should be refactored to leveraging `@ember/destroyable` system directly, but in the meantime this case (and any others where folks have implemented a destroy method in Mixins or other subclasses of Ember.CoreObject) should **absolutely** be fixed. This fix ensures that any `destroy(someInstance)` calls on a `CoreObject` directly invoke the `destroy` method on the instance in the same manner as Ember < 3.20 (`.destroy()` is called eagerly, before `willDestroy`). (cherry picked from commit 4d55dae3a7ebe2d9ba5b2181179c587c69c33fc7) --- .../runtime/lib/system/core_object.js | 19 +++++++++++- .../tests/mixins/container_proxy_test.js | 22 ++++++++++++++ .../runtime/tests/system/core_object_test.js | 17 +++++++++++ .../@ember/application/tests/reset_test.js | 29 +++++++++++-------- 4 files changed, 74 insertions(+), 13 deletions(-) diff --git a/packages/@ember/-internals/runtime/lib/system/core_object.js b/packages/@ember/-internals/runtime/lib/system/core_object.js index a8f209d9153..f90d38fdd3b 100644 --- a/packages/@ember/-internals/runtime/lib/system/core_object.js +++ b/packages/@ember/-internals/runtime/lib/system/core_object.js @@ -39,6 +39,14 @@ const prototypeMixinMap = new WeakMap(); const initCalled = DEBUG ? new WeakSet() : undefined; // only used in debug builds to enable the proxy trap +const destroyCalled = new Set(); + +function ensureDestroyCalled(instance) { + if (!destroyCalled.has(instance)) { + instance.destroy(); + } +} + function initialize(obj, properties) { let m = meta(obj); @@ -256,6 +264,7 @@ class CoreObject { }); } + registerDestructor(self, ensureDestroyCalled, true); registerDestructor(self, () => self.willDestroy()); // disable chains @@ -512,7 +521,15 @@ class CoreObject { @public */ destroy() { - destroy(this); + // Used to ensure that manually calling `.destroy()` does not immediately call destroy again + destroyCalled.add(this); + + try { + destroy(this); + } finally { + destroyCalled.delete(this); + } + return this; } diff --git a/packages/@ember/-internals/runtime/tests/mixins/container_proxy_test.js b/packages/@ember/-internals/runtime/tests/mixins/container_proxy_test.js index 860be7f0005..7119e9616e3 100644 --- a/packages/@ember/-internals/runtime/tests/mixins/container_proxy_test.js +++ b/packages/@ember/-internals/runtime/tests/mixins/container_proxy_test.js @@ -4,6 +4,7 @@ import ContainerProxy from '../../lib/mixins/container_proxy'; import EmberObject from '../../lib/system/object'; import { run, schedule } from '@ember/runloop'; import { moduleFor, AbstractTestCase } from 'internal-test-helpers'; +import { destroy } from '@glimmer/runtime'; moduleFor( '@ember/-internals/runtime/mixins/container_proxy', @@ -44,5 +45,26 @@ moduleFor( this.instance.destroy(); }); } + + '@test being destroyed by @ember/destroyable properly destroys the container and created instances'( + assert + ) { + assert.expect(1); + + this.registry.register( + 'service:foo', + class FooService extends EmberObject { + willDestroy() { + assert.ok(true, 'is properly destroyed'); + } + } + ); + + this.instance.lookup('service:foo'); + + run(() => { + destroy(this.instance); + }); + } } ); diff --git a/packages/@ember/-internals/runtime/tests/system/core_object_test.js b/packages/@ember/-internals/runtime/tests/system/core_object_test.js index 7007285dd5a..c788ad15e46 100644 --- a/packages/@ember/-internals/runtime/tests/system/core_object_test.js +++ b/packages/@ember/-internals/runtime/tests/system/core_object_test.js @@ -3,6 +3,8 @@ import { get, set, observer } from '@ember/-internals/metal'; import CoreObject from '../../lib/system/core_object'; import { moduleFor, AbstractTestCase, buildOwner, runLoopSettled } from 'internal-test-helpers'; import { track } from '@glimmer/validator'; +import { destroy } from '@glimmer/runtime'; +import { run } from '@ember/runloop'; moduleFor( 'Ember.CoreObject', @@ -129,5 +131,20 @@ moduleFor( assert.ok(true, 'We did not error'); }); } + '@test destroy method is called when being destroyed by @ember/destroyable'(assert) { + assert.expect(1); + + class TestObject extends CoreObject { + destroy() { + assert.ok(true, 'destroy was invoked'); + } + } + + let instance = TestObject.create(); + + run(() => { + destroy(instance); + }); + } } ); diff --git a/packages/@ember/application/tests/reset_test.js b/packages/@ember/application/tests/reset_test.js index 1cbb81e9899..b3454826dc7 100644 --- a/packages/@ember/application/tests/reset_test.js +++ b/packages/@ember/application/tests/reset_test.js @@ -3,6 +3,7 @@ import { get } from '@ember/-internals/metal'; import Controller from '@ember/controller'; import { Router } from '@ember/-internals/routing'; import { moduleFor, AutobootApplicationTestCase } from 'internal-test-helpers'; +import { CoreObject } from '@ember/-internals/runtime'; moduleFor( 'Application - resetting', @@ -71,31 +72,35 @@ moduleFor( let eventDispatcherWasSetup = 0; let eventDispatcherWasDestroyed = 0; - let mockEventDispatcher = { + class FakeEventDispatcher extends CoreObject { setup() { eventDispatcherWasSetup++; - }, + } + destroy() { + if (this.isDestroying) { + return; + } + super.destroy(); + eventDispatcherWasDestroyed++; - }, - }; + } + } run(() => { this.createApplication(); - this.add('event_dispatcher:main', { - create: () => mockEventDispatcher, - }); + this.add('event_dispatcher:main', FakeEventDispatcher); - assert.equal(eventDispatcherWasSetup, 0); - assert.equal(eventDispatcherWasDestroyed, 0); + assert.equal(eventDispatcherWasSetup, 0, 'not setup yet'); + assert.equal(eventDispatcherWasDestroyed, 0, 'not destroyed yet'); }); - assert.equal(eventDispatcherWasSetup, 1); - assert.equal(eventDispatcherWasDestroyed, 0); + assert.equal(eventDispatcherWasSetup, 1, 'setup'); + assert.equal(eventDispatcherWasDestroyed, 0, 'still not destroyed'); this.application.reset(); - assert.equal(eventDispatcherWasDestroyed, 1); + assert.equal(eventDispatcherWasDestroyed, 1, 'destroyed'); assert.equal(eventDispatcherWasSetup, 2, 'setup called after reset'); }