Deal with accumulations of INPUT_EVENT_RESPONSE_MS across sleep/wake

RESOLVED FIXED in Firefox 54

Status

()

Firefox
General
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: chutten, Assigned: chutten)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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

a year ago
Yes, #1.
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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?
Attachment #8859620 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

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

Comment 6

a year ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e9b46dbf4b5
Drop INPUT_EVENT_RESPONSE_MS measurements across sleep/wake r=smaug
Well, are we getting weird looking data?
(Assignee)

Comment 8

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e9b46dbf4b5
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 10

a year ago
You should probably backport this to beta since we are using this telemetry to evaluate chrome input event latency.
(Assignee)

Comment 11

a year ago
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

a year ago
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

a year ago
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.

Updated

a year ago
See Also: → bug 1360361
(Assignee)

Comment 14

a year ago
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
Attachment #8859620 - Flags: approval-mozilla-beta?
Attachment #8859620 - Flags: approval-mozilla-aurora?
Assignee: nobody → chutten
status-firefox54: --- → affected
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.
Attachment #8859620 - Flags: approval-mozilla-beta?
Attachment #8859620 - Flags: approval-mozilla-beta+
Attachment #8859620 - Flags: approval-mozilla-aurora?
Attachment #8859620 - Flags: approval-mozilla-aurora-

Comment 16

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/e1952afe7b5e
status-firefox54: affected → fixed
(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.
Flags: qe-verify-
Out of curiosity, why dont we return NS_OK in this case?
(Assignee)

Updated

8 months ago
Blocks: 1390263
(Assignee)

Comment 19

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

Thanks for the catch!
You need to log in before you can comment on or make changes to this bug.