Closed
Bug 737857
Opened 12 years ago
Closed 12 years ago
Report number of ghost windows in telemetry
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
(Whiteboard: [MemShrink:P1])
Attachments
(2 files)
12.21 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
7.48 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee: nobody → justin.lebar+bug
Comment 1•12 years ago
|
||
It might be nice to mark them in about:memory too.
Assignee | ||
Comment 2•12 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 | ||
Comment 3•12 years ago
|
||
Attachment #608824 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 4•12 years ago
|
||
These names were becoming unweildy.
Attachment #608825 -
Flags: review?(n.nethercote)
Comment 5•12 years ago
|
||
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 6•12 years ago
|
||
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+
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Comment 7•12 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•12 years ago
|
||
Thanks, Ed. Not sure what went wrong here, yet.
Assignee | ||
Comment 9•12 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.
Comment 10•12 years ago
|
||
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•12 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•12 years ago
|
||
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f8fa7e076c
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d2f8fa7e076c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in
before you can comment on or make changes to this bug.
Description
•