Closed Bug 1384337 Opened 3 years ago Closed 3 years ago

Ghost window telemetry is broken

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 ? fixed
firefox57 --- fix-optional

People

(Reporter: mccr8, Assigned: erahm)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [MemShrink:P1])

Attachments

(1 file)

Since 7/12, ghost windows have been reported as 0, which is bad.
This is bad because we're trying to track no regressions in this metric for 57.
Flags: needinfo?(erahm)
Keywords: regression
Version: unspecified → Trunk
(Temporarily giving to mccr8 as he's the most connected to this issue right now)
Assignee: nobody → continuation
Priority: -- → P1
Eric said he'd look at it.
Assignee: continuation → erahm
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
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)
Attachment #8891105 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/c8f400fc23a7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---
also backedout from m-c
https://hg.mozilla.org/mozilla-central/rev/69b75c23e030
Hey Eric, what's the plan here, reland?
Flags: needinfo?(erahm)
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
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
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)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.