905364 - Startup crash when operation callback fires under nsScriptSecurityManager::Init

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

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

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.

Comment 1

[Bobby Holley (:bholley)]

As noted on IRC, we should avoid the AutoSafeJSContext in nsScriptSecurityManager::Init, and intern the string lazily.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stop using the SafeJSContext in nsScriptSecurityManager::Init. v1

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.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 790434 [details] [diff] [review]
Part 3 - Stop using the SafeJSContext in nsScriptSecurityManager::Init. v1

This patch is amazing.

Comment 4

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=2330b28a9e22

Comment 5

[Scoobidiver (away)]

Reports at: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsContentUtils%3A%3AGetSubjectPrincipal%28%29

Comment 6

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=9f119cf83935

Comment 7

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=859b471cadd8

Comment 8

[Bobby Holley (:bholley)]

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

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Phil Ringnalda (:philor)]

Still had two bits of failure, xpcshell like https://tbpl.mozilla.org/php/getParsedLog.php?id=26673704&tree=Mozilla-Inbound and b2g desktop make check like https://tbpl.mozilla.org/php/getParsedLog.php?id=26673058&tree=Mozilla-Inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6b1496e690f9

Comment 11

[Robert Kaiser]

This is still the #4 crash for trunk builds from the last 3 days. Bobby, where are we on this?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Robert Kaiser (:[e-mail]) 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.

Comment 13

[Bobby Holley (:bholley)]

Let's try the workaround again without the dependencies:

https://tbpl.mozilla.org/?tree=Try&rev=b5e23595c655

Comment 14

[Bobby Holley (:bholley)]

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.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't call into AllowXULXBLForPrincipal during SafeJSContext initialization. v1

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.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 2 - Force the SafeJSContext to fire up in xpcshell. v1

Comment 17

[Bobby Holley (:bholley)]

Full try push: https://tbpl.mozilla.org/?tree=Try&rev=df316444aaf9

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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;

Comment 19

[Bobby Holley (:bholley)]

This is green. Let's get this pushed to fix the crashes.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 800221 [details] [diff] [review]
Part 2 - Force the SafeJSContext to fire up in xpcshell. v1

r=me

Comment 22

[Bobby Holley (:bholley)]

Landing with the workaround to avoid the need to land the dependent bug. Full green try push in comment 17.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1ca7188e84
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/56d1c3a7fd4e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b943e64032

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b1ca7188e84
https://hg.mozilla.org/mozilla-central/rev/56d1c3a7fd4e
https://hg.mozilla.org/mozilla-central/rev/e1b943e64032