Closed Bug 1364502 Opened 3 years ago Closed 3 years ago

GHOST_WINDOWS needs to become "release criteria" quality

Categories

(Core :: General, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: chutten, Assigned: chutten)

Details

(Whiteboard: [measurement:client])

Attachments

(1 file)

A quantum release criterion is to have no ghost windows. :bsmedberg's proposal is that this is to be measured using GHOST_WINDOWS.

To do that we need to at least make GHOST_WINDOWS opt-out. We also need to verify that the MemoryReporterManager on release-channel builds report ghost windows (pretty sure it does, but it pays to be thorough).

Note: GHOST_WINDOWS is reported via the same mechanism as other Memory Reporter metrics: occasionally, TelemetrySession decides to grab all of the memory information it can think of to ask. It does this multiple times a session to figure out some picture of typical values within sessions. Probably not relevant for implementation, but might strike people unawares if they haven't run across this before.
Comment on attachment 8869572 [details]
bug 1364502 - Allow GHOST_WINDOWS to be recorded opt-out data-r=bsmedberg

https://reviewboard.mozilla.org/r/141154/#review144714

Yeah, I'm pretty sure the ghost windows should work on release. I've certainly seen users reporting ghost windows in about:memory on release.
Attachment #8869572 - Flags: review?(continuation) → review+
Comment on attachment 8869572 [details]
bug 1364502 - Allow GHOST_WINDOWS to be recorded opt-out data-r=bsmedberg

data-review-r?
Attachment #8869572 - Flags: feedback?(benjamin)
Comment on attachment 8869572 [details]
bug 1364502 - Allow GHOST_WINDOWS to be recorded opt-out data-r=bsmedberg

https://reviewboard.mozilla.org/r/141154/#review146214

The only concern is that the code which gathers this telemetry isn't gated somewhere on an opt-in check. So we should test/verify this explicitly.

data-r=me
Attachment #8869572 - Flags: review+
Oh Right! _That_ was the reason I was looking into writing a scalar directly into the ghost windows detector. GHOST_WINDOWS _is_ presently opt-in gated within TelemetrySession: http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm#1099

So we have a choice:

1) Make GHOST_WINDOWS collect opt-out by moving the canRecordExtended check down a couple-dozen lines and the GHOST_WINDOWS accumulation up a dozen lines or so.
2) Accumulate to an opt-out scalar here: http://searchfox.org/mozilla-central/source/dom/base/nsWindowMemoryReporter.cpp#795

I'm very slightly leaning towards #1, so that's where I'll manoeurvre the patch for now.
Priority: P2 → P1
Attachment #8869572 - Flags: feedback?(benjamin)
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20027d10c52c
Allow GHOST_WINDOWS to be recorded opt-out data-r=bsmedberg r=bsmedberg,mccr8
https://hg.mozilla.org/mozilla-central/rev/20027d10c52c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.