Closed Bug 1034920 Opened 6 years ago Closed 5 years ago

nsNativeModuleLoader does not need to be a refcounted class


(Core :: XPCOM, defect)

Other Branch
Not set





(Reporter: bjacob, Assigned: mccr8)




(3 files)

In bug 1028588 we removed dangerous public destructors of XPCOM-refcounted classes outside of a finite whitelist, see HasDangerousPublicDestructor. Now we are going over the entries in this whitelist.

One of them is: nsNativeModuleLoader
Assignee: nobody → continuation
This is ugly.   nsComponentManagerImpl has a field with type nsNativeModuleLoader (not a pointer).  nsNativeModuleLoader implements refcounting by forwarding it to nsComponentManagerImpl::gComponentManager.  The only thing that ever addrefs nsNativeModuleLoader as far as I can see is some runnable.

The nsNativeModuleLoader field can't just be turned into a ref ptr as this causes a "cycle" because it just addrefs gComponentManager, unless we just manually clear the native module loader field at some point in shutdown.  Maybe that's not so bad.
I kind of want to disaggregate nsNativeModuleLoader (there are only a handful of classes that use it), by making the runnable directly hold onto gComponentManager directly, but I'm nervous that it will make things too fragile.
Summary: Remove dangerous public destructor of nsNativeModuleLoader → nsNativeModuleLoader does not need to be a refcounted class
Attachment #8463431 - Flags: review?(nfroyd)
It is only used as a field of gComponentManager, so the full nsISupports and COM treatment isn't needed.
Attachment #8463433 - Flags: review?(nfroyd)
Attachment #8463430 - Flags: review?(nfroyd) → review+
Attachment #8463431 - Flags: review?(nfroyd) → review+
Comment on attachment 8463433 [details] [diff] [review]
part 3 - nsNativeModuleLoader doesn't need to implement ModuleLoader.

Review of attachment 8463433 [details] [diff] [review]:

::: xpcom/components/nsNativeModuleLoader.cpp
@@ +89,5 @@
>          return NS_OK;
>      }
> +    nsRefPtr<nsComponentManagerImpl> mManager;
> +    nsNativeModuleLoader* mLoader;

Hooray for more explicit lifetime management.

::: xpcom/components/nsNativeModuleLoader.h
@@ -11,3 @@
>  #include "nsDataHashtable.h"
>  #include "nsHashKeys.h"
> -#include "mozilla/Module.h"

All these headers just to cargo-cult FileLocation and the nsISupports bits?  Ugh.
Attachment #8463433 - Flags: review?(nfroyd) → review+
You need to log in before you can comment on or make changes to this bug.