552341 - leak on closing tab if an image on the page had a context menu opened on it

Status: VERIFIED FIXED

(Core :: General, enhancement)

Description

[IU]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a3pre) Gecko/20100314 Firefox/3.5.8 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a3pre) Gecko/20100314 Firefox/3.5.8 (.NET CLR 3.5.30729) ID:20100314105610

After closing a tab or navigating away from a page, Firefox takes a while to release the memory -- sometimes never releasing it at all, thus requiring a browser restart.  Even when Firefox's memory management is working "properly," it can sometimes take several minutes before memory is released: a major problem if Firefox has just consumed over 1 GB of RAM for a single page load (this has happened to me).

The problem, however, is more complicated.  Whatever conditions Firefox is currently using to determine when to free memory sometimes does not appear to be met and Firefox will not release memory at all (no matter how long I have been surfing lower-impact sites or waiting, since closing the high-memory site/tab).  I am then forced to restart the browser.  I encounter this situation quite often (especially after extended browsing sessions).  It also seems to be independent of fast back cache settings, as setting it to '0' makes no discernible difference.

Chrome, on the other hand, releases memory immediately upon navigating away from the high-memory page (e.g. clicking the home button).  Firefox should at least come close, or add an about:config timer pref that users can tweak to determine how long Firefox can hang on to such memory.

I conducted over 10 timing checks to see when Firefox released memory, and it turns out it is very inconsistent, taking anywhere from about 20 seconds to several minutes.

I believe it would be better to use Chrome's approach of releasing memory immediately when navigating away from a site (or at least within a few seconds of the event).  In the alternative, an about:config pref to control this would be appreciated.

Thanks

Reproducible: Always

Steps to Reproduce:
I have tried to come up with steps, but the behavior is so inconsistent that this has been difficult.

1.
2.
3.

Comment 1

[Boris Zbarsky [:bzbarsky]]

IU, is the problem reliably reproducible on the page in the url field?  That is, every time you navigate away from it the memory is not freed?

The 20s lag you see is likely to be the cycle collection lag.  Minutes or longer is not expected, but is _very_ unlikely to be a jemalloc problem.

Chrome's behavior is due to it just killing off the entire process when you navigate away from the page.  The memory isn't freed quite as expeditiously if some other page in that process is still alive (e.g. via same-site window.open), last I checked.  We may end up with Chrome's behavior at some point using the same mechanism, obviously.

And yes, the steps to reproduce, or even a page that reliably shows the problem, are the hard things here....

Just as a quick check; I assume the same problem occurs in safe mode?

Comment 2

[IU]

(In reply to comment #1)
> IU, is the problem reliably reproducible on the page in the url field?  That
> is, every time you navigate away from it the memory is not freed?

No.  But I think I've discovered something: seems the issue can be triggered by saving (detailed below).  I've added a different URL to the URL field.

> Just as a quick check; I assume the same problem occurs in safe mode?

I've just tested in safe mode, using a new profile, and discovered a way to reproduce the issue of memory not being freed, using the following STR:

PART I:
1. Create a new profile and launch it in safe mode
2. Disable "Block reported attach sites" and "Block reported web forgeries" in Tools --> Options... --> Security
3. Visit http://www.micron.com/media/gallery/locations
4. Open Task Manager and observe firefox.exe memory consumption
5. Wait a few seconds for memory consumption to settle then note the value
6. Click the link for the image of the "Boise, Idaho" location (third from top)
7. Once the image has fully loaded, note the memory consumption
8. Close the tab containing the image
9. Wait for memory to be released and note how long it took
10. Close the browser

PART II:
11. Relaunch the profile in safe mode
12. Repeat Steps 3 to 7
13. Right-click the image and Save As... to somewhere on your hard drive
14. Close the tab containing the image
15. Wait for memory to be released.  In my case, it never got freed.

The following are my results from the above STR:

[Click image link]: Mem consumption from 42.1 MB --> 70.5 MB
[Close image tab]:  Mem consumption from 70.5 MB --> 42.4 MB (in 8 sec)

[Restart browser in safe mode]

[Click image link]: Mem consumption from 42.5 MB --> 70.9 MB
[Save image]
[Close image tab]:  Mem consumption from 74.8 MB --> UNCHANGED even after 5 min

So, for whatever reason, save operations seems to cause memory to not get freed.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I assume you don't see that effect if you just right-click the image (to bring up the context menu) but then don't actually select "Save As"?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, and confirming.  Thank you very much for the reliable steps to reproduce!

Comment 5

[Boris Zbarsky [:bzbarsky]]

One other question.  Does closing the window the save was performed in free memory?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and I assyme you step 13 is to use "Save Image As.." ?

Comment 7

[IU]

(In reply to comment #3)
> I assume you don't see that effect if you just right-click the image (to bring
> up the context menu) but then don't actually select "Save As"?

Interestingly.  Merely right-clicking the image or the page leads to memory not being freed when the tab is closed.  So I suppose this has something to do with the context menu, instead of saving.

(In reply to comment #5)
> One other question.  Does closing the window the save was performed in free
> memory?

Yes.  If I have two windows open and close the one where the image was loaded, the memory is freed -- even though it did not free it when the tab containing the image was closed.

(In reply to comment #6)
> Oh, and I assyme you step 13 is to use "Save Image As.." ?

Correct.  But based on my response to your comment 3, it appears only the appearance of the context menu is required to trigger this.

Comment 8

[Dão Gottwald [:dao]]

gContextMenu.target is set to the target node when the context menu opens:

http://hg.mozilla.org/mozilla-central/annotate/fe3dc8da8e9f/browser/base/content/nsContextMenu.js#l522

gContextMenu is set to null when the context menu closes:

http://hg.mozilla.org/mozilla-central/annotate/fe3dc8da8e9f/browser/base/content/browser.xul#l212

Comment 9

[Boris Zbarsky [:bzbarsky]]

Just one more thing.  If you then open a context menu on some other node on some other page in some tab in that same window, the memory is released, right?

From peterv on irc:

<peterv>     0x008A28E0 [nsGlobalWindow [5/7]]
<peterv>         --[mChromeEventHandler]-> 0x1592A940 [nsWindowRoot]
<peterv>         --[mPopupNode]-> 0x1E3738C0 [nsGenericElement (xhtml) img]

Comment 10

[Boris Zbarsky [:bzbarsky]]

So it looks like we just always hold on to the last popup node for the window, whatever it happens to be.  That sucks.  Can we drop mPopupNode at some point?  Do we have a way to tell what that point is?

Comment 11

[[e-mail]]

(In reply to comment #10)
> Can we drop mPopupNode at some point?
Yes.

> Do we have a way to tell what that point is?
Who's "we"? If you mean the browser window, then it only needs the popup node when the context menu opens, however some extensions may possibly depend on it existing but even then probably no later than when the context menu closes.

In the worst case we can drop the popup node when we close or navigate its tab.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> > Do we have a way to tell what that point is?
> Who's "we"?

nsXULPopupListener? Kind of seems logical to me that the setter should also clear it, and I don't think it needs to outlive the popup. Is clearing it in nsXULPopupListener::ClosePopup feasible/correct?

Comment 13

[IU]

(In reply to comment #9)
> Just one more thing.  If you then open a context menu on some other node on
> some other page in some tab in that same window, the memory is released, right?

Not necessarily.  It depends on order:

SCENARIO I: Memory is freed

1. Middle-click the link for the Boise, Idaho "High-resolution image"
2. Middle-click the link for the East Kilbride, Scotland "High-resolution image"
3. Switch to the Boise Idaho tab and right-click the image
4. Close the Boise Idaho tab
5. Switch to the East Kilbride tab and right-click the image
6. Close the East Kilbride tab
7. Memory is freed


SCENARIO II: Memory is NOT freed (note that Steps 3,4 and 5,6 are swapped)

1. Middle-click the link for the Boise, Idaho "High-resolution image"
2. Middle-click the link for the East Kilbride, Scotland "High-resolution image"
3. Switch to the East Kilbride tab and right-click the image
4. Close the East Kilbride tab
5. Switch to the Boise Idaho tab and right-click the image
6. Close the Boise Idaho tab
7. Memory NOT freed

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Not necessarily.  It depends on order:

OK, sounds like something other than the mPopupNode might be involved, maybe.  We should fix the mPopupNode thing and then retest.

Comment 15

[Neil Deakin]

Also related is bug 480191.


(In reply to comment #12)
> nsXULPopupListener? Kind of seems logical to me that the setter should also
> clear it, and I don't think it needs to outlive the popup. Is clearing it in
> nsXULPopupListener::ClosePopup feasible/correct?

It isn't, as any caller can set popupNode before it opens a popup, not just the context menu handler (and several others do). It would be ok to clear it when the popup is hidden, unfortunately since it isn't known what caller set popupNode, we don't know for which popup to clear it.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Can we change the API to require passing in the needed information?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> It isn't, as any caller can set popupNode before it opens a popup, not just the
> context menu handler (and several others do).

The only other real callers of SetPopupNode with an argument other than "null" seem to be tests (and ChromeContextMenuListener::ContextMenu which I think is unused). Clearing the popupNode only for the common context menu case may not be sufficient to address the general problem of mPopupNode being held on to forever, but it will certainly solve the common problem.

Comment 18

[Neil Deakin]

It would probably be sufficient to handle most cases to just clear the popup node when all popups have been closed.

ChromeContextMenuListener::ContextMenu is still used for embedding, where we don't know what contextmenu mechanism the embedder is using, nor do we get a notification when it is closed that I know of.

Or, I can just finish the patch in bug 383930.

Comment 19

[Damon Sicore (:damons)]

Blocking 1.9.3 final+

Comment 20

[Neil Deakin]

Should be fixed by bug 383930. (checkin http://hg.mozilla.org/mozilla-central/rev/ed8906789d08 )

Reporter, could this be retested?

Comment 21

[IU]

Nice.  Verified fixed by bug 383930, using steps in comment 13.  Thanks.

Mozilla/5.0 (Windows NT 5.1; rv:2.0b4pre) Gecko/20100809 Minefield/4.0b4pre (.NET CLR 3.5.30729) ID:20100809191130
http://hg.mozilla.org/mozilla-central/rev/f26255ffe0c9