Last Comment Bug 740689 - Context-menu module leaks memory, serious issue
: Context-menu module leaks memory, serious issue
Status: RESOLVED FIXED
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Alexandre Poirot [:ochameau]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-29 18:41 PDT by Mingyi Liu
Modified: 2012-04-10 11:25 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testsdkleak.xpi (132.12 KB, text/plain)
2012-03-29 18:41 PDT, Mingyi Liu
no flags Details
Pull request 391 (165 bytes, text/html)
2012-04-02 15:11 PDT, Alexandre Poirot [:ochameau]
rFobic: review+
Details

Description Mingyi Liu 2012-03-29 18:41:40 PDT
Created attachment 610787 [details]
testsdkleak.xpi

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.79 Safari/535.11

Steps to reproduce:

Created a simple demo addon using SDK 1.5 or 1.6rc1. The addon (attached to this report) requires only one module, context-menu, and does nothing in exports.main.  Install the addon in FF13.0a2, then go to any page, like news.google.com.  Check about:compartments?verbose, you'd see two compartments for the Google page. Then close the Google page, refresh the compartment page, and you'd see both compartments gone.  Now open news.google.com again, and RIGHT click on the page once (or more if you want). Close the page, refresh the compartment page again.


Actual results:

None of the two compartments for news.google.com are gone. You could use about:memory?verbose and try to minimize the memory many times, it doesn't help. The two zoombie compartments will persist for a long time.


Expected results:

The two compartments for the closed page should've been gone once the page is closed.  This should be a blocking bug.
Comment 1 Alexandre Poirot [:ochameau] 2012-04-02 15:11:30 PDT
Created attachment 611612 [details]
Pull request 391

Confirmed. Thanks for the report, it was really helpfull!!

More details in the pull request for the fine details about what/how we were leaking.
Comment 2 Alexandre Poirot [:ochameau] 2012-04-02 15:22:04 PDT
The issue is quite important and the patch is quite small. 
I was wondering if we still have a chance to get it included in a last minute 1.6 RC?

Any addon that simply require("context-menu") (even if they do not use this API, the simple fact of requiring it enable the leak), will leak the last webpage where they right clicked. But it shouldn't leak all documents where user right-clicked. So that it should not leak more than one document per browser window.

I fixed one particular leak that was easy to reproduce, there might be others. I'm waiting for Mingyi confirmation that all leaks are gone.
Comment 3 Dave Townsend [:mossop] 2012-04-02 15:26:36 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #2)
> The issue is quite important and the patch is quite small. 
> I was wondering if we still have a chance to get it included in a last
> minute 1.6 RC?

We're currently due to ship 1.6 tomorrow so if we think this is serious enough I think our options are to either do a new RC tomorrow instead and delay 1.6 for maybe a week, or do 1.6 as normal and do this as a 1.6.1 shortly after.

> Any addon that simply require("context-menu") (even if they do not use this
> API, the simple fact of requiring it enable the leak), will leak the last
> webpage where they right clicked. But it shouldn't leak all documents where
> user right-clicked. So that it should not leak more than one document per
> browser window.

Based on this summary, one document leaked per window, I don't think it is worth delaying the release, but let's talk about it in the weekly meeting.
Comment 4 Mingyi Liu 2012-04-02 17:00:50 PDT
Yes, it's fixed! Thanks for the quick work - I look forward to the final release.
Comment 5 [github robot] 2012-04-03 10:28:06 PDT
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/21181c0aca414078a410c6fe521944855912c303
Merge pull request #391 from ochameau/bug/740689-context-menu-leak

Bug 740689: Fix context-menu leak of tabs where we opened a context menu r=@gozala
Comment 6 [github robot] 2012-04-03 12:22:44 PDT
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/6ed75d9fc7b287590ebdf7bd5187e8959f8924a3
Merge pull request #391 from ochameau/bug/740689-context-menu-leak

Bug 740689: Fix context-menu leak of tabs where we opened a context menu r=@gozala
Comment 7 Wes Kocher (:KWierso) 2012-04-03 12:36:14 PDT
(In reply to Dave Townsend (:Mossop) from comment #3)
> (In reply to Alexandre Poirot (:ochameau) from comment #2)
> > The issue is quite important and the patch is quite small. 
> > I was wondering if we still have a chance to get it included in a last
> > minute 1.6 RC?
> 
> We're currently due to ship 1.6 tomorrow so if we think this is serious
> enough I think our options are to either do a new RC tomorrow instead and
> delay 1.6 for maybe a week, or do 1.6 as normal and do this as a 1.6.1
> shortly after.
> 
> > Any addon that simply require("context-menu") (even if they do not use this
> > API, the simple fact of requiring it enable the leak), will leak the last
> > webpage where they right clicked. But it shouldn't leak all documents where
> > user right-clicked. So that it should not leak more than one document per
> > browser window.
> 
> Based on this summary, one document leaked per window, I don't think it is
> worth delaying the release, but let's talk about it in the weekly meeting.

So, this landed on the master development branch. It didn't make it to stabilization branch in time for the 1.6 release, but we're going to be pushing out a 1.6.1 hotfix for this sometime in the next week.
Comment 8 [github robot] 2012-04-10 11:25:06 PDT
Commit pushed to release at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/6ed75d9fc7b287590ebdf7bd5187e8959f8924a3
Merge pull request #391 from ochameau/bug/740689-context-menu-leak

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