917489 - Fix an observer leak in about:memory.

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

File this under "embarrassing".

about:memory registers an observer for "child-memory-reporter-update" events,
which are received when child processes provide memory reports.  The observer
is removed when the page is unloaded.

This is fine, except that about:memory registers a *new* observer every time
you hit the "measure" button, forgetting about any old observers in the
process.  So if you hit "measure" more than once, you leak an observer and the
page contents are never freed.

Sample output obtained after doing this, closing the tab, and then opening a
new about:memory tab:

│   ├───1,855,824 B (00.74%) -- top(none)/detached
│   │   ├──1,740,160 B (00.70%) -- window([system])
│   │   │  ├──1,207,776 B (00.48%) ++ js-compartment([System Principal],
about:memory)

In case you're wondering, I discovered this when working on bug 904321.
test_aboutMemory2.xul loads about:memory and hits "measure" twice, thus
triggering the leak.  And then test_memoryReporters2.xul (the new test added by
bug 904321) runs, and it has child processes that send the
"child-memory-reporter-update" message.  Even though test_memoryReporters2.xul
doesn't even load about:memory, there were about:memory actions happening
because of the leak.  Those actions were invisible, because the visible page
was long gone, but fortunately we apparently have a bug in some of our
windows-only gfx reporters ("gpu-shared" and "gpu-dedicated", specifically) and
they produce negative amounts and the leaked about:memory page was hitting
assertions as a result.  Wow.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Fix an observer leak in about:memory.

The fix is pretty simple.  about:memory now just registers the observer at the
start of the script, and removes it when onUnload is run.  (Does onUnload
always run?  I sure hope so.)

If you think the existing code looks more complex than it needed to be, you're
right -- it's that way because the extra complexity was required when
about:compartments existed, and I missed the opportunity to simplify when I
removed about:compartments.

Comment 2

[Hugh Nougher [:Hughman]]

Ah. Could this be the reason why hang pause gets longer the more times I "measure" memory with a child process open?

Comment 3

[Nicholas Nethercote [inactive]]

> Ah. Could this be the reason why hang pause gets longer the more times I
> "measure" memory with a child process open?

I don't think so.  A single observer shouldn't be much memory.  The main effect of this leak occurs when you close about:memory.

Comment 4

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb1ea0648a2

Thanks for the super-fast review!

Comment 5

[Hugh Nougher [:Hughman]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> > Ah. Could this be the reason why hang pause gets longer the more times I
> > "measure" memory with a child process open?
> 
> I don't think so.  A single observer shouldn't be much memory.  The main
> effect of this leak occurs when you close about:memory.

So it would not result in the child process sending its reports though multiple times? If not I'll have to continue trying to get a nice testcase for my problem.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6bb1ea0648a2

Comment 7

[Nicholas Nethercote [inactive]]

> So it would not result in the child process sending its reports though
> multiple times? If not I'll have to continue trying to get a nice testcase
> for my problem.

Thinking some more... it might.  If we end up registering N observers, it might be the case that about:memory regenerates N times per child process.  Can you try a Nightly that contains a fix and see if the increasing delay has gone?

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 806205 [details] [diff] [review]
Fix an observer leak in about:memory.

[Approval Request Comment]

Bug caused by (feature/regressing bug #):  bug 856917, which shipped in FF23.

User impact if declined: about:memory causes memory leaks :(  It's not the end of the world, but it's a simple fix and we're early in the dev cycle, so it seems worth backporting to Aurora.  (Backporting to Beta would be harder, because bug 911641 isn't on Beta.)

Testing completed (on m-c, etc.): been on m-c for a couple of days.

Risk to taking this patch (and alternatives if risky): low risk.

String or IDL/UUID changes made by this patch: none.

Comment 9

[Hugh Nougher [:Hughman]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> Thinking some more... it might.  If we end up registering N observers, it
> might be the case that about:memory regenerates N times per child process. 
> Can you try a Nightly that contains a fix and see if the increasing delay
> has gone?

I have done various testing including the most easily replicated testcase of Bug 907804 (which had a nasty result of longer hang times) and have not seen the growing hang time. When the thumbnail process is around the measure takes ~7sec every time now instead of quickly reaching 60sec and beyond.

Comment 10

[Nicholas Nethercote [inactive]]

Thanks, Hugh!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e35292fe5eaf