509824 - Accessing ScriptSecurityManager off the main thread

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P2)

Description

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

Stack:

#0  nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject (this=0x1ac373c0, ccx=@0xb0aa1ebc, jsobj=0x234d8a60, aIID=@0xba1dd0) at /src/mozilla/domthreads/js/src/xpconnect/src/xpcwrappedjsclass.cpp:262
#1  0x00b15848 in nsXPCWrappedJSClass::GetRootJSObject (this=0x1ac373c0, ccx=@0xb0aa1ebc, aJSObj=0x234d8a60) at /src/mozilla/domthreads/js/src/xpconnect/src/xpcwrappedjsclass.cpp:837
#2  0x00b0df9a in nsXPCWrappedJS::GetNewOrUsed (ccx=@0xb0aa1ebc, aJSObj=0x234d8a60, aIID=@0xb0aa1d14, aOuter=0x0, wrapperResult=0xb0aa195c) at /src/mozilla/domthreads/js/src/xpconnect/src/xpcwrappedjs.cpp:326
#3  0x00af1cf2 in XPCConvert::JSObject2NativeInterface (ccx=@0xb0aa1ebc, dest=0xb0aa1c44, src=0x234d8a60, iid=0xb0aa1d14, aOuter=0x0, pErr=0xb0aa1d10) at /src/mozilla/domthreads/js/src/xpconnect/src/xpcconvert.cpp:1451
#4  0x00af3343 in XPCConvert::JSData2Native (ccx=@0xb0aa1ebc, d=0xb0aa1c44, s=592284256, type=@0xb0aa1d47, useAllocator=0, iid=0xb0aa1d14, pErr=0xb0aa1d10) at /src/mozilla/domthreads/js/src/xpconnect/src/xpcconvert.cpp:1017
#5  0x00b1e511 in XPCWrappedNative::CallMethod (ccx=@0xb0aa1ebc, mode=XPCWrappedNative::CALL_SETTER) at /src/mozilla/domthreads/js/src/xpconnect/src/xpcwrappednative.cpp:2550
#6  0x00b2fc8d in XPCWrappedNative::SetAttribute (ccx=@0xb0aa1ebc) at xpcprivate.h:2396
#7  0x00b2b3cb in XPC_WN_GetterSetter (cx=0x169ce00, obj=0x234c4e00, argc=1, argv=0x16b1a30, vp=0xb0aa1fe0) at /src/mozilla/domthreads/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1772
#8  0x00326108 in js_Invoke (cx=0x169ce00, argc=1, vp=0x16b1a28, flags=2) at jsinterp.cpp:1371
#9  0x003264b0 in js_InternalInvoke (cx=0x169ce00, obj=0x234c4e00, fval=592284288, flags=0, argc=1, argv=0xb0aa2658, rval=0xb0aa2658) at jsinterp.cpp:1451
#10 0x00326747 in js_InternalGetOrSet (cx=0x169ce00, obj=0x234c4e00, id=505212076, fval=592284288, mode=JSACC_WRITE, argc=1, argv=0xb0aa2658, rval=0xb0aa2658) at jsinterp.cpp:1521
#11 0x00333c39 in js_SetSprop (cx=0x169ce00, sprop=0x140c2f0, obj=0x234c4e00, vp=0xb0aa2658) at jsscope.h:585
#12 0x00340ddd in js_SetPropertyHelper (cx=0x169ce00, obj=0x234c4e00, id=505212076, cacheResult=1, vp=0xb0aa2658) at /src/mozilla/domthreads/js/src/jsobj.cpp:4510
#13 0x0030dd39 in js_Interpret (cx=0x169ce00) at /src/mozilla/domthreads/js/src/jsinterp.cpp:4833
#14 0x003249b1 in js_Execute (cx=0x169ce00, chain=0x234c4e00, script=0x1da53360, down=0x0, flags=0, result=0xb0aa2b4c) at jsinterp.cpp:1644
#15 0x002a08bd in JS_ExecuteScript (cx=0x169ce00, obj=0x234c4e00, script=0x1da53360, rval=0xb0aa2b4c) at /src/mozilla/domthreads/js/src/jsapi.cpp:5025
#16 0x11ea2704 in nsDOMWorkerScriptLoader::ExecuteScripts (this=0x23529a60, aCx=0x169ce00) at /Users/bent/src/mozilla/domthreads/dom/src/threads/nsDOMWorkerScriptLoader.cpp:269
#17 0x11ea467b in nsDOMWorkerScriptLoader::LoadScripts (this=0x23529a60, aCx=0x169ce00, aURLs=@0xb0aa2bf8, aForWorker=1) at /Users/bent/src/mozilla/domthreads/dom/src/threads/nsDOMWorkerScriptLoader.cpp:148
#18 0x11ea54b2 in nsDOMWorkerScriptLoader::LoadScript (this=0x23529a60, aCx=0x169ce00, aURL=@0x183f37ec, aForWorker=1) at /Users/bent/src/mozilla/domthreads/dom/src/threads/nsDOMWorkerScriptLoader.cpp:164
#19 0x11e92dbc in nsDOMWorker::CompileGlobalObject (this=0x183f37a0, aCx=0x169ce00) at /Users/bent/src/mozilla/domthreads/dom/src/threads/nsDOMWorker.cpp:1597
#20 0x11e92f0b in nsDOMWorker::SetGlobalForContext (this=0x183f37a0, aCx=0x169ce00) at /Users/bent/src/mozilla/domthreads/dom/src/threads/nsDOMWorker.cpp:1504
#21 0x11e88991 in nsDOMWorkerRunnable::Run (this=0x183f3990) at /Users/bent/src/mozilla/domthreads/dom/src/threads/nsDOMThreadService.cpp:384
#22 0x005a8625 in nsThreadPool::Run (this=0x1da73e20) at /Users/bent/src/mozilla/domthreads/xpcom/threads/nsThreadPool.cpp:219
#23 0x005a3b52 in nsThread::ProcessNextEvent (this=0x1da56ee0, mayWait=1, result=0xb0aa2ecc) at /Users/bent/src/mozilla/domthreads/xpcom/threads/nsThread.cpp:527
#24 0x005281ce in NS_ProcessNextEvent_P (thread=0x1da56ee0, mayWait=1) at nsThreadUtils.cpp:230
#25 0x005a3d61 in nsThread::ThreadFunc (arg=0x1da56ee0) at /Users/bent/src/mozilla/domthreads/xpcom/threads/nsThread.cpp:254
#26 0x0016b363 in _pt_root (arg=0x1da68280) at /src/mozilla/domthreads/nsprpub/pr/src/pthreads/ptthread.c:228
#27 0x95b13155 in _pthread_start ()
#28 0x95b13012 in thread_start ()

Blake says we need to not do XPCWrapper::GetSecurityManager if we're not on the main thread.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This is not the best fix -- it would be better to use nsXPConnect::GetSecurityManagerForJSContext but that addrefs (so is too slow) and hands back an nsIXPCSecurityManager which doesn't have all of the right functions on it. Since, as of now, we can't do security checks like this off the main thread that matter, this patch should be fine.

Comment 2

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

Comment on attachment 393892 [details] [diff] [review]
Proposed fix

Don't you want to reverse the order of those two checks? Seems like you'll avoid the IsMainThread check more often if it comes second.

r=me either way.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

the IsMainThread condition is actually a lot faster than the STOBJ_IS_SYSTEM one, so I'd prefer to keep the checks in that order. Did you mean to stamp the patch?

Comment 4

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

Comment on attachment 393892 [details] [diff] [review]
Proposed fix

I did!

Comment 5

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

Blake, you gonna check this in?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 7

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

This is needed on 1.9.2, also :)

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 393892 [details] [diff] [review]
Proposed fix

Have I mentioned that we have too many branches?

Comment 9

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

You're already blocking1.9.2+ (assuming that means what it used to...)

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

That means I can't keep track of the rules!

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4bcf06d8ce71

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed based on check-ins on 1.9.2 and trunk.

Comment 13

[Daniel Veditz [:dveditz]]

Is attachment 393892 [details] [diff] [review] what you want to check in on the 1.9.1 branch?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 393892 [details] [diff] [review]
Proposed fix

Yes.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 393892 [details] [diff] [review]
Proposed fix

Approved for 1.9.1.8, a=dveditz for release-drivers

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d27ac705e2d6