Ghost window telemetry is broken

RESOLVED WORKSFORME

Status

()

Core
DOM
P1
normal
RESOLVED WORKSFORME
5 months ago
3 months ago

People

(Reporter: mccr8, Assigned: erahm)

Tracking

(Blocks: 1 bug, {regression})

Trunk
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56? fixed, firefox57 fix-optional)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
Since 7/12, ghost windows have been reported as 0, which is bad.
(Reporter)

Comment 1

5 months ago
This is bad because we're trying to track no regressions in this metric for 57.
status-firefox56: --- → affected
tracking-firefox56: --- → ?
Flags: needinfo?(erahm)
Keywords: regression
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
Version: unspecified → Trunk
(Temporarily giving to mccr8 as he's the most connected to this issue right now)
Assignee: nobody → continuation
Priority: -- → P1
(Reporter)

Comment 3

5 months ago
Eric said he'd look at it.
Assignee: continuation → erahm
(Reporter)

Updated

5 months ago
Whiteboard: [MemShrink]
Flags: needinfo?(erahm)
Whiteboard: [MemShrink] → [MemShrink:P1]
We know that ghost window reporting still works when generating memory reports so at least that part isn't broken. That implies that the logic to periodically calculate ghost windows triggered by windows closing [1] or cycle collection finishing [2] is somehow broken. Before we just always calculated the value when telemetry asked for it, but now we depend on a cached value from the periodic calcuation.

Andrew pointed out a few spikes on telemetry [1] where we see a drop on 7/1 (corresponding with bug 1375281 which switched to request idle callback) and rise on 7/6 (corresponding with bug 1377767 which fixed us using an old api for idle callback), so those are probably red herrings. It's pretty clear that bug 1376038 is the regression.

[1] http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/dom/base/nsWindowMemoryReporter.cpp#611-634
[2] http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/dom/base/nsWindowMemoryReporter.cpp#636-648
[3] https://mzl.la/2w1fU1E
Created attachment 8891105 [details] [diff] [review]
Always update cached ghost window count

Currently the cached count is only updated if a table was passed in to keep
track of the window IDs. This changes the behavior to always update the count
regardless of whether a table is passed in.

MozReview-Commit-ID: EkfzLemVJyV
Attachment #8891105 - Flags: review?(continuation)
(Reporter)

Updated

5 months ago
Attachment #8891105 - Flags: review?(continuation) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f400fc23a72fab985d3baa08b5cc22d2190326
Bug 1384337 - Always update cached ghost window count. r=mccr8

Comment 7

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c8f400fc23a7
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Reporter)

Comment 8

4 months ago
I'm seeing ghost windows in telemetry again. Thanks, Eric.
Status: RESOLVED → VERIFIED
Backed out from mozilla-central along with bug 1376038 to help with the investigation of bug 1388111.

https://hg.mozilla.org/integration/mozilla-inbound/rev/69b75c23e0301ffda2e2bb5bb40d4e803ac6e242

Note that the status is a little screwy here because Beta56 still has the patches in question.
Status: VERIFIED → REOPENED
status-firefox57: --- → affected
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---

Comment 10

4 months ago
backoutbugherder
also backedout from m-c
https://hg.mozilla.org/mozilla-central/rev/69b75c23e030

Comment 11

4 months ago
Hey Eric, what's the plan here, reland?
status-firefox57: affected → fix-optional
Flags: needinfo?(erahm)
(Reporter)

Comment 12

4 months ago
This bug is "fixed" in the sense that ghost window telemetry is working. If erahm relands, I assume he'll just roll that into his patch for the other bug.
Status: REOPENED → RESOLVED
Last Resolved: 5 months ago4 months ago
Resolution: --- → FIXED
(Reporter)

Updated

4 months ago
Resolution: FIXED → WORKSFORME
Yeah I can look into relanding tomorrow. Keeping ni for now.
Okay as :mccr8 noted I'll just roll this up into bug 1376038.
Flags: needinfo?(erahm)
You need to log in before you can comment on or make changes to this bug.