"empty" SDK add-on creates zombie compartments in SDK 1.4.2

RESOLVED FIXED

Status

Add-on SDK
General
P1
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: nmaier, Assigned: ochameau)

Tracking

(Blocks: 1 bug, {mlk, regression})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
STR:
- Create a new add-on with the following main.js (and nothing else):
require("panel");
- cfx xpi
- Install xpi in a fresh profile
- Disable add-on again
- Go about:memory?verbose
- Minimize memory usage
- Wait a couple of minutes, just to be sure
- Minimize memory usage
-> Observe that all the jetpack compartments are still present

Tested and affected SDK version: 1.4.2
Tested and NOT affected SDK version: 1.3
Tested with the latest Nightly Win64 and official Firefox 10 Win32

Might be related to bug 724404
Keywords: regression
Does this bug mean only that an extension's memory is not released when it's disabled?  Are there any other ill effects?

If it's only the former, it's something that should be fixed, but doesn't seem like a huge deal.
Whiteboard: [MemShrink]
(Reporter)

Comment 2

6 years ago
I tested a bit further and found out that it doesn't seem to be a problem with panel, actually...
Using a completely empty main.js will still leak all the compartments from the add-on initialization.

New STR:
- Create a new add-on with a completely empty main.js (and nothing else):
- cfx xpi
- Install xpi in a fresh profile
- Disable add-on again
- Go about:memory?verbose
- Minimize memory usage
- Wait a couple of minutes, just to be sure
- Minimize memory usage
-> Observe that all the jetpack compartments are still present

(In reply to Justin Lebar [:jlebar] from comment #1)
> Does this bug mean only that an extension's memory is not released when it's
> disabled?  Are there any other ill effects?

Don't know if there are any other ill-effects, but I'd guess it's possible.
E.g. - and this is just a guess - observers might still be around and notified of stuff they shouldn't, thus carrying out operations they shouldn't or do carry out the same action twice if the add-on is reloaded. If this is the case, then there might even be some performance penalty, other than garbage collection taking longer of course.

> If it's only the former, it's something that should be fixed, but doesn't
> seem like a huge deal.

Might turn out not to be a huge deal, but it is nasty anyway. Consider very long running instances, e.g. due to hibernation, and add-ons that are disabled and enabled a couple of times in between e.g. when updated. This can keep a lot of stuff around.
Summary: require("panel") creates zombie compartments in SDK 1.4.2 → "empty" SDK add-on creates zombie compartments in SDK 1.4.2
Whiteboard: [MemShrink] → [MemShrink:P2]
Created attachment 595400 [details]
Pull request 339

Ouch!

Thanks for having taken time to test the SDK, we totally missed that scenario and we are leaking almost everything when addons are disabled :/
Here is a first patch that fixes leaks for an empty addon.
It still leaks when Widget module is used, we will need additional work on top of this one in bug 724404.
Assignee: nobody → poirot.alex
Attachment #595400 - Flags: review?(rFobic)
Blocks: 724404
Priority: -- → P1
Blocks: 725603
Blocks: 726485
With this patch applied to master we are still leaking the main.js compartment:

161,864 B (00.28%) -- compartment([System Principal], resource://jid1-t81mmgf4fgbddq-at-jetpack/test/lib/main.js, 0x113045000)

Tested by creating a new add-on with an empty main.js and deactivated/uninstalled in Firefox followed by a reduce memory request.
Status: NEW → ASSIGNED
An issue which should be considered as blocker for 1.5?
(In reply to Henrik Skupin (:whimboo) from comment #5)
> An issue which should be considered as blocker for 1.5?

I don't think I'd block 1.5 on this right now however if the patch lands today (Irakli is going to take a look at that) then we'll look into whether cherry-picking it for the last RC tomorrow is a good idea or not.
(In reply to Henrik Skupin (:whimboo) from comment #5)
> An issue which should be considered as blocker for 1.5?

It may be worth noting that our current AMO policies would deny full review to an add-on with these issues if they were not SDK issues that the add-on cannot circumvent.

Comment 8

5 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/46fe1cefd1d8f84862d1bd1fbb3103a8189f6895
Merge pull request #339 from ochameau/fix-addon-disabling-leaks

fix Bug 724433 - Fix various memory leaks when addon is disabled. r=@ochameau

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
This fix looks great and removes all the compartments whenever the extension gets disabled or uninstalled. Nothing is leaked anymore by an empty SDK add-on.
Status: RESOLVED → VERIFIED
Attachment #595400 - Flags: review?(rFobic) → review+
Keywords: mlk
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Sorry reopend wrong bug.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.