Closed Bug 1430173 Opened 6 years ago Closed 6 years ago

Set all timers' rounding to 2ms for 52.7

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox-esr52 59+ fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

(Keywords: sec-want, Whiteboard: [adv-esr52.7-])

Attachments

(4 files)

Bug 1430173 Reduce the precision of all explicit clocks to 2ms
59 bytes, text/x-review-board-request
baku
: review+
RyanVM
: approval-mozilla-esr52+
Details
Bug 1430173 Add Timer Rounding tests backported from -central to -esr
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1430173 Update tests for 2ms timers
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1430173 Add qualifiers to BorrowedAttrInfo
59 bytes, text/x-review-board-request
baku
: review+
Details 
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.
Keywords: sec-want
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.
See Also: → 1435296
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.
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
Flags: needinfo?(ryanvm)
Summary: Set all timers' rounding to <something> for 52.something → Set all timers' rounding to 2ms for 52.7
Group: dom-core-security
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).
Flags: needinfo?(ryanvm)
Priority: -- → P2
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.
Attachment #8952618 - Flags: review?(amarchesini) → review-
Comment on attachment 8952619 [details]
Bug 1430173 Add Timer Rounding tests backported from -central to -esr

https://reviewboard.mozilla.org/r/221858/#review229226
Attachment #8952619 - Flags: review?(amarchesini) → review+
Comment on attachment 8952620 [details]
Bug 1430173 Update tests for 2ms timers

https://reviewboard.mozilla.org/r/221860/#review229228
Attachment #8952620 - Flags: review?(amarchesini) → review+
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?
Attachment #8952618 - Flags: review?(amarchesini) → review+
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.
Attachment #8954156 - Flags: review?(amarchesini) → review+
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.
Attachment #8952618 - Flags: approval-mozilla-esr52?
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.
Attachment #8952618 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Filed Bug 1442127
Flags: needinfo?(tom)
Whiteboard: [adv-esr52.7-]
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: