873622 - make nsXPConnect::GetXpconnect() infallible and faster

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: speed up nsXPConnect::GetXPConnect() and rename it since it never returns null

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: remove some useless xpconnect getters

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: bug 873622 - remove nsIXPConnectWrappedNative::GetXPConnect()

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: bug 873622 - remove XPCCallContext::GetXPConnect()

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 751180 [details] [diff] [review]
speed up nsXPConnect::GetXPConnect() and rename it since it never returns null

I'm not really wild about having nsContentUtils own XPConnect lifetime. If we're to do something like this, I think it should happen in nsLayoutStatics directly, and probably be the first thing it does.

In the old world, this definitely would have crashed in places where we used XPConnect without having spun up layout. We might be ok post bug 864363, but I want bsmedberg to sign off on this plan as well.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Bobby Holley (:bholley) from comment #5)
> Comment on attachment 751180 [details] [diff] [review]
> speed up nsXPConnect::GetXPConnect() and rename it since it never returns
> null
> 
> I'm not really wild about having nsContentUtils own XPConnect lifetime. If
> we're to do something like this, I think it should happen in nsLayoutStatics
> directly, and probably be the first thing it does.

I picked that spot because it is where we create xpconnect now, but I'm not going to argue its particularly sane, and starting it sooner seems reasonable.

> In the old world, this definitely would have crashed in places where we used
> XPConnect without having spun up layout. We might be ok post bug 864363, but
> I want bsmedberg to sign off on this plan as well.

makes sense, that's  a crazy bug :/

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 751202 [details] [diff] [review]
bug 873622 - remove XPCCallContext::GetXPConnect()

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

Canceling these reviews until the other stuff is addressed.

Comment 8

[Benjamin Smedberg]

I'm not that worried about where you *start* XPConnect: doing it from nsLayoutStatics sounds fine. I'm primarily worried about where you shut it down. It has to be really late in shutdown because of JS-implemented services.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> I'm not that worried about where you *start* XPConnect: doing it from
> nsLayoutStatics sounds fine. I'm primarily worried about where you shut it
> down. It has to be really late in shutdown because of JS-implemented
> services.

Ok. Then how about we force the initialization in nsLayoutStatics, but let ReleaseXPConnectSingleton be called on module shutdown, as it's done now?

Though I wonder - is there any reason we can't just do XPConnect initialization in nsXPConnect::InitStatics? Stuff might not be far enough along, but if it is, it would solve our problems here. Trevor, can you give that a try?

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to Bobby Holley (:bholley) from comment #9)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> > I'm not that worried about where you *start* XPConnect: doing it from
> > nsLayoutStatics sounds fine. I'm primarily worried about where you shut it
> > down. It has to be really late in shutdown because of JS-implemented
> > services.
> 
> Ok. Then how about we force the initialization in nsLayoutStatics, but let
> ReleaseXPConnectSingleton be called on module shutdown, as it's done now?

so an interesting side question is if we can end up in a world where we don't actually refcount nsXPConnect at all, given that I suspect we can, though I'm not sure its worth the effort to go farther than what this bug gives us.

> Though I wonder - is there any reason we can't just do XPConnect
> initialization in nsXPConnect::InitStatics? Stuff might not be far enough
> along, but if it is, it would solve our problems here. Trevor, can you give
> that a try?

sure, I've been meaning to get back to this anyway.

Comment 11

[Benjamin Smedberg]

Given the lifetime we're talking about, I don't think we need to refcount xpconnect. And if we do that, then we can make a bunch of nsCOMPtr and already_AddRefed into raw pointers and avoid a bunch of (threadsafe!) refcounting, which could show up on benchmark perf metrics.

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: bug 873622 - speed up nsXPConnect::GetXPConnect() and rename it since it never returns null

create teh singleton in nsXPConnect::InitStatics()

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 755607 [details] [diff] [review]
bug 873622 - speed up nsXPConnect::GetXPConnect() and rename it since it never returns null

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

FWIW, it would have been helpful to do the rename in a separate patch.

r=bholley with comments addressed

::: caps/src/nsScriptSecurityManager.cpp
@@ +2421,5 @@
>  }
>  
>  nsresult nsScriptSecurityManager::Init()
>  {
> +    NS_ADDREF(sXPConnect = nsXPConnect::XPConnect());

The refcount for XPConnect is no longer necessary, right? Let's fix that in a followup patch.

::: js/xpconnect/src/XPCComponents.cpp
@@ +4420,2 @@
>                                                        NS_GET_IID(nsIRunnable),
>                                                        getter_AddRefs(run));

indentation

::: js/xpconnect/src/nsXPConnect.cpp
@@ +153,5 @@
>  
> +    // Set XPConnect as the main thread observer.
> +    if (NS_FAILED(nsThread::SetMainThreadObserver(gSelf))) {
> +        NS_RELEASE(gSelf);
> +        gSelf = nullptr;

Let's just MOZ_CRASH here.

::: js/xpconnect/src/xpcprivate.h
@@ +486,5 @@
> +
> +        return gSelf;
> +    }
> +
> +    static nsXPConnect*  FastGetXPConnect() { return gSelf ? gSelf : XPConnect(); }

This function can go away now.

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Bobby Holley (:bholley) from comment #13)
> Comment on attachment 755607 [details] [diff] [review]
> bug 873622 - speed up nsXPConnect::GetXPConnect() and rename it since it
> never returns null
> 
> Review of attachment 755607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> FWIW, it would have been helpful to do the rename in a separate patch.
> 
> r=bholley with comments addressed
> 
> ::: caps/src/nsScriptSecurityManager.cpp
> @@ +2421,5 @@
> >  }
> >  
> >  nsresult nsScriptSecurityManager::Init()
> >  {
> > +    NS_ADDREF(sXPConnect = nsXPConnect::XPConnect());
> 
> The refcount for XPConnect is no longer necessary, right? Let's fix that in
> a followup patch.

yeah, there's all sorts of stuff that is  even more useless now and that's one of the better cases.

> ::: js/xpconnect/src/nsXPConnect.cpp
> @@ +153,5 @@
> >  
> > +    // Set XPConnect as the main thread observer.
> > +    if (NS_FAILED(nsThread::SetMainThreadObserver(gSelf))) {
> > +        NS_RELEASE(gSelf);
> > +        gSelf = nullptr;
> 
> Let's just MOZ_CRASH here.

sgtm

> ::: js/xpconnect/src/xpcprivate.h
> @@ +486,5 @@
> > +
> > +        return gSelf;
> > +    }
> > +
> > +    static nsXPConnect*  FastGetXPConnect() { return gSelf ? gSelf : XPConnect(); }
> 
> This function can go away now.

yeah, its in one of the later patches in this bug I do believe.

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 751182 [details] [diff] [review]
remove some useless xpconnect getters

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

r=bholley

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +2997,5 @@
>  /* readonly attribute nsIXPConnect XPConnect; */
>  NS_IMETHODIMP XPCWrappedNative::GetXPConnect(nsIXPConnect * *aXPConnect)
>  {
>      if (IsValid()) {
> +        NS_IF_ADDREF(*aXPConnect = nsXPConnect::XPConnect());

Please do:

nsCOMPtr<nsIXPConnect> ref = nsXPConnect::XPConnect();
ref.forget(aXPConnect);

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 751201 [details] [diff] [review]
bug 873622 - remove nsIXPConnectWrappedNative::GetXPConnect()

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

r=bholley assuming nothing changed in those big blocks other than the GetXPConnect call and indentation.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 751202 [details] [diff] [review]
bug 873622 - remove XPCCallContext::GetXPConnect()

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

r=bholley

Comment 18

[Trevor Saunders (:tbsaunde)]

landed part 1 lets wait a day or so to make sure the world doesn't end before landing the rest
https://hg.mozilla.org/projects/cypressrev/eb7402ddbe54

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/projects/cypress/rev/a6ce38ac4a15

https://tbpl.mozilla.org/php/getParsedLog.php?id=23608590&tree=Cypress

Comment 20

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eea9248f39a9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/68b49e541e3e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f91f8a164a11
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/94ecfdd0231b

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eea9248f39a9
https://hg.mozilla.org/mozilla-central/rev/68b49e541e3e
https://hg.mozilla.org/mozilla-central/rev/f91f8a164a11
https://hg.mozilla.org/mozilla-central/rev/94ecfdd0231b

Comment 22

[Daniel Holbert [:dholbert]]

Trevor: Did you want this to still be [leave open], or can this be closed now?

(Looks like you added [leave open] back before most of the patches here were reviewed, whereas now it looks like things have mostly (all?) landed.)

Comment 23

[Trevor Saunders (:tbsaunde)]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> Trevor: Did you want this to still be [leave open], or can this be closed
> now?
> 
> (Looks like you added [leave open] back before most of the patches here were
> reviewed, whereas now it looks like things have mostly (all?) landed.)

nah, I have one patch left to post and get reviewed.

Comment 24

[Trevor Saunders (:tbsaunde)]

Attachment: bug 873622 - remove nsScriptSecurityManager::sXPConnect

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 758211 [details] [diff] [review]
bug 873622 - remove nsScriptSecurityManager::sXPConnect

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

r=bholley

Comment 26

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c82c44986cc7

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c82c44986cc7