make nsXPConnect::GetXpconnect() infallible and faster

RESOLVED FIXED in mozilla24

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 751180 [details] [diff] [review]
speed up nsXPConnect::GetXPConnect() and rename it since it never returns null
Attachment #751180 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 2

5 years ago
Created attachment 751182 [details] [diff] [review]
remove some useless xpconnect getters
Attachment #751182 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 3

5 years ago
Created attachment 751201 [details] [diff] [review]
bug 873622 - remove nsIXPConnectWrappedNative::GetXPConnect()
Attachment #751201 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 4

5 years ago
Created attachment 751202 [details] [diff] [review]
bug 873622 - remove XPCCallContext::GetXPConnect()
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)
(Assignee)

Comment 6

5 years ago
(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)

Comment 8

5 years ago
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?
(Assignee)

Comment 10

5 years ago
(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

5 years ago
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.
(Assignee)

Comment 12

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

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+
(Assignee)

Comment 14

5 years ago
(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.
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Updated

5 years ago
Attachment #751202 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
Attachment #751182 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 18

5 years ago
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)
(Assignee)

Comment 23

5 years ago
(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)
(Assignee)

Comment 24

5 years ago
Created attachment 758211 [details] [diff] [review]
bug 873622 - remove nsScriptSecurityManager::sXPConnect
Attachment #758211 - Flags: review?(bobbyholley+bmo)
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+
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/c82c44986cc7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.