1382920 - Consider making XPCOM service getter methods return raw pointers

Status: RESOLVED WONTFIX

(Core :: XPCOM, enhancement)

Description

[(no longer active)]

I have seen the virtual AddRef/Release calls caused by these methods returning an already_AddRefed show up in profiles, we should consider making them return a T* since they return pointers to objects that are services and long living.  Consumers can choose to keep them alive extensively if desired.

Comment 1

[(no longer active)]

Attachment: Make XPCOM service getters return a raw pointer

Comment 2

[(no longer active)]

Attachment: Make XPCOM service getters return a raw pointer

Comment 3

[Eric Rahm [:erahm]]

Comment on attachment 8888620 [details] [diff] [review]
Make XPCOM service getters return a raw pointer

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

This gets pretty sketchy around shutdown. We can get into situations where something grabs a pointer to a service that gets nuked out from under it. There's a *lot* that happens after we call |mozilla::services::Shutdown| [1] and it's not clear to me that nothing has pointer to a service at that point. I'm not saying we shouldn't do this, just we'd need to audit a lot of code.

I think Nathan might have a better grasp of what should be left running at that point, I'd at least like to get his opinion on whether the risk is worth it.

[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/xpcom/build/XPCOMInit.cpp#941

Comment 4

[Eric Rahm [:erahm]]

Nathan can you take a look at this proposal when you're back?

Comment 5

[Nathan Froyd [:froydnj]]

I think Eric's comment 3 is on point: I'm not confident that we can grab raw pointers to services and guarantee that those raw pointers will be available for the duration of the service's use if things can be grabbed during shutdown.  I'm happy to hear counterarguments, though.

Alternative proposal: can we set things up so that the affected service(s), assuming they're builtinclass, use non-virtual calls to AddRef, at least?  Making Release non-virtual would involve larger changes, like converting nsCOMPtr<nsIObserverService> to RefPtr<ObserverService>, but the AddRef bits should be reasonably well hidden behind the fast service getters.  (The Release changes are probably worthwhile to make on their own, but that would be a separate bug.)

Comment 6

[(no longer active)]

Firstly about what Eric said in comment 3 about this requiring auditing a lot of code, that's actually not true since all users of this API right now are bound to use nsCOMPtr's whether they need them or not, so we could land this change if we wanted to with 0 audit of the current code quite safely.

(In reply to Nathan Froyd [:froydnj] from comment #5)
> Alternative proposal: can we set things up so that the affected service(s),
> assuming they're builtinclass, use non-virtual calls to AddRef, at least? 
> Making Release non-virtual would involve larger changes, like converting
> nsCOMPtr<nsIObserverService> to RefPtr<ObserverService>, but the AddRef bits
> should be reasonably well hidden behind the fast service getters.  (The
> Release changes are probably worthwhile to make on their own, but that would
> be a separate bug.)

Well, firstly that will only work out if the compiler successfully devirtualizes those calls, and it's not obvious to me how much we should be counting on that.  If we really wanted to *count* on it, we would need to have a RefPtr like class that calls these functions statically (aka, call ptr->Type::AddRef() instead of ptr->AddRef() for example).  I honestly don't know to what degree one must rely on compiler assisted devirtualization these days, but it seems kind of a hard sell to rely on that for code where refcounting cost is showing up in profiles.  :/

Which brings me to the sad outcome of having to bypass mozilla::services for such code and just use raw pointers like a sane person would.  I suppose that's fine.

Just one last point, I was curious to know whether the already_AddRefed return type was actually added at some point in order to solve some practical concern or as a defense against a theoretical concern.  I traced things back to the introduction of this code around bug 516085 comment 45 where Taras says he doesn't see the benefit of refcounting singletons (amen!) to the comment 68 of the same bug where he changes his mind seemingly due to the service manager dropping its references to services during shutdown, leaving the question of why solving that problem in the few consumers who would be affected by it wasn't deemed acceptable unanswered...

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #6)
> Firstly about what Eric said in comment 3 about this requiring auditing a
> lot of code, that's actually not true since all users of this API right now
> are bound to use nsCOMPtr's whether they need them or not, so we could land
> this change if we wanted to with 0 audit of the current code quite safely.

This is a good point.  I'm not confident that we can rely on people to correctly use the raw pointers in other contexts, however.

OTOH, the service getters already return nullptr if mozilla::services::Shutdown() has been called, so maybe we can correctly assume that the services are always alive?  The service manager itself is shutdown after mozilla::services::Shutdown(), and I think we can assume that the service manager will be left holding the last references to the services, thus keeping them alive.

People could still do something like:

  nsIObserverService* mObsService;

and use that post service-manager shutdown, but ideally things like that wouldn't happen...

> (In reply to Nathan Froyd [:froydnj] from comment #5)
> > Alternative proposal: can we set things up so that the affected service(s),
> > assuming they're builtinclass, use non-virtual calls to AddRef, at least? 
> > Making Release non-virtual would involve larger changes, like converting
> > nsCOMPtr<nsIObserverService> to RefPtr<ObserverService>, but the AddRef bits
> > should be reasonably well hidden behind the fast service getters.  (The
> > Release changes are probably worthwhile to make on their own, but that would
> > be a separate bug.)
> 
> Well, firstly that will only work out if the compiler successfully
> devirtualizes those calls, and it's not obvious to me how much we should be
> counting on that.  If we really wanted to *count* on it, we would need to
> have a RefPtr like class that calls these functions statically (aka, call
> ptr->Type::AddRef() instead of ptr->AddRef() for example).  I honestly don't
> know to what degree one must rely on compiler assisted devirtualization
> these days, but it seems kind of a hard sell to rely on that for code where
> refcounting cost is showing up in profiles.  :/

We rely on it in other places; mystor convinced me that devirtualization Just Works for cases like this (assuming your class definitions are visible and marked as `final`).

> Which brings me to the sad outcome of having to bypass mozilla::services for
> such code and just use raw pointers like a sane person would.  I suppose
> that's fine.

I think acquiring the raw pointers you want is difficult with the XPCOM-ish APIs and would only be possible with the mozilla::services APIs.

Comment 8

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> OTOH, the service getters already return nullptr if
> mozilla::services::Shutdown() has been called, so maybe we can correctly
> assume that the services are always alive?  The service manager itself is
> shutdown after mozilla::services::Shutdown(), and I think we can assume that
> the service manager will be left holding the last references to the
> services, thus keeping them alive.
> 
> People could still do something like:
> 
>   nsIObserverService* mObsService;
> 
> and use that post service-manager shutdown, but ideally things like that
> wouldn't happen...

I guess one thing that I don't really understand about the objection here is why we are worrying about the case where people store the pointer returned from mozilla::services::GetFoo() so much.  To me it seems like this is the API you'd probably use in the cases where you _don't_ want to store the said pointer for later use since mozilla::services getters are already doing that for you.  But maybe that's just me...

But to answer your question, yes as you note this kind of issue is already possible if people store such raw pointers and use them past shutdown.  In general *storing* raw pointers to XPCOM services in any code is bad practice unless there is some hard guarantee about that code running before shutdown (and we regularly trip over crashes when people get this wrong, which still happens even with these functions return already_AddRefed in ways similar to the above.)

> > (In reply to Nathan Froyd [:froydnj] from comment #5)
> > > Alternative proposal: can we set things up so that the affected service(s),
> > > assuming they're builtinclass, use non-virtual calls to AddRef, at least? 
> > > Making Release non-virtual would involve larger changes, like converting
> > > nsCOMPtr<nsIObserverService> to RefPtr<ObserverService>, but the AddRef bits
> > > should be reasonably well hidden behind the fast service getters.  (The
> > > Release changes are probably worthwhile to make on their own, but that would
> > > be a separate bug.)
> > 
> > Well, firstly that will only work out if the compiler successfully
> > devirtualizes those calls, and it's not obvious to me how much we should be
> > counting on that.  If we really wanted to *count* on it, we would need to
> > have a RefPtr like class that calls these functions statically (aka, call
> > ptr->Type::AddRef() instead of ptr->AddRef() for example).  I honestly don't
> > know to what degree one must rely on compiler assisted devirtualization
> > these days, but it seems kind of a hard sell to rely on that for code where
> > refcounting cost is showing up in profiles.  :/
> 
> We rely on it in other places; mystor convinced me that devirtualization
> Just Works for cases like this (assuming your class definitions are visible
> and marked as `final`).

Just to clarify, I have tested it and agree that it works pretty reliably on simple test cases, but I'm a bit too nervous about relying on that for all of our compilers in the general case mostly because I don't understand the exact criteria used by the compiler to choose when to devirtualize.  I checked with mystor and he didn't seem to recall those details.  Do you happen to remember where we rely on this currently, out of curiosity?

(But for now, I think my preference is still with relying on using raw pointers and switching away from using mozilla::services over relying on devirtualization, as cowardly as that may sound!)

> > Which brings me to the sad outcome of having to bypass mozilla::services for
> > such code and just use raw pointers like a sane person would.  I suppose
> > that's fine.
> 
> I think acquiring the raw pointers you want is difficult with the XPCOM-ish
> APIs and would only be possible with the mozilla::services APIs.

Yep.  The motivation behind this for now was bug 1382923, where I found a nice C++ey way to do this without the involvement of XPCOM which bounced due to Android not using the same C++ places code as other platforms, so now I'll have to mint some cross-platform C++ API for getting a raw IHistory* there.  In the general case most services do have some kind of a singleton getter which will allow you to do the efficient thing with varying degrees of grossness depending on the code involved.

In the interest of moving ahead with a solution, I'm gonna close this bug now and invent a custom solution over in bug 1382923.