Bug 1250469 - Reduce number of ObserverService observers subscribed by the SDK Disposable class. draft
authorLuca Greco <lgreco@mozilla.com>
Thu, 19 May 2016 17:18:18 +0200
changeset 369674 e594053a76d599d8aa4d0864d3278a48edf466d5
parent 358343 5e9136916f72c2e28363b9be5ca90220d78ebe3b
child 372903 504a74206e100c6e607dcf15110c24eadfb3abc3
child 372904 73662301a26dd707763893a4de42b48ee95d5f89
child 372919 3dee0c4ed07408581e8fd6eb670d714a2053dee9
child 374994 b70252cf8381ab5464f08ff83cf83f922c8b9950
push id18890
push userluca.greco@alcacoop.it
push dateMon, 23 May 2016 11:29:29 +0000
bugs1250469
milestone49.0a1
Bug 1250469 - Reduce number of ObserverService observers subscribed by the SDK Disposable class. MozReview-Commit-ID: 32X9M07FbHO
addon-sdk/source/lib/sdk/core/disposable.js
addon-sdk/source/test/test-disposable.js
--- a/addon-sdk/source/lib/sdk/core/disposable.js
+++ b/addon-sdk/source/lib/sdk/core/disposable.js
@@ -3,31 +3,29 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 module.metadata = {
   "stability": "experimental"
 };
 
-
 const { Class } = require("./heritage");
 const { Observer, subscribe, unsubscribe, observe } = require("./observer");
-const { isWeak, WeakReference } = require("./reference");
+const { isWeak } = require("./reference");
+const SDKWeakSet = require("../lang/weak-set");
+
 const method = require("../../method/core");
 
 const unloadSubject = require('@loader/unload');
 const addonUnloadTopic = "sdk:loader:destroy";
 
-
-
 const uninstall = method("disposable/uninstall");
 exports.uninstall = uninstall;
 
-
 const shutdown = method("disposable/shutdown");
 exports.shutdown = shutdown;
 
 const disable = method("disposable/disable");
 exports.disable = disable;
 
 const upgrade = method("disposable/upgrade");
 exports.upgrade = upgrade;
@@ -37,38 +35,104 @@ exports.downgrade = downgrade;
 
 const unload = method("disposable/unload");
 exports.unload = unload;
 
 const dispose = method("disposable/dispose");
 exports.dispose = dispose;
 dispose.define(Object, object => object.dispose());
 
-
 const setup = method("disposable/setup");
 exports.setup = setup;
 setup.define(Object, (object, ...args) => object.setup(...args));
 
+// DisposablesUnloadObserver is the class which subscribe the
+// Observer Service to be notified when the add-on loader is
+// unloading to be able to dispose all the existent disposables.
+const DisposablesUnloadObserver = Class({
+  implements: [Observer],
+  initialize: function(...args) {
+    // Set of the non-weak disposables registered to be disposed.
+    this.disposables = new Set();
+    // Target of the weak disposables registered to be disposed
+    // (and tracked on this target using the SDK weak-set module).
+    this.weakDisposables = {};
+  },
+  subscribe(disposable) {
+    if (isWeak(disposable)) {
+      SDKWeakSet.add(this.weakDisposables, disposable);
+    } else {
+      this.disposables.add(disposable);
+    }
+  },
+  unsubscribe(disposable) {
+    if (isWeak(disposable)) {
+      SDKWeakSet.remove(this.weakDisposables, disposable);
+    } else {
+      this.disposables.delete(disposable);
+    }
+  },
+  tryUnloadDisposable(disposable) {
+    try {
+      if (disposable) {
+        unload(disposable);
+      }
+    } catch(e) {
+      console.error("Error unloading a",
+                    isWeak(disposable) ? "weak disposable" : "disposable",
+                    disposable, e);
+    }
+  },
+  unloadAll() {
+    // Remove all the subscribed disposables.
+    for (let disposable of this.disposables) {
+      this.tryUnloadDisposable(disposable);
+    }
+
+    this.disposables.clear();
+
+    // Remove all the subscribed weak disposables.
+    for (let disposable of SDKWeakSet.iterator(this.weakDisposables)) {
+      this.tryUnloadDisposable(disposable);
+    }
+
+    SDKWeakSet.clear(this.weakDisposables);
+  }
+});
+const disposablesUnloadObserver = new DisposablesUnloadObserver();
+
+// The DisposablesUnloadObserver instance is the only object which subscribes
+// the Observer Service directly, it observes add-on unload notifications in
+// order to trigger `unload` on all its subscribed disposables.
+observe.define(DisposablesUnloadObserver, (obj, subject, topic, data) => {
+  const isUnloadTopic = topic === addonUnloadTopic;
+  const isUnloadSubject = subject.wrappedJSObject === unloadSubject;
+  if (isUnloadTopic && isUnloadSubject) {
+    unsubscribe(disposablesUnloadObserver, addonUnloadTopic);
+    disposablesUnloadObserver.unloadAll();
+  }
+});
+
+subscribe(disposablesUnloadObserver, addonUnloadTopic, false);
 
 // Set's up disposable instance.
 const setupDisposable = disposable => {
-  subscribe(disposable, addonUnloadTopic, isWeak(disposable));
+  disposablesUnloadObserver.subscribe(disposable);
 };
 exports.setupDisposable = setupDisposable;
 
 // Tears down disposable instance.
 const disposeDisposable = disposable => {
-  unsubscribe(disposable, addonUnloadTopic);
+  disposablesUnloadObserver.unsubscribe(disposable);
 };
 exports.disposeDisposable = disposeDisposable;
 
 // Base type that takes care of disposing it's instances on add-on unload.
 // Also makes sure to remove unload listener if it's already being disposed.
 const Disposable = Class({
-  implements: [Observer],
   initialize: function(...args) {
     // First setup instance before initializing it's disposal. If instance
     // fails to initialize then there is no instance to be disposed at the
     // unload.
     setup(this, ...args);
     setupDisposable(this);
   },
   destroy: function(reason) {
@@ -81,50 +145,39 @@ const Disposable = Class({
     // Implement your initialize logic here.
   },
   dispose: function() {
     // Implement your cleanup logic here.
   }
 });
 exports.Disposable = Disposable;
 
-// Disposable instances observe add-on unload notifications in
-// order to trigger `unload` on them.
-observe.define(Disposable, (disposable, subject, topic, data) => {
-  const isUnloadTopic = topic === addonUnloadTopic;
-  const isUnloadSubject = subject.wrappedJSObject === unloadSubject;
-  if (isUnloadTopic && isUnloadSubject) {
-    unsubscribe(disposable, topic);
-    unload(disposable);
-  }
-});
-
 const unloaders = {
   destroy: dispose,
   uninstall: uninstall,
   shutdown: shutdown,
   disable: disable,
   upgrade: upgrade,
   downgrade: downgrade
-}
+};
+
 const unloaded = new WeakMap();
 unload.define(Disposable, (disposable, reason) => {
   if (!unloaded.get(disposable)) {
     unloaded.set(disposable, true);
     // Pick an unload handler associated with an unload
     // reason (falling back to destroy if not found) and
     // delegate unloading to it.
     const unload = unloaders[reason] || unloaders.destroy;
     unload(disposable);
   }
 });
 
-
-// If add-on is disabled munally, it's being upgraded, downgraded
-// or uniststalled `dispose` is invoked to undo any changes that
+// If add-on is disabled manually, it's being upgraded, downgraded
+// or uninstalled `dispose` is invoked to undo any changes that
 // has being done by it in this session.
 disable.define(Disposable, dispose);
 downgrade.define(Disposable, dispose);
 upgrade.define(Disposable, dispose);
 uninstall.define(Disposable, dispose);
 
 // If application is shut down no dispose is invoked as undo-ing
 // changes made by instance is likely to just waste of resources &
--- a/addon-sdk/source/test/test-disposable.js
+++ b/addon-sdk/source/test/test-disposable.js
@@ -4,25 +4,53 @@
 "use strict";
 
 const { Loader } = require("sdk/test/loader");
 const { Class } = require("sdk/core/heritage");
 const { Disposable } = require("sdk/core/disposable");
 const { Cc, Ci, Cu } = require("chrome");
 const { setTimeout } = require("sdk/timers");
 
+exports["test disposeDisposable"] = assert => {
+  let loader = Loader(module);
+
+  const { Disposable, disposeDisposable } = loader.require("sdk/core/disposable");
+  const { isWeak, WeakReference } = loader.require("sdk/core/reference");
+
+  let disposals = 0;
+
+  const Foo = Class({
+    extends: Disposable,
+    implements: [WeakReference],
+    dispose(...params) {
+      disposeDisposable(this);
+      disposals = disposals + 1;
+    }
+  });
+
+  const f1 = new Foo();
+  assert.equal(isWeak(f1), true, "f1 has WeakReference support");
+
+  f1.dispose();
+  assert.equal(disposals, 1, "disposed on dispose");
+
+  loader.unload("uninstall");
+  assert.equal(disposals, 1, "after disposeDisposable, dispose is not called anymore");
+};
+
 exports["test destroy reasons"] = assert => {
+  let disposals = 0;
+
   const Foo = Class({
     extends: Disposable,
     dispose: function() {
       disposals = disposals + 1;
     }
   });
 
-  let disposals = 0;
   const f1 = new Foo();
 
   f1.destroy();
   assert.equal(disposals, 1, "disposed on destroy");
   f1.destroy();
   assert.equal(disposals, 1, "second destroy is ignored");
 
   disposals = 0;
@@ -33,19 +61,20 @@ exports["test destroy reasons"] = assert
   f2.destroy("uninstall")
   f2.destroy();
   assert.equal(disposals, 1, "disposal happens just once");
 
   disposals = 0;
   const f3 = new Foo();
 
   f3.destroy("shutdown");
-  assert.equal(disposals, 0, "shutdown doesn't invoke disposal");
+  assert.equal(disposals, 0, "shutdown invoke disposal");
+  f3.destroy("shutdown");
   f3.destroy();
-  assert.equal(disposals, 0, "shutdown already skipped disposal");
+  assert.equal(disposals, 0, "shutdown disposal happens just once");
 
   disposals = 0;
   const f4 = new Foo();
 
   f4.destroy("disable");
   assert.equal(disposals, 1, "disable invokes disposal");
   f4.destroy("disable")
   f4.destroy();
@@ -144,17 +173,17 @@ exports["test different unload hooks"] =
 
   const u7 = new UberUnload();
   u7.destroy("whatever");
   u7.destroy();
   u7.destroy("uninstall");
   assert.deepEqual(u7.log, ["dispose"], "dispose hook invoked");
 };
 
-exports["test disposables are desposed on unload"] = function(assert) {
+exports["test disposables are disposed on unload"] = function(assert) {
   let loader = Loader(module);
   let { Disposable } = loader.require("sdk/core/disposable");
 
   let arg1 = {}
   let arg2 = 2
   let disposals = 0
 
   let Foo = Class({