The (trivial) patch is attached--but this causes trunk MinefieldDebug to abort for me on Mac OS X. Apparently Firefox does not use the SpiderMonkey request model correctly everywhere. Brendan, this is a surprise, right? Do we care?

Yeah, we care... maybe not enough to fix it, but certainly enough to know what the bad stacks are.

We care more than that. There's no point in being half-correct here. Songbird (Ben Turner) did a bunch of work to get the request model enabled. This bug should be fixed. /be

Thankfully it's easy to fix these with JSAutoRequest. And once we work out all the existing violations I'd love for this patch to get checked in so that the boxes go orange when someone forgets.

Ben, can you take this and track down the offenders? Blocking bugs as needed. /be

My plate is a little full atm because we're stabilizing for a release, but we certainly don't want more racing GC crashes. I'll file a bug in our bugzilla and take this as soon as it gets prioritized. Is that acceptable?

Unless someone (Jason may) beats you to it, sure. Jason, can you get the stack you saw into this bug? /be

Here's the stack. It looks a little funny to me: apparently when mozilla enters JS_ExecuteScript at frame #11, the context is in a request. The script calls back into C++, which then calls back into the JSAPI. That's when the assertion fails. Somewhere we left the request. I haven't looked into it; I'm a bit busy too. Program received signal SIGTRAP, Trace/breakpoint trap. JS_Assert (s=0x10c0034 "cx->requestDepth", file=0x10bfff8 "/Users/jason/dev/am/actionmonkey-ffbuild/js/src/jsapi.cpp", ln=3287) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/jsutil.cpp:63 63 abort(); (gdb) where #0 JS_Assert (s=0x10c0034 "cx->requestDepth", file=0x10bfff8 "/Users/jason/dev/am/actionmonkey-ffbuild/js/src/jsapi.cpp", ln=3287) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/jsutil.cpp:63 #1 0x0100d52b in JS_GetProperty (cx=0x2135e00, obj=0x302b308, name=0x333bd268 "EXPORTED_SYMBOLS", vp=0xbfffe064) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/jsapi.cpp:3287 #2 0x333b193a in mozJSComponentLoader::ImportInto (this=0x2133b60, aLocation=@0x2157170, targetObj=0x3026548, cc=0xbfffe5ec, _retval=0xbfffe17c) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/xpconnect/loader/mozJSComponentLoader.cpp:1458 #3 0x333ab932 in mozJSComponentLoader::Import (this=0x2133b60, registryLocation=@0x2157170) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/xpconnect/loader/mozJSComponentLoader.cpp:1378 #4 0x3336162c in nsXPCComponents_Utils::Import (this=0x2156e80, registryLocation=@0x2157170) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/xpconnect/src/xpccomponents.cpp:3597 #5 0x01621ebf in NS_InvokeByIndex_P (that=0x2156e80, methodIndex=7, paramCount=1, params=0xbfffe458) at /Users/jason/dev/am/actionmonkey-ffbuild/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179 #6 0x3338ec66 in XPCWrappedNative::CallMethod (ccx=@0xbfffe5ec, mode=CALL_METHOD) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/xpconnect/src/xpcwrappednative.cpp:2326 #7 0x33397801 in XPC_WN_CallMethod (cx=0x2135e00, obj=0x3027a08, argc=1, argv=0x2156a54, vp=0xbfffe724) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1467 #8 0x0105bf08 in js_Invoke (cx=0x2135e00, argc=1, flags=0) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/jsinterp.cpp:1378 #9 0x01052625 in js_Interpret (cx=0x2135e00, pc=0x2875d97 ":", result=0xbfffec2c) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/jsinterp.cpp:4111 #10 0x0105b16f in js_Execute (cx=0x2135e00, chain=0x3026548, script=0x2875c00, down=0x0, flags=0, result=0xbfffee40) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/jsinterp.cpp:1645 #11 0x01010007 in JS_ExecuteScript (cx=0x2135e00, obj=0x3026548, script=0x2875c00, rval=0xbfffee40) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/jsapi.cpp:4710 #12 0x333b0db1 in mozJSComponentLoader::GlobalForLocation (this=0x2133b60, aComponent=0x2133610, aGlobal=0x2150cc4, aLocation=0x2150cc8) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/xpconnect/loader/mozJSComponentLoader.cpp:1261 #13 0x333b256b in mozJSComponentLoader::LoadModule (this=0x2133b60, aComponentFile=0x2133610, aResult=0xbffff0fc) at /Users/jason/dev/am/actionmonkey-ffbuild/js/src/xpconnect/loader/mozJSComponentLoader.cpp:598 #14 0x01605608 in nsComponentManagerImpl::AutoRegisterComponent (this=0x2114420, aComponentFile=0x2133610, aDeferred=@0xbffff270, minLoader=0) at /Users/jason/dev/am/actionmonkey-ffbuild/xpcom/components/nsComponentManager.cpp:3032 #15 0x01605bab in nsComponentManagerImpl::LoadLeftoverComponents (this=0x2114420, aLeftovers=@0xbffff274, aDeferred=@0xbffff270, minLoader=0) at /Users/jason/dev/am/actionmonkey-ffbuild/xpcom/components/nsComponentManager.cpp:3087 #16 0x01607989 in nsComponentManagerImpl::AutoRegister (this=0x2114420, aSpec=0x0) at /Users/jason/dev/am/actionmonkey-ffbuild/xpcom/components/nsComponentManager.cpp:3335 #17 0x015b6d18 in NS_InitXPCOM3_P (result=0xbffff64c, binDirectory=0x210c780, appFileLocationProvider=0xbffff5b8, staticComponents=0x259868, componentCount=1) at /Users/jason/dev/am/actionmonkey-ffbuild/xpcom/build/nsXPComInit.cpp:657 #18 0x00207f94 in ScopedXPCOMStartup::Initialize (this=0xbffff64c) at /Users/jason/dev/am/actionmonkey-ffbuild/toolkit/xre/nsAppRunner.cpp:878 #19 0x002160fe in XRE_main (argc=1, argv=0xbffff88c, aAppData=0x210c550) at /Users/jason/dev/am/actionmonkey-ffbuild/toolkit/xre/nsAppRunner.cpp:2881 #20 0x00002be2 in main (argc=1, argv=0xbffff88c) at /Users/jason/dev/am/actionmonkey-ffbuild/browser/app/nsBrowserApp.cpp:153

That stack implicates Cu.import.

Oh, XPConnect suspends the request (intentionally, of course) in XPCWrappedNative::CallMethod. http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappednative.cpp#2320 Assuming we are going to lift the request model into MMgc for Mozilla 2, then XPConnect will no longer be a boundary between request-model and non-request-model code. So this call to JS_SuspendRequest will go away. Right? But for the time being, of course, the fix is probably a JSAutoRequest in mozJSComponentLoader::Import, somewhere a bit after this: http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#1338 (BTW the stack I listed is for an ActionMonkey build, but exactly the same thing happens in trunk. Should be real easy to reproduce.)

Going to take a bit more than that one JSAutoRequest. I'll work through them. Thread 0 Crashed: 0 libmozjs.dylib 0x010a4b53 JS_Assert + 63 (jsutil.c:63) 1 libmozjs.dylib 0x01017932 JS_GetReservedSlot + 61 (jsapi.c:3995) 2 libxpconnect.dylib 0x2a351dbd XPC_SJOW_Finalize(JSContext*, JSObject*) + 39 (XPCSafeJSObjectWrapper.cpp:658) 3 libmozjs.dylib 0x0106b177 js_FinalizeObject + 76 (jsobj.c:2798) 4 libmozjs.dylib 0x010490c5 js_GC + 2481 (jsgc.c:2714) 5 libmozjs.dylib 0x01021bd8 js_DestroyContext + 481 (jscntxt.c:430) 6 libmozjs.dylib 0x01010d42 JS_DestroyContext + 25 (jsapi.c:994) 7 libxpconnect.dylib 0x2a32c646 XPCJSContextStack::SetSafeJSContext(JSContext*) + 38 (xpcthreadcontext.cpp:266) ... Thread 0 Crashed: 0 libmozjs.dylib 0x010a4b53 JS_Assert + 63 (jsutil.c:63) 1 libmozjs.dylib 0x0100a69b JS_TypeOfValue + 60 (jsapi.c:596) 2 libgklayout.dylib 0x0da34719 nsJSContext::BindCompiledEventHandler(nsISupports*, void*, nsIAtom*, void*) + 495 (nsJSEnvironment.cpp:1895) ...

This passes mochitest with CHECK_REQUEST enabled.

Comment on attachment 281247 [details] [diff] [review] Add missing request calls >Index: js/src/xpconnect/loader/mozJSComponentLoader.cpp >=================================================================== >RCS file: /cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v >retrieving revision 1.143 >diff -u -8 -p -r1.143 mozJSComponentLoader.cpp >--- js/src/xpconnect/loader/mozJSComponentLoader.cpp 11 Aug 2007 00:53:53 -0000 1.143 >+++ js/src/xpconnect/loader/mozJSComponentLoader.cpp 17 Sep 2007 23:02:13 -0000 >@@ -1334,16 +1334,18 @@ mozJSComponentLoader::Import(const nsACS > "Components.utils.import called from another utils method."); > } > #endif > > JSContext *cx = nsnull; > rv = cc->GetJSContext(&cx); > NS_ENSURE_SUCCESS(rv, rv); > >+ JSAutoRequest ar(cx); >+ Nice (note blank line). >@@ -1450,16 +1452,17 @@ mozJSComponentLoader::ImportInto(const n > mod = newEntry; > } > > NS_ASSERTION(mod->global, "Import table contains entry with no global"); > *_retval = mod->global; > > jsval symbols; > if (targetObj) { >+ JSAutoRequest ar(mContext); > if (!JS_GetProperty(mContext, mod->global, Would be nice to have a blank line after the ar decl here too, I think. > JS_STATIC_DLL_CALLBACK(void) > XPC_XOW_Finalize(JSContext *cx, JSObject *obj) > { >+ JSAutoRequest ar(cx); Aha, this shows the JS_PARANOID_REQUEST check in jsapi.c is wrong, I think. We should not require JS_GetReservedSlot called from a finalizer, called from the JS GC, to be in a request, because the GC is special: it blocks all other requests that are trying to begin, and object locking is optimized to know this (so it's not just about GC scheduling, but also object locking). Blake and Igor should weigh in here, but I think this ar and the next one: > XPC_SJOW_Finalize(JSContext *cx, JSObject *obj) > { >+ JSAutoRequest ar(cx); should be removed from the patch, and instead the jsapi.c CHECK_REQUEST macro shold test either ((cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread). The rest is nice, so I'm stamping sr+me -- good for future iterations with the above changes or similar, no need to resolicit. Thanks for testing and patching this! /be

(In reply to comment #12) > > JS_STATIC_DLL_CALLBACK(void) > > XPC_XOW_Finalize(JSContext *cx, JSObject *obj) > > { > >+ JSAutoRequest ar(cx); > > I think this ar and the next one: > > > XPC_SJOW_Finalize(JSContext *cx, JSObject *obj) > > { > >+ JSAutoRequest ar(cx); ... > > should be removed from the patch, and instead the jsapi.c CHECK_REQUEST macro > shold test either ((cx)->requestDepth || (cx)->thread == > (cx)->runtime->gcThread). Right, the engine should not require any requests calls in callbacks. Moreover, any call to Request API from the finalizers is just wrong and the engine should check for that. But that can wait another bug. And changing CHECK_REQUEST in the above way should fix the problem nicely. > > The rest is nice, so I'm stamping sr+me -- good for future iterations with the > above changes or similar, no need to resolicit. > > Thanks for testing and patching this! > > /be >

I filed bug 396499 about extra asserts against finalizers calling *Request API.

Comment on attachment 281247 [details] [diff] [review] Add missing request calls What brendan said.

Jesse should try some DOM variations on this when it's ready.

Comment on attachment 281388 [details] [diff] [review] turn on the macro too With the style nit on the CHECK_REQUEST macro body layout, a=me. /be