Last Comment Bug 737857 - Report number of ghost windows in telemetry
: Report number of ghost windows in telemetry
Status: RESOLVED FIXED
[MemShrink:P1]
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 738011 738624
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-21 07:25 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-04-03 10:56 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Report number of ghost windows in telemetry. (12.21 KB, patch)
2012-03-23 12:42 PDT, Justin Lebar (not reading bugmail)
n.nethercote: review+
Details | Diff | Review
Part 2: Rename the two ghost memory reporters. (7.48 KB, patch)
2012-03-23 12:43 PDT, Justin Lebar (not reading bugmail)
n.nethercote: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-03-21 07:25:29 PDT
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 [:njn] 2012-03-21 17:11:18 PDT
It might be nice to mark them in about:memory too.
Comment 2 Justin Lebar (not reading bugmail) 2012-03-21 19:27:30 PDT
(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) 2012-03-23 12:42:48 PDT
Created attachment 608824 [details] [diff] [review]
Part 1: Report number of ghost windows in telemetry.
Comment 4 Justin Lebar (not reading bugmail) 2012-03-23 12:43:08 PDT
Created attachment 608825 [details] [diff] [review]
Part 2: Rename the two ghost memory reporters.

These names were becoming unweildy.
Comment 5 Nicholas Nethercote [:njn] 2012-03-25 22:48:50 PDT
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 [:njn] 2012-03-25 22:49:53 PDT
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] 2012-03-31 20:00:47 PDT
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) 2012-04-01 00:01:22 PDT
Thanks, Ed.  Not sure what went wrong here, yet.
Comment 9 Justin Lebar (not reading bugmail) 2012-04-01 17:25:10 PDT
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) 2012-04-02 12:53:52 PDT
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 2012-04-02 19:04:24 PDT
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/jlebar@mozilla.com-11eff1681672
Comment 12 Justin Lebar (not reading bugmail) 2012-04-02 19:50:19 PDT
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f8fa7e076c
Comment 13 Matt Brubeck (:mbrubeck) 2012-04-03 10:56:21 PDT
https://hg.mozilla.org/mozilla-central/rev/d2f8fa7e076c

Note You need to log in before you can comment on or make changes to this bug.