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.

Comment on attachment 8919134 [details] Bug 1409249: Require singleton constructors to return explicit already_AddRefed. https://reviewboard.mozilla.org/r/190046/#review195440 I was just thinking about this the other day, thank you for tackling it. ::: caps/nsScriptSecurityManager.cpp:1506 (Diff revision 1) > -SystemPrincipal * > +already_AddRefed<SystemPrincipal> > nsScriptSecurityManager::SystemPrincipalSingletonConstructor() > { > - nsIPrincipal *sysprin = nullptr; > if (gScriptSecMan) > - NS_ADDREF(sysprin = gScriptSecMan->mSystemPrincipal); > + return do_AddRef(gScriptSecMan->mSystemPrincipal.get()).downcast<SystemPrincipal>(); 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? ::: 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? ::: extensions/cookie/nsPermissionManager.cpp:918 (Diff revision 1) > // teardowns - by the time our module destructor is called, it's too late to > // Release our members, since GC cycles have already been completed and > // would result in serious leaks. > // See bug 209571. > - gPermissionManager = new nsPermissionManager(); > - if (gPermissionManager) { > + auto permManager = MakeRefPtr<nsPermissionManager>(); > + if (permManager && NS_SUCCEEDED(permManager->Init())) { We could remove the `permManager` check here because the allocation is infallible. There are a couple of other instances of this pattern elsewhere. I don't think we necessarily have to remove the checks in this patch, though. ::: modules/libpref/Preferences.cpp:3919 (Diff revision 1) > Result<Ok, const char*> res = sPreferences->Init(); > if (res.isErr()) { > // The singleton instance will delete sRootBranch and sDefaultRootBranch. > gCacheDataDesc = res.unwrapErr(); > NS_RELEASE(sPreferences); > + sPreferences = nullptr; It is highly non-obvious, but `NS_RELEASE` nulls the pointer here.

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3feb5338f65c83d2af52fb8a53f866155e88dc15 Bug 1409249: Require singleton constructors to return explicit already_AddRefed. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/86cb23533a6f02536152d18781dcd40996276c15 Bug 1409249: Follow-up: Fix Android build bustage. r=bustage

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*.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b219c2cb0d6ca49ef1cc419756ccdee430f0258 Bug 1409249: Follow-up: Fix libjar ref counting bustage. r=bustage

(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.

Ah, looks like you're right. That's surprising. Nothing to worry about, then.

https://hg.mozilla.org/mozilla-central/rev/3feb5338f65c https://hg.mozilla.org/mozilla-central/rev/86cb23533a6f