Accessing ScriptSecurityManager off the main thread

VERIFIED FIXED in mozilla1.9.3a1



8 years ago
8 years ago


(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: mrbkap)



Bug Flags:
blocking1.9.2 +
blocking1.9.0.19 -
wanted1.9.0.x +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .8+, status1.9.1 .8-fixed)


(Whiteboard: [needs 1.9.1 approval request])


(1 attachment)

1.14 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review

#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.
Flags: blocking1.9.2?

Comment 1

8 years ago
Created attachment 393892 [details] [diff] [review]
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.
Attachment #393892 - Flags: superreview?(jst)
Attachment #393892 - Flags: review?


8 years ago
Attachment #393892 - Flags: review? → review?(bent.mozilla)


8 years ago
Attachment #393892 - Flags: superreview?(jst) → superreview+
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

8 years ago
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?
Attachment #393892 - Flags: review?(bent.mozilla) → review+
Comment on attachment 393892 [details] [diff] [review]
Proposed fix

I did!


8 years ago
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Blake, you gonna check this in?

Comment 6

8 years ago
Last Resolved: 8 years ago
Resolution: --- → FIXED
This is needed on 1.9.2, also :)

Comment 8

8 years ago
Comment on attachment 393892 [details] [diff] [review]
Proposed fix

Have I mentioned that we have too many branches?
Attachment #393892 - Flags: approval1.9.2?
You're already blocking1.9.2+ (assuming that means what it used to...)

Comment 10

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

Comment 11

8 years ago
Keywords: fixed1.9.2
status1.9.2: --- → beta1-fixed
Keywords: fixed1.9.2
Verified fixed based on check-ins on 1.9.2 and trunk.
Keywords: verified1.9.2
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a1
blocking1.9.1: --- → .7+
status1.9.1: --- → wanted
Attachment #393892 - Flags: approval1.9.2?
Is attachment 393892 [details] [diff] [review] what you want to check in on the 1.9.1 branch?
Whiteboard: [needs 1.9.1 approval request]


8 years ago
Attachment #393892 - Flags: approval1.9.1.8?

Comment 14

8 years ago
Comment on attachment 393892 [details] [diff] [review]
Proposed fix

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

Approved for, a=dveditz for release-drivers
Attachment #393892 - Flags: approval1.9.1.8? → approval1.9.1.8+

Comment 16

8 years ago
status1.9.1: wanted → .8-fixed
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.18+
Flags: blocking1.9.0.18+ → blocking1.9.0.19?
Flags: blocking1.9.0.19? → blocking1.9.0.19-
You need to log in before you can comment on or make changes to this bug.