The default bug view has changed. See this FAQ.

Avoid leaking all modules when one is leaking

RESOLVED FIXED

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ochameau, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
Attachment #633160 - Flags: review?(rFobic)
(Reporter)

Comment 1

5 years ago
Some numbers, this partch allows to leak only 9 modules instead of 40+ on bug 764831.
Comment on attachment 633160 [details]
Pull request 464

details in pull request.
Attachment #633160 - Flags: review?(rFobic) → review-
> 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.

Updated

5 years ago
Priority: -- → P1
(Reporter)

Updated

5 years ago
Attachment #633160 - Flags: review- → review?(rFobic)
Attachment #633160 - Flags: review?(rFobic) → review+

Comment 4

5 years ago
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
Is this fixed?
(Reporter)

Comment 6

5 years ago
Yes.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.