1357742 - Deal with accumulations of INPUT_EVENT_RESPONSE_MS across sleep/wake

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Chris H-C :chutten]

Input events may come in before a computer goes to sleep and not be handled until after it wakes. On platforms where TimeStamp measures elapsed time (like on Windows), this can result in incorrectly-long values being accumulated into INPUT_EVENT_RESPONSE_MS.

So let's not accumulate those. They're wrong.

How to handle this:

1) Just ditch all input events created before sleep and handled after wake
2) Try to be clever and subtract the sleep duration from the handling time

I'm tempted to go for #1 since users are less likely to care about the response time to an input event that happens across a sleep/wake cycle. Also, it's easier and I expect not many events will get caught by this.

Comment 1

[Chris H-C :chutten]

For reference, sleep notifies NS_WIDGET_SLEEP_OBSERVER_TOPIC and wake notifies NS_WIDGET_WAKE_OBSERVER_TOPIC[1]

[1]: https://dxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#226

Comment 2

[Benjamin Smedberg]

Yes, #1.

Comment 3

[Chris H-C :chutten]

Attachment: bug 1357742 - Drop INPUT_EVENT_RESPONSE_MS measurements across sleep/wake

It is possible that events created before the OS goes to sleep will remain
unhandled until after the OS wakes. They will have long response times on
platforms where TimeStamp increments during sleep (like Windows) even though
they really shouldn't. (and the user likely doesn't care if they do).

So don't record those.

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859620 [details]
bug 1357742 - Drop INPUT_EVENT_RESPONSE_MS measurements across sleep/wake

https://reviewboard.mozilla.org/r/131630/#review134408

Seems like rather edge case. Does this actually cause some issues in telemetry?

Comment 5

[Chris H-C :chutten]

We have no way to know if it's causing issues until we shut down this avenue of error :)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e9b46dbf4b5
Drop INPUT_EVENT_RESPONSE_MS measurements across sleep/wake r=smaug

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Well, are we getting weird looking data?

Comment 8

[Chris H-C :chutten]

We're seeing a low mean time before failure (when by "failure" we mean "input lag of over 10s") which is making us wonder, at the very least.

Other concerns about these sorts of measures are broached on bug 1357457.

The idea is to polish these to a high sheen of certainty so they can be used as indicators of product quality.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/1e9b46dbf4b5

Comment 10

[(no longer active)]

You should probably backport this to beta since we are using this telemetry to evaluate chrome input event latency.

Comment 11

[Chris H-C :chutten]

Would it be worth keeping an eye on it for a week or so in Nightly first to see if the distribution actually changes?

Comment 12

[Benjamin Smedberg]

This change should probably either rename the histogram or at least be noted in the documentation.

Will this patch also affect the COALESCED version in the other bug?

With a _V2 histogram rename, I'd suggest uplifting immediately or along with the COALESCED bug. Without, I'd suggest doing some nightly comparisons first.

Comment 13

[(no longer active)]

I think we should have definitely renamed the histogram before landing this, but I'm late to the game...  Now that this landed, not sure, up to you.  But with or without renaming, we should validate the data before using it -- renaming just makes it unnecessary to wait before uplifting.

Comment 14

[Chris H-C :chutten]

Comment on attachment 8859620 [details]
bug 1357742 - Drop INPUT_EVENT_RESPONSE_MS measurements across sleep/wake

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: A key measurement for quantum release criteria will not be baselineable.
[Is this code covered by automated tests?]: Telemetry alerts and hindsight will notify us if something goes awry with data collection
[Has the fix been verified in Nightly?]: I have verified that data is still coming through as it used to.
[Needs manual test from QE? If yes, steps to reproduce]: Would be pretty tricky to manually test.
[List of other uplifts needed for the feature/fix]: bug 1357457
[Is the change risky?]: Nope
[Why is the change risky/not risky?]: It's small, self-contained, and has little effect on the data collected (while simultaneously greatly improving our confidence in the numbers it reports)
[String changes made/needed]: None

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8859620 [details]
bug 1357742 - Drop INPUT_EVENT_RESPONSE_MS measurements across sleep/wake

A key measurement for quantum release criteria. Beta54+. Should be in 54 beta 5.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e1952afe7b5e

Comment 17

[Andrei Vaida [:avaida]]

(In reply to Chris H-C :chutten from comment #14)
> [Is this code covered by automated tests?]: Telemetry alerts and hindsight
> will notify us if something goes awry with data collection
> [Has the fix been verified in Nightly?]: I have verified that data is still
> coming through as it used to.
> [Needs manual test from QE? If yes, steps to reproduce]: Would be pretty
> tricky to manually test.

Setting qe-verify- based on Chris' assessment on manual testing needs and the fact that this fix has telemetry coverage.

Comment 18

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Out of curiosity, why dont we return NS_OK in this case?

Comment 19

[Chris H-C :chutten]

...likely due to a mistake on my part. I've filed bug 1390263 to rectify it.

Thanks for the catch!