764866 - Avoid leaking all modules when one is leaking

Status: RESOLVED FIXED

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

Description

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 464

The past has proven that SDK is having some more or less severe leaks.
We are now used to identify leaks where is particular module is leaked when the addon is disabled. (See bug 764831 for ex) 
Leaking this particular module is one thing, leaking all modules because one has an issue is another thing.
This bug is mostly a followup of this:
https://github.com/mozilla/addon-sdk/pull/428#discussion_r796752

So, no, I'm not paranoid. Most of the time I'm handling memory leak reports and I have to explain in bug reports: "Yes, the SDK is leaking, and, we are leaking all modules". That's bad and give a *really* bad image of the project.
We should do our best in order to avoid leaking everything as soon as some hidden piece of code appear to be broken.

Here is a way to address that. Just avoid keeping reference to modules sandboxes or objects from the loader on unload. All tests are passing, but that doesn't mean that everything is fine. We may want to delay this cleanup. We may want to throw an exception when we try to call require() after unload cleanup.
I'll be more than happy to iterate on this.

Comment 1

[Alexandre Poirot [:ochameau]]

Some numbers, this partch allows to leak only 9 modules instead of 40+ on bug 764831.

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 633160 [details]
Pull request 464

details in pull request.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> Here is a way to address that. 

I'm fine with doing this, but in the scope of sdk loader rather then general loader code.

> Just avoid keeping reference to modules sandboxes or objects from the loader on
> unload. All tests are passing, but that doesn't mean that everything is fine.
> We may want to delay this cleanup.

Yes I think we should delay cleanup for a few turns.

> We may want to throw an exception when we try to call require() after unload
> cleanup.
I'll be more than happy to iterate on this.

Comment 4

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/de79689042dceca225f1583e98624491b1ea02fe
Bug 764866: Avoid leaking all modules when one goes wrong and keep the loader alive.

https://github.com/mozilla/addon-sdk/commit/44abce6a3cb6e43a7d6a3f4ae3d035d03c5eb302
Merge pull request #464 from ochameau/leak/best-effort-loader

Bug 764866: Avoid leaking all modules when one goes wrong and keep the loader alive. r=@gozala

Comment 5

[Dave Townsend [:mossop]]

Is this fixed?

Comment 6

[Alexandre Poirot [:ochameau]]

Yes.