Last Comment Bug 571289 - XPConnect uses nsScriptSecurityManager off the main thread [@ nsScriptSecurityManager::CheckPropertyAccessImpl(unsigned int, nsAXPCNativeCallContext*, JSContext*, JSObject*, nsISupports*, nsIURI*, nsIClassInfo*, char const*, int, void**) ]
: XPConnect uses nsScriptSecurityManager off the main thread [@ nsScriptSecurit...
: verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86 Windows 7
-- critical with 2 votes (vote)
: ---
Assigned To: Justin Lebar (not reading bugmail)
: Andrew Overholt [:overholt]
: 597287 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2010-06-10 11:21 PDT by Blake Kaplan (:mrbkap)
Modified: 2010-09-23 16:14 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Testcase (4.47 KB, patch)
2010-09-13 16:45 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v1 (4.47 KB, patch)
2010-09-13 17:44 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v1.1 (4.72 KB, patch)
2010-09-13 18:43 PDT, Justin Lebar (not reading bugmail)
mrbkap: review+
christian: approval1.9.2.11+
Details | Diff | Splinter Review

Description User image Blake Kaplan (:mrbkap) 2010-06-10 11:21:43 PDT
We've seen a couple reports where extensions that run JS code that deals with XPConnect off the main thread end up calling into nsScriptSecurityManager and then crashing ( is one example).

We need to figure out why XPConnect is getting a hold of the main thread's security manager on these other threads. Without this, any extension that runs JS off the main thread (even if it's careful about what objects it uses off the main thread) risks crashing.
Comment 1 User image timeless 2010-06-10 13:22:22 PDT

All of the magic wrapper code uses this which accesses a global

1048 XPCConvert::NativeInterface2JSObject(XPCLazyCallContext& lccx,
1244         if(NS_SUCCEEDED(rv) && wrapper)
1255             if (allowNativeWrapper &&
1256                 !xpc_SameScope(wrapper->GetScope(), xpcscope, &sameOrigin))
1309                 if(!JS_IsSystemObject(ccx, flat))
1329                         nsIScriptSecurityManager *ssm =
1330                             XPCWrapper::GetSecurityManager();
Comment 2 User image Blake Kaplan (:mrbkap) 2010-06-13 10:06:30 PDT
Surprisingly, that's not actually the problem (I have no clue why, I suspect it might be because it's impossible for !xpc_SameScope to be true off the main thread). The problem shown in the crash report in comment 0 is that the "default" security manager for a not-main thread is not null, which is definitely a bug.
Comment 3 User image Justin Lebar (not reading bugmail) 2010-09-13 13:06:16 PDT
For my reference, these are my notes from talking with mrbkap about how to reproduce this issue:

From chrome, using nsIThreadManager, dispatch a runnable to some other code I own.

Touch xpconnect. Create an object using Components.classes.(some threadsafe interface), then touch it in a way that isn't quickstubbed.

Maybe crashes in nsScriptSecurityManager (somewhere), due to bug in GetDefaultSecurityManager.
 * Could put NS_ABORT_IF_FALSE(IsMainThread) in nsScriptSecurityManager::CanAccess
Comment 4 User image Justin Lebar (not reading bugmail) 2010-09-13 16:45:00 PDT
Created attachment 474904 [details] [diff] [review]

Testcase.  The new browser test triggers an assertion I added to GetDefaultSecurityManager and kills the browser.
Comment 5 User image Justin Lebar (not reading bugmail) 2010-09-13 17:44:50 PDT
Created attachment 474934 [details] [diff] [review]
Patch v1

Returns null when we're not the main thread.
Comment 6 User image Josh Matthews [:jdm] 2010-09-13 18:42:03 PDT
qref, perhaps?
Comment 7 User image Justin Lebar (not reading bugmail) 2010-09-13 18:43:28 PDT
Created attachment 474954 [details] [diff] [review]
Patch v1.1

Ack, sorry.  qref'ed.
Comment 8 User image Justin Lebar (not reading bugmail) 2010-09-14 09:36:38 PDT
Blake, since this is crashing plugins, should this block?
Comment 9 User image Janek B. 2010-09-15 07:29:52 PDT
Yes please. Firefox crashes every few minutes and that really gets my goat.
Comment 10 User image Blake Kaplan (:mrbkap) 2010-09-15 10:04:27 PDT
Yeah, I think so.
Comment 11 User image Janek B. 2010-09-17 11:01:00 PDT
Could you please manage fixing this already in the next release? Current Firefox is nearly unusable because of so many crashes (mainly invoked by Yoono which I don't want to miss) and I don't want to switch back to 3.6.4 (should be Xulrunner, right?) where the problem didn't exist.
Comment 12 User image Daniel Veditz [:dveditz] 2010-09-17 11:11:10 PDT
*** Bug 597287 has been marked as a duplicate of this bug. ***
Comment 14 User image christian 2010-09-17 11:14:50 PDT
We want this on the branch but wouldn't block a release on it, as it looks like there are only ~1000 crashes total per release. Please ask for approval on the patch when ready and we'll get it in the current in-flight update.
Comment 15 User image christian 2010-09-17 11:30:07 PDT
Is there a possibility of our own code calling nsScriptSecurityManager off the main thread and thus this fix would cause issues?
Comment 17 User image Justin Lebar (not reading bugmail) 2010-09-17 14:51:47 PDT
Comment 18 User image Justin Lebar (not reading bugmail) 2010-09-17 14:52:42 PDT
Whoops; didn't mean to resolve this, since we still need to land on branches.
Comment 19 User image christian 2010-09-22 10:51:45 PDT
Does this same fix apply to 1.9.1? We have it marked needed there as well. Approving for Please ask for approval for if it is needed there as well.
Comment 20 User image Daniel Veditz [:dveditz] 2010-09-22 10:52:05 PDT
We use the status1.9.x fields to track branch status
Comment 21 User image Daniel Veditz [:dveditz] 2010-09-22 10:58:33 PDT
IOW change status1.9.2 from "wanted" to ".11-fixed" when you land, and similarly for the 1.9.1 branch
Comment 23 User image Al Billings [:abillings] 2010-09-23 16:14:32 PDT
Verified for 1.9.1 and 1.9.2 based on passing unit test.

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