Closed Bug 873622 Opened 6 years ago Closed 6 years ago

make nsXPConnect::GetXpconnect() infallible and faster

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(6 files)

No description provided.
Attachment #751182 - Flags: review?(bobbyholley+bmo)
Attachment #751202 - Flags: review?(bobbyholley+bmo)
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.
Attachment #751180 - Flags: review?(bobbyholley+bmo) → review-
Flags: needinfo?(benjamin)
(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 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.
Attachment #751202 - Flags: review?(bobbyholley+bmo)
Attachment #751201 - Flags: review?(bobbyholley+bmo)
Attachment #751182 - Flags: review?(bobbyholley+bmo)
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.
Flags: needinfo?(benjamin)
(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?
(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.
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.
create teh singleton in nsXPConnect::InitStatics()
Attachment #755607 - Flags: review?(bobbyholley+bmo)
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.
Attachment #755607 - Flags: review?(bobbyholley+bmo) → review+
(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.
Whiteboard: [leave open]
Attachment #751202 - Flags: review?(bobbyholley+bmo)
Attachment #751182 - Flags: review?(bobbyholley+bmo)
Attachment #751201 - Flags: review?(bobbyholley+bmo)
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);
Attachment #751182 - Flags: review?(bobbyholley+bmo) → review+
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.
Attachment #751201 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 751202 [details] [diff] [review]
bug 873622 - remove XPCCallContext::GetXPConnect()

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

r=bholley
Attachment #751202 - Flags: review?(bobbyholley+bmo) → review+
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
Depends on: 879029
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.)
Flags: needinfo?(trev.saunders)
(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.
Flags: needinfo?(trev.saunders)
Comment on attachment 758211 [details] [diff] [review]
bug 873622 - remove nsScriptSecurityManager::sXPConnect

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

r=bholley
Attachment #758211 - Flags: review?(bobbyholley+bmo) → review+
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/c82c44986cc7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.