Report number of ghost windows in telemetry

RESOLVED FIXED in mozilla14

Status

()

Toolkit
Telemetry
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Updated

6 years ago
Depends on: 738011
It might be nice to mark them in about:memory too.
(Assignee)

Comment 2

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

Bug 738011.
(Assignee)

Updated

6 years ago
Depends on: 738624
(Assignee)

Comment 3

6 years ago
Created attachment 608824 [details] [diff] [review]
Part 1: Report number of ghost windows in telemetry.
Attachment #608824 - Flags: review?(n.nethercote)
(Assignee)

Comment 4

6 years ago
Created attachment 608825 [details] [diff] [review]
Part 2: Rename the two ghost memory reporters.

These names were becoming unweildy.
Attachment #608825 - Flags: review?(n.nethercote)
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?
Attachment #608824 - Flags: review?(n.nethercote) → review+
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?
Attachment #608825 - Flags: review?(n.nethercote) → review+
Whiteboard: [MemShrink] → [MemShrink:P1]

Comment 7

6 years ago
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
(Assignee)

Comment 8

6 years ago
Thanks, Ed.  Not sure what went wrong here, yet.
(Assignee)

Comment 9

6 years ago
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.
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

6 years ago
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
(Assignee)

Comment 12

6 years ago
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f8fa7e076c
https://hg.mozilla.org/mozilla-central/rev/d2f8fa7e076c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.