467900 - nsScriptSecurityManager not thread-safe called by IsCallerChrome

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[timeless]

Attachment: check thread first

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/mozilla.org/mozilla-central/caps/src/nsScriptSecurityManager.cpp", 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 "@mozilla.org/scriptsecuritymanager;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.org\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

Comment 1

[Johnny Stenback (:jst)]

Comment on attachment 351356 [details] [diff] [review]
check thread first

 static PRBool
-IsCallerChrome()
+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...

Comment 2

[Ben Newman (:bnewman) (:benjamn)]

Attachment: slight variation based on jst's remarks

essentially the same functionality

Comment 3

[timeless]

Attachment: try to actually grab the right security manager

Comment 4

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

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.

Comment 5

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

Attachment: better?

timeless, what do you think of this?

Comment 6

[timeless]

Comment on attachment 355504 [details] [diff] [review]
better?

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?

Comment 7

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

Should this block?

Comment 8

[Johnny Stenback (:jst)]

Ben, feel free to commit this unless timeless beats you to it.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 355504 [details] [diff] [review]
better?

Sorry, jumped the gun there w/o really looking at the bug. Timeless brings up good points in his above comment.

Comment 10

[timeless]

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).

Comment 11

[Frank Wein [:mcsmurf]]

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):
timeless/bent:
Mac:
twinopen: 508.95/497.00
ts: 1182.26/1182.32
tp: 298.40/295.45
tp_RSS: 299.0MB/-
-------
Linux:
twinopen: 190.63/191.11
ts: 1151.26/1148.37
tp: 420.03/419.17
tp_RSS: 79.8MB/79.1MB
Mac:
twinopen: 498.11/495.89
ts: 1176.63/1170.32
tp: 298.02/295.36
tp_RSS: 305.7MB/280.9MB
Windows:
twinopen: 256.42/256.16
ts: 1143.11/1151.00
tp: 430.70/433.09
tp_memset: 62.3MB/63.0MB

Comment 12

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

Attachment: Final patch

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:

http://dromaeo.com/?id=58697,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.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 358047 [details] [diff] [review]
Final patch

Looks good.

Comment 14

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

Pushed changeset ebec887ae5c6 to mozilla-central.

Comment 15

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

Pushed changeset 68ae77410686 to mozilla-1.9.1.