49748 - crash because JSLoader holds XPConnect Service too long

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[John Bandhauer]

XPCOM tells XPConnect to shut down before the JS Loader is shutdown.

XPConnect does not sever the connections between wrappers and the native objects 
wrapped until its dtor. The assumption is that when XPCOM releases xpc dll's 
nsIModule and causes a call to ReleaseXPConnectSingleton, then the last 
relference on the xpconnect service will be released. At this point (when xpcom 
has told xpconnect that the system is going down) xpconnect marks all wrapper so 
that no calls will cross the wrappers into the great unknown.

However, the JS Loader holds a ref to the xpconnect service and these 
connections do not get severed when they should.

I'll attach a stack tht shows this.

This is reproducable by doing 'touch components/*.js' then running and closing 
mozilla. JSObject Finalize calls that happen when the JS Loader deletes its 
JSContext (causing a GC) go through xpconnect and become Release calls on native 
objects that are not in a godd state anymore after xpcom has otherwise shutdown 
much of the native system.

I've been through the same problem elsewhere... It would be nice if the system 
were robust enough to always do the right thing during shutdown, but much of 
mozilla is not. It is better to leak than crash.

I'll attach a proposed fix. 

The fix changes the loader to only get the xpconnect servive when it needs ti 
rather than holding a longterm ref. I also added code to xpconnect to make sure 
that the service is not brought back from the dead after being shutdown - I've 
seen this before. The patch has the InitClassesWithNewWrappedGlobal code too.

Comment 1

[John Bandhauer]

Attachment: statck of crash location

Comment 2

[John Bandhauer]

Attachment: proposed fix

Comment 3

[John Bandhauer]

shaver, review?
brendan, approval?

Comment 4

[Brendan Eich [:brendan]]

a=[e-mail].

Some day, tell me your thoughts on how to prettify the xpcTempGlobal situation.

/be

Comment 5

[John Bandhauer]

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

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Wow, I totally missed this one.  r=shaver, with enthusiasm; the speed win for
caching the XPConnect service isn't worth anything even _resembling_ a crash.

Thanks, jband.

Comment 7

[Brendan Eich [:brendan]]

This still open?  It's got r&a=.

/be

Comment 8

[John Bandhauer]

I was getting a synched up tree. 

fix checked in.

Comment 9

[John Bandhauer]

Note: I checked in an additional fix (r=shaver) for a reduandant and premature 
call to XPTI_FreeInterfaceInfoManager in NS_ShutdownXPCOM. Before this final 
checkin one might still see a crash on the "'touch components/*.js' and run 
mozilla" testcase. Now this should *really* be fixed.