Merge pull request #19106 from emberjs/core-object-with-destroy

[BUGFIX lts] Ensure `destroy` methods on `CoreObject` are invoked.

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;
   }
 

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);
+      });
+    }
   }
 );

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);
+      });
+    }
   }
 );

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');
     }