810196 - Allow earlier registration of memory reporters

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

Sometimes the natural place to register a memory reporter is before 
nsMemoryReporterManager has been initialized.  I've managed to work around 
this limitation on two prior occasions, but for bug 810142 it's harder.  Instead, I just want to allow earlier registration.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Allow earlier registration of memory reporters.

This patch allows earlier memory registration by making the reporters lists in
nsMemoryReporterManager static.  This works because there's only ever one 
nsMemoryReporterManager.

There's one wrinkle I haven't worked out, which is marked with an "njn" 
comment -- I have to call
|do_GetService("@mozilla.org/memory-reporter-manager;1")| at some point
otherwise I get a crash in B2G child processes (one which I haven't been able
to catch in a debugger, and so I don't know exactly how it manifests.)

ÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂâÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂ

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Register the category manager's memory reporter earlier.

This patch removes the work-around for the category manager.

Comment 3

[Justin Lebar (not reading bugmail)]

I strongly suspect this will turn the tree quite orange, as it will claim you're leaking mutexes (and nsCOMArrays).

The canonical way of doing this sort of thing these days is with ClearOnShutdown and Static{Auto,Ref}Ptr.

Also, you don't need to check that |new| doesn't return null; operator new is infallible in Gecko.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Register the atom table's memory reporter earlier, and split it in two.

This patch:

- Moves the "explicit/atom-table" reporter into nsAtomTable.cpp.
    
- Splits it into "explicit/atom-table/{static,non-static}".

  Ideally I would have done this in two patches, but that would have required
  going against the grain of the code... with two tables, it's more natural
  have two reporters and register/unregister them when their corresponding
  tables are initialized/destroyed.

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 680498 [details] [diff] [review]
(part 3) - Register the atom table's memory reporter earlier, and split it in two.

>+static nsCOMPtr<nsIMemoryReporter> gStaticReporter = nullptr;
>+static nsCOMPtr<nsIMemoryReporter> gNonStaticReporter = nullptr;

static nsCOMPtr is a no-no; it adds a static constructor, which slows down startup.

We have StaticRefPtr for this purpose, although that may require you to use a
concrete class instead of nsIMemoryReporter.

It's not clear to me why you need to keep a global ref to these objects,
though.  Why do we care about unregistering the reporters?

Comment 6

[Nicholas Nethercote [inactive]]

> The canonical way of doing this sort of thing these days is with
> ClearOnShutdown and Static{Auto,Ref}Ptr.

I just tried that, but it's causing problems because it seems that KillClearOnShutdown() is called early enough that some memory reporters get unregistered afterwards, by which time |sMutex| has been freed.  Oh dear.

In an earlier version of the patch, |sMutex| and |sReporters| and |sMultiReporters| weren't pointers.  But I was getting some runtime warnings about static constructors, so I changed them to pointers.  Things were certainly easier when they weren't pointers...

Comment 7

[Justin Lebar (not reading bugmail)]

Yeah, I think they have to be pointers.  :(

Can you just ignore calls to unregister which happen when the pointers are null?

Comment 8

[Nicholas Nethercote [inactive]]

> Can you just ignore calls to unregister which happen when the pointers are
> null?

Maybe, but it feels pretty dirty.  Possibly racy, too, since I think reporters can be unregistered from a non-main-thread.

What a hassle this is turning out to be.

Comment 9

[Justin Lebar (not reading bugmail)]

> Possibly racy, too, since I think reporters can be unregistered from a non-main-thread.

KillClearOnShutdown() runs after all non-main-thread XPCOM threads have shut down.  That may be sufficient to guarantee sanity here.

We suck at static data.  :(

Comment 10

[Nicholas Nethercote [inactive]]

Bug 810142 was WONTFIXed, so this bug can be too.