199465 - scriptProto leaks in nsXULDocument

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[David Bradley]

I just startup, get the dialog to select a profile, exit, and get the leak. This
is from a trunk build from late yesterday.

It looks like the scriptProto gets leaked in nsXULDocument::OnStreamComplete.
Purify shows this as a common point, and I find no leak of a XUL document. (But
I'm not 100% sure on that).

I'll attach some supporting info.

This was fairly recent. I didn't see this leak over the weekend, and my build
was from Friday the 21st.

Comment 1

[David Bradley]

Attachment: Purify stack and info

Comment 2

[David Bradley]

I thought I put this in the original comment, but the patch for bug 124412 might
be a candidate.

Comment 3

[Paul Wyskoczka]

Nominating...

Comment 4

[Johnny Stenback (:jst)]

Could you compile the JS engine with symbols too? This stack doesn't show what's
leaked, just that something leaks when compiling a script. I verified that we
don't leak nsXULPrototypeScript's when starting, selecting a profile, and
closing Mozilla, but I dunno what happens beyond that yet.

Comment 5

[David Bradley]

Attachment: Stacks from three leaks

This is full stacks from three leaks. All the leaks I've seen traced back to
the scriptProto. The script proto itself may be getting cleaned up (Purify
leaves a bit to be desired when it comes to tracking down relationships between
allocated memory), but the JS script object looks like it doesn't, or at least
many of the things it holds on to.

Comment 6

[Samir Gehani]

Nav triage team: nsbeta1+/adt2

Reassigning to jst to help us fix this for mozilla 1.4.

Comment 7

[David Bradley]

Attachment: Stack to the destructor call

Ok, I spent a little time on this. What appears to be happening is that
unrooting that occurs in the destructor of nsXULPrototypeScript occurs after
the last GC is run. And that's why the script object is leaking. I've attached
a stack to where the destructor is getting called. I don't know if this stuff
was getting cleaned up earlier, but it didn't leak before the 25th, or near
there.

Comment 8

[Brendan Eich [:brendan]]

We are facing a problem that I recall discussing with jband long ago: XPCOM
shutdown is not well-ordered.  Here the specific detail is that JS global GC
roots are held past the destruction of the last JS context in the runtime.

Bryner just suggested we tweak things a little, so that the XBL service releases
its reference to the XUL cache early, on "xpcom-shutdown" notification.  That
might do the job, for now.  THe problem is that we're counting on no other
module caching a reference to the XUL cache, and then that module shuts down
after the last context has been destroyed.

To fix this right, we need a well-ordered shutdown procedure that destroys the
last JS context after all global GC roots have been removed.

/be

Comment 9

[Johnny Stenback (:jst)]

Attachment: Fix. Flush the XUL cache before running the last GC

Comment 10

[Johnny Stenback (:jst)]

The attached patch isn't an ideal catch-all fix for all shutdown leaks of this
type, but it should take care of this one no matter who holds on to the XUL cache.

Since I can't repro this leak, I can't say for sure that this fixes the leak,
David, could you attach the patch and test?

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 119138 [details] [diff] [review]
Fix. Flush the XUL cache before running the last GC

Cool -- I didn't know the DOM did a final GC.  That's a pretty darn good place
to flush the XUL cache.

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 119138 [details] [diff] [review]
Fix. Flush the XUL cache before running the last GC

Just talked to bryner, after thinking about this more.	Would it be better to
have the XUL cache itself observe xpcom-shutdown, and flush itself and then
disable itself (so no new entries can be filled in the XUL cache)?

Then the DOM wouldn't need to know about the XUL cache.  Better still, we
wouldn't have to worry about whether this GC was really the "last" one (the JS
component loader, if used, can do a "last GC" also, using a context it creates
just for that purpose).

/be

Comment 13

[Brendan Eich [:brendan]]

bryner had thought maybe the XBL service, on xpcom-shutdown, should release its
ref to the XUL cache after flushing the cache.  That's less modular than the
idea in comment #12, but it does suggest fixing the XBL service to release its
XUL cache ref on shutdown -- in addition to making the XUL cache observe
shutdown and self-flush and self-disable.

/be

Comment 14

[Johnny Stenback (:jst)]

If we do that then the XUL cache has to run GC on shutdown as well since there's
no guarantee that the DOM code's shutdown observer will be called *after* the
XUL cache's observer is called. If the order happens to be reversed, then the
roots that are removed in the XUL cache's shutdown observer would never be GC'd...

Comment 15

[David Bradley]

I tried jst's patch and it clears the leaks up.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 119138 [details] [diff] [review]
Fix. Flush the XUL cache before running the last GC

Ok, I'm sold.  Unless anyone has a more modular way to do it, this seems good
for 1.4b.  Bryner, what do you say?

/be

Comment 17

[David Bradley]

Comment on attachment 119138 [details] [diff] [review]
Fix. Flush the XUL cache before running the last GC

r=dbradley

I also verified that this wasn't holding the component manager in some indirect
way. Looks like oji is the only one left. Thanks, this greatly reduces the
noise in Purify

Comment 18

[Johnny Stenback (:jst)]

Fixed.

Comment 19

[Cathleen]

I was happy to see this leak gets fixed!  :-)
but, this fix also increased libjsdom.so by 4k+ 
is there any better way not to impact static footprint as much?

libjsdom.so
  	Total:	      +4735 (+4747/-12)
  	Code:	       +220 (+232/-12)
  	Data:	      +4515 (+4515/+0)
  	      +4515 (+4515/+0)	R (DATA)
  		      +4515 (+4515/+0)	UNDEF:libjsdom.so:R
  			      +4499	kXULPrototypeCacheCID.2844
  			        +16	_Q220nsIXULPrototypeCache33GetIID__20nsIXULPrototypeCache..0.iid
  	       +220 (+232/-12)	T (CODE)
  		       +220 (+232/-12)	UNDEF:libjsdom.so:T
  			       +204	nsDOMSOFactory::Observe(nsISupports *, char const *, unsigned
short const *)
  			        +28	nsIXULPrototypeCache::GetIID(void)
  			        -12	nsJSProtocolHandler::AllowPort(int, char const *, int *)
  

Comment 20

[Johnny Stenback (:jst)]

Attachment: Duh, ignore this.

Comment 21

[Johnny Stenback (:jst)]

Attachment: Maybe this would be less code?

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 119248 [details] [diff] [review]
Maybe this would be less code?

This is 58 bytes smaller on disk, dunno how much of a runtime impact this has
tho.

Comment 23

[Johnny Stenback (:jst)]

*** Bug 102655 has been marked as a duplicate of this bug. ***

Comment 24

[Paul Wyskoczka]

Marking verified per comments.