Closed Bug 467900 Opened 14 years ago Closed 14 years ago

nsScriptSecurityManager not thread-safe called by IsCallerChrome


(Core :: XPConnect, defect, P2)

Windows XP





(Reporter: timeless, Assigned: timeless)



(Keywords: fixed1.9.1)


(1 file, 4 obsolete files)

Attached patch check thread first (obsolete) — Splinter Review
bug 119646 comment 29 notes about when SSM became thread unsafe.

NS_DebugBreak_P(unsigned int aSeverity = 1, char * aStr = 0x011c99a0 "nsScriptSecurityManager not thread-safe", char * aExpr = 0x011c996c "_mOwningThread.GetThread() == PR_GetCurrentThread()", char * aFile = 0x011c9920 "c:/home/", int aLine = 480)
03 caps!nsScriptSecurityManager::AddRef
04 xpcom_core!nsCOMPtr<nsISupports>::nsCOMPtr<nsISupports>(class nsCOMPtr<nsISupports> * aSmartPtr = 0x00d5167c)
05 xpcom_core!nsComponentManagerImpl::GetServiceByContractID
06 xpcom_core!CallGetService(char * aContractID = 0x0110ae88 ";1", struct nsID * aIID = 0x010ef3f8, void ** aResult = 0x01ede5f8)
07 xpcom_core!nsGetServiceByContractID::operator()
08 xpc3250!nsCOMPtr<nsIScriptSecurityManager>::assign_from_gs_contractid(class nsGetServiceByContractID gs = class nsGetServiceByContractID, struct nsID * aIID = 0x010ef3f8)
09 xpc3250!nsCOMPtr<nsIScriptSecurityManager>::nsCOMPtr<nsIScriptSecurityManager>(class nsGetServiceByContractID gs = class nsGetServiceByContractID)
0a xpc3250!IsCallerChrome(void)+0x29 [c:\home\\mozilla-central\js\src\xpconnect\src\xpcthrower.cpp @ 268]
0b xpc3250!XPCThrower::ThrowExceptionObject
0c xpc3250!XPCThrower::CheckForPendingException
0d xpc3250!XPCThrower::ThrowBadResult
0e xpc3250!ThrowBadResult
0f xpc3250!XPCWrappedNative::CallMethod
10 xpc3250!XPC_WN_CallMethod
11 js3250!js_Invoke
12 js3250!js_Interpret
13 js3250!js_Invoke
14 xpc3250!nsXPCWrappedJSClass::CallMethod
15 xpc3250!nsXPCWrappedJS::CallMethod
16 xpcom_core!PrepareAndDispatch
17 xpcom_core!SharedStub
18 xpcom_core!nsThread::ProcessNextEvent
19 xpcom_core!NS_ProcessNextEvent_P
1a xpcom_core!nsThread::ThreadFunc
1b nspr4!_PR_NativeRunThread
1c nspr4!pr_root
1d MSVCR80D!_beginthreadex
1e MSVCR80D!_beginthreadex
1f kernel32!BaseThreadStart
Attachment #351356 - Flags: review?(bent.mozilla)
Attachment #351356 - Flags: superreview-
Attachment #351356 - Flags: review?(bent.mozilla)
Attachment #351356 - Flags: review-
Comment on attachment 351356 [details] [diff] [review]
check thread first

 static PRBool
+IsCallerChrome(JSContext* cx)
+    if (!XPCPerThreadData::IsMainThread(cx))
+        return PR_TRUE;

Just because we're off the main thread doesn't mean we're chrome. We need to do this check elsewhere and not call IsCallerChrome off the main thread, and probably assert here...
essentially the same functionality
Attachment #351356 - Attachment is obsolete: true
Attachment #351819 - Flags: review?(jst)
Comment on attachment 351819 [details] [diff] [review]
try to actually grab the right security manager

You're not actually ever calling SubjectPrincipalIsSystem in the non-main case. Also GetSecurityManagerForJSContext returns an nsIXPCSecurityManager, not nsIScriptSecurityManager, so we need a QI or it doesn't compile.
Attachment #351819 - Flags: review?(jst) → review-
Attached patch better? (obsolete) — Splinter Review
timeless, what do you think of this?
Attachment #355504 - Flags: superreview?(jst)
Attachment #355504 - Flags: review?(timeless)
Comment on attachment 355504 [details] [diff] [review]

maybe.... some stupid questions...
* if we always took the else path, even on MainThread, would anything bad happen?
* would it actually be slower than using getService?
Should this block?
Flags: blocking1.9.1?
Attachment #355504 - Flags: superreview?(jst)
Attachment #355504 - Flags: superreview+
Attachment #355504 - Flags: review?(timeless)
Attachment #355504 - Flags: review+
Ben, feel free to commit this unless timeless beats you to it.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
Comment on attachment 355504 [details] [diff] [review]

Sorry, jumped the gun there w/o really looking at the bug. Timeless brings up good points in his above comment.
Attachment #355504 - Flags: superreview+
Attachment #355504 - Flags: review+
i think my questions should more be of the form "please try it as i asked and let us know if the perf is horribly bad or better".

if the else only form works, let's go w/ that (i think that'll probably get free r=me, sr=jst).
I submitted both patches to the try server, maybe it helps (had to submit twice because all talos boxen except Mac were orange the first time, so I submitted again a few hours later):
twinopen: 508.95/497.00
ts: 1182.26/1182.32
tp: 298.40/295.45
tp_RSS: 299.0MB/-
twinopen: 190.63/191.11
ts: 1151.26/1148.37
tp: 420.03/419.17
tp_RSS: 79.8MB/79.1MB
twinopen: 498.11/495.89
ts: 1176.63/1170.32
tp: 298.02/295.36
tp_RSS: 305.7MB/280.9MB
twinopen: 256.42/256.16
ts: 1143.11/1151.00
tp: 430.70/433.09
tp_memset: 62.3MB/63.0MB
Attached patch Final patchSplinter Review
Ok, here's the final version of the patch I'd like to check in. This uses a fast getter on the main thread and then goes the slow route for any other thread.

Dromaeo results:,58700

Looks like some things are slightly faster and some slightly slower, though I've done a few different runs with other patches and I don't have a good feel for how noisy these results tend to be.
Attachment #351649 - Attachment is obsolete: true
Attachment #351819 - Attachment is obsolete: true
Attachment #355504 - Attachment is obsolete: true
Attachment #358047 - Flags: superreview?(jst)
Attachment #358047 - Flags: review?(jst)
Comment on attachment 358047 [details] [diff] [review]
Final patch

Looks good.
Attachment #358047 - Flags: superreview?(jst)
Attachment #358047 - Flags: superreview+
Attachment #358047 - Flags: review?(jst)
Attachment #358047 - Flags: review+
Pushed changeset ebec887ae5c6 to mozilla-central.
Closed: 14 years ago
Resolution: --- → FIXED
Pushed changeset 68ae77410686 to mozilla-1.9.1.
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.