936964 - Make singleton objects report their own memory consumption, instead of having a separate reporter object

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

We have a lot of singleton structs that are paired with a memory reporter:
nsCategoryManager, nsObserverService, nsScriptNameSpaceManager,
nsStyleSheetService, nsDiskCacheDevice, and many more.

For these cases, the reporter is created and registered by the singleton when
the singleton is created, and unregistered by the singleton when the singleton
is destroyed.  Example code:

  class nsFooReporter : public mozilla::MemoryUniReporter {
  public:
    nsFooReporter(nsFoo* aFoo)
      : MemoryUniReporter("explicit/foo", KIND_HEAP, UNITS_BYTES,
                          "Memory used by the Foo service."),
        mFoo(aFoo)

    int64_t Amount() {  // inherited from MemoryUniReporter
      return mFoo->SizeOfIncludingThis(MallocSizeOf);
    }

  private:
    nsFoo* mFoo;        // weak reference
  };

  class nsFoo : public nsIFoo {
  public:
    nsFoo() {
      ...
      mReporter = new nsFooReporter(this);
      NS_RegisterMemoryReporter(mReporter);
    }

    ~nsFoo() {
      ...
      NS_UnregisterMemoryReporter(mReporter);
    }

    size_t nsFoo:SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) {
      ...
    }

    ...

  private:
    nsCOMPtr<nsIMemoryReporter> mReporter;
    ...
  };

  NS_IMPL_ISUPPORTS1(nsFoo, nsIFoo)

This is a case where the nsFooReporter holds a weak reference to the nsFoo
object.  It has to be a weak reference otherwise the reporter would unnaturally
keep the nsFoo object alive until the memory reporter manager (which holds a
strong reference to all registered reporters) is destroyed.

*But* this is dangerous.  If some code calls enumerateReporters, that creates a
temporary enumerator that holds a strong reference to all the registered
reporters.  If the nsFoo is destroyed while the enumerator lives, nsFooReporter
will end up pointing to the dead nsFoo, and we will try to access it when
nsFooReporter::Amount() is called.

Now, because these are singletons, usually there is a way to access them
through some global variable.  So Amount() can be (and in many existing cases,
is) written more safely like this:

    int64_t Amount() {
      return gFoo ? gFoo->SizeOfIncludingThis(MallocSizeOf) : 0;
    }

But it's still conceivable that might go wrong, e.g. if the nsFoo is destroyed
between the |gFoo| check and the |gFoo->SizeOfIncludingThis()| call.

I'm not aware of any actual crashes caused by this problem.  Most of these
singletons are only destroyed at shutdown, when we're unlikely to be
enumerating reporters.  But still, it is unsettling and something I've wanted
to fix for some time.  (jlebar first identified the problem almost a year ago,
in bug 831193 comment 41.)

We can avoid these subtle problems if we merge nsFoo and nsFooReporter, i.e.
make nsFoo its own reporter.  It ends up looking like this:

  class nsFoo
    : public mozilla::MemoryUniReporter
    , public nsIFoo
  {
  public:
    nsFoo::nsFoo() {
      ...
      NS_RegisterMemoryReporter(this);
    }

    nsFoo:~nsFoo() {
      ...
      NS_UnregisterMemoryReporter(this);
    }

    int64_t nsFoo::Amount() {
      return SizeOfIncludingThis(MallocSizeOf);
    }

    size_t nsFoo::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) {
      ...
    }

    ...
  };

  NS_IMPL_ISUPPORTS_INHERITED1(nsFoo, MemoryUniReporter, nsIFoo)

This avoids the subtle lifetime issues and is more concise to boot.  The only
other change required is that the memory reporter manager must only hold a weak
reference to nsFoo, so as not to keep it alive artificially.  (And note that if
nsFoo fails to unregister itself in its own destructor, crashes are likely,
because the memory reporter manager will still hold that (now invalid) weak
reference.)

Comment 1

[Nicholas Nethercote [inactive]]

bz, does the plan in comment 0 sound reasonable to you?  I have a patch in progress and I haven't hit any major obstacles, but this issue is subtle enough that I'd appreciate a sanity-check.  Thanks!

Comment 2

[Nicholas Nethercote [inactive]]

One interesting wrinkle is that some of these singleton classes are manually allocated/deleted rather than being refcounted, e.g. StartupCache, ns{Disk,Memory}CacheDevice.  For them to safely report themselves, they must implement nsIMemoryReporter, which means they must use refcounting instead.  Converting them doesn't appear to be difficult.

Comment 3

[Nicholas Nethercote [inactive]]

Another interesting wrinkle...

>     nsFoo::nsFoo() {
>       ...
>       NS_RegisterMemoryReporter(this);
>     }

If we have something like this:

  nsRefPtr<nsFoo> mFoo = new nsFoo();

Here's the sequence:

- nsFoo() is allocated and constructed, and the constructor calls NS_RegisterMemoryReporter.  The refcount at this point is zero.

- NS_RegisterMemoryReporter stores a weak reference to the nsFoo, leaving its refcount unchanged at zero.

- The assignment happens, increasing the nsFoo's refcount to one.

This is ok... except NS_RegisterMemoryReporter better not do anything that involves taking a temporary strong reference to the nsFoo, otherwise the refcount will rise to one and then drop to zero, triggering destruction before we've even left nsFoo's constructor.  Hmm.

Comment 4

[Josh Matthews [:jdm]]

Right, that's why we have the nsFoo::Init idiom in many places, since there's a non-zero refcount at that point.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Merging the object and the reporter in cases like this could work, yes.

Another option is to have ~nsFoo in the original code null out the weak mFoo pointer on the memory reporter (basically allowing it to use a pattern similar to the gFoo one, but with a member variable).

But yes, if we merge we'll probably want the Init() thing.

Comment 6

[Nicholas Nethercote [inactive]]

Thanks for the feedback!  The Init() thing is a bit annoying because there are about 20 instances of this pattern...

Comment 7

[Nicholas Nethercote [inactive]]

> Thanks for the feedback!  The Init() thing is a bit annoying because there
> are about 20 instances of this pattern...

It's not so bad, actually.  Eight instances already register their reporter from an Init()-like function, another four have an Init()-like function that registration could move into, and only five need an Init()-like function added.  So I'll do that.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Make StartupCache ref-counted.

In order for StartupCache to report its own memory consumption (i.e. be a
memory reporter) it must first implement nsISupports and be ref-counted.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Make SurfaceCache ref-counted.

In order for SurfaceCache to report its own memory consumption (i.e. be a
memory reporter) it must first implement nsISupports and be ref-counted.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Make nsCycleCollector ref-counted.

In order for nsCycleCollector to report its own memory consumption (i.e. be a
memory reporter) it must first implement nsISupports and be ref-counted.

BTW, the use of nsAutoPtr in nsCycleCollector_startup() was really gratuitous
and so I got rid of it.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Make MediaMemoryTracker ref-counted.

In order for MediaMemoryTracker to report its own memory consumption (i.e. be a
memory reporter) it must first implement nsISupports and be ref-counted.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Make WebGLMemoryReporterWrapper ref-counted.

In order for WebGLMemoryReporterWrapper to report its own memory consumption
(i.e. become a memory reporter) it must first implement nsISupports and be
ref-counted.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Rename WebGLMemoryReporterWrapper as WebGLMemoryTracker.

The next patch is going to merge WebGLMemoryReporter into
WebGLMemoryReporterWrapper, and the "Wrapper" part of the name will no longer
be true.  So this patch renames it as WebGLMemoryTracker, which follows the
precedent of the similar MediaMemoryTracker class we have in content/media/.

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8338274 [details] [diff] [review]
(part 3) - Make nsCycleCollector ref-counted.

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

Looks reasonable.  The auto pointer is a little silly given there's no interesting control flow there.  I am a bit concerned that the nsRefPtr will cause a static initializer.

Kyle, does this look reasonable to you?  I'm not entirely clear on the way you set this up.

Comment 15

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

Comment on attachment 8338277 [details] [diff] [review]
(part 5) - Make WebGLMemoryReporterWrapper ref-counted.

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

For my edification, could you explain what memory reporters gain from inheriting nsISupports?

Comment 16

[Nicholas Nethercote [inactive]]

> For my edification, could you explain what memory reporters gain from
> inheriting nsISupports?

AIUI, all classes defined in .idl files that are used from JS code (as memory reporters are) must inherit from nsISupports.

Comment 17

[Nicholas Nethercote [inactive]]

> I am a bit concerned that the nsRefPtr will cause a static initializer.

I'd be surprised...

 struct CollectorData {
   nsRefPtr<nsCycleCollector> mCollector;
   CycleCollectedJSRuntime* mRuntime;
 };

 static mozilla::ThreadLocal<CollectorData*> sCollectorData;

There are no static CollectorData instances, so the nsRefPtr only gets created dynamically.

Comment 18

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

(In reply to Nicholas Nethercote [:njn] from comment #16)
> > For my edification, could you explain what memory reporters gain from
> > inheriting nsISupports?
> 
> AIUI, all classes defined in .idl files that are used from JS code (as
> memory reporters are) must inherit from nsISupports.

Ah OK, so in other words the reason for nsISupports here is that this is still using XPIDL. Is there already plans / a bug filed to port the memory reporters IDL to Web IDL / new-bindings ?

Comment 19

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

For reference:

./xpcom/base/nsIMemoryReporter.idl
./xpcom/base/nsIMemory.idl
./xpcom/base/nsIMemoryInfoDumper.idl

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Make lots of classes report their own memory consumption, instead of using a separate reporter class.

This is the big patch that merges most of the existing Foo/FooReporter pairs.
It's a long patch, but it mostly does the same thing over and over again.  

- mccr8: can you please review the whole thing.  Sorry for the length, but I
  figure it's better to have one person check N conversions rather than N
  people check 1 conversion each.  I suggest starting with
  nsIMemoryReporter.idl and nsMemoryReporterManager.{h,cpp}.  If there are any
  individual cases that you are uncertain about, please tell me and I'll get
  someone more familiar with the individual case to double-check it. 

- bz: can you please super-review this, looking in particular at
  nsIMemoryReporter.idl, and a few of the mergings just to see that they are
  reasonable.

The typical procedure for Foo defined in Foo.h/Foo.cpp with a uni-reporter:

- Move |#include "nsIMemoryReporter.h"| from Foo.cpp to Foo.h.

- Make Foo inherit from MemoryUniReporter.

- Change NS_IMPL_ISUPPORTS<n>(Foo, ...) to 
  NS_IMPL_ISUPPORTS_INHERITED<n>(Foo, MemoryUniReporter, ...).
  (A few cases were a bit different to this.)

- Move FooReporter::FooReporter's call to the super-class constructor to
  Foo::Foo().

- Remove the |mReporter| member of Foo.

- Change NS_{R,Unr}egisterMemoryReporter(mReporter) to
  {R,Unr}egisterTemporaryMemoryReporter(this).

- If |RegisterTemporaryReporter(this)| is in the constructor, move it to an
  Init()-type function.  We must finish constructing a reporter before we can
  register it, otherwise the registration-time refcount checks fail.

- Move FooReporter::Amount() into Foo, and change Amount() to call
  Foo::SizeOfIncludingThis directly, instead of going through a global or local
  pointer to the Foo instance.  E.g.:
  
    return SizeOfIncludingThis(MallocSizeOf);
 
  instead of:

    return gFoo ? gFoo->SizeOfIncludingThis(MallocSizeOf) : 0;

- Remove all remaining traces of FooReporter.

Multi-reporters are similar, but have CollectReports() instead of Amount().

Some specific things worth mentioning:

- In mozIHunspell, I had to rename |name| as |engineName| because it clashed
  with nsIMemoryReporter::name.

- Amount() is a pretty generic method name.  This was fine when it was used in
  a FooReporter class, but within a larger Foo class it's meaning is less
  clear.  Maybe I should change it to UniReportAmount() or something like that.

- StartupCache had two uni-reporters, which I combined into a single
  multi-reporter (duo-reporter?)

- For nsUrlClassifier I inherited from nsIMemoryReporter instead of
  MemoryUniReporter, because the reporter path gets set in Init() instead of
  the constructor, and so cannot be const.

- Fun fact:  nsCycleCollector::mReporter was never set, so the
  NS_UnregisterMemoryReporter() call always took a nullptr and so did nothing.

- Stats:  70 files changed, 906 insertions(+), 1049 deletions(-)

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Make nsCacheService report the disk and memory cache devices.

Currently, nsDiskCacheDevice and nsMemoryCacheDevice both have their own
reporters.  

I tried merging the reporters into the classes, so that the devices could
report their own memory, as I did for lots of other cases in patch 7.
However, this didn't work because it could cause nsDiskCacheDevice to outlive
nsCacheService -- because of references held by the memory reporter manager --
and ~nsDiskCacheDevice() needs nsCacheService to still be alive.

So instead I made nsCacheService be the reporter, and it reports the usage of
both the disk and memory cache devices.  This is nicer overall, I think.

Comment 22

[Nicholas Nethercote [inactive]]

> Is there already plans / a bug filed to port the memory reporters IDL to Web IDL / new-bindings ?

Nope! :)

Comment 23

[u408661]

Comment on attachment 8338724 [details] [diff] [review]
(part 8) - Make nsCacheService report the disk and memory cache devices.

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

LGTM

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8338717 [details] [diff] [review]
(part 7) - Make lots of classes report their own memory consumption, instead of using a separate reporter class.

How did you check for consumers that might be broken by the mozISpellCheckingEngine change?

I pretty much skimmed most of this, but it looks reasonable in general, apart from that change.

Comment 25

[Nicholas Nethercote [inactive]]

> How did you check for consumers that might be broken by the
> mozISpellCheckingEngine change?

Good question.  On the C++ side the compiler identified them, but I didn't do anything about the JS side.

Perhaps I should rename the nsIMemoryReporter property instead.  At least I have a pretty good idea of where that property is being used.  (In fact, I'm actually contemplating removing that property because it's not actually used meaningfully anywhere...)

Comment 26

[Boris Zbarsky [:bzbarsky]]

That seems like it would be safer, esp. in terms of addon compat.

Comment 27

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8338272 [details] [diff] [review]
(part 2) - Make SurfaceCache ref-counted.

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

Looks good.

Comment 28

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8338717 [details] [diff] [review]
(part 7) - Make lots of classes report their own memory consumption, instead of using a separate reporter class.

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

::: image/src/SurfaceCache.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */

Is this change needed? Seems like it doesn't belong in this patch.

Comment 29

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Make lots of classes report their own memory consumption, instead of using a separate reporter class.

Updated patch which doesn't modify |name| for the spell checker.  I will open a
new bug report for the |name|-removal patch (which this bug relies on) shortly.

Comment 30

[Nicholas Nethercote [inactive]]

> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> 
> Is this change needed? Seems like it doesn't belong in this patch.

It's needed to (a) comply with our style guide, and (b) for vim to use the appropriate indentation for this file.

I fix modelines in this way as I touch new files and find they need fixing.  (I do a similar thing with trailing whitespace, to a degree.)  It's not logically part of this patch but isn't worth the effort of separating.

Comment 31

[Nicholas Nethercote [inactive]]

> I will open a new bug report for the |name|-removal patch (which this
> bug relies on) shortly.

Bug 943660.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8338901 [details] [diff] [review]
(part 7) - Make lots of classes report their own memory consumption, instead of using a separate reporter class.

sr=me

Comment 33

[Andrew McCreight [:mccr8]]

Comment on attachment 8338901 [details] [diff] [review]
(part 7) - Make lots of classes report their own memory consumption, instead of using a separate reporter class.

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

It is nice to be rid of this layer of memory reporter boilerplate!

Please remove the various mode line changes.  They can be annoying in files that haven't had their indentation updated, so these kinds of changes should get review from the appropriate module peer.

r=me with that

::: content/canvas/src/WebGLMemoryTracker.h
@@ +19,4 @@
>  
>  namespace mozilla {
>  
> +class WebGLMemoryTracker : public mozilla::MemoryMultiReporter

Do you need the mozilla:: prefix, as you are inside the mozilla namespace already?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +3079,5 @@
>          NS_RUNTIMEABORT("xpc_LocalizeRuntime failed.");
>  
>      // Register memory reporters and distinguished amount functions.
> +    RegisterMemoryReporter(new JSMainRuntimeCompartmentsReporter);
> +    RegisterMemoryReporter(new JSMainRuntimeTemporaryPeakReporter());

Might as well make these consistent about whether they have the () there while you are here.

::: storage/src/mozStorageService.h
@@ +24,5 @@
>  namespace mozilla {
>  namespace storage {
>  
>  class Connection;
> +class Service : public mozilla::MemoryMultiReporter

You are in namespace mozilla, does it need to be explicitly marked?

::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp
@@ +50,5 @@
>  
>  NS_IMETHODIMP
>  nsUrlClassifierPrefixSet::Init(const nsACString& aName)
>  {
> +  mMemoryReportPath =

You could also do two .Append() here, I think.  Maybe that's a little simpler.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +937,5 @@
>  nsMemoryReporterManager::~nsMemoryReporterManager()
>  {
> +    delete mPermanentReporters;
> +    delete mTemporaryReporters;
> +    delete mSavedPermanentReporters;

Maybe assert that the two saved reporters are null?  A test should clean up after itself.  Though maybe that will just make failures blow up on TBPL in an ugly way.

@@ +1203,5 @@
> +    // addref'ed and released |aReporter| before finally addref'ing it for
> +    // good, it would free aReporter!  The kung fu death grip could itself be
> +    // problematic if PutEntry didn't addref |aReporter| (because then when the
> +    // death grip goes out of scope, we would delete the reporter).  In debug
> +    // mode, we check that this doesn't happen.

It might be nice to MOZ_CRASH in release builds.

::: xpcom/base/nsMemoryReporterManager.h
@@ +158,4 @@
>    Mutex mMutex;
>    bool mIsRegistrationBlocked;
>  
> +  PermanentReportersTable* mPermanentReporters;

Why are these * and not just PermanentReportersTable?  Same with the next one.

Also it would be nice to have a comment explaining that permanent reporters are strongly held and temporary are weakly held.  I can see what you are going for, but this is a bit of a deviation from the standard terminology so it would be nice to have that documented more.

@@ +161,5 @@
> +  PermanentReportersTable* mPermanentReporters;
> +  TemporaryReportersTable* mTemporaryReporters;
> +
> +  // These two are only used for testing purposes.
> +  PermanentReportersTable* mSavedPermanentReporters;

For these two, maybe make them nsAutoPtr rather than raw pointers?

Comment 34

[Andrew McCreight [:mccr8]]

Comment on attachment 8338274 [details] [diff] [review]
(part 3) - Make nsCycleCollector ref-counted.

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

I see this is the only thing remaining here, so I'm going to r+ it.  Hopefully Kyle will chime in if it is going to do bad things.

Comment 35

[Nicholas Nethercote [inactive]]

> ::: xpcom/base/nsMemoryReporterManager.h
> @@ +158,4 @@
> >    Mutex mMutex;
> >    bool mIsRegistrationBlocked;
> >  
> > +  PermanentReportersTable* mPermanentReporters;
> 
> Why are these * and not just PermanentReportersTable?  Same with the next
> one.

Making them pointers allows them to be swapped into the |Saved| fields easily.  If they weren't pointers it would be much harder.

> Also it would be nice to have a comment explaining that permanent reporters
> are strongly held and temporary are weakly held.  I can see what you are
> going for, but this is a bit of a deviation from the standard terminology so
> it would be nice to have that documented more.

What's the more standard terminology?

Comment 36

[Nicholas Nethercote [inactive]]

BTW, I didn't expect to get nine reviews of eight patches (including one quite large one) from seven people in less than 24 hours.  In fact, I didn't expect I would be able to land this before Thanksgiving.

Excellent work, people, excellent work.

Comment 37

[Nicholas Nethercote [inactive]]

> Please remove the various mode line changes.  They can be annoying in files
> that haven't had their indentation updated, so these kinds of changes should
> get review from the appropriate module peer.

That seems to be the complaint du jour, so I'll remove them.  But FWIW, I set the indentation to 2 or 4 depending on what the file currently uses, so it shouldn't cause problems (and I've added them lots of times in the past without complaint).

Comment 38

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #35)
> Making them pointers allows them to be swapped into the |Saved| fields
> easily.  If they weren't pointers it would be much harder.
Ah, righto.

> What's the more standard terminology?

Well, just "strong" and "weak".  Or nothing and "weak".  Like Preferences:AddStrongObserver and AddWeakObserver.

Comment 39

[Nicholas Nethercote [inactive]]

> Well, just "strong" and "weak".  Or nothing and "weak".  Like
> Preferences:AddStrongObserver and AddWeakObserver.

Ok, I might switch to Register{Strong,Weak}MemoryReporter, etc, then.

Comment 40

[Nicholas Nethercote [inactive]]

Green try run:
https://tbpl.mozilla.org/?tree=Try&rev=555ac8434e42

Landing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1459e71585
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e9c3137804
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae3a61182db
https://hg.mozilla.org/integration/mozilla-inbound/rev/a87ffc992f38
https://hg.mozilla.org/integration/mozilla-inbound/rev/da6465ad476f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dbb8333960c
https://hg.mozilla.org/integration/mozilla-inbound/rev/25312eb71998
https://hg.mozilla.org/integration/mozilla-inbound/rev/85486c4aa3d8

Comment 41

[(no longer active)]

Backed out because of use-after-free errors detected in Linux ASAN bc and Moth jobs:

https://hg.mozilla.org/integration/mozilla-inbound/rev/190eedf8577a
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=85486c4aa3d8

Comment 42

[Nicholas Nethercote [inactive]]

Attempt 2 for parts 1--6:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72a3b33fcf83
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd644d027bb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/645a4563010f
https://hg.mozilla.org/integration/mozilla-inbound/rev/68863a950216
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d41531be6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdecdea7f12b

Comment 43

[Nicholas Nethercote [inactive]]

Attachment: (part 7b) - Fix a use-after-free found by ASan.

In patch 7 I changed gInterfaceInfoManager to use StaticRefPtr instead of
manual refcounting.  That apparently caused the use-after-free found by ASan,
and reverting (as this patch does) fixes it.

But these two approaches look functionally identical to me.  bz, can you
see how they differ?

Comment 44

[Nicholas Nethercote [inactive]]

Parts 7 and 8, attempt 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece8c99958a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c339e9c197b

Here's the green ASan try run:
https://tbpl.mozilla.org/?tree=Try&rev=6ee421e8d047

Comment 45

[Boris Zbarsky [:bzbarsky]]

So those two stacks show the destructor being reentered.  Is that actually what's ending up happening somehow?

It's not obvious to me why that happens, exactly.  Two slightly fishy things worth checking on:

1)  ~XPTInterfaceInfoManager sets gInterfaceInfoManager to null.  This _looks_ to me like
    it should be ok, because by the time we get to the dtor the mRawPtr in
    gInterfaceInfoManager should be null (since setting that happens before
    oldPtr->Release()).

2)  After this patch, XPTInterfaceInfoManager has two refcount members: one of its own
    and one inherited from MemoryUniReporter.  It should all be ok because Release/AddRef
    are virtual and hence calling them should call the most-derived class in this case,
    which should then forward on to the base class (so the refcount in
    XPTInterfaceInfoManager is unused). But maybe something is screwing up and messing
    with the wrong refcount members?

Nothing else is obviously jumping out at me...

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/72a3b33fcf83
https://hg.mozilla.org/mozilla-central/rev/fd644d027bb5
https://hg.mozilla.org/mozilla-central/rev/645a4563010f
https://hg.mozilla.org/mozilla-central/rev/68863a950216
https://hg.mozilla.org/mozilla-central/rev/c2d41531be6d
https://hg.mozilla.org/mozilla-central/rev/bdecdea7f12b

Comment 47

[Nicholas Nethercote [inactive]]

jcranmer: FYI, this will break comm-central.  The fix should be simple:  in /mailnews/db/msgdb/src/nsMsgDatabase.cpp, replace NS_{R,Unr}egisterMemoryReporter() with mozilla::{R,Unr}egisterWeakMemoryReporter().

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ece8c99958a6
https://hg.mozilla.org/mozilla-central/rev/8c339e9c197b

Comment 49

[Joshua Cranmer [:jcranmer]]

(In reply to Nicholas Nethercote [:njn] from comment #47)
> jcranmer: FYI, this will break comm-central.  The fix should be simple:  in
> /mailnews/db/msgdb/src/nsMsgDatabase.cpp, replace
> NS_{R,Unr}egisterMemoryReporter() with
> mozilla::{R,Unr}egisterWeakMemoryReporter().

Fixed, but it would have been slightly more helpful to have given me a heads up and landed not during a US holiday. :-)

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8339814 [details] [diff] [review]
(part 7b) - Fix a use-after-free found by ASan.

I would really like us to understand why this changes anything....

Comment 51

[Nicholas Nethercote [inactive]]

> (part 7b) - Fix a use-after-free found by ASan.
> 
> I would really like us to understand why this changes anything....

So, we have the following:

  class MemoryUniReporter : public nsIMemoryReporter
  {
  public:
    ...
    NS_DECL_THREADSAFE_ISUPPORTS
    ...
  };

  NS_IMPL_ISUPPORTS1(nsMemoryReporterManager, nsIMemoryReporterManager)

  class XPTInterfaceInfoManager MOZ_FINAL
      : public mozilla::MemoryUniReporter
      , public nsIInterfaceInfoManager 
  {
      NS_DECL_THREADSAFE_ISUPPORTS
      ...
  };

  NS_IMPL_ISUPPORTS_INHERITED1(
    XPTInterfaceInfoManager,
    MemoryUniReporter,      
    nsIInterfaceInfoManager)

mccr8 suggested that the XPTIInterfaceInfoManager should actually use
NS_DECL_ISUPPORTS_INHERITED.  Could that be relevant?

Comment 52

[Boris Zbarsky [:bzbarsky]]

That's what I talked about in comment 45 item 2, no?

Comment 53

[Nicholas Nethercote [inactive]]

I tried converting XPTInterfaceManager to inherit directly from nsIMemoryReporter and not use MemoryUniReporter, and I still get a use-after-free error from ASan, but now it's a different one.  Progress, I guess.

> ==2414==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000c43910 at pc 0x7f971926fdf7 bp 0x7fff972fa3b0 sp 0x7fff972fa3a8
> READ of size 8 at 0x604000c43910 thread T0
>     #0 0x7f971926fdf6 in XPT_DestroyArena /builds/slave/try-l64-asan-00000000000000000/build/xpcom/typelib/xpt/src/xpt_arena.c:143
>     #1 0x7f9712fce1a7 in ~Mutex /builds/slave/try-l64-asan-00000000000000000/build/xpcom/reflect/xptinfo/src/xptiWorkingSet.cpp:50
>     #2 0x7f9712fce1a7 in mozilla::XPTInterfaceInfoManager::~XPTInterfaceInfoManager() /builds/slave/try-l64-asan-00000000000000000/build/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp:101
>     #3 0x7f9712fcde31 in Release /builds/slave/try-l64-asan-00000000000000000/build/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp:25
>     #4 0x7f9712fcde31 in AssignAssumingAddRef /builds/slave/try-l64-asan-00000000000000000/build/obj-firefox/xpcom/reflect/xptinfo/src/../../../../dist/include/mozilla/StaticPtr.h:158
>     #5 0x7f9712fcde31 in AssignWithAddref /builds/slave/try-l64-asan-00000000000000000/build/obj-firefox/xpcom/reflect/xptinfo/src/../../../../dist/include/mozilla/StaticPtr.h:150
>     #6 0x7f9712fcde31 in operator= /builds/slave/try-l64-asan-00000000000000000/build/obj-firefox/xpcom/reflect/xptinfo/src/../../../../dist/include/mozilla/StaticPtr.h:114
>     #7 0x7f9712fcde31 in mozilla::XPTInterfaceInfoManager::FreeInterfaceInfoManager() /builds/slave/try-l64-asan-00000000000000000/build/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp:81
>     #8 0x7f9712eaae59 in mozilla::ShutdownXPCOM(nsIServiceManager*) /builds/slave/try-l64-asan-00000000000000000/build/xpcom/build/nsXPComInit.cpp:801
>     #9 0x7f9717f7d6d8 in ScopedXPCOMStartup::~ScopedXPCOMStartup() /builds/slave/try-l64-asan-00000000000000000/build/toolkit/xre/nsAppRunner.cpp:1137
>     #10 0x7f9717f8e9d6 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/try-l64-asan-00000000000000000/build/toolkit/xre/nsAppRunner.cpp:4077
>     #11 0x7f9717f8f83b in XRE_main /builds/slave/try-l64-asan-00000000000000000/build/toolkit/xre/nsAppRunner.cpp:4254
>     #12 0x459dcd in do_main /builds/slave/try-l64-asan-00000000000000000/build/browser/app/nsBrowserApp.cpp:280
>     #13 0x459dcd in main /builds/slave/try-l64-asan-00000000000000000/build/browser/app/nsBrowserApp.cpp:648

Comment 54

[Boris Zbarsky [:bzbarsky]]

Do you have a stack to where it was freed, by any chance?

Comment 55

[Nicholas Nethercote [inactive]]

Not yet.  I'm still analyzing.

Comment 56

[Andrew McCreight [:mccr8]]

ASAN prints out the place that freed it right after it prints where the use-after-free was.

Comment 57

[Nicholas Nethercote [inactive]]

In FreeInterfaceInfoManager() we null gInterfaceInfoManager, which triggers its destructor.  The destructor then nulls gInterfaceInfoManager again.  AFAICT this shouldn't matter because of the way StaticRefPtr is implemented, but I'm doing a try push that removes the nulling from the destructor.

Comment 58

[Nicholas Nethercote [inactive]]

> ASAN prints out the place that freed it right after it prints where the
> use-after-free was.

Not in this case, AFAICT:
https://tbpl.mozilla.org/php/getParsedLog.php?id=31657965&tree=Try

It does say 

  ==2414==AddressSanitizer: while reporting a bug found another one.Ignoring.

Which might explain why it doesn't get printed?

Comment 59

[Nicholas Nethercote [inactive]]

> In FreeInterfaceInfoManager() we null gInterfaceInfoManager, which triggers
> its destructor.  The destructor then nulls gInterfaceInfoManager again. 
> AFAICT this shouldn't matter because of the way StaticRefPtr is implemented,
> but I'm doing a try push that removes the nulling from the destructor.

This was bz's item (1) in comment 45... and it's the cause -- my try run was green.  WTF?  I tried and failed several times to get Valgrind to make the same complaint.  I wonder if ASan is buggy.

Anyway, removing the unnecessary nulling is a good change, so I'll work up a new patch for review tomorrow.

Comment 60

[Nicholas Nethercote [inactive]]

> Anyway, removing the unnecessary nulling is a good change, so I'll work up a
> new patch for review tomorrow.

Bug 948201.

Comment 61

[Nicholas Nethercote [inactive]]

Just for posterity, here's the full output of the error:

> ==2414==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000c43910 at pc 0x7f971926fdf7 bp 0x7fff972fa3b0 sp 0x7fff972fa3a8
>  READ of size 8 at 0x604000c43910 thread T0
>      #0 0x7f971926fdf6 in XPT_DestroyArena /builds/slave/try-l64-asan-00000000000000000/build/xpcom/typelib/xpt/src/xpt_arena.c:143
>      #1 0x7f9712fce1a7 in ~Mutex /builds/slave/try-l64-asan-00000000000000000/build/xpcom/reflect/xptinfo/src/xptiWorkingSet.cpp:50
>      #2 0x7f9712fce1a7 in mozilla::XPTInterfaceInfoManager::~XPTInterfaceInfoManager() /builds/slave/try-l64-asan-00000000000000000/build/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp:101
>      #3 0x7f9712fcde31 in Release /builds/slave/try-l64-asan-00000000000000000/build/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp:25
>      #4 0x7f9712fcde31 in AssignAssumingAddRef /builds/slave/try-l64-asan-00000000000000000/build/obj-firefox/xpcom/reflect/xptinfo/src/../../../../dist/include/mozilla/StaticPtr.h:158
>      #5 0x7f9712fcde31 in AssignWithAddref /builds/slave/try-l64-asan-00000000000000000/build/obj-firefox/xpcom/reflect/xptinfo/src/../../../../dist/include/mozilla/StaticPtr.h:150
>      #6 0x7f9712fcde31 in operator= /builds/slave/try-l64-asan-00000000000000000/build/obj-firefox/xpcom/reflect/xptinfo/src/../../../../dist/include/mozilla/StaticPtr.h:114
>      #7 0x7f9712fcde31 in mozilla::XPTInterfaceInfoManager::FreeInterfaceInfoManager() /builds/slave/try-l64-asan-00000000000000000/build/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp:81
>      #8 0x7f9712eaae59 in mozilla::ShutdownXPCOM(nsIServiceManager*) /builds/slave/try-l64-asan-00000000000000000/build/xpcom/build/nsXPComInit.cpp:801
>      #9 0x7f9717f7d6d8 in ScopedXPCOMStartup::~ScopedXPCOMStartup() /builds/slave/try-l64-asan-00000000000000000/build/toolkit/xre/nsAppRunner.cpp:1137
>      #10 0x7f9717f8e9d6 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/try-l64-asan-00000000000000000/build/toolkit/xre/nsAppRunner.cpp:4077
>      #11 0x7f9717f8f83b in XRE_main /builds/slave/try-l64-asan-00000000000000000/build/toolkit/xre/nsAppRunner.cpp:4254
>      #12 0x459dcd in do_main /builds/slave/try-l64-asan-00000000000000000/build/browser/app/nsBrowserApp.cpp:280
>      #13 0x459dcd in main /builds/slave/try-l64-asan-00000000000000000/build/browser/app/nsBrowserApp.cpp:648
>      #14 0x7f9722e8676c (/lib/x86_64-linux-gnu/libc.so.6+0x2176c)
>      #15 0x45934c in _start (/builds/slave/test/build/application/firefox/firefox+0x45934c)
>  ASAN:SIGSEGV
>  ==2414==AddressSanitizer: while reporting a bug found another one.Ignoring.

The "ASAN:SIGSEGV" on the second-last line is suspicious...

Comment 62

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8338274 [details] [diff] [review]
(part 3) - Make nsCycleCollector ref-counted.

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

Just use NS_DECL_INLINE_REFCOUNTING?  Don't see why you need a QI method.

Comment 63

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #59)
> > In FreeInterfaceInfoManager() we null gInterfaceInfoManager, which triggers
> > its destructor.  The destructor then nulls gInterfaceInfoManager again. 
> > AFAICT this shouldn't matter because of the way StaticRefPtr is implemented,
> > but I'm doing a try push that removes the nulling from the destructor.
> 
> This was bz's item (1) in comment 45... and it's the cause -- my try run was
> green.  WTF?  I tried and failed several times to get Valgrind to make the
> same complaint.  I wonder if ASan is buggy.
> 
> Anyway, removing the unnecessary nulling is a good change, so I'll work up a
> new patch for review tomorrow.

bz, FYI: decoder was able to locally reproduce the double-entry into the destructor (demonstrated with diagnostic printfs) without using ASan. But using the same patch, I couldn't reproduce it with clang or GCC. So it sounds like it's some kind of clang front-end bug that's causing the double-entry, and then ASan instruments the double-entry and produces a warning that is correct from its point of view, but incorrect from the user's point of view.