All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2][qa-])

Attachments

(9 attachments, 1 obsolete attachment)

6.20 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
4.35 KB, patch
seth
: review+
Details | Diff | Splinter Review
3.79 KB, patch
mccr8
: review+
khuey
: feedback-
Details | Diff | Splinter Review
2.60 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
2.92 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
13.10 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
13.13 KB, patch
nwgh
: review+
Details | Diff | Splinter Review
197.36 KB, patch
mccr8
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
2.27 KB, patch
bzbarsky
: feedback-
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.)
(Assignee)

Comment 1

5 years ago
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!
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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

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

Updated

5 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 6

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

Comment 7

5 years ago
> 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.
(Assignee)

Comment 8

5 years ago
Created attachment 8338270 [details] [diff] [review]
(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.
Attachment #8338270 - Flags: review?(taras.mozilla)
(Assignee)

Comment 9

5 years ago
Created attachment 8338272 [details] [diff] [review]
(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.
Attachment #8338272 - Flags: review?(seth)
(Assignee)

Comment 10

5 years ago
Created attachment 8338274 [details] [diff] [review]
(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.
Attachment #8338274 - Flags: review?(continuation)
(Assignee)

Comment 11

5 years ago
Created attachment 8338276 [details] [diff] [review]
(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.
Attachment #8338276 - Flags: review?(kinetik)
(Assignee)

Comment 12

5 years ago
Created attachment 8338277 [details] [diff] [review]
(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.
Attachment #8338277 - Flags: review?(bjacob)
(Assignee)

Comment 13

5 years ago
Created attachment 8338280 [details] [diff] [review]
(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/.
Attachment #8338280 - Flags: review?(bjacob)
Attachment #8338276 - Flags: review?(kinetik) → review+
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.
Attachment #8338274 - Flags: feedback?(khuey)

Updated

5 years ago
Attachment #8338270 - Flags: review?(taras.mozilla) → review?(aklotz)

Updated

5 years ago
Attachment #8338270 - Flags: review?(aklotz) → review+
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?
Attachment #8338277 - Flags: review?(bjacob) → review+
Attachment #8338280 - Flags: review?(bjacob) → review+
(Assignee)

Comment 16

5 years ago
> 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.
(Assignee)

Comment 17

5 years ago
> 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.
(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 ?
For reference:

./xpcom/base/nsIMemoryReporter.idl
./xpcom/base/nsIMemory.idl
./xpcom/base/nsIMemoryInfoDumper.idl
(Assignee)

Comment 20

5 years ago
Created attachment 8338717 [details] [diff] [review]
(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(-)
Attachment #8338717 - Flags: review?(continuation)
(Assignee)

Comment 21

5 years ago
Created attachment 8338724 [details] [diff] [review]
(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.
Attachment #8338724 - Flags: review?(hurley)
(Assignee)

Updated

5 years ago
Attachment #8338717 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 22

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

Nope! :)
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
Attachment #8338724 - Flags: review?(hurley) → review+
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.
(Assignee)

Comment 25

5 years ago
> 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...)
That seems like it would be safer, esp. in terms of addon compat.
Comment on attachment 8338272 [details] [diff] [review]
(part 2) - Make SurfaceCache ref-counted.

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

Looks good.
Attachment #8338272 - Flags: review?(seth) → review+
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.
(Assignee)

Comment 29

5 years ago
Created attachment 8338901 [details] [diff] [review]
(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.
Attachment #8338901 - Flags: review?(continuation)
(Assignee)

Updated

5 years ago
Attachment #8338717 - Attachment is obsolete: true
Attachment #8338717 - Flags: superreview?(bzbarsky)
Attachment #8338717 - Flags: review?(continuation)
(Assignee)

Comment 30

5 years ago
> > +/* -*- 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.
(Assignee)

Comment 31

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

Bug 943660.
Depends on: 943660
(Assignee)

Updated

5 years ago
Attachment #8338901 - Flags: superreview?(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
Attachment #8338901 - Flags: superreview?(bzbarsky) → superreview+
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?
Attachment #8338901 - Flags: review?(continuation) → review+
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.
Attachment #8338274 - Flags: review?(continuation) → review+
(Assignee)

Comment 35

5 years ago
> ::: 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?
(Assignee)

Comment 36

5 years ago
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.
(Assignee)

Comment 37

5 years ago
> 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).
(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.
(Assignee)

Comment 39

5 years ago
> 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 41

5 years ago
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
(Assignee)

Comment 43

5 years ago
Created attachment 8339814 [details] [diff] [review]
(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?
(Assignee)

Updated

5 years ago
Attachment #8339814 - Flags: feedback?(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...
(Assignee)

Comment 47

5 years ago
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().
Flags: needinfo?(Pidgeot18)
(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. :-)
Flags: needinfo?(Pidgeot18)
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....
Attachment #8339814 - Flags: feedback?(bzbarsky) → feedback-
(Assignee)

Comment 51

5 years ago
> (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?
Flags: needinfo?(bzbarsky)
That's what I talked about in comment 45 item 2, no?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 53

5 years ago
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
Do you have a stack to where it was freed, by any chance?
(Assignee)

Comment 55

5 years ago
Not yet.  I'm still analyzing.
ASAN prints out the place that freed it right after it prints where the use-after-free was.
(Assignee)

Comment 57

5 years ago
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.
(Assignee)

Comment 58

5 years ago
> 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?
(Assignee)

Comment 59

5 years ago
> 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.
(Assignee)

Comment 60

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

Bug 948201.
(Assignee)

Comment 61

5 years ago
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 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.
Attachment #8338274 - Flags: feedback?(khuey) → feedback-
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][leave open] → [MemShrink:P2]
Target Milestone: --- → mozilla29

Updated

5 years ago
Blocks: 950391
(Assignee)

Comment 63

5 years ago
(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.

Updated

5 years ago
Whiteboard: [MemShrink:P2] → [MemShrink:P2][qa-]
You need to log in before you can comment on or make changes to this bug.