Closed Bug 724361 Opened 13 years ago Closed 13 years ago

KillSpinners 1.0.3 (preliminary reviewed) add-on causes immortal zombie content compartments

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: TheOne, Unassigned)

References

(
URL
)

Details

(Whiteboard: [MemShrink:P3]) 

This add-on leaks content compartments.

STR:

1) Install the add-on from the url above
2) In the preferences, set the timeout to something very low to make testing easier.
3) Open a new tab with http://addons.mozilla.org/
2) Close tab and go to about:memory?verbose
4) Click <minimize memory usage>
5) http://addons.mozilla.org is still listed

The author has been informed about this bug.
Summary: KillSpinners add-on causes immortal zombie compartments → KillSpinners add-on causes immortal zombie content compartments
Thanks for filing these bugs, Andreas!
Justin, checking for compartment leaks is now part of the review process for add-ons submitted to AMO, so expect more of these bugs.
Fantastic!
(In reply to Andreas Wagner [:TheOne] from comment #2)
> Justin, checking for compartment leaks is now part of the review process for
> add-ons submitted to AMO, so expect more of these bugs.

To be clear, are you finding these bugs as part of the AMO review process?
(In reply to Justin Lebar [:jlebar] from comment #4)
> To be clear, are you finding these bugs as part of the AMO review process?

Yes. Do you want us to not file bugs like this found during our review?
(In reply to Andreas Wagner [:TheOne] from comment #5)
> (In reply to Justin Lebar [:jlebar] from comment #4)
> > To be clear, are you finding these bugs as part of the AMO review process?
> 
> Yes. Do you want us to not file bugs like this found during our review?

No; these are great!  I just want to understand the process.  The add-on author submits a new version of the add-on, you find a leak, and then you go back and test the current version to see if the leak is there too?  If so, you file a bug, and either way, you reject the update until the leak is fixed?

(If this is what you're doing, the next logical step, in my mind, anyway, would be to test the current versions of the most popular add-ons.)
Not quite. The add-on author submits a new version, we find a leak and give only preliminary review until the leak is fixed. We're not rejecting the version cause this would make it completely unavailable.
Unfortunately we don't have the time to everytime re-test the current public version of an add-on we review a new version of. We also expect that add-on developers fix this leaks in their own interest to get full approval.

https://developer.mozilla.org/en/Zombie_Compartments#Proactive_checking_of_add-ons says to file a bug if you found zombie compartments. I just noticed though, that I'm the only editor that files bugs for zombie compartments in add-ons I review. So I wondered whether I should continue with this.

Checking the current public versions of the (maybe top100) add-ons is a good idea, but I don't think editors can do this because of lack of time. I'm pretty sure we will cover them "automatically" pretty soon because their authors are usually pretty active and update their add-ons fairly often, so we just check them when they send in new versions.
If the add-on in question is under review, can someone who's not an AMO editor get access to it, or view its source?  A link to an XPI or source (or both!) that you tested would certainly be helpful.  But I suppose the most helpful thing is a diff showing the changes which fixed the leak; that helps the MemShrink team understand what kinds of issues are leading to add-on leaks.

In the past, these bugs have also helped add-on authors fix their leaks by giving them access to developers with expertise in this area.

Yours is a good point that most of the top add-ons will be tested when they submit a new version.  What we can do is, after a few months, go through and see which add-ons have not been updated since we started testing for leaks.  That should be a relatively small list that we could hand to QA.  Heck, I'd test the add-ons myself.  (I went through a few of the top add-ons, and most but not all have been updated lately.  Personas Plus was updated 11 months ago.  Flashblock, 7 months ago.  Web Developer, 13 months ago.  And that's just in the top 20.)
(In reply to Justin Lebar [:jlebar] from comment #8)
> If the add-on in question is under review, can someone who's not an AMO
> editor get access to it, or view its source?  A link to an XPI or source (or
> both!) that you tested would certainly be helpful.  But I suppose the most
> helpful thing is a diff showing the changes which fixed the leak; that helps
> the MemShrink team understand what kinds of issues are leading to add-on
> leaks.

Add-on versions that are currently pending review (as well as prelim. reviewed versions) are accessible via the versions page, e.g. https://addons.mozilla.org/en-US/firefox/addon/killspinners/versions/ With having both the leaking and the fixed xpi there, you can easily diff them locally.
If an add-on version that leaks had to be rejected (because of other reasons), it's not available publicly anymore, because we only reject for security issues. Only amo admins (not even editors) have access to such files, so if you want one of these, you could ask jorgev or fligtar.

If all editors filed bug reports about leaking add-ons and add-on developers comment on it, like it happened with bug 723843, I guess this would be helpful for you, wouldn't it?
Jorge, what do you think about that?


> In the past, these bugs have also helped add-on authors fix their leaks by
> giving them access to developers with expertise in this area.

Good catch! When we have some nice examples, we could put them on MDN, too, because we include a link to the "Zombie Compartments" MDN page in our review notes to the add-on author.
I've rejected or warned 7 add-on updates since this policy was initiated. If you guys want, I can file bugs for these types of rejections and include the link in the review message. I haven't been doing so because it seemed like it would generate quite a lot of noise. It's probably worth noting that I don't go back and check the current public version when these issues arise in updates, but it's generally a safe bet that they're not new to the update.

The following are the add-ons at issue. Let me know if you want me to file bugs for them, or if there's a better place to have this discussion:

https://addons.mozilla.org/addon/zotero/
https://addons.mozilla.org/addon/inline-translator/
https://addons.mozilla.org/addon/invisiblehand/
https://addons.mozilla.org/addon/suggest-me/
https://addons.mozilla.org/addon/seoquake-seo-extension/
https://addons.mozilla.org/addon/evernote-web-clipper/
https://addons.mozilla.org/addon/wisestamp-email-signature-gmai/
I'll bring this up at the next MemShrink meeting (Tuesdays, 2p Pacific) and we'll get back to you about what we think is the best way to keep track of leaky add-ons (or whether we'd like to track them at all).

Would filing bugs (and closing them when they're fixed) be too much of an administrative burden on you guys?  We don't want to make your lives more difficult...
(In reply to Andreas Wagner [:TheOne] from comment #0)
> This add-on leaks content compartments.
> 
> STR:
> 
> 1) Install the add-on from the url above
> 2) In the preferences, set the timeout to something very low to make testing
> easier.
> 3) Open a new tab with http://addons.mozilla.org/
> 2) Close tab and go to about:memory?verbose
> 4) Click <minimize memory usage>
> 5) http://addons.mozilla.org is still listed
> 
> The author has been informed about this bug.

hi Andreas,

i've checked my add-on as you described and couldn't replicate the zombie compartments (the pages do disappear after minimize memory or a while later).
tested also all kinds of scenarios (including closing the page while loading, disabling the add-on while page is loading etc) and still can't replicate the result using ff10 on vista (also tried ff9, ff10 on xp).
most reasonable suspect is the notification (which should be freed by the browser when the page is closed), the relevant code is:

var reloadbutton = { label: "Allow", accessKey: "A", callback: function() { aBrowser.m_nTimeoutID=-1; aBrowser.loadURI(aUri.spec); } };
var nb = Browser.ownerDocument.defaultView.gBrowser.getNotificationBox(aBrowser);
nb.appendNotification(...);

which holds reference to aBrowser.

it would be helpful if you can check if the problem persist when closing the notification before closing the page or checking disable notification in the option dialog.

thanks 
    yochai
I can't reproduce it either.

Justin: I don't think it would be an administrative problem for us. If you guys want the bugs filed, we should be able to handle filing and closing them.
Sorry to clog up your bug, Yochai.

Kris, If you guys are up for filing bugs, I think the MemShrink folks would appreciate it, at least until finding and fixing these issues becomes more routine.  I'll bring it up at Tuesday's MemShrink meeting and let you know if the consensus is that we'd like something other than bug reports.

It would be helpful if you'd explicitly mark bugs found when testing pre-release versions, since we also get bugs filed on released versions.
Summary: KillSpinners add-on causes immortal zombie content compartments → KillSpinners 1.0.3 (preliminary reviewed) add-on causes immortal zombie content compartments
I cannot reproduce it on Windows either.

I tested on Mac 10.7 with Firefox 10 yesterday where I was able to reproduce it. Unfortunately I don't have access to a Mac now, but I will try again when I'm home tonight.
Whiteboard: [MemShrink] → [MemShrink:P3]
Hm, very weird. I can't reproduce it anymore...

Marking as wfm for now.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
Hi all,
considering the current state of affairs, 
what is needed for version 1.0.3 to be approved???

p.s
i found a peculiar way to create zombie compartment by pasting into a text field
(in www.google.com for example). after that the compartment will not die although it can be changed by pasting in another page...
(In reply to yochai meltzer from comment #17)
> Hi all,
> considering the current state of affairs, 
> what is needed for version 1.0.3 to be approved???

I just moved this version to the public, per comment #16.

> p.s
> i found a peculiar way to create zombie compartment by pasting into a text
> field
> (in www.google.com for example). after that the compartment will not die
> although it can be changed by pasting in another page...

Is this happening only with your add-on installed, or all the time?
thanks...

no this is unrelated issue confirmed on vista/xp with new profile(In reply to Jorge Villalobos [:jorgev] from comment #18)
> (In reply to yochai meltzer from comment #17)
> > Hi all,
> > considering the current state of affairs, 
> > what is needed for version 1.0.3 to be approved???
> 
> I just moved this version to the public, per comment #16.

thanks...

> > p.s
> > i found a peculiar way to create zombie compartment by pasting into a text
> > field
> > (in www.google.com for example). after that the compartment will not die
> > although it can be changed by pasting in another page...
> 
> Is this happening only with your add-on installed, or all the time?

no this is unrelated issue confirmed for ff 10.0.2 on vista/xp with new profile 
to reproduce:
1) paste text into any page with a textbox (ex. the searchbox on this page) 
2) close the page tab

that will create a zombie compartment, it will change when pasting in a different page which probably means the paste operation holds reference to the last pasted page...
(In reply to yochai meltzer from comment #19)
> no this is unrelated issue confirmed for ff 10.0.2 on vista/xp with new
> profile 
> to reproduce:
> 1) paste text into any page with a textbox (ex. the searchbox on this page) 
> 2) close the page tab
> 
> that will create a zombie compartment, it will change when pasting in a
> different page which probably means the paste operation holds reference to
> the last pasted page...

This happens only when pasting via the context menu (who does this?). Please file a bug with ZombieCompartments as a dependency.
I can reproduce the bug in 10 but not in 11. It's probably fixed already.
You need to log in before you can comment on or make changes to this bug.