421303 - Crash [@ jsds_ScriptHookProc]

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[Honza Bambas (:mayhemer)]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/jsd/jsd_xpc.cpp&rev=1.85&root=/cvsroot&mark=716#716 

on gJsds being null. 

Reason:
In jsdService::Off is missing call to JSD_SetScriptHook(mCx, NULL, NULL) that would disallow call of the jsds_ScriptHookProc after gJsds is released (set to NULL) in jsdService's destructor.

However that is not all we have to do, because the cleanup code in :Off method might not be called anyway when jsdService is released from inside of JS_GC call like this:

#0  jsdService::Off (this=0x3343a920) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/jsd/jsd_xpc.cpp:2558
#1  0x024b265a in jsdService::~jsdService (this=0x3343a920) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/jsd/jsd_xpc.cpp:3282
#2  0x024af1fc in jsdService::Release (this=0x3343a920) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/jsd/jsd_xpc.cpp:2305
#3  0x0b044c6b in XPCWrappedNative::~XPCWrappedNative (this=0x3343a970) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:691
#4  0x0b044f54 in XPCWrappedNative::Release (this=0x3343a970) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:936
#5  0x0b04526b in XPCWrappedNative::FlatJSObjectFinalized (this=0x3343a970, cx=0x2b5eacb0) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:1077
#6  0x0b04f6ca in XPC_WN_NoHelper_Finalize (cx=0x2b5eacb0, obj=0x327f36a0) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:640
#7  0x0142527b in js_FinalizeObject (cx=0x2b5eacb0, obj=0x327f36a0) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/src/jsobj.c:2817
#8  0x013f2a52 in js_GC (cx=0x2b5eacb0, gckind=GC_NORMAL) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/src/jsgc.c:3315
#9  0x013a8755 in JS_GC (cx=0x2b5eacb0) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/src/jsapi.c:2393
#10 0x0b00a91a in nsXPConnect::Collect (this=0x23296c0) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:526
#11 0x01737c4c in nsCycleCollector::Collect (this=0xad000, aTryCollections=5) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/xpcom/base/nsCycleCollector.cpp:2191
#12 0x01737d6b in nsCycleCollector::Shutdown (this=0xad000) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/xpcom/base/nsCycleCollector.cpp:2412
#13 0x01737da0 in nsCycleCollector_shutdown () at /Users/starapica/Documents/mozilla/TRUNK/mozilla/xpcom/base/nsCycleCollector.cpp:2849
#14 0x016d3877 in NS_ShutdownXPCOM_P (servMgr=0x0) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/xpcom/build/nsXPComInit.cpp:783
#15 0x0020800a in ScopedXPCOMStartup::~ScopedXPCOMStartup (this=0xbffff43c) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/toolkit/xre/nsAppRunner.cpp:909
#16 0x0020fa4d in XRE_main (argc=4, argv=0xbffff75c, aAppData=0x230b900) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/toolkit/xre/nsAppRunner.cpp:3195
#17 0x00002798 in main (argc=6, argv=0xbffff75c) at /Users/starapica/Documents/mozilla/TRUNK/mozilla/browser/app/nsBrowserApp.cpp:158

::Off method early leaves here:

http://mxr.mozilla.org/mozilla/source/js/jsd/jsd_xpc.cpp#2571 (return NS_ERROR_NOT_AVAILABLE)

This is description from timeless:

"jsd assumed that the componentmanager would hold the last reference. I'm assuming there is *a* reference in the componentmanager but it sounds like the component manager is being reaped before the final JS_GC which sadly makes sense too. jsd can't shut down before JS is done but JS can't shut down before the component(service) manager lets go of all the JS components which, unfortunately, includes non js components like jsd"


Quick solution: add !NULL check to the hook method for now to prevent the crash.

True solution: hold the jsdService longer then JS GC finished its work.

STR on Mac OS X:
1. Run trace-malloc trunk build (3 hours old since this comment post)
2. I have open two tabs with Minefield home page
3. Open Preferences at the Applications tab
4. Quit Minefield => should crash

I will submit a patch for the quick solution soon.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: Draft, has problems, depends on another patch

With this patch I got a loop. That should be fixed with http://pastebin.mozilla.org/356859 patch. With both patches there is no problem.

Comment 2

[timeless]

Attachment: sufficient

FWIW, mayhemer's patch isn't necessary. This patch alone is sufficient (thanks to mayhemer for testing).

Here's another summary (which I believe is equivalent to what's written above, but which if I remember, I'd like to include in the checkin comment):

If we reach ~jsdService, that means our client doesn't care about us, so we can (and should) drop all references to any callbacks (if they cared, they'd have kept us alive!*).

I think jsdService::Off should clear all the hooks, the strange magic of not clearing it isn't really a great idea. So for Off, we'll now clear the ScriptHook too (consumers who use off should really drop any references they have to our objects...). I'm still on the fence on this point, I suspect we can actually move it from ::Off to ~jsdService (it must be cleared at some point, otherwise if jsd_xpc's library manages to get unloaded, the function pointer would be invalid, which would be *BAD*).

jsds_NotifyPendingDeadScripts needs to clear gDeadScripts whether or not there's a service or hooks, so it does. Because it's a static callback and because of the scary way GC works, I'd rather ensure (deathgrip) that jsds is available (and consistent!) for the duration of the function call. The code already handles the lack of a hook, so there's no reason to do magical returns....

The real problem which mayhemer found was that jsdService::Off was returning early (failure) because gGCStatus wasn't JSGC_END when called from ~jsdService from JS_GC from the cyclecollector, so we make sure that ~jsdService forces ::Off to act as if it is JSGC_END (after ensuring that there are no callbacks available).

* a pure javascript (xpcom component, not DOM hosted!) version of a jsdService consumer means that jsdService will need to talk to the CycleCollector eventually (this is another bug for the future).

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 307894 [details] [diff] [review]
sufficient

Your explanation makes sense, and I think I agree.  r=caillon

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

There are a lot of crashes with following stack. Timeless, in bug 411249 comment 24 you mentioned that it will be fixed by bug 303821. Is this still true or not? If it's the latter one, will it be covered by your latest patch on this bug? 

Crasher id: bp-2fb83f3a-ed3c-11dc-b609-001a4bd43e5c

0  	jsds_ScriptHookProc  	 mozilla/js/jsd/jsd_xpc.cpp:720
1 	jsd_DestroyScriptHookProc 	mozilla/js/jsd/jsd_scpt.c:630
2 	js3250.dll@0x4c679 	
3 	JS_GC 	mozilla/js/src/jsapi.c:2418
4 	nsXPConnect::Collect() 	mozilla/js/src/xpconnect/src/nsXPConnect.cpp:526
5 	nsCycleCollector::Collect(unsigned int) 	mozilla/xpcom/base/nsCycleCollector.cpp:2192
6 	nsCycleCollector_shutdown() 	mozilla/xpcom/base/nsCycleCollector.cpp:2850
7 	NS_ShutdownXPCOM_P 	mozilla/xpcom/build/nsXPComInit.cpp:783

Each of these crashes happens on shutdown of Firefox. I think that mostly Firebug 1.05 is causing this crash.

Comment 5

[timeless]

this patch should handle it, yes.

Comment 6

[Damon Sicore (:damons)]

Comment on attachment 307894 [details] [diff] [review]
sufficient

a1.9+=damons

Comment 7

[timeless]

Comment on attachment 307894 [details] [diff] [review]
sufficient

mozilla/js/jsd/jsd_xpc.cpp 	1.88

Comment 11

[timeless]

this needs to be backported :(