1001198 - Clean up XPConnect and SSM startup

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I'm having to do some startup munging to put me in a place where I can initialize a Sandbox at XPConnect startup (which I want for bug 997906). I'm splitting that work out into a separate bug.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Explicitly fire up the SSM from nsXPConnect. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop firing onNewGlobalObject for the SafeJSContext global. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove the call to InitClasses for the SafeJSContextGlobal. v1

In contrast to InitClassesWithNewWrappedGlobal, InitClasses doesn't do much
these days. It pretty much only exists to support JSD globals that are created
without XPConnect's knowledge, and then are suddenly handed to XPConnect.

It really only has two observable effects. The first is ensuring the existence
of an XPCWrappedNativeScope, which we know we have here, because we went through
xpc::CreateGlobalObject. The second is to set up the XPCNativeWrapper
constructor (eww), which definitely doesn't matter here, especially now that
we're asserting that we never run script in this global.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove useless (and footgun) XPCWrappedNativeScope::InitStatics. v1

All this does is null out some stuff that's already in the BSS. And it causes
leaks if we create a global before it's called, which the upcoming patch does.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Explicitly create the SafeJSContext from XPConnect. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 6 - Stop getting XPConnect as a service during Sandbox creation. v1

This causes problems when we try to create a Sandbox during XPConnect
initialization. Luckily, we don't need it at all.

Comment 7

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

Comment on attachment 8417830 [details] [diff] [review]
Part 1 - Explicitly fire up the SSM from nsXPConnect. v1

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

::: js/xpconnect/src/XPCJSContextStack.cpp
@@ +74,1 @@
>          if ((e.cx == cx) && ssm) {

Remove null-check?

::: js/xpconnect/src/xpcprivate.h
@@ +283,2 @@
>      {
> +        return gScriptSecurityManager;

Maybe assert mainthread?

Comment 8

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8417830 [details] [diff] [review]
Part 1 - Explicitly fire up the SSM from nsXPConnect. v1

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

::: dom/bindings/Exceptions.cpp
@@ +24,2 @@
>  bool
>  IsCallerChrome()

Is there a reason to keep this function?

::: js/xpconnect/src/xpcprivate.h
@@ +278,5 @@
>      XPCJSRuntime* GetRuntime() {return mRuntime;}
>  
>      static bool IsISupportsDescendant(nsIInterfaceInfo* info);
>  
> +    static nsIScriptSecurityManager* SSM()

This is not the most descriptive name to be honest... can we just call it SecurityManager() or ScriptSecurityManager()?

Comment 9

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8417835 [details] [diff] [review]
Part 6 - Stop getting XPConnect as a service during Sandbox creation. v1

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

\o/ This is really awesome, I always hated this line :)

Comment 10

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #7)
> Comment on attachment 8417830 [details] [diff] [review]
> ::: js/xpconnect/src/XPCJSContextStack.cpp
> @@ +74,1 @@
> >          if ((e.cx == cx) && ssm) {
> 
> Remove null-check?

Yeah, good call.


> ::: js/xpconnect/src/xpcprivate.h
> @@ +283,2 @@
> >      {
> > +        return gScriptSecurityManager;
> 
> Maybe assert mainthread?

Done.


(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> ::: dom/bindings/Exceptions.cpp
> @@ +24,2 @@
> >  bool
> >  IsCallerChrome()
> 
> Is there a reason to keep this function?

It actually just went away, in bug 997987.

> ::: js/xpconnect/src/xpcprivate.h
> > +    static nsIScriptSecurityManager* SSM()
> 
> This is not the most descriptive name to be honest... can we just call it
> SecurityManager() or ScriptSecurityManager()?

Yeah, I guess SecurityManager() is better.

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Bobby Holley (:bholley)]

Followup for compilation bustage on some platform: https://tbpl.mozilla.org/?tree=Try&rev=a3c23e9801e8

Comment 13

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d8790c6d6756
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73d49c3410cb

Comment 14

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #13)
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d8790c6d6756
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73d49c3410cb

Er, scratch that.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8d915f7fd1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/54b2ec1e2874
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2aad820d07ec
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb8eda80fe6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8eee89463dee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f8bcebe4a6a6

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d8d915f7fd1
https://hg.mozilla.org/mozilla-central/rev/54b2ec1e2874
https://hg.mozilla.org/mozilla-central/rev/2aad820d07ec
https://hg.mozilla.org/mozilla-central/rev/cbb8eda80fe6
https://hg.mozilla.org/mozilla-central/rev/8eee89463dee
https://hg.mozilla.org/mozilla-central/rev/f8bcebe4a6a6