396452 - Enforce SpiderMonkey request model with assertions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Attachment: enable the CHECK_REQUEST macro by default in jsapi.c

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?

Comment 1

[Benjamin Smedberg]

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

Comment 2

[Brendan Eich [:brendan]]

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

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 4

[Brendan Eich [:brendan]]

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

/be

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 6

[Brendan Eich [:brendan]]

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

/be

Comment 7

[Jason Orendorff [:jorendorff]]

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

Comment 8

[Benjamin Smedberg]

That stack implicates Cu.import.

Comment 9

[Jason Orendorff [:jorendorff]]

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.)

Comment 10

[Robert Sayre]

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)
...

Comment 11

[Robert Sayre]

Attachment: Add missing request calls

This passes mochitest with CHECK_REQUEST enabled.

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Igor Bukanov]

(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
> 

Comment 14

[Igor Bukanov]

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

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 281247 [details] [diff] [review]
Add missing request calls

What brendan said.

Comment 16

[Robert Sayre]

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

Comment 17

[Robert Sayre]

Attachment: fix more request sites, but not finalizers

Comment 18

[Robert Sayre]

Attachment: turn on the macro too

Comment 19

[Brendan Eich [:brendan]]

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