1364502 - GHOST_WINDOWS needs to become "release criteria" quality

Status: RESOLVED FIXED

(Core :: General, enhancement, P1)

Description

[Chris H-C :chutten]

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 1

[Chris H-C :chutten]

Attachment: bug 1364502 - Allow GHOST_WINDOWS to be recorded opt-out data-r=bsmedberg

GHOST_WINDOWS will be used to satisfy Quantum release criteria. As such, we
need release-channel collection.

Near as I can tell the memory manager works just as well in Release as in
pre-release, so this should be all that's necessary.

Review commit: https://reviewboard.mozilla.org/r/141154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141154/

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Chris H-C :chutten]

Comment on attachment 8869572 [details]
bug 1364502 - Allow GHOST_WINDOWS to be recorded opt-out data-r=bsmedberg

data-review-r?

Comment 4

[Benjamin Smedberg]

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

Comment 5

[Chris H-C :chutten]

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.

Comment 6

[Chris H-C :chutten]

Comment on attachment 8869572 [details]
bug 1364502 - Allow GHOST_WINDOWS to be recorded opt-out data-r=bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141154/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20027d10c52c
Allow GHOST_WINDOWS to be recorded opt-out data-r=bsmedberg r=bsmedberg,mccr8

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/20027d10c52c