Closed Bug 905364 Opened 6 years ago Closed 6 years ago

Startup crash when operation callback fires under nsScriptSecurityManager::Init

Categories

(Core :: XPConnect, defect, critical)

26 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox25 --- unaffected
firefox26 + fixed

People

(Reporter: bent.mozilla, Assigned: bholley)

References

Details

(Keywords: crash, regression, topcrash, Whiteboard: [startupcrash])

Crash Data

Attachments

(3 files)

xul.dll!GetSubjectPrincipal - nscontentutils.cpp:2342
xul.dll!OperationCallback - xpcjsruntime.cpp:1249
mozjs.dll!js_InvokeOperationCallback - jscntxt.cpp:1032
mozjs.dll!js_HandleExecutionInterrupt - jscntxt.cpp:1039
mozjs.dll!Interpret - interpreter.cpp:1672
mozjs.dll!RunScript - interpreter.cpp:446
mozjs.dll!Invoke - interpreter.cpp:508
mozjs.dll!Interpret - interpreter.cpp:2484
mozjs.dll!RunScript - interpreter.cpp:446
mozjs.dll!ExecuteKernel - interpreter.cpp:630
mozjs.dll!Execute - interpreter.cpp:667
mozjs.dll!Evaluate - jsapi.cpp:5156
mozjs.dll!Evaluate - jsapi.cpp:5186
mozjs.dll!initSelfHosting - selfhosting.cpp:762
mozjs.dll!NewContext - jscntxt.cpp:201
xul.dll!GetSafeJSContext - xpcjscontextstack.cpp:150
xul.dll!Init - nsscriptsecuritymanager.cpp:2332
xul.dll!GetScriptSecurityManager - nsscriptsecuritymanager.cpp:2413
xul.dll!Init - nscontentutils.cpp:372
xul.dll!Initialize - nslayoutstatics.cpp:165
xul.dll!Initialize - nslayoutmodule.cpp:405
xul.dll!Load - nscomponentmanager.cpp:770
xul.dll!GetFactory - nscomponentmanager.cpp:1778
xul.dll!CreateInstanceByContractID - nscomponentmanager.cpp:1093
xul.dll!GetServiceByContractID - nscomponentmanager.cpp:1453
xul.dll!CallGetService - nscomponentmanagerutils.cpp:62
xul.dll!operator() - nscomponentmanagerutils.cpp:246
xul.dll!assign_from_gs_contractid - nscomptr.cpp:92
xul.dll!NS_InitXPCOM2 - nsxpcominit.cpp:559
xul.dll!Initialize - nsapprunner.cpp:1191
xul.dll!XRE_main - nsapprunner.cpp:3919

We die in GetSubjectPrincipal() because the script security manager has not yet been initialized.
As noted on IRC, we should avoid the AutoSafeJSContext in nsScriptSecurityManager::Init, and intern the string lazily.
Assignee: nobody → bobbyholley+bmo
Summary: Startup crash on slow machines → Startup crash when operation callback fires under nsScriptSecurityManager::Init
With this patch, I've confirmed that we instantiate the SafeJSContext much later
in startup, during nsAppStartupNotifier::Observe (which ends up invoking an
XPCWrappedJS). As such, this should solve a number of our startup ordering woes.
Attachment #790434 - Flags: review?(bent.mozilla)
Attachment #790434 - Flags: review?(bent.mozilla) → review?(mrbkap)
Comment on attachment 790434 [details] [diff] [review]
Part 3 - Stop using the SafeJSContext in nsScriptSecurityManager::Init. v1

This patch is amazing.
Attachment #790434 - Flags: review?(mrbkap) → review+
Reports at: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsContentUtils%3A%3AGetSubjectPrincipal%28%29
Severity: normal → critical
Crash Signature: [@ nsContentUtils::GetSubjectPrincipal() ]
Keywords: crash, regression
Whiteboard: [startupcrash]
Version: unspecified → 26 Branch
Keywords: topcrash
Depends on: 905926
Scoobidiver, how bad are the crashes? This patch is ready, but needs the dependent bug as well, which I would estimate will land on tuesday or wednesday. If it's important, I have a workaround that would allow this patch to land without the dependent bug (just access the SafeJSContext from xpcshell).
Flags: needinfo?(scoobidiver)
I decided to just land with the workaround. The workaround locally fixes the try issues from comment 4 (which was the only push without the patches from the dependent bug), so hopefully it should do the trick, and fix the pain here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c39d60483813
Flags: needinfo?(scoobidiver)
This is still the #4 crash for trunk builds from the last 3 days. Bobby, where are we on this?
Flags: needinfo?(bobbyholley+bmo)
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #11)
> This is still the #4 crash for trunk builds from the last 3 days. Bobby,
> where are we on this?

Yes. I'm really sorry it's taking so long. The dependent bug is turning out to be a lot trickier than we'd thought. This is now my top priority.
Flags: needinfo?(bobbyholley+bmo)
Let's try the workaround again without the dependencies:

https://tbpl.mozilla.org/?tree=Try&rev=b5e23595c655
Alright, that looks green. I think we should land this with the workaround to fix the crashes, and then sort out bug 905926 on its own.

Looks like mrbkap is on PTO, so I'll flag boris for these, who's already in the loop on the AllowXULXBLForPrincipal stuff.
In the old world, we'd be saved by initializing the SafeJSContext early enough
that we'd short-circuit in the nsContentUtils::IsInitialized() check. That's not
the case anymore, so let's hande this explicitly.
Attachment #800220 - Flags: review?(bzbarsky)
Attachment #790434 - Attachment description: Stop using the SafeJSContext in nsScriptSecurityManager::Init. v1 → Part 3 - Stop using the SafeJSContext in nsScriptSecurityManager::Init. v1
Comment on attachment 790434 [details] [diff] [review]
Part 3 - Stop using the SafeJSContext in nsScriptSecurityManager::Init. v1

Review of attachment 790434 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/src/nsScriptSecurityManager.cpp
@@ +76,5 @@
>  JSRuntime       *nsScriptSecurityManager::sRuntime   = 0;
>  bool nsScriptSecurityManager::sStrictFileOriginPolicy = true;
>  
> +// Lazily initialized. Use the getter below.
> +static jsid sEnabledID = JSID_VOID;

Should probably be JS::UndefinedValue() for static constructors

@@ +84,5 @@
> +    if (sEnabledID != JSID_VOID)
> +        return sEnabledID;
> +    AutoSafeJSContext cx;
> +    sEnabledID = INTERNED_STRING_TO_JSID(cx, JS_InternString(cx, "enabled"));
> +    return sEnabledID;

I'd write it as

if (sEnabledID.isUndefined()) {
    AutoSafeJSContext cx;
    sEnabledID = INTERNED_STRING_TO_JSID(cx, JS_InternString(cx, "enabled"));
}
return sEnabledID;
This is green. Let's get this pushed to fix the crashes.
Comment on attachment 800220 [details] [diff] [review]
Part 1 - Don't call into AllowXULXBLForPrincipal during SafeJSContext initialization. v1

Name the class SafeJSContextGlobalClass, and r=me
Attachment #800220 - Flags: review?(bzbarsky) → review+
Comment on attachment 800221 [details] [diff] [review]
Part 2 - Force the SafeJSContext to fire up in xpcshell. v1

r=me
Attachment #800221 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.