Closed Bug 571289 Opened 9 years ago Closed 9 years ago
XPConnect uses ns
Script Security Manager off the main thread [@ ns Script Security Manager::Check Property Access Impl(unsigned int, ns AXPCNative Call Context*, JSContext*, JSObject*, ns ISupports*, ns IURI*, ns IClass Info*, char const*, int, void**) ]
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 (http://crash-stats.mozilla.com/report/index/2dcc69c6-ff24-4d30-9594-ba34f2100517 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.
http://mxr.mozilla.org/mozilla-central/ident?i=GetSecurityManager 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();
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.
Assignee: nobody → justin.lebar+bug
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
Testcase. The new browser test triggers an assertion I added to GetDefaultSecurityManager and kills the browser.
Returns null when we're not the main thread.
OS: Mac OS X → Windows 7
Ack, sorry. qref'ed.
Attachment #474954 - Flags: review?(mrbkap) → review+
Blake, since this is crashing plugins, should this block?
Yes please. Firefox crashes every few minutes and that really gets my goat.
Yeah, I think so.
blocking1.9.2: --- → ?
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 126.96.36.199, right?) where the problem didn't exist.
Summary: XPConnect uses nsScriptSecurityManager off the main thread → XPConnect uses nsScriptSecurityManager off the main thread [@ nsScriptSecurityManager::CheckPropertyAccessImpl(unsigned int, nsAXPCNativeCallContext*, JSContext*, JSObject*, nsISupports*, nsIURI*, nsIClassInfo*, char const*, int, void**) ]
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.
Attachment #474954 - Flags: approval188.8.131.52?
Is there a possibility of our own code calling nsScriptSecurityManager off the main thread and thus this fix would cause issues?
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whoops; didn't mean to resolve this, since we still need to land on branches.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Does this same fix apply to 1.9.1? We have it marked needed there as well. Approving for 184.108.40.206. Please ask for approval for 220.127.116.11 if it is needed there as well.
We use the status1.9.x fields to track branch status
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
IOW change status1.9.2 from "wanted" to ".11-fixed" when you land, and similarly for the 1.9.1 branch
You need to log in before you can comment on or make changes to this bug.