Last Comment Bug 723843 - UI Enhancer add-on causes immortal zombie compartments
: UI Enhancer add-on causes immortal zombie compartments
Status: VERIFIED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Girish Sharma [:Optimizer]
:
Mentors:
https://addons.mozilla.org/en-US/fire...
Depends on:
Blocks: LeakyAddons ZombieCompartments
  Show dependency treegraph
 
Reported: 2012-02-03 00:52 PST by Andreas Wagner [:TheOne]
Modified: 2012-02-07 20:18 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
non-author patch, v0 (585 bytes, patch)
2012-02-03 05:01 PST, Nils Maier [:nmaier]
no flags Details | Diff | Review
Patched XPI, v0 (96.58 KB, application/x-xpinstall)
2012-02-03 05:02 PST, Nils Maier [:nmaier]
no flags Details
fix 1 (96.30 KB, application/octet-stream)
2012-02-03 09:14 PST, Girish Sharma [:Optimizer]
maierman: review-
Details
Fix 2 (96.32 KB, application/octet-stream)
2012-02-03 10:12 PST, Girish Sharma [:Optimizer]
awagner: review+
Details

Description Andreas Wagner [:TheOne] 2012-02-03 00:52:38 PST
STR:

1) Install the add-on from the url above
2) Close all tabs and go to about:memory?verbose
3) Disable add-on, close the add-ons manager tab
4) Click <minimize memory usage>

I also tried even closing the about:memory tab after disabling the add-on and opening it in a new one. Same result.
Comment 1 Girish Sharma [:Optimizer] 2012-02-03 01:08:23 PST
I am trying to get rid of this problem, is this totally the add-on's problem? or Firefox's also ?
Please assign this to me.
Comment 2 Nils Maier [:nmaier] 2012-02-03 05:01:05 PST
Created attachment 594134 [details] [diff] [review]
non-author patch, v0

The add-on also leaks without disabling it first.

After applying the above patch I cannot seem to reproduce the leaks any longer, on Nightly at least.
Andreas and Girish, can you verify the problem is gone with the patch?
Comment 3 Nils Maier [:nmaier] 2012-02-03 05:02:33 PST
Created attachment 594135 [details]
Patched XPI, v0
Comment 4 Andreas Wagner [:TheOne] 2012-02-03 05:11:39 PST
Nils, I cannot confirm that this is resolved in your patched XPI (tested on Firefox 11.0a2):

After disabling the add-on and clicking the <minimize memory usage> button (and waiting some time and clicking it again), I still see the following branch in about:memory:

├─────360,976 B (00.92%) -- compartment([System Principal], jar:file:///C:/FirefoxProfiles/develop_4.0/extensions/UIEnhancer@girishsharma.xpi!/bootstrap.js, 0x95c0000)
│  │     ├──184,320 B (00.47%) -- gc-heap
│  │     │  ├───87,352 B (00.22%) -- arena
│  │     │  │   ├──85,304 B (00.22%) -- unused
│  │     │  │   ├───1,328 B (00.00%) -- padding
│  │     │  │   └─────720 B (00.00%) -- headers
│  │     │  ├───40,032 B (00.10%) -- objects
│  │     │  │   ├──20,480 B (00.05%) -- non-function
│  │     │  │   └──19,552 B (00.05%) -- function
│  │     │  ├───30,464 B (00.08%) -- scripts
│  │     │  ├───25,256 B (00.06%) -- shapes
│  │     │  │   ├──18,888 B (00.05%) -- tree
│  │     │  │   ├───3,392 B (00.01%) -- base
│  │     │  │   └───2,976 B (00.01%) -- dict
│  │     │  ├────1,184 B (00.00%) -- type-objects
│  │     │  └───────32 B (00.00%) -- strings
│  │     ├───86,008 B (00.22%) -- script-data
│  │     ├───65,536 B (00.17%) -- mjit-code
│  │     ├───15,368 B (00.04%) -- shapes-extra
│  │     │   ├───7,176 B (00.02%) -- compartment-tables
│  │     │   ├───3,904 B (00.01%) -- tree-tables
│  │     │   ├───3,072 B (00.01%) -- tree-shape-kids
│  │     │   └───1,216 B (00.00%) -- dict-tables
│  │     ├────8,224 B (00.02%) -- object-slots
│  │     ├────1,248 B (00.00%) -- type-inference
│  │     │    └──1,248 B (00.00%) -- script-main
│  │     └──────272 B (00.00%) -- string-chars
Comment 5 Girish Sharma [:Optimizer] 2012-02-03 05:13:06 PST
No help.
With your patch applied, after uninstalling the add-on, the add-on still consumes around 700 KB space, that is a zombie compartment.

The problem is caused due to non-null DOM references which still remain after uninstalling. I am looking into it and will update soon.
Comment 6 Nils Maier [:nmaier] 2012-02-03 05:16:13 PST
Oops, forgot to look after the bootstrap compartment.
My patch should address the leaky inner-windows/content compartments, though
Comment 7 Girish Sharma [:Optimizer] 2012-02-03 09:14:31 PST
Created attachment 594210 [details]
fix 1

It seems that the async function that I have been using for delayed call to a function was leaking memory.
Fixed and updated in the xpi attached.
Comment 8 Nils Maier [:nmaier] 2012-02-03 09:35:15 PST
Comment on attachment 594210 [details]
fix 1

Leaks content compartments for me:

STR:
- Have UIEnh enabled
- Open http://google.de/ in a tab
- Close tab
- Check about:memory after minimizing for google.de compartments/windows
-> Still present and hence leaked
- Disable UIEnh
-> All gone now (so that leak is fixed)
Comment 9 Girish Sharma [:Optimizer] 2012-02-03 09:38:59 PST
(In reply to Nils Maier [:nmaier] from comment #8)
> Leaks content compartments for me:
> 
> STR:
> - Have UIEnh enabled
> - Open http://google.de/ in a tab
> - Close tab
> - Check about:memory after minimizing for google.de compartments/windows
> -> Still present and hence leaked
> - Disable UIEnh
> -> All gone now (so that leak is fixed)
Did your patch v0 earlier address this issue ?
My patch only addresses the zombie compartment formed after uninstalling/disabling the add-on.
Comment 10 Nils Maier [:nmaier] 2012-02-03 09:55:55 PST
(In reply to scrapmachines from comment #9)
> (In reply to Nils Maier [:nmaier] from comment #8)
> Did your patch v0 earlier address this issue ?
> My patch only addresses the zombie compartment formed after
> uninstalling/disabling the add-on.

It does, indeed. Patched my v0 into your fix1 and all leaks are gone.

I did glance over:
https://github.com/scrapmac/UIEnhancer/commit/646fed82c3c3ac25632ecc564dc29f35fc6360d9#L0R2166
Thought this was my fix already, but in fact it isn't

But there is a new issue, unrelated to the leaks.
STR
- Open some URL, e.g about about:memory?verbose
- Click/focus into the urlbar so that it will be in "edit mode"
- Leave the urlbar (click somewhere else)
-> The urlbar is left empty
Comment 11 Girish Sharma [:Optimizer] 2012-02-03 10:01:56 PST
Yes I know of that issue, I will upload the fixed xpi on AMO in few minutes (including your patch).

Is it now good to set this bug to resolved fixed ?
Comment 12 Andreas Wagner [:TheOne] 2012-02-03 10:08:48 PST
Well, you could set it to fixed but we will reopen if the add-on is still leaking in our tests.

Maybe you want to upload it here to bugzilla first, so we can test it before you upload it to amo.
Comment 13 Girish Sharma [:Optimizer] 2012-02-03 10:12:04 PST
Created attachment 594231 [details]
Fix 2

Merged patch v0.
Comment 14 Andreas Wagner [:TheOne] 2012-02-03 10:18:41 PST
Comment on attachment 594231 [details]
Fix 2

Yes, that works fine. Thanks you!

Just as a note: Another lightly unrelated issue is that the height of the urlbar changes depending on whether it is focused or not. This makes the elements below the urlbar "jump" up and down which looks quite ugly.
This won't prevent you from getting full approval, but while you're at fixing stuff, you might wanna fix this too, for the sake of your Mac users :)

Thanks.
Comment 15 Girish Sharma [:Optimizer] 2012-02-03 10:22:40 PST
Yes I know, but I don't have a mac to properly debug this issue :(
Will try my best.
Comment 16 Nils Maier [:nmaier] 2012-02-03 10:26:12 PST
The review queues are really fast right now.
I suggest you first upload your fixed version and address any Mac layout issues in subsequent releases. ;)
Comment 17 Nils Maier [:nmaier] 2012-02-03 11:02:07 PST
Version 3.6 released and available from AMO
Comment 18 Nicholas Nethercote [:njn] 2012-02-07 20:07:31 PST
I can't find this add-on on AMO now.  The URL given above is a 404.
Comment 19 Girish Sharma [:Optimizer] 2012-02-07 20:18:34 PST
https://addons.mozilla.org/en-US/firefox/addon/ui-enhancer/

Here you go.

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