This is the basis of our component technology; this covers the mozilla/xpcom source directory and includes the "repository". Unlikely a tester would be able to tell there was an XPCOM problem specifically.

Right now, NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR expects singleton constructors to return already-addrefed raw pointers, and while it accepts constructors that return already_AddRefed, most existing don't do so.
Meanwhile, the convention elsewhere is that a raw pointer return value is owned by the callee, and that the caller needs to addref it if it wants to keep its own reference to it.
The difference in convention makes it easy to leak (I've definitely caused more than one shutdown leak this way), so it would be better if we required the singleton getters to return an explicit already_AddRefed, which would behave the same for all callers.

Right now, NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR expects singleton
constructors to return already-addrefed raw pointers, and while it accepts
constructors that return already_AddRefed, most existing don't do so.
Meanwhile, the convention elsewhere is that a raw pointer return value is
owned by the callee, and that the caller needs to addref it if it wants to
keep its own reference to it.
The difference in convention makes it easy to leak (I've definitely caused
more than one shutdown leak this way), so it would be better if we required
the singleton getters to return an explicit already_AddRefed, which would
behave the same for all callers.
This also cleans up several singleton constructors that left a dangling
pointer to their singletons when their initialization methods failed, when
they released their references without clearing their global raw pointers.
Review commit: https://reviewboard.mozilla.org/r/190046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190046/

Sorry for the slow response. I was sick most of last week.
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Perhaps we should consider adding `template<typename T> already_AddRefed<T>
> do_AddRef(const nsCOMPtr<T>&)` so we wouldn't have to `.get()` here? Can
> you file a followup for that?
Yeah, I was actually considering that anyway. This is the third time in the past
few weeks that I've run into this situation.
> ::: extensions/cookie/nsPermissionManager.cpp:917
> (Diff revision 1)
> > - gPermissionManager = new nsPermissionManager();
> > - if (gPermissionManager) {
> > - NS_ADDREF(gPermissionManager);
> > - if (NS_FAILED(gPermissionManager->Init())) {
> > + auto permManager = MakeRefPtr<nsPermissionManager>();
> > + if (permManager && NS_SUCCEEDED(permManager->Init())) {
> > + gPermissionManager = permManager.get();
> > + return permManager.forget();
>
> This code (and many other examples throughout this patch) gives me pause:
> we're storing a raw pointer to the sole owning reference at this point, and
> then passing out the owning reference to
> `NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR`, which isn't guaranteed to keep
> it alive. We're assuming that the `QueryInterface` call in that macro is
> going to succeed...which is not guaranteed.
>
> This patch doesn't alter the pre-existing behavior, but perhaps it makes it
> more obvious that we're doing very dodgy things here?
Yeah, this made me nervous, too. And it's even worse after service manager
shutdown begins, when the service may have been destroyed, and leaves a dangling
singleton pointer. We do this all over the place :(
I was considering replacing these with StaticRefPtrs and ClearOnShutdown, but
there are places where I think we rely on these pointers after the latest
ClearOnShutdown phase. I think we probably need to put some thought into a
standard way to handle service singletons.
> It is highly non-obvious, but `NS_RELEASE` nulls the pointer here.
Ah. That makes me feel better about the old code in some of these places.

Jorg, this is probably going to break comm-central in a few places. Basically, any place where you're calling NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR, the function passed to the second argument needs to be updated to return an already_AddRefed<T> rather than a T*.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #6)
> Jorg, this is probably going to break comm-central in a few places.
> Basically, any place where you're calling
> NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR, the function passed to the second
> argument needs to be updated to return an already_AddRefed<T> rather than a
> T*.
Thanks for the heads-up, but we don't use NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR unless I'm missing something. Otherwise, I'll deal with the bustage.