embedded gecko can suffer from too little GC in some cases

RESOLVED FIXED in mozilla1.1beta

Status

()

Core
DOM: Core & HTML
P2
normal
RESOLVED FIXED
16 years ago
9 years ago

People

(Reporter: Harshal Pradhan, Assigned: jst)

Tracking

Trunk
mozilla1.1beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX])

Attachments

(2 attachments, 1 obsolete attachment)

167 bytes, text/html
Details
602 bytes, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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?
(Reporter)

Comment 1

16 years ago
Created attachment 90921 [details]
testcase
(Reporter)

Comment 2

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

That should have been nsJSEventListener not nsJSEventHandler.
(Reporter)

Comment 3

16 years ago
Created attachment 90925 [details] [diff] [review]
one line patch

Any reason why this will not work?
(Assignee)

Comment 4

16 years ago
Created attachment 90926 [details] [diff] [review]
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?
(Assignee)

Comment 5

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

Comment 6

16 years ago
Yep, that should work as well. Thanks.
(Reporter)

Updated

16 years ago
Attachment #90925 - Attachment is obsolete: true
(Reporter)

Comment 7

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

Comment 8

16 years ago
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.
(Reporter)

Comment 9

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

Comment 10

16 years ago
No, not at all, you were right on track...
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
OS: Windows 2000 → All
Priority: -- → P2
Hardware: PC → All
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla1.1beta
(Reporter)

Comment 11

16 years ago
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 on attachment 90926 [details] [diff] [review]
Another one-line patch

r=caillon
Attachment #90926 - Flags: review+
Attachment #90926 - Flags: superreview+
Comment on attachment 90926 [details] [diff] [review]
Another one-line patch

sr=bzbarsky
Checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.