1430173 - Set all timers' rounding to 2ms for 52.7

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Tom Ritter [:tjr]]

Right now we don't know when we're going to turn on all-timer rounding in 58 or what we're going to set it to. Until we do, it doesn't seem like a good idea to set 52.6 to a value we're just guessing at or to turn on 20us for all timers without that being tested.

(Turning all timers to 20 us is probably safe, but we don't know with absolute certainty - for instance Bug 1429764 just popped up.)

We can hardcode all the timers in 52.7, or in a 52.6.1.  (Or maybe even in 52.6 if we figure it out fast enough.)  This bug is to figure out what value we want to set the timers to and what version we'll land it in ESR.

Comment 1

[Tom Ritter [:tjr]]

Per Bug 1435296 - I think the answer to this is 2ms in 52.7 - although we don't know when the 2ms bump will land in Release. So if we ship it in a dot release for 58, should we ship a 52.6.x? Or maybe just as a ridealong.

Comment 2

[Tom Ritter [:tjr]]

I need to double check these, but making a list:

Not implemented in 52:
- performance.TimeOrigin
- Performance Navigation timing 
- AsyncOpenHighRes
- WorkerStartHighRes
- SecureConnectionStartHighRes
- TimeToNonBlankPaint

We don't do anything for CSS Animations because they're really tricky and we don't even know how to do them correctly in -central so I'll never get it correct in -esr.

I'm going to need to backport a whole bunch of test stuff. Tests to make sure what I did works; and changes to tests that are now broken.

Comment 3

[Tom Ritter [:tjr]]

Hey Ryan, we should talk about this tomorrow. It seems like I can't run the full testsuite in TC because all the TaskCluster configuration stuff is way out of date. It thinks buildbot is still around and going to make binaries to run tests on I think.

Here's a try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=4778916458b22213b2d483b5a965835133d401cf

Comment 4

[Tom Ritter [:tjr]]

Attachment: Bug 1430173 Reduce the precision of all explicit clocks to 2ms

Backport to ESR where we don't have the ResistFingerprinting component.

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

Comment 5

[Tom Ritter [:tjr]]

Attachment: Bug 1430173 Add Timer Rounding tests backported from -central to -esr

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

Comment 6

[Tom Ritter [:tjr]]

Attachment: Bug 1430173 Update tests for 2ms timers

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

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 8952620 [details]
Bug 1430173 Update tests for 2ms timers

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

ESR52 still does use buildbot for a lot of its jobs. Add "--buildbot" to your syntax (it's a checkbox under "Additional test options" on the right side).

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 8952620 [details]
Bug 1430173 Update tests for 2ms timers

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

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 8952620 [details]
Bug 1430173 Update tests for 2ms timers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221860/diff/3-4/

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8952618 [details]
Bug 1430173 Reduce the precision of all explicit clocks to 2ms

https://reviewboard.mozilla.org/r/221856/#review229216

There is a lot of duplicated code here. I think it would be better if we introduce new method such us:

namespace mozilla {
double
ReduceMsTimeValue(double value) {
  static double maxResolutionMs = 2;
  return fllor(value / maxResolutionMs) * maxResolutionMs;
}

double
ReduceSTimeValue(double value) {
  static double maxResolutionS = 0.02;
  return fllor(value / maxResolutionS) * maxResolutionS;
}

double
ReduceUSTimeValue(double value) {
  static double maxResolutionUs = 2000;
  return fllor(value / maxResolutionUs) * maxResolutionUs;
}
} // mozilla

and then we use these functions everywhere in our code base. This will make easier to change the 2/0.02/2000 values if needed.

Probably you can create a total new header file for these functions.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8952619 [details]
Bug 1430173 Add Timer Rounding tests backported from -central to -esr

https://reviewboard.mozilla.org/r/221858/#review229226

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8952620 [details]
Bug 1430173 Update tests for 2ms timers

https://reviewboard.mozilla.org/r/221860/#review229228

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 8952618 [details]
Bug 1430173 Reduce the precision of all explicit clocks to 2ms

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

Comment 15

[Tom Ritter [:tjr]]

Comment on attachment 8952619 [details]
Bug 1430173 Add Timer Rounding tests backported from -central to -esr

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

Comment 16

[Tom Ritter [:tjr]]

Comment on attachment 8952620 [details]
Bug 1430173 Update tests for 2ms timers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221860/diff/4-5/

Comment 17

[Tom Ritter [:tjr]]

Attachment: Bug 1430173 Add qualifiers to BorrowedAttrInfo

It's not actually clear to me why these are needed after I added nsTimerClamping.h/cpp,
but they are, and an error is produced (both locally and on try) without them.

src/dom/base/nsXHTMLContentSerializer.cpp(309): error C2065: 'BorrowedAttrInfo': undeclared identifier

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

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8952618 [details]
Bug 1430173 Reduce the precision of all explicit clocks to 2ms

https://reviewboard.mozilla.org/r/221856/#review229368

::: dom/base/nsTimerClamping.h:8
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef nsTimerClamping_h___
> +#define nsTimerClamping_h___

Thanks for doing this.
Can I ask you to rename it to
TimerClamping.h
and put it in a mozilla namespace?

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8954156 [details]
Bug 1430173 Add qualifiers to BorrowedAttrInfo

https://reviewboard.mozilla.org/r/223316/#review229370

It's needed because we compile unified cpp files, merging included headers. Adding a new file, you are probably moving nsXHTMLContentSerializer.cpp in the next unified block where 'using mozilla::dom' is missing.

Comment 20

[Tom Ritter [:tjr]]

Comment on attachment 8952618 [details]
Bug 1430173 Reduce the precision of all explicit clocks to 2ms

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

Comment 21

[Tom Ritter [:tjr]]

Comment on attachment 8952619 [details]
Bug 1430173 Add Timer Rounding tests backported from -central to -esr

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

Comment 22

[Tom Ritter [:tjr]]

Comment on attachment 8952620 [details]
Bug 1430173 Update tests for 2ms timers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221860/diff/5-6/

Comment 23

[Tom Ritter [:tjr]]

Comment on attachment 8954156 [details]
Bug 1430173 Add qualifiers to BorrowedAttrInfo

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

Comment 24

[Tom Ritter [:tjr]]

Comment on attachment 8952618 [details]
Bug 1430173 Reduce the precision of all explicit clocks to 2ms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221856/diff/3-4/

Comment 25

[Tom Ritter [:tjr]]

Comment on attachment 8952619 [details]
Bug 1430173 Add Timer Rounding tests backported from -central to -esr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221858/diff/3-4/

Comment 26

[Tom Ritter [:tjr]]

Comment on attachment 8952620 [details]
Bug 1430173 Update tests for 2ms timers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221860/diff/6-7/

Comment 27

[Tom Ritter [:tjr]]

Comment on attachment 8954156 [details]
Bug 1430173 Add qualifiers to BorrowedAttrInfo

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

Comment 28

[Tom Ritter [:tjr]]

I think this is good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2e53af0faf613f324a6f452a22a52de3874338d

Comment 29

[Tom Ritter [:tjr]]

Comment on attachment 8952618 [details]
Bug 1430173 Reduce the precision of all explicit clocks to 2ms

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Improves security mitigations for Spectre

Fix Landed on Version: 58

Risk to taking this patch (and alternatives if risky): Web Breakage. But we have let this run in Beta for a while (and Nightly) and performed manual QA testing.

String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8952618 [details]
Bug 1430173 Reduce the precision of all explicit clocks to 2ms

Spectre mitigation needed for parity with release Firefox. Approved for 52.7.0.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/9124c3972e2b
https://hg.mozilla.org/releases/mozilla-esr52/rev/e5430577798d
https://hg.mozilla.org/releases/mozilla-esr52/rev/7ba05ca85095
https://hg.mozilla.org/releases/mozilla-esr52/rev/9f6dc031be51

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Looks like there's another media test that needs some adjusting:
https://treeherder.mozilla.org/logviewer.html#?job_id=164983416&repo=mozilla-esr52
https://treeherder.mozilla.org/logviewer.html#?job_id=164983429&repo=mozilla-esr52
https://treeherder.mozilla.org/logviewer.html#?job_id=164985947&repo=mozilla-esr52
https://treeherder.mozilla.org/logviewer.html#?job_id=164968193&repo=mozilla-esr52

Bug 1373627 appears to have gone permafail too:
https://treeherder.mozilla.org/logviewer.html#?job_id=164960210&repo=mozilla-esr52

Comment 33

[Tom Ritter [:tjr]]

Filed Bug 1442127