440982 - To avoid calling JS at unsafe times from JS_GC, jsds_ScriptHookProc should not get the script hook unless it needs to and it is safe to call

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[bronek]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.8.1.14) Gecko/20080617 Firefox/2.0.0.14
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.8.1.14) Gecko/20080617 Firefox/2.0.0.14

#0  JS_Assert (s=0x281582b2 "!rt->gcRunning", file=0x28158291 "jsgc.c", ln=1393) at jsutil.c:63
63          abort();
[New Thread 0x834b100 (LWP 100215)]
[New Thread 0x9475500 (LWP 100213)]
[New Thread 0x94f9300 (LWP 100220)]
[New Thread 0x8dd2100 (LWP 100217)]
[New Thread 0x89f3600 (LWP 100216)]
[New Thread 0x8106800 (LWP 100211)]
[New Thread 0x8106500 (LWP 100210)]
[New Thread 0x8103100 (LWP 100159)]
(gdb) bt
#0  JS_Assert (s=0x281582b2 "!rt->gcRunning", file=0x28158291 "jsgc.c", ln=1393) at jsutil.c:63
#1  0x280e1008 in js_NewGCThing (cx=0x8ed2680, flags=1, nbytes=8) at jsgc.c:1393
        rt = (JSRuntime *) 0x81fa000
        flindex = 0
        doGC = Variable "doGC" is not available.
#2  0x28138baa in js_NewString (cx=0x8ed2680, chars=0x8608c20, length=42, gcflag=0) at jsstr.c:2442
        str = Variable "str" is not available.
#3  0x280b8de1 in js_AtomizeString (cx=0x8ed2680, str=0xbfbfdc80, flags=Variable "flags" is not available.
) at jsatom.c:660
        gen = 168171
        keyHash = 2150442760
        key = -1077945212
        state = (JSAtomState *) 0x81fa0e8
        table = (JSHashTable *) 0x81a7160
        he = Variable "he" is not available.
#4  0x280b9044 in js_Atomize (cx=0x8ed2680, bytes=0x28eec0d8 "global_for_XPCJSContextStack_SafeJSContext", length=42, flags=0)
    at jsatom.c:735
        chars = (jschar *) 0x8608c20
        str = Variable "str" is not available.
#5  0x2810393c in js_GetClassId (cx=0x8ed2680, clasp=0x28ef27c0, idp=0xbfbfdcf8) at jsobj.c:2379
#6  0x28109628 in js_NewObject (cx=0x8ed2680, clasp=0x28ef27c0, proto=0x0, parent=0x0) at jsobj.c:2401
#7  0x280a8449 in JS_NewObject (cx=0x8ed2680, clasp=0x28ef27c0, proto=0x0, parent=0x0) at jsapi.c:2393
#8  0x28eb9ba7 in XPCJSContextStack::GetSafeJSContext (this=0x81ac150, aSafeJSContext=0xbfbfde18) at xpcthreadcontext.cpp:180
#9  0x28e94327 in XPCCallContext (this=0xbfbfddfc, callerLanguage=XPCContext::LANG_NATIVE, cx=0x0, obj=0x0, funobj=0x0, name=0,
    argc=4294967295, argv=0x0, rval=0x0) at xpccallcontext.cpp:93
#10 0x28ec0ce5 in nsXPCWrappedJSClass::DelegatedQueryInterface (this=0xb123f40, self=0xb123fa0, aIID=@0x2909beb4,
    aInstancePtr=0xbfbfdf18) at xpcwrappedjsclass.cpp:544
#11 0x28ebb9a5 in nsXPCWrappedJS::QueryInterface (this=0xb123fa0, aIID=@0x2909beb4, aInstancePtr=0xbfbfdf18) at xpcwrappedjs.cpp:106
#12 0x28190731 in nsQueryInterface::operator() (this=Variable "this" is not available.
) at nsCOMPtr.cpp:47
#13 0x2909af0d in nsCOMPtr<jsdIScriptHook>::assign_from_qi (this=0xbfbfdf44, qi={mRawPtr = 0xb123fa0}, aIID=@0x2909beb4)
    at nsCOMPtr.h:1232
#14 0x29098676 in jsds_ScriptHookProc (jsdc=0x8188050, jsdscript=0xa07acc0, creating=0, callerdata=0x0) at nsCOMPtr.h:645
#15 0x2908dabb in jsd_DestroyScriptHookProc (cx=0x818b300, script=0x9e2f500, callerdata=0x8188050) at jsd_scpt.c:628
#16 0x2813499b in js_CallDestroyScriptHook (cx=0x818b300, script=0x9e2f500) at jsscript.c:1458
#17 0x281355c4 in js_DestroyScript (cx=0x818b300, script=0x9e2f500) at jsscript.c:1464
#18 0x280d9765 in fun_finalize (cx=0x818b300, obj=0xea04e38) at jsfun.c:1211
#19 0x2810887c in js_FinalizeObject (cx=0x818b300, obj=0xea04e38) at jsobj.c:2762
#20 0x280e021f in js_GC (cx=0x818b300, gckind=GC_NORMAL) at jsgc.c:3031
#21 0x280ac405 in JS_GC (cx=0x818b300) at jsapi.c:1873
#22 0x2995443c in nsJSContext::Notify (this=0x8652300, timer=0xac081c0) at nsJSEnvironment.cpp:2227
#23 0x282006e6 in nsTimerImpl::Fire (this=0xac081c0) at nsTimerImpl.cpp:397
#24 0x2820197f in handleTimerEvent (event=0x9d13a30) at nsTimerImpl.cpp:459
#25 0x281f94b4 in PL_HandleEvent (self=0x9d13a30) at plevent.c:688
#26 0x281f984c in PL_ProcessPendingEvents (self=0x81e5b80) at plevent.c:623
#27 0x281fcce6 in nsEventQueueImpl::ProcessPendingEvents (this=0x81e5b50) at nsEventQueue.cpp:448
#28 0x290ea845 in event_processor_callback (source=0x85effc0, condition=G_IO_IN, data=0x81e5b50) at nsAppShell.cpp:67
#29 0x28a8af29 in g_io_channel_unix_get_fd () from /usr/local/lib/libglib-2.0.so.0
#30 0x28a5968c in g_main_context_dispatch () from /usr/local/lib/libglib-2.0.so.0
#31 0x28a5c6d2 in g_main_context_check () from /usr/local/lib/libglib-2.0.so.0
#32 0x28a5c995 in g_main_loop_run () from /usr/local/lib/libglib-2.0.so.0
#33 0x2841d294 in gtk_main () from /usr/local/lib/libgtk-x11-2.0.so.0
#34 0x290eac05 in nsAppShell::Run (this=0x82b6c80) at nsAppShell.cpp:139
#35 0x291b24f8 in nsAppStartup::Run (this=0x82b7af0) at nsAppStartup.cpp:151
#36 0x08054476 in XRE_main (argc=1, argv=0xbfbfe794, aAppData=0x8069720) at nsAppRunner.cpp:2817
#37 0x0804aff1 in main (argc=100159, argv=0x0) at nsBrowserApp.cpp:61


Reproducible: Always

Comment 1

[timeless]

Attachment: only grab hook if we need it

right, so the js we're running isn't obvious, 

It's listed as coming from this:
nsCOMPtr<jsdIScriptHook>::assign_from_qi, thankfully there's only one of those in this function. The script hook of course is written in JS.

702 jsds_ScriptHookProc (JSDContext* jsdc, JSDScript* jsdscript, JSBool creating,
703                      void* callerdata)
704 {

710     nsCOMPtr<jsdIScriptHook> hook;
711     gJsds->GetScriptHook (getter_AddRefs(hook));

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 380028 [details] [diff] [review]
only grab hook if we need it

That was easy. Good fixing there!

/be

Comment 3

[Brendan Eich [:brendan]]

Isn't this a problem on trunk and 1.9.1 branch? Please nominate if so.

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

A checkin message that describes the change ("only get script hook if we're going to call it, to avoid possible calls back into script under JS_GC's notification", perhaps) rather than the state that, post-patch, no longer exists would probably be more useful.

Comment 5

[timeless]

brendan: yes, it is, the bug is ancient and should go to all live branches.

shaver: yes, thanks.

Comment 6

[timeless]

Attachment: updated comment

asking for approval for all branches (and someone should push when approved)

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 380799 [details] [diff] [review]
updated comment

a191=shaver

Comment 8

[Samuel Sidler (old account; do not CC)]

This needs to land somewhere and bake before we'll take it for 1.9.0.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/040cb204cd92

Comment 10

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9f212010c4d2

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 380799 [details] [diff] [review]
updated comment

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 380799 [details] [diff] [review]
updated comment

Approved for 1.8.1.23, a=dveditz for release-drivers

Comment 13

[Daniel Veditz [:dveditz]]

Checking in js/jsd/jsd_xpc.cpp;
/cvsroot/mozilla/js/jsd/jsd_xpc.cpp,v  <--  jsd_xpc.cpp
new revision: 1.91; previous revision: 1.90

Comment 14

[Al Billings [:abillings - ex-MoCo]]

I assume that there is no real method to verify this fix?

Comment 15

[Mark Banner (:standard8)]

Checked for 1.8.1.24:
Checking in js/jsd/jsd_xpc.cpp;
/cvsroot/mozilla/js/jsd/jsd_xpc.cpp,v  <--  jsd_xpc.cpp
new revision: 1.72.2.3; previous revision: 1.72.2.2
done

Comment 16

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #14)
> I assume that there is no real method to verify this fix?

And now, almost eight months later, I ask again. Mark?