649334 - Unregister destructors when they are called

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

I highlighted this leak in:
https://bugzilla.mozilla.org/show_bug.cgi?id=642306#c17

I've added in bug 642306 a new method in unload: unload.unregister
That allow to remove a destructor reference when we manually call a destructor.
If we don't do that we are leaking all objects until firefox close or addon removal!

We need to unregister all desctuctor over all our codebase.

Comment 1

[Drew Willcoxon :adw]

Isn't this what unload.ensure() is for?

Comment 2

[Alexandre Poirot [:ochameau]]

(In reply to comment #1)
> Isn't this what unload.ensure() is for?

Yes, you are right. I didn't saw this method, thanks for this highlight!
The problem is that you need to rename the destructor to unload, and change some API :( 
So either we use unregister with custom name, or, we change ensure to use destroy and refactor API using unload to use destroy too.
Because destroy has been used in high level API, whereas unload seems to be limited to low level ones.
Or we choose another option to avoid any API change:
- another ensure that use destroy ?
- a new parameter in ensure with destructor name ?
- ... ?

Comment 3

[Drew Willcoxon :adw]

See bug 551824 for a conversation Atul and I had about this over a year ago.

Comment 4

[Alexandre Poirot [:ochameau]]

Myk: Sorry, I only work on API addition/modification this time :p
We need your thoughts about this one too.
Having a second parameter in ensure seems pretty cool and will respond to most cases where we already use unload.when!

Comment 5

[Myk Melez [:myk] [@mykmelez]]

I'm happy to provide feedback, but note that you don't need my API review for low-level API changes, only for those being exposed to addon developers in high-level APIs.  So please feel free to go ahead with changes to ensure as appropriate to solve the issues you are facing.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Add destructor name in second argument of unload.ensure

And use it to fix symbiont/worker leak.

Drew, as you seem deeply involved in this unload improvement, I'm asking for your review.

Note for review:
- I removed unload.addMethod as noone used that since it's creation, It isn't documented.
- I faced complex issues around traits, that are monitored with a specific test

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 525989 [details] [diff] [review]
Add destructor name in second argument of unload.ensure

Thanks Alex.  This looks fine, but I have some questions about the traits workarounds, so I'll leave the r? for now.

>diff --git a/packages/api-utils/docs/unload.md b/packages/api-utils/docs/unload.md

>+  1. It replaces a destructor method with a wrapper method that will never call
>+     destructor more than once.

never call destructor more than once -> never call the destructor more than once

>+  Destructor will be called with a single argument describing the reason

Destructor will be called -> The destructor will be called

>+@param name {String}
>+  Optional name of the destructor method. Default is `unload`.

String -> string

Also, "name" needs to be in brackets since it's optional, i.e.:

  @param [name] {string}

>diff --git a/packages/api-utils/lib/unload.js b/packages/api-utils/lib/unload.js

> var when = exports.when = function when(observer) {
>+  let idx = observers.indexOf(observer);
>+  if (idx != -1) return console.warn("Destructor already registered");

Hmm, a couple of thoughts:

1) If you add a return statement with a return value here, then you should add a return statement with a return value in the non-erroneous case so that when() always explicitly returns a value (even if it's undefined).  But, when() doesn't currently return a value, and I don't think it makes sense for it to return a value, so please don't return a value here.  Either use an if-else or a return statement with no return value.

2) I'm not sure when() should warn at all actually.  With the idx != -1 check, which I think is fine, it doesn't seem harmful to attempt to register the same observer multiple times.  Do you agree?

Also, please put the branch on the next line, i.e.:

  if (idx != -1)
    console.warn("Destructor already registered");

>+  // Fix a bug around composed traits.

Is it really a bug in traits?  If it really is a bug, then can we fix that bug first?  If it's not really a bug, you should use a word like "consequence" instead.

It's weird that this function needs to make assumptions about the traits implementation when it has nothing to do with traits.

>+  // If we composed two traits with two destructors, and so call ensure twice
>+  // for the same final object instance, we end up calling destructors twice.
>+  // By doing this we are calling unloadWrapper when we call the second 
>+  // constructor
>+  // (see unit test)
>+  if (obj._public && obj._public[destructorName])
>+    originalDestructor = obj._public[destructorName];

I don't understand this part.  If I want to call obj.destroy(), I just call obj.destroy(), not obj._public.destroy(), even if obj is composed of traits.  Why the need to use _public?

>   function unloadWrapper(reason) {
>     if (!called) {
>       called = true;
>-      var index = unloaders.indexOf(unloadWrapper);
>+      let index = unloaders.indexOf(unloadWrapper);
>       if (index == -1)
>         throw new Error("internal error: unloader not found");
>       unloaders.splice(index, 1);
>-      unloader.apply(obj, [reason]);
>+      originalDestructor.apply(obj, [reason]);

Could you change this to originalDestructor.call(obj, reason) so it doesn't need to make a new array?

>+  // Mandatory on traits objects, or the destructor won't be called when we
>+  // call the public destructor function
>+  // (see unit test)
>+  if (obj._public && obj._public[destructorName])
>+    obj._public[destructorName] = unloadWrapper;
>+  else
>+    obj[destructorName] = unloadWrapper;

Shouldn't the traits implementation check whether obj has its own "foo" property before checking _public.foo?

>diff --git a/packages/api-utils/tests/test-unload.js b/packages/api-utils/tests/test-unload.js

>@@ -45,33 +45,19 @@ exports.testUnloading = function(test) {
>   var ul = loader.require("unload");
>   var unloadCalled = 0;
>   function unload() { unloadCalled++; }
>-  var obj1 = {};
>-  var obj2 = {};
>-  ul.addMethod(obj1, unload);
>-  ul.addMethod(obj2, unload);
>+  ul.when(unload);
>   loader.unload();
>-  test.assertEqual(unloadCalled, 2,
>-                   "All unloaders are called on unload.");
>-};
>-
>-exports.testAddMethod = function(test) {
>-  var obj = {unloadCalled: 0};
>-  function unloadObj() { this.unloadCalled++; }
>-
>-  unload.addMethod(obj, unloadObj);
>-
>-  obj.unload();
>-  test.assertEqual(obj.unloadCalled, 1,
>-                   "unloader function should be called");
>-  obj.unload();
>-  test.assertEqual(obj.unloadCalled, 1,
>-                   "unloader func should not be called more than once");
>+  test.assertEqual(unloadCalled, 1,
>+                   "Unloader is called on unload.");
> };

This test is currently checking that > 1 loaders are called on unload, so to preserve that, could you add one more unload function and check that unloadCalled == 2?

Also, since you added the idx != -1 check in when(), you should probably test passing the same unloader function twice to when() to make sure it's called only once.

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to comment #7)

Thanks for the review, and especially english mistakes ;)

> >+  // If we composed two traits with two destructors, and so call ensure twice
> >+  // for the same final object instance, we end up calling destructors twice.
> >+  // By doing this we are calling unloadWrapper when we call the second 
> >+  // constructor
> >+  // (see unit test)
> >+  if (obj._public && obj._public[destructorName])
> >+    originalDestructor = obj._public[destructorName];
> 
> I don't understand this part.  If I want to call obj.destroy(), I just call
> obj.destroy(), not obj._public.destroy(), even if obj is composed of traits. 
> Why the need to use _public?
> 

I'm not able to tell how exactly Traits is working but there is two differents objects: one public `obj` and one private `this`, like these ones:
let Worker = Trait.compose({ 
  constructor: function () { this },
  method: function () { this }
});
let obj = Worker();

So this != obj but obj = this._public. obj is almost the same than this but without any property starting with underscore.
I hope it helps you understand all these workaround Traits!

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Same patch but with all review comments adressed

Here is a first option, exactly the same patch than previous one, but with all your comments adressed.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Another approach that remove Traits stuff from unload

Now, a second option that remove the hack from unload module but introduce it in every traits modules.
So instead of doing :
  unload.ensure(this);
You have to call:
  unload.ensure(this._public);

This is not really obvious and even really complex for new comers, but it's not the first time we have to use _public in our codebase.

Comment 11

[Drew Willcoxon :adw]

Thanks for the two approaches, Alex.  I think the second one is definitely better, since it keeps the traits-specific code out of the unload module and in code that uses traits, and, as you say, is consistent with other places we have to use _public.

(In reply to comment #8)
> I'm not able to tell how exactly Traits is working but there is two differents
> objects: one public `obj` and one private `this`, like these ones:
> let Worker = Trait.compose({ 
>   constructor: function () { this },
>   method: function () { this }
> });
> let obj = Worker();
> 
> So this != obj but obj = this._public. obj is almost the same than this but
> without any property starting with underscore.
> I hope it helps you understand all these workaround Traits!

Hmm, I understand that _public is what a trait constructor returns and that `this` inside a trait method is a different object (the object on which _public is defined), but... OK, I think I get it.

For example, if Worker's constructor() does this:

  unload.ensure(this, "destroy");

Which calls unload.ensure():

  // privateObj is `this` in the previous line
  let oldDestructor = privateObj.destroy;
  privateObj.destroy = wrapper;

Then, privateObj.destroy == wrapper, which is OK.

The problem is publicObj.destroy:  when the trait was composed, the traits code set publicObj.destroy to oldDestructor bound to privateObj.  i.e., publicObj.destroy == oldDestructor.bind(privateObj).  So when you call publicObj.destroy(), the old, unwrapped destructor is called, and if you call publicObj.destroy() again, the old, unwrapped constructor is called again, which is not what we want.

Thanks for working through that.

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 526742 [details] [diff] [review]
Same patch but with all review comments adressed

r- in favor of the second approach.

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 526746 [details] [diff] [review]
Another approach that remove Traits stuff from unload

r+ on the second approach.  Please remove all trailing whitespace before landing:

>diff --git a/packages/api-utils/docs/unload.md b/packages/api-utils/docs/unload.md

>+  for the unload; see `when()`.  If `object` does not have the expected 

here

>diff --git a/packages/api-utils/lib/unload.js b/packages/api-utils/lib/unload.js

>+var ensure = exports.ensure = function ensure(obj, destructorName) {
>+  if (!destructorName)
>+    destructorName = "unload";
>+  if (!(destructorName in obj))
>+    throw new Error("object has no '" + destructorName + "' property");
> 
>+  let called = false;
>+  let originalDestructor = obj[destructorName];
>+  

here

>-var ensure = exports.ensure = function ensure(obj) {
>-  if (!("unload" in obj))
>-    throw new Error("object has no 'unload' property");
>-
>-  addMethod(obj, obj.unload);
>+  

here

>diff --git a/packages/api-utils/tests/test-unload.js b/packages/api-utils/tests/test-unload.js

>-  var obj1 = {};
>-  var obj2 = {};
>-  ul.addMethod(obj1, unload);
>-  ul.addMethod(obj2, unload);
>+  ul.when(unload);
>+  

here

>+  // This is going to be ignored, as we already registered it

going to be ignored -> should be ignored

>+  ul.when(unload);
>+  

here

>+exports.testEnsureWithTraits = function(test) {
>+
>+  let { Trait } = require("traits");
>+  let loader = test.makeSandboxedLoader();
>+  let ul = loader.require("unload");
>+  

here

>+  let obj = Trait.compose(
>+    composedTrait.resolve({
>+      constructor: "_constructor",
>+      unload : "_unload" 

here

>+      unload: function unload() {
>+        called++;
>+        this._unload();
>+      }
>+    })();
>+  

here

>+  obj.unload();
>+  test.assertEqual(called, 1,
>+                   "unload() should be called");
>+                   

here

>+  test.assertEqual(composedCalled, 1,
>+                   "composed object unload() should be called");
>+  

here

>+  obj.unload();
>+  test.assertEqual(called, 1,
>+                   "unload() should be called only once");
>+  test.assertEqual(composedCalled, 1,
>+                   "composed object unload() should be called only once");
>+  

here

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to comment #11)
> For example, if Worker's constructor() does this:
> 
>   unload.ensure(this, "destroy");
> 
> Which calls unload.ensure():
> 
>   // privateObj is `this` in the previous line
>   let oldDestructor = privateObj.destroy;
>   privateObj.destroy = wrapper;
> 
> Then, privateObj.destroy == wrapper, which is OK.
> 
> The problem is publicObj.destroy:  when the trait was composed, the traits code
> set publicObj.destroy to oldDestructor bound to privateObj.  i.e.,
> publicObj.destroy == oldDestructor.bind(privateObj).  So when you call
> publicObj.destroy(), the old, unwrapped destructor is called, and if you call
> publicObj.destroy() again, the old, unwrapped constructor is called again,
> which is not what we want.
> 
> Thanks for working through that.

Yes, it should be that!

Thanks for the quick review, it allow us to ship another quit important leak fix :)

Landed:
https://github.com/mozilla/addon-sdk/commit/6a5b1e7689f60e6947872d9ecc6b8af690e45297

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Commited patch.

Comment 16

[Alexandre Poirot [:ochameau]]

I forgot that others API may be leaking by using unload.when.

Here is files using `when`:
./packages/addon-kit/lib/windows.js
./packages/api-utils/docs/unload.md
./packages/api-utils/lib/e10s.js
./packages/api-utils/lib/hidden-frame.js
./packages/api-utils/lib/keyboard/observer.js
./packages/api-utils/lib/memory.js
./packages/api-utils/lib/observer-service.js
./packages/api-utils/lib/timer.js
./packages/api-utils/lib/unload.js
./packages/api-utils/lib/utils/registry.js
./packages/api-utils/lib/window-utils.js
./packages/api-utils/lib/windows/observer.js
./packages/api-utils/lib/xhr.js
./packages/api-utils/lib/xpcom.js
./packages/api-utils/tests/test-cuddlefish.js
./packages/api-utils/tests/test-unload.js
./packages/development-mode/lib/bootstrap.js
./packages/test-harness/lib/harness.js

We need to replace `when` with `ensure` if the object is destroyed before addon unload.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Replace unload.when by unload.ensure over all codebase, when it is needed

There was not that much code using unload.when on objects that need to be destroyed before add-on unload.
I found only api-utils/utils/registry and I fixed addon-kit/windows. Windows is destroyed on addon unload, but I'd like to avoid showing bad practices in our codebase.
I've added some comment in unload unit test to clarify Traits case and I added another test against private destructors.

Comment 18

[Drew Willcoxon :adw]

Comment on attachment 528323 [details] [diff] [review]
Replace unload.when by unload.ensure over all codebase, when it is needed

Thanks Alex.

>diff --git a/packages/api-utils/tests/test-unload.js b/packages/api-utils/tests/test-unload.js

>@@ -93,6 +93,9 @@ exports.testEnsureWithTraits = function(test) {
>   let composedCalled = 0;
>   let composedTrait = Trait.compose({
>       constructor: function () {
>+        // We have to give "public interface" of this trait, as we want to 
>+        // call public `unload` method and ensure that we call it only once, 
>+        // when we call this public function manually or on add-on unload.

when we call this -> either when we call this

Please remove the trailing whitespace from the first two lines.

>@@ -105,6 +108,7 @@ exports.testEnsureWithTraits = function(test) {
>       unload : "_unload" 
>     }), {
>       constructor: function constructor() {
>+        // Same thing apply here, we need to pass public interface

apply -> applies

Comment 19

[Alexandre Poirot [:ochameau]]

Thanks for the fast review!

Landed:
https://github.com/mozilla/addon-sdk/commit/f68e59e0dc035915c6152038a98fb8e0630551f3