1034920 - nsNativeModuleLoader does not need to be a refcounted class

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Rename nsNativeComponentLoader.{h,cpp} to the actual class name.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Fix names of NativeLoadData.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 3 - nsNativeModuleLoader doesn't need to implement ModuleLoader.

It is only used as a field of gComponentManager, so the full nsISupports and COM treatment isn't needed.

Comment 6

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=506fa16ce0bb

Comment 7

[Nathan Froyd [:froydnj]]

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.

Comment 8

[Andrew McCreight [:mccr8]]

> All these headers just to cargo-cult FileLocation and the nsISupports bits?  Ugh.
Yeah it is awesome.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ccfca130bbe3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e814aa7bab58
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b84ac541b00e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ccfca130bbe3
https://hg.mozilla.org/mozilla-central/rev/e814aa7bab58
https://hg.mozilla.org/mozilla-central/rev/b84ac541b00e