737857 - Report number of ghost windows in telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Justin Lebar (not reading bugmail)]

I propose we define a ghost window (*) as a window which:

  1) shows up in about:memory under window-objects/top(none),
  2) does not share a domain name with any window under window-objects/active, and
  3) has met criteria (1) and (2) for at least X seconds, for some reasonable X.  (X=120s should be more than enough.)

Criterion (1) is the basic definition.  A window shows up under window-objects/top(none) because its containing tab/iframe has been destroyed.

Criterion (2) limits ghosts to those windows which we think are unlikely to have a reason to be alive.  We could use origin instead of domain name, but in my session, I see that nytimes and Google open windows from a bunch of different origins.  Using domain name is conservative and limits us to the most likely-leaked windows.

Criterion (3) limits ghosts to those windows which have lived through a few GC/CCs.

A window can be a ghost for entirely legitimate reasons -- for example, a website opens a popup, closes it, and keeps a reference to the popup window.  But it is my hypothesis that the vast majority of windows satisfying the criteria above are due to bugs, either in Firefox or add-on code.

It is my hope that having a high number of ghost windows will be correlated with having a leaky add-on installed.  We could then use this information to pinpoint bad add-ons.

I may relax criterion (3), depending on how difficult it is to implement, but I think (3) as stated is the optimal criterion.

(*) "Ghost" because their corporeal being has been lost.  I wanted to reference the undead, since they're related to zombie compartments, but I didn't want to call them "zombie windows" because they can happen legitimately, unlike zombie compartments.  But suggestions for alternative names are welcome.

Comment 1

[Nicholas Nethercote [inactive]]

It might be nice to mark them in about:memory too.

Comment 2

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> It might be nice to mark them in about:memory too.

Bug 738011.

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Report number of ghost windows in telemetry.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Rename the two ghost memory reporters.

These names were becoming unweildy.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 608824 [details] [diff] [review]
Part 1: Report number of ghost windows in telemetry.

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

::: dom/base/nsWindowMemoryReporter.cpp
@@ +664,5 @@
> +nsGhostWindowMemoryReporter::GetDescription(nsACString& aDesc)
> +{
> +  nsPrintfCString str(1024,
> +"The number of ghost windows present (so the number of nodes underneath \
> +explicit/window-objects/top(none)/ghost).  A ghost window is not shown in any \

It might be worth mentioning that the "ghost-windows" may not match the number of nodes in that sub-tree if GC happens at an inopportune time?  I did that for "user-compartments" and "system-compartments".

I find the "so the" a bit unclear.  Maybe "i.e. the" instead?

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 608825 [details] [diff] [review]
Part 2: Rename the two ghost memory reporters.

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

Much better!  Does it make sense to fold this into the other patch before landing?

Comment 7

[Ed Morley [:emorley]]

Push backed out for mochitest-oth orange:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9e4d09efa335

https://tbpl.mozilla.org/php/getParsedLog.php?id=10542171&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=10540830&tree=Mozilla-Inbound
...and a few more.

(Had trouble loading the logs due to size and I've got to be up in a few hours, so don't have anything more specific right now, sorry.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/abe59de81f87

Comment 8

[Justin Lebar (not reading bugmail)]

Thanks, Ed.  Not sure what went wrong here, yet.

Comment 9

[Justin Lebar (not reading bugmail)]

Heh, maybe this was dying because of a bajillion

WARNING: NS_ENSURE_TRUE(aURI) failed: file e:/builds/moz2_slave/m-in-w32-dbg/build/netwerk/dns/nsEffectiveTLDService.cpp, line 125

in the logs.

Comment 10

[Matt Brubeck (:mbrubeck)]

Sorry, this was backed out again on inbound because of WinXP debug reftest logs overflowing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61aa02b5a7a1

Comment 11

[Mozilla RelEng Bot]

Try run for 11eff1681672 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=11eff1681672
Results (out of 217 total builds):
    success: 182
    warnings: 34
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-11eff1681672

Comment 12

[Justin Lebar (not reading bugmail)]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f8fa7e076c

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d2f8fa7e076c