Context-menu module leaks memory, serious issue

RESOLVED FIXED

Status

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

People

(Reporter: Mingyi Liu, Assigned: ochameau)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → poirot.alex
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #611612 - Flags: review?(rFobic)
(Assignee)

Comment 2

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

Comment 4

5 years ago
Yes, it's fixed! Thanks for the quick work - I look forward to the final release.

Comment 5

5 years ago
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
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 6

5 years ago
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
(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

5 years ago
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
You need to log in before you can comment on or make changes to this bug.