950391 - Cannot register and later unregister Javascript nsIMemoryReporter

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nils Maier [:nmaier]]

Bug 936964 split nsIMemoryReporter registration into strong and weak reporters.
- Strong reporters will be owned by the manager and cannot be unregistered.
- Weak reporters will not be owned, and must be unregistered. The manager just holds a flat pointer (not to be confused with weak references)

Because of this change, I cannot install my memory reporter any longer, because that memory reporter is implemented in Javascript (will spit out some internal stats) in a restartless add-on.

I cannot use the strong variant, as I need to unregister the reporter when the add-on gets unloaded, which may happen multiple times throughout a session (upgrades, user disabled/enabled the add-on, and so on).

I cannot use the weak variant, as the C++-land wrapper created by XPConnect will be unrefed (to 0) and deleted as soon as nsMemoryReporterManager::RegisterWeakReporter is done, leaving behind an invalid pointer in nsMemoryReporterManager::mWeakReporters which thus causes subsequent crashes
https://crash-stats.mozilla.com/report/index/5947f12d-bad9-4205-bc7e-d63ee2131214

Not sure if this counts as a regression or just a feature request, but I need a way to register and then later *unregister* a strong reporter.

Comment 1

[Nicholas Nethercote [inactive]]

Yeah, I'd call it a regression.  Sorry.  It will be easy to fix, fortunately.  I'll take a look tomorrow.

Comment 2

[Nils Maier [:nmaier]]

Attachment: Add nsIMemoryReporterManager.unregisterStrongReporter.

Sorry, I forgot to mention I already had a patch, but my build was still running over night.

This adds unregisterStrongReporter, and also adds a check against JS Components when registering weak reporters. Regarding the latter, users could come up with hacks to create a peristent WrappedJS, but this is error-prone so I figured we don't want to support this and just instruct them to always register their JS components as strong reporters.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8347722 [details] [diff] [review]
Add nsIMemoryReporterManager.unregisterStrongReporter.

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

Thanks!  Feel free to nominate for uplift to Aurora.

::: xpcom/base/nsIMemoryReporter.idl
@@ +205,5 @@
>  
>    /*
>     * Unregister the given memory reporter, which must have been registered with
> +   * registerStrongReporter().  You normally don't need to unregister your
> +   * strong reporters, as nsIMemoryReporterManager will take care of that.

Nit: change "of that" to "of that at shutdown"?

Comment 4

[Nils Maier [:nmaier]]

Attachment: Add nsIMemoryReporterManager.unregisterStrongReporter.

Well, I think it is a good idea to actually have tests for this.
Since I couldn't find general JS reporter unit tests, I made the new tests not only test registration, but also the basic (generating/consuming reports).

I also started a try build: https://tbpl.mozilla.org/?tree=Try&rev=fcfb501754dd

Comment 5

[Nils Maier [:nmaier]]

Attachment: Add nsIMemoryReporterManager.unregisterStrongReporter.

Tests as before + missing nsIXPConnect.h include per failed try builds.

New try: https://tbpl.mozilla.org/?tree=Try&rev=7106c8846e14

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8348125 [details] [diff] [review]
Add nsIMemoryReporterManager.unregisterStrongReporter.

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

The test is great!  But can you move its code into toolkit/components/aboutmemory/tests/test_memoryReporters.xul?  Even though this is JS-only code, I run these tests a lot (both locally and on try), and having all the memory reporter and about:memory tests in one place is very convenient.

::: xpcom/tests/unit/test_memoryreporters.js
@@ +22,5 @@
> +  // the |amount| will be checked instead.
> +  tests: {
> +    "test-memory-reporter-bytes1": {
> +      units: Ci.nsIMemoryReporter.UNITS_BYTES,
> +      amount: function() 0

That syntax works?  I thought you could only do that with arrow functions.  Huh.

Comment 7

[Nils Maier [:nmaier]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Comment on attachment 8348125 [details] [diff] [review]
> Add nsIMemoryReporterManager.unregisterStrongReporter.
> 
> Review of attachment 8348125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The test is great!  But can you move its code into
> toolkit/components/aboutmemory/tests/test_memoryReporters.xul?  Even though
> this is JS-only code, I run these tests a lot (both locally and on try), and
> having all the memory reporter and about:memory tests in one place is very
> convenient.

Sure, even though this has not much to do with about:memory per se... Let me find some time (likely this afternoon or evening, so in a couple of hours).

> ::: xpcom/tests/unit/test_memoryreporters.js
> @@ +22,5 @@
> > +  // the |amount| will be checked instead.
> > +  tests: {
> > +    "test-memory-reporter-bytes1": {
> > +      units: Ci.nsIMemoryReporter.UNITS_BYTES,
> > +      amount: function() 0
> 
> That syntax works?  I thought you could only do that with arrow functions. 
> Huh.

Not a widely advertised fact I guess, since our engine is the only one to support it, but it works since Firefox 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.8

Comment 8

[Nils Maier [:nmaier]]

Attachment: Add nsIMemoryReporterManager.unregisterStrongReporter.

Per your suggestion, I moved the tests over to mochitest-chrome.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8348910 [details] [diff] [review]
Add nsIMemoryReporterManager.unregisterStrongReporter.

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

Looks great!  I just have a few nits below.

::: toolkit/components/aboutmemory/tests/test_memoryReporters.xul
@@ +211,5 @@
> +  // The test memory reporter, testing the various report units.
> +  // Also acts as a report collector, verifying the reported values match the
> +  // expected ones after passing through XPConnect / nsMemoryReporterManager
> +  // and back.
> +  function MemoryReporter() {

So this is both a reporter and a reporter callback, which is non-obvious.  Could you rename it MemoryReporterAndCallback?

@@ +218,5 @@
> +  MemoryReporter.prototype = {
> +    // The test reports.
> +    // Each test key corresponds to the path of the report.  |amount| is a
> +    // function called when generating the report.  |expected| is a function to be
> +    // tested when receiving a report during collection.  If |expected is omitted

Nit: missing '|' after "expected".

@@ +223,5 @@
> +    // the |amount| will be checked instead.
> +    tests: {
> +      "test-memory-reporter-bytes1": {
> +        units: BYTES,
> +        amount: function() 0

Can you make these arrow functions?  Using egregious non-standard syntax feels like a bad idea...

@@ +227,5 @@
> +        amount: function() 0
> +      },
> +      "test-memory-reporter-bytes2": {
> +        units: BYTES,
> +        amount: function() (1<<30) * 8 // awkward way to say 8G in JS

I'd write "8 * 1024 * 1024 * 1024" because it doesn't need a comment and so ends up shorter :)

@@ +288,5 @@
> +  };
> +
> +  // General memory reporter + registerStrongReporter tests.
> +  function test_register_strong() {
> +    let reporter = new MemoryReporter();

And here |reporter| is less confusing if it's called |reporterAndCallback|.

@@ +299,5 @@
> +
> +    // Unregistration works.
> +    mgr.unregisterStrongReporter(reporter);
> +
> +    // The reporter was unregistered, hence there shouldn't see any reports from

Nit: change "hence... reporter" to "therefore we shouldn't see any reports from it"?

@@ +312,5 @@
> +  // re-registered.
> +  test_register_strong();
> +
> +
> +  // Check that you cannot register weak reporters.

s/weak reporters/weak reporters from JS code/

Comment 10

[Nils Maier [:nmaier]]

Attachment: Add nsIMemoryReporterManager.unregisterStrongReporter. r=njn

Carrying over r=njn.
Previous green try (with xpcshell tests still):
https://tbpl.mozilla.org/?tree=Try&rev=7106c8846e14

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44ba76de0954

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44ba76de0954