156849 - embedded gecko can suffer from too little GC in some cases

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Harshal Pradhan]

I have been tracking down a few reports of an embedded app of mine (dont ask,
can't tell ... sorry.) It is mysteriously consuming too much memory in some
cases. I think I finally tracked down the problem to strange situation where
there is a possibility of too little javascript GC.

To be able to reproduce this you have to use an embedding testbed .... something
like mfcembed will do. Startup mfcembed and set the home page of mfcembed to
point to the testcase that I am about to attach. Watch the memory consumption/VM
size for mfcembed using Task Manager or something like that. Now open a lot of
new windows till you see the consumption go up a few MB (15-20 windows or more).
Now close all the windows but one. Notice that you dont get back all of the
memory. Repeat if necessary if you want to confirm that the consumption keeps
increasing. Continue to watch memory consumtion as you do this and you will see
a steady increase until you do something to trigger a GC. While doing this dont
do  something like clicking on a link to navigate somewhere else will, since
that does trigger a GC.

This is what I believe is happening: 
As far as I can tell, there are two places in DOM that usually trigger GC.
GlobalWindowImpl::SetNewDocument() and the nsJSContext destructor. But since
someone is holding on to the nsJSContext(in this case a nsJSEventHandler), the
nsJSContext will not be released till the document is destroyed. However, the
SetNewDocument() will not trigger a GC when it gets called during
nsDocShell::Destroy(), since it called at that time with aDocument == nsnull.
So, we can open the window, load the document, and close the window,  without
ever triggering GC. If you keep doing that for too long, without doing anything
else, then you will end up with alarmingly large amounts of memory in use. 

The solution seems to be to simply allow a GC to happen when
GlobalWindowImpl::SetNewDocument() is called with a null document (which happens
while a docshell is being destroyed). Is there a problem with doing this?

Comment 1

[Harshal Pradhan]

Attachment: testcase

Comment 2

[Harshal Pradhan]

> someone is holding on to the nsJSContext(in this case a nsJSEventHandler), the

That should have been nsJSEventListener not nsJSEventHandler.

Comment 3

[Harshal Pradhan]

Attachment: one line patch

Any reason why this will not work?

Comment 4

[Johnny Stenback (:jst)]

Attachment: Another one-line patch

I don't like changing how often we potentially do a GC during document
destruction (even though the GC timer code should eliminate any GC's that the
first patch would possibly add) so I'd rather see us make sure we schedule a GC
when a window is torn down. This patch does that. Does this patch also fix the
problem?

Comment 5

[Johnny Stenback (:jst)]

Er, s/destruction/transitions/, duh! (it's getting late, or something)

Comment 6

[Harshal Pradhan]

Yep, that should work as well. Thanks.

Comment 7

[Harshal Pradhan]

Well ... on thinking about it a bit ...

Seems to me that the two patches are for all practical purposes, equivalent. We
are doing gc on document transition anyway. If we have a document transition to
a new document, SetNewDocument() will be called with a non-null aDocument. The
current code triggers gc in that case anyway, and that doesn't change in anyway
with my patch.

Its only when aDocument is null that the my patch changes behaviour. And I
believe that only happens when the window is being torn down. Right? Which leads
me to ask: Why was the original code written such that you do have a gc when
transitioning to some new document, but don't have one when you transition to no
document?

That was just a bit of wondering aloud though .... I don't have any favorites
amongst the two patches. Either does of them does the job for me.

Comment 8

[Johnny Stenback (:jst)]

We call SetNewDocument(nsnull) during every document transition, i.e. when you
go from page to page we first call SetNewDocument(nsnull) to clear out the old
document and then we call SetNewDocument(aNewDocument) to insert the new document.

Comment 9

[Harshal Pradhan]

Oh. Of course.  I hadn't realized that. 

Thats what ususally happens when I start "thinking a little bit" without looking
at the code :-) I suppose I should shut up now. 

Comment 10

[Johnny Stenback (:jst)]

No, not at all, you were right on track...

Comment 11

[Harshal Pradhan]

Ugh .. everyone seems to have forgotten about this one.

Bug has a patch but no review. If this is going to go in, would it not be a good
idea to get this in before the 1.2alpha freeze. Any chance of that happening?

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 90926 [details] [diff] [review]
Another one-line patch

r=caillon

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 90926 [details] [diff] [review]
Another one-line patch

sr=bzbarsky

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in