Last Comment Bug 509824 - Accessing ScriptSecurityManager off the main thread
: Accessing ScriptSecurityManager off the main thread
[needs 1.9.1 approval request]
: verified1.9.2
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
P2 normal (vote)
: mozilla1.9.3a1
Assigned To: Blake Kaplan (:mrbkap)
: Andrew Overholt [:overholt]
Depends on:
Blocks: 503926
  Show dependency treegraph
Reported: 2009-08-11 14:55 PDT by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2010-02-10 12:39 PST (History)
4 users (show)
benjamin: blocking1.9.2+
mbeltzner: blocking1.9.0.19-
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proposed fix (1.14 KB, patch)
2009-08-11 15:09 PDT, Blake Kaplan (:mrbkap)
bent.mozilla: review+
jst: superreview+
dveditz: approval1.9.1.8+
Details | Diff | Splinter Review

Description User image Ben Turner (not reading bugmail, use the needinfo flag!) 2009-08-11 14:55:13 PDT

#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 User image Blake Kaplan (:mrbkap) 2009-08-11 15:09:28 PDT
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.
Comment 2 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2009-08-12 12:24:10 PDT
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 User image Blake Kaplan (:mrbkap) 2009-08-12 12:37:25 PDT
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 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2009-08-12 12:54:17 PDT
Comment on attachment 393892 [details] [diff] [review]
Proposed fix

I did!
Comment 5 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2009-08-18 09:22:01 PDT
Blake, you gonna check this in?
Comment 6 User image Blake Kaplan (:mrbkap) 2009-08-18 20:57:48 PDT
Comment 7 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2009-08-23 20:06:27 PDT
This is needed on 1.9.2, also :)
Comment 8 User image Blake Kaplan (:mrbkap) 2009-08-23 23:31:30 PDT
Comment on attachment 393892 [details] [diff] [review]
Proposed fix

Have I mentioned that we have too many branches?
Comment 9 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2009-08-24 21:45:53 PDT
You're already blocking1.9.2+ (assuming that means what it used to...)
Comment 10 User image Blake Kaplan (:mrbkap) 2009-08-24 22:36:34 PDT
That means I can't keep track of the rules!
Comment 11 User image Blake Kaplan (:mrbkap) 2009-08-25 15:15:33 PDT
Comment 12 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-09-04 03:52:24 PDT
Verified fixed based on check-ins on 1.9.2 and trunk.
Comment 13 User image Daniel Veditz [:dveditz] 2010-01-12 14:53:24 PST
Is attachment 393892 [details] [diff] [review] what you want to check in on the 1.9.1 branch?
Comment 14 User image Blake Kaplan (:mrbkap) 2010-01-13 11:49:02 PST
Comment on attachment 393892 [details] [diff] [review]
Proposed fix

Comment 15 User image Daniel Veditz [:dveditz] 2010-01-15 13:23:57 PST
Comment on attachment 393892 [details] [diff] [review]
Proposed fix

Approved for, a=dveditz for release-drivers
Comment 16 User image Blake Kaplan (:mrbkap) 2010-01-19 15:40:12 PST

Note You need to log in before you can comment on or make changes to this bug.