476643 - JS_REQUIRES_STACK errors in nsXPCWrappedJSClass::CallMethod

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Benjamin Smedberg]

/builds/gcc-dehydra/installed/bin/g++ -o xpcwrappedjsclass.o -c -I../../../../dist/include/system_wrappers -include /builds/static-analysis-buildbot/slave/full/build/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -DOSTYPE=\"Linux2.6.27.5-41.fc9\" -DOSARCH=Linux -DJSFILE -DJS_THREADSAFE -DEXPORT_XPC_API -I/builds/static-analysis-buildbot/slave/full/build/js/src/xpconnect/src/../loader  -I/builds/static-analysis-buildbot/slave/full/build/js/src/xpconnect/src -I. -I../../../../dist/include/xpcom -I../../../../dist/include/string -I../../../../dist/include/js -I../../../../dist/include/caps -I../../../../dist/include/necko -I../../../../dist/include/dom -I../../../../dist/include/content -I../../../../dist/include/editor -I../../../../dist/include/layout -I../../../../dist/include/rdf -I../../../../dist/include/svg -I../../../../dist/include/xuldoc -I../../../../dist/include/xultmpl -I../../../../dist/include/widget -I../../../../dist/include   -I../../../../dist/include/xpconnect -I/builds/static-analysis-buildbot/slave/full/build/obj-buildbot/dist/include/nspr     -I/builds/static-analysis-buildbot/slave/full/build/obj-buildbot/dist/sdk/include    -fPIC   -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -fno-strict-aliasing -fshort-wchar -pthread -pipe  -DDEBUG -D_DEBUG -DDEBUG_bsmedberg -DTRACING -g -fno-inline -fplugin=/builds/gcc-dehydra/dehydra/gcc_treehydra.so -fplugin-arg='/builds/static-analysis-buildbot/slave/full/build/config/static-checking.js --topsrcdir=/builds/static-analysis-buildbot/slave/full/build --objdir=../../../.. --dehydra-modules=/builds/static-analysis-buildbot/slave/full/build/xpcom/analysis/final.js,/builds/static-analysis-buildbot/slave/full/build/layout/generic/frame-verify.js --treehydra-modules=/builds/static-analysis-buildbot/slave/full/build/xpcom/analysis/outparams.js,/builds/static-analysis-buildbot/slave/full/build/xpcom/analysis/stack.js,/builds/static-analysis-buildbot/slave/full/build/xpcom/analysis/flow.js,/builds/static-analysis-buildbot/slave/full/build/js/src/jsstack.js ' -Os -freorder-blocks -fno-reorder-functions    -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -Wp,-MD,.deps/xpcwrappedjsclass.pp /builds/static-analysis-buildbot/slave/full/build/js/src/xpconnect/src/xpcwrappedjsclass.cpp
/builds/static-analysis-buildbot/slave/full/build/js/src/xpconnect/src/xpcwrappedjsclass.cpp: In member function 'virtual nsresult nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, uint16, const XPTMethodDescriptor*, nsXPTCMiniVariant*)':
/builds/static-analysis-buildbot/slave/full/build/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1471: warning: ISO C++ forbids casting between pointer-to-function and pointer-to-object
/builds/static-analysis-buildbot/slave/full/build/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1477: warning: ISO C++ forbids casting between pointer-to-function and pointer-to-object
/builds/static-analysis-buildbot/slave/full/build/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1376: error: cannot call JS_REQUIRES_STACK function js_AllocStack(typedef JSContext*, typedef uintN, void**)
/builds/static-analysis-buildbot/slave/full/build/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1606: error: cannot call JS_REQUIRES_STACK function js_Invoke(typedef JSContext*, typedef uintN, typedef jsval*, typedef uintN)
/builds/static-analysis-buildbot/slave/full/build/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1851: error: cannot call JS_REQUIRES_STACK function js_FreeStack(typedef JSContext*, void*)

Comment 1

[Benjamin Smedberg]

Attachment: Bail off trace before reentering using private APIs, rev. 1

after discussion with mrbkap, I think this is what we want, but I want jorendorff to confirm.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 360344 [details] [diff] [review]
Bail off trace before reentering using private APIs, rev. 1

This seems reasonable to me.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 360344 [details] [diff] [review]
Bail off trace before reentering using private APIs, rev. 1

"This adds a fair amount of complexity, ..."

Yikes!  Thank goodness for Treehydra, and thank you for following up on this--it's a real crashing bug, looks like.

Comment 4

[Jesse Ruderman]

Is this patch ready to go in?  It would be really nice to get the "static-analysis-bsmedberg" Tinderbox column back in action.  (In hindsight, we should have turned JS_REQUIRES_STACK into a warning while this bug was outstanding.)

Comment 5

[Benjamin Smedberg]

Attachment: Bail off trace before reentering using private APIs, rev. 1.1 with JS_FRIEND love

This is what's needed to get Windows to link correctly. Unfortunately, there's a bug: JS_TRACER is never defined in the mozilla build, because it's a JS-internal define.

jorendorff suggests that we have a non-inlined JS_LeaveTrace API, perhaps

Comment 6

[Jason Orendorff [:jorendorff]]

This existing code that jsstack.js is tripping over is kind of gross.  Whatever is the least amount of work for you right now is fine with me.  I feel we shouldn't expose a JS_LeaveTrace API yet.

Long term, I want to fix this by unexposing js_AllocStack.  We only abuse this API in a few places.  "Friendly" applications should use malloc + JSAutoTempValueRooter instead.  Even longer term, maybe we should expose some real APIs (C++-only?) for fast, convenient temporary rooting.

Comment 7

[Benjamin Smedberg]

Attachment: Bail off trace before reentering using private APIs, rev. 1.2

This should work. I had to move js_LeaveTrace to jscntxt.h because #including jstracer.h pulled in parts, but not enough of nanojit, and pulling that thread caused lots of REQUIRES changes.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 362304 [details] [diff] [review]
Bail off trace before reentering using private APIs, rev. 1.2

*wince* I know I said "whatever's easiest for you", but...
could you please change it to work like this?

----
JS_FORCES_STACK JS_FRIEND_API(void)
js_DeepBail(JSContext *cx);

static JS_FORCES_STACK JS_INLINE void
js_LeaveTrace(JSContext *cx)
{
#ifdef JS_TRACER
    if (JS_ON_TRACE(cx))
        js_DeepBail(cx);
#endif
}

static JS_FORCES_STACK JS_INLINE JSStackFrame *
js_GetTopStackFrame(JSContext *cx)
{
    js_LeaveTrace(cx);
    return cx->fp;
}
----

I think js_DeepBail should be declared *inside* the JS_END_EXTERN_C ...although you know plenty more about those things than me, so alternatively just explain why it should be outside.

js_DeepBail can be defined the way js_GetTopStackFrame is now: in jstracer.cpp, as below, if JS_TRACER; otherwise in jscntxt.cpp, as a no-op.

JS_FORCES_STACK JS_FRIEND_API(void)
js_DeepBail(JSContext *cx)
{
    /* It's a bug if a non-FAIL_STATUS builtin gets here. */
    JS_ASSERT(cx->bailExit);

    JS_TRACE_MONITOR(cx).onTrace = false;
    JS_TRACE_MONITOR(cx).prohibitRecording = true;
    LeaveTree(*cx->interpState, cx->bailExit);
    cx->bailExit = NULL;
    cx->builtinStatus |= JSBUILTIN_BAILED;
}

Comment 9

[Jason Orendorff [:jorendorff]]

Bug 478215 is the follow-up bug to unexpose js_AllocStack.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: v2

This plus the patch in bug 477143 should turn bsmedberg-static-analysis green.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 362932 [details] [diff] [review]
v2

Please file followup bugs on making these two places use temp value rooters instead of JS_REQUIRES_STACK js_AllocStack.

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to comment #11)
> Please file followup bugs on making these two places use temp value rooters
> instead of JS_REQUIRES_STACK js_AllocStack.

I'll do this last Friday at the latest.

http://hg.mozilla.org/tracemonkey/rev/e8cc3865df16

Comment 13

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3c1c9b9d81f4

Comment 14

[Johnny Stenback (:jst)]

Was this for some reason not landed on the trunk yet?

Comment 15

[Benjamin Smedberg]

This definitely landed on trunk through a TM merge (static-analysis box is green)

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e8cc3865df16

It's there, sorry.