Last Comment Bug 764866 - Avoid leaking all modules when one is leaking
: Avoid leaking all modules when one is leaking
Status: RESOLVED FIXED
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-14 09:18 PDT by Alexandre Poirot [:ochameau]
Modified: 2012-08-30 03:27 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Pull request 464 (165 bytes, text/html)
2012-06-14 09:18 PDT, Alexandre Poirot [:ochameau]
rFobic: review+
Details

Description Alexandre Poirot [:ochameau] 2012-06-14 09:18:54 PDT
Created attachment 633160 [details]
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] 2012-06-14 09:31:32 PDT
Some numbers, this partch allows to leak only 9 modules instead of 40+ on bug 764831.
Comment 2 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-06-14 10:59:16 PDT
Comment on attachment 633160 [details]
Pull request 464

details in pull request.
Comment 3 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-06-14 11:03:54 PDT
> 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] 2012-07-09 06:47:41 PDT
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] 2012-08-30 03:19:53 PDT
Is this fixed?
Comment 6 Alexandre Poirot [:ochameau] 2012-08-30 03:27:10 PDT
Yes.

Note You need to log in before you can comment on or make changes to this bug.