Closed Bug 476643 Opened 14 years ago Closed 14 years ago

JS_REQUIRES_STACK errors in nsXPCWrappedJSClass::CallMethod

Categories

(Core :: XPConnect, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: jorendorff)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

/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*)
after discussion with mrbkap, I think this is what we want, but I want jorendorff to confirm.
Assignee: nobody → benjamin
Attachment #360344 - Flags: review?(mrbkap)
Attachment #360344 - Flags: review?(jorendorff)
Comment on attachment 360344 [details] [diff] [review]
Bail off trace before reentering using private APIs, rev. 1

This seems reasonable to me.
Attachment #360344 - Flags: review?(mrbkap) → review+
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.
Attachment #360344 - Flags: review?(jorendorff) → review+
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.)
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
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.
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.
Attachment #360344 - Attachment is obsolete: true
Attachment #361266 - Attachment is obsolete: true
Attachment #362304 - Flags: review?(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;
}
Attachment #362304 - Flags: review?(jorendorff)
Bug 478215 is the follow-up bug to unexpose js_AllocStack.
Blocks: 472553
Assignee: benjamin → jorendorff
Attached patch v2Splinter Review
This plus the patch in bug 477143 should turn bsmedberg-static-analysis green.
Attachment #362304 - Attachment is obsolete: true
Attachment #362932 - Flags: review?(mrbkap)
Attachment #362932 - Flags: superreview+
Attachment #362932 - Flags: review?(mrbkap)
Attachment #362932 - Flags: review+
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.
(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
Whiteboard: fixed-in-tracemonkey
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Was this for some reason not landed on the trunk yet?
This definitely landed on trunk through a TM merge (static-analysis box is green)
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.