1435296 - Reduce Timer Resolution to 2ms

Status: RESOLVED FIXED

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

Description

[Tom Ritter [:tjr]]

Now that we have some QA Validation, (we think) we're going to reduce the timer resolution to 2ms in beta.

Comment 1

[Tom Ritter [:tjr]]

Attachment: Bug 1435296 Bump the default timer precision resolution to 2ms

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

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8947899 [details]
Bug 1435296 Bump the default timer precision resolution to 2ms

https://reviewboard.mozilla.org/r/217574/#review223610

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/905555709846
Bump the default timer precision resolution to 2ms r=baku

Comment 4

[Narcis Beleuzu [:NarcisB]]

Backed out for mochitest GPU failures on /browser_profiling-webgl.js

Push: https://hg.mozilla.org/integration/autoland/rev/905555709846bec6c67819be1803d53dc026389d
Backout push: https://hg.mozilla.org/integration/autoland/rev/85a29d81a039619b09991e143e4a8542d8bf02e3
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=160598231&repo=autoland&lineNumber=3384

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Many more tests failed, e.g. also these devtools failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=905555709846bec6c67819be1803d53dc026389d&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&filter-searchStr=10.10+dt

Please do a full Try run.

Comment 6

[Tom Ritter [:tjr]]

Attachment: Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

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

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 8947899 [details]
Bug 1435296 Bump the default timer precision resolution to 2ms

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

Comment 8

[Tom Ritter [:tjr]]

Attachment: Bug 1435296 Do not apply timer clamping to CSS animations.

This patch creates the capability to have callsites specify if timestamps
should be clamped only in Resist Fingerprinting Mode, or in the more expansive
Timer PRecision Reduction Mode.

Then it changes the CSS Animation callsite to only apply in RFP Mode.

This avoids regressing RFP.

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

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

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

Comment 10

[Tom Ritter [:tjr]]

Attachment: Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

This commit does several subtle things.

1: It changes ok() to opener.ok()

ok is not defined, we have to use opener.ok. This was not caught before because
this call is used to provide additional debugging information when a test fails.
Test didn't fail, didn't hit that line.

2: It disables the call to opener.ok() we just fixed.

As the comment there describes, we expect that function to fail, so we don't want
to assert(false).

3: It inverts failures to successes if only the reduceTimerPrecision pref is set

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

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 8947899 [details]
Bug 1435296 Bump the default timer precision resolution to 2ms

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

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 8949206 [details]
Bug 1435296 Do not apply timer clamping to CSS animations.

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

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

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

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

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

Comment 15

[Tom Ritter [:tjr]]

Okay I think this is ready. I found a lot of intermittent test failures during my try runs and resolved them; but it's possible some still exist and didn't manifest themselves. I should be able to resolve them easily though.




Brian: We'd like to start rolling this out to Nightly/Beta populations so I flagged you on a patch that just turns off Timer Clamping on CSS Animations for the general case until we can understand it better/investigate and develop the correct solution. (It keeps clamping on for Resist Fingerprinting Mode, which is a non-default mode and doesn't change the behavior of how that mode has operated for several releases.)

Of note; however, it does _not_ disable timer clamping for the changes that were made in Bug 1430975 as it is my understanding that is not affecting animation playback.  If you search for 'ReduceTimePrecision' you'll see that there are actually fairly few uses of it, so that may be something you can use to reassure yourself that I a) am only affecting CSS Animations and b) I didn't omit a ReduceTimePrecision callsite I should (temporarily) disable.

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8949206 [details]
Bug 1435296 Do not apply timer clamping to CSS animations.

https://reviewboard.mozilla.org/r/218584/#review224436

I suppose we'll need further changes in future but when RFP is off, but this seems fine for now.

::: toolkit/components/resistfingerprinting/nsRFPService.h:167
(Diff revision 2)
> -  static double ReduceTimePrecisionAsMSecs(double aTime);
> -  static double ReduceTimePrecisionAsUSecs(double aTime);
> -  static double ReduceTimePrecisionAsSecs(double aTime);
> +  static double ReduceTimePrecisionAsMSecs(double aTime, TimerPrecisionType aType = TimerPrecisionType::All);
> +  static double ReduceTimePrecisionAsUSecs(double aTime, TimerPrecisionType aType = TimerPrecisionType::All);
> +  static double ReduceTimePrecisionAsSecs(double aTime, TimerPrecisionType aType = TimerPrecisionType::All);

Perhaps this module has different conventions but normally for C++ code we wrap lines to 80 columns.[1]

I believe `./mach clang-format` will do this for you.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:112
(Diff revision 2)
> +  if (aType == TimerPrecisionType::RFPOnly)
> +    return IsResistFingerprintingEnabled();

Really minor nit, but again, the coding style would have us use {} here. See the examples at:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code#Return_from_errors_immediately

Comment 17

[Tom Ritter [:tjr]]

Comment on attachment 8949206 [details]
Bug 1435296 Do not apply timer clamping to CSS animations.

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

Comment 18

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

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

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

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

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

https://reviewboard.mozilla.org/r/218378/#review224456

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

https://reviewboard.mozilla.org/r/218624/#review224458

Comment 22

[Code Review Bot [:reviewbot]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

https://reviewboard.mozilla.org/r/218624/#review224468


Code analysis found 5 defects in this patch:
 - 5 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/resistfingerprinting/test/mochitest/file_animation_api.html:57
(Diff revision 1)
>          }
>        }
>  
> -      ok(false, "Looming Test Failure, Additional Debugging Info: Expected Precision: " + expectedPrecision + " Measured Value: " + x +
> -        " Rounded Vaue: " + rounded + " Fuzzy1: " + Math.abs(rounded - x + expectedPrecision) +
> -        " Fuzzy 2: " + Math.abs(rounded - x));
> +      // We are temporarily disabling this extra debugging failure because we expect to return false in some instances
> +      // When we correct things we will re-enable it for debugging assistance
> +      //opener.ok(false, "Looming Test Failure, Additional Debugging Info: Expected Precision: " + expectedPrecision + " Measured Value: " + x +

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: browser/components/resistfingerprinting/test/mochitest/file_animation_api.html:73
(Diff revision 1)
>        () => animation.currentTime > 100,
>          () => {
> -          opener.ok(isRounded(animation.startTime),
> +
> +          // We have disabled Time Precision Reduction for CSS Animations, so we expect those tests to fail.
> +          // If we are testing that preference, turn failures into successes and successes into failures
> +          var maybeInvert = function (value) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

::: browser/components/resistfingerprinting/test/mochitest/file_animation_api.html:74
(Diff revision 1)
>          () => {
> -          opener.ok(isRounded(animation.startTime),
> +
> +          // We have disabled Time Precision Reduction for CSS Animations, so we expect those tests to fail.
> +          // If we are testing that preference, turn failures into successes and successes into failures
> +          var maybeInvert = function (value) {
> +            if (opener.prefName.indexOf("privacy.reduceTimerPrecision") >= 0 &&

Error: Use .includes instead of .indexof [eslint: mozilla/use-includes-instead-of-indexOf]

::: browser/components/resistfingerprinting/test/mochitest/file_animation_api.html:75
(Diff revision 1)
> -          opener.ok(isRounded(animation.startTime),
> +
> +          // We have disabled Time Precision Reduction for CSS Animations, so we expect those tests to fail.
> +          // If we are testing that preference, turn failures into successes and successes into failures
> +          var maybeInvert = function (value) {
> +            if (opener.prefName.indexOf("privacy.reduceTimerPrecision") >= 0 &&
> +                opener.prefName.indexOf("privacy.resistFingerprinting") === -1)

Error: Use .includes instead of .indexof [eslint: mozilla/use-includes-instead-of-indexOf]

::: browser/components/resistfingerprinting/test/mochitest/file_animation_api.html:78
(Diff revision 1)
> +          var maybeInvert = function (value) {
> +            if (opener.prefName.indexOf("privacy.reduceTimerPrecision") >= 0 &&
> +                opener.prefName.indexOf("privacy.resistFingerprinting") === -1)
> +              return !value;
> +            return value;
> +          }

Error: Missing semicolon. [eslint: semi]

Comment 23

[Code Review Bot [:reviewbot]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

https://reviewboard.mozilla.org/r/218624/#review224470


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/resistfingerprinting/test/mochitest/file_animation_api.html:73
(Diff revision 2)
>        () => animation.currentTime > 100,
>          () => {
> -          opener.ok(isRounded(animation.startTime),
> +
> +          // We have disabled Time Precision Reduction for CSS Animations, so we expect those tests to fail.
> +          // If we are testing that preference, turn failures into successes and successes into failures
> +          var maybeInvert = function (value) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

Comment 24

[Tom Ritter [:tjr]]

Alright let's try to land this in Nightly.

Comment 25

[Natalia Csoregi [:nataliaCs]]

Is this safe to land since the command detected an ESlint issue?

Comment 26

[Tom Ritter [:tjr]]

(In reply to Natalia Csoregi [:nataliaCs] from comment #25)
> Is this safe to land since the command detected an ESlint issue?

I had thought I resolved all lint errors; which one are you referring to?

Comment 27

[Natalia Csoregi [:nataliaCs]]

(In reply to Tom Ritter [:tjr] (OOTO 2/9) from comment #26)
> I had thought I resolved all lint errors; which one are you referring to?

Based on comment #23, wanted to check if it won't cause any issues (ESlint failures) after landing the patch.

Comment 28

[Tom Ritter [:tjr]]

(In reply to Natalia Csoregi [:nataliaCs] from comment #27)
> (In reply to Tom Ritter [:tjr] (OOTO 2/9) from comment #26)
> > I had thought I resolved all lint errors; which one are you referring to?
> 
> Based on comment #23, wanted to check if it won't cause any issues (ESlint
> failures) after landing the patch.

They are resolved.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f94f155318e
Bump the default timer precision resolution to 2ms r=baku
https://hg.mozilla.org/integration/autoland/rev/be9496eff7b8
Do not apply timer clamping to CSS animations. r=birtles
https://hg.mozilla.org/integration/autoland/rev/89c121b45b30
Address test failures caused by bumping timer precision to 2 ms r=baku
https://hg.mozilla.org/integration/autoland/rev/1f07c08daa41
Update the CSS Animations tests to handle our new Timer Precision decision r=baku

Comment 30

[Natalia Csoregi [:nataliaCs]]

Backed out for for failing devtools' browser_webconsole_check_stubs_console_api.js and mochitest's dom/smil/test/test_smilTimeEvents.xhtml.

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1f07c08daa416a247b6e5b76ef61a795ce72a406 

Failure log for devtools failure: https://treeherder.mozilla.org/logviewer.html#?job_id=161334981&repo=autoland&lineNumber=5685

[task 2018-02-09T15:01:53.811Z] 15:01:53     INFO - TEST-UNEXPECTED-FAIL | devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_check_stubs_console_api.js | The consoleApi stubs file needs to be updated by running `mach test devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_update_stubs_console_api.js` - 
[task 2018-02-09T15:01:53.811Z] 15:01:53     INFO - Stack trace:
[task 2018-02-09T15:01:53.812Z] 15:01:53     INFO -     chrome://mochitests/content/browser/devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_check_stubs_console_api.js:null:17
[task 2018-02-09T15:01:53.813Z] 15:01:53     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:1058:9
[task 2018-02-09T15:01:53.815Z] 15:01:53     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:958:9
[task 2018-02-09T15:01:53.816Z] 15:01:53     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2018-02-09T15:01:53.817Z] 15:01:53     INFO - Leaving test bound 
[task 2018-02-09T15:01:53.818Z] 15:01:53     INFO - GECKO(2301) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.

Failure log for mochitest failure: https://treeherder.mozilla.org/logviewer.html#?job_id=161335335&repo=autoland&lineNumber=4354

[task 2018-02-09T14:54:01.129Z] 14:54:01     INFO - Buffered messages finished
[task 2018-02-09T14:54:01.131Z] 14:54:01     INFO - TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilTimeEvents.xhtml | Event timeStamp (110) should be > 0 but before the current time (110) 
[task 2018-02-09T14:54:01.131Z] 14:54:01     INFO -     sanityCheckEvent@dom/smil/test/test_smilTimeEvents.xhtml:241:5
[task 2018-02-09T14:54:01.132Z] 14:54:01     INFO -     checkExpectedEvent@dom/smil/test/test_smilTimeEvents.xhtml:252:3
[task 2018-02-09T14:54:01.133Z] 14:54:01     INFO -     handleOnBegin@dom/smil/test/test_smilTimeEvents.xhtml:217:3
[task 2018-02-09T14:54:01.134Z] 14:54:01     INFO -     onbeginEvent@dom/smil/test/test_smilTimeEvents.xhtml:1:1

Comment 31

[Natalia Csoregi [:nataliaCs]]

Backout link: https://hg.mozilla.org/integration/autoland/rev/f60e36262f5f13cd49a43a7e916e0274a58f47e6

Comment 32

[Natalia Csoregi [:nataliaCs]]

Also failing https://treeherder.mozilla.org/logviewer.html#?job_id=161340697&repo=autoland&lineNumber=3698 

[task 2018-02-09T15:24:21.534Z] 15:24:21     INFO - TEST-START | /navigation-timing/nav2_test_redirect_server.html
[task 2018-02-09T15:24:21.684Z] 15:24:21     INFO - PID 8042 | JavaScript error: , line 0: uncaught exception: Error: assert_true: Expected startTime to be no greater than redirectStart. expected true got false
[task 2018-02-09T15:24:21.749Z] 15:24:21     INFO - 
[task 2018-02-09T15:24:21.750Z] 15:24:21     INFO - TEST-UNEXPECTED-FAIL | /navigation-timing/nav2_test_redirect_server.html | Navigation Timing 2 WPT - uncaught exception: Error: assert_true: Expected startTime to be no greater than redirectStart. expected true got false
[task 2018-02-09T15:24:21.750Z] 15:24:21     INFO - verifyTimingEventOrder@http://web-platform.test:8000/navigation-timing/nav2_test_redirect_server.html:15:21
[task 2018-02-09T15:24:21.750Z] 15:24:21     INFO - onload_test@http://web-platform.test:8000/navigation-timing/nav2_test_redirect_server.html:33:17
[task 2018-02-09T15:24:21.751Z] 15:24:21     INFO - onload@http://web-platform.test:8000/navigation-timing/nav2_test_redirect_server.html:1:1
[task 2018-02-09T15:24:21.751Z] 15:24:21     INFO - TEST-OK | /navigation-timing/nav2_test_redirect_server.html | took 216ms

Comment 33

[Tom Ritter [:tjr]]

(In reply to Natalia Csoregi [:nataliaCs] from comment #30)
> [task 2018-02-09T15:01:53.811Z] 15:01:53     INFO - TEST-UNEXPECTED-FAIL |
> devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/
> browser_webconsole_check_stubs_console_api.js | The consoleApi stubs file
> needs to be updated by running `mach test
> devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/
> browser_webconsole_update_stubs_console_api.js` - 


Nicolas - I was told you might be able to help me understand this failure? I thought maybe it needed to regenerate some file for checkin, but after I ran that command I don't see anything changed in my local tree.

Comment 34

[Tom Ritter [:tjr]]

Attachment: Bug 1435296 Clean 0ms durations in stub generation

If the duration was 0ms, it would not be cleaned; and thus lead to a mismatch and
ultimately test failure.

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

Comment 35

[Tom Ritter [:tjr]]

Comment on attachment 8947899 [details]
Bug 1435296 Bump the default timer precision resolution to 2ms

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

Comment 36

[Tom Ritter [:tjr]]

Comment on attachment 8949206 [details]
Bug 1435296 Do not apply timer clamping to CSS animations.

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

Comment 37

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

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

Comment 38

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

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

Comment 40

[Tom Ritter [:tjr]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

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

Comment 41

[Tom Ritter [:tjr]]

Attachment: Bug 1435296 Remove unneeded script import from test_timeOrigin.html

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

Comment 42

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

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

Comment 43

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

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

Comment 44

[Tom Ritter [:tjr]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

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

Comment 46

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

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

Comment 47

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

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

Comment 48

[Tom Ritter [:tjr]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

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

Comment 49

[Tom Ritter [:tjr]]

Comment on attachment 8949996 [details]
Bug 1435296 Remove unneeded script import from test_timeOrigin.html

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

Comment 50

[Tom Ritter [:tjr]]

Attachment: Bug 1435296 Address xpcshell test failures from increasing timer precision

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

Comment 51

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

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

Comment 52

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

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

Comment 53

[Tom Ritter [:tjr]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

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

Comment 54

[Tom Ritter [:tjr]]

Comment on attachment 8949996 [details]
Bug 1435296 Remove unneeded script import from test_timeOrigin.html

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

Comment 57

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218378/diff/8-9/

Comment 58

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

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

Comment 59

[Tom Ritter [:tjr]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

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

Comment 60

[Tom Ritter [:tjr]]

Comment on attachment 8949996 [details]
Bug 1435296 Remove unneeded script import from test_timeOrigin.html

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

Comment 61

[Tom Ritter [:tjr]]

Comment on attachment 8950103 [details]
Bug 1435296 Address xpcshell test failures from increasing timer precision

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

Comment 62

[Tom Ritter [:tjr]]

Comment on attachment 8947899 [details]
Bug 1435296 Bump the default timer precision resolution to 2ms

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

Comment 63

[Tom Ritter [:tjr]]

Comment on attachment 8949206 [details]
Bug 1435296 Do not apply timer clamping to CSS animations.

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

Comment 64

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218378/diff/9-10/

Comment 65

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218624/diff/8-9/

Comment 66

[Tom Ritter [:tjr]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

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

Comment 67

[Tom Ritter [:tjr]]

Comment on attachment 8949996 [details]
Bug 1435296 Remove unneeded script import from test_timeOrigin.html

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

Comment 68

[Tom Ritter [:tjr]]

Comment on attachment 8950103 [details]
Bug 1435296 Address xpcshell test failures from increasing timer precision

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

Comment 69

[Code Review Bot [:reviewbot]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

https://reviewboard.mozilla.org/r/218378/#review225096


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/sourceeditor/test/browser_codemirror.js:28
(Diff revision 10)
> +   *
> +   * In theory we don't need to set the pref for all of CodeMirror, in practice
> +   * it seems very difficult to set a pref for just one of the tests.
> +   */
> +  SpecialPowers.pushPrefEnv({set:
> +    [["privacy.reduceTimerPrecision", true],

Error: Expected indentation of 2 spaces but found 4. [eslint: indent-legacy]

::: devtools/client/sourceeditor/test/browser_codemirror.js:30
(Diff revision 10)
> +   * it seems very difficult to set a pref for just one of the tests.
> +   */
> +  SpecialPowers.pushPrefEnv({set:
> +    [["privacy.reduceTimerPrecision", true],
> +     ["privacy.reduceTimerPrecision.resistFingerprinting.microseconds", 2000]]},
> +    function() {

Error: Missing space before function parentheses. [eslint: space-before-function-paren]

Comment 70

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

https://reviewboard.mozilla.org/r/219236/#review225124

Looks good to me, thanks !

Comment 71

[Andrea Marchesini [:baku]]

Comment on attachment 8949996 [details]
Bug 1435296 Remove unneeded script import from test_timeOrigin.html

https://reviewboard.mozilla.org/r/219274/#review225156

Comment 72

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218378/diff/10-11/

Comment 73

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218624/diff/9-10/

Comment 74

[Tom Ritter [:tjr]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

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

Comment 75

[Tom Ritter [:tjr]]

Comment on attachment 8949996 [details]
Bug 1435296 Remove unneeded script import from test_timeOrigin.html

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

Comment 76

[Tom Ritter [:tjr]]

Comment on attachment 8950103 [details]
Bug 1435296 Address xpcshell test failures from increasing timer precision

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

Comment 77

[Andrea Marchesini [:baku]]

Comment on attachment 8950103 [details]
Bug 1435296 Address xpcshell test failures from increasing timer precision

https://reviewboard.mozilla.org/r/219378/#review225160

::: browser/components/extensions/test/xpcshell/test_ext_browsingData_cookies_cache.js:88
(Diff revision 3)
>  
>    await extension.unload();
>  });
>  
>  add_task(async function testCookies() {
> +  let timerPrecision = Preferences.get("privacy.reduceTimerPrecision");

Can you simply use:

await SpecialPowers.pushPrefEnv({"set": [["privacy.reduceTimerPrecision", false]]});

 ?

::: browser/extensions/formautofill/test/unit/test_addressRecords.js:378
(Diff revision 3)
>  
>  add_task(async function test_update() {
> +  // Test assumes that when an entry is saved a second time, it's last modified date will
> +  // be different from the first. With high values of precision reduction, we execute too
> +  // fast for that to be true.
> +  let timerPrecision = Preferences.get("privacy.reduceTimerPrecision");

same here.

::: browser/extensions/formautofill/test/unit/test_creditCardRecords.js:296
(Diff revision 3)
>    Assert.throws(() => profileStorage.creditCards.add(TEST_CREDIT_CARD_EMPTY_AFTER_NORMALIZE),
>      /Record contains no valid field\./);
>  });
>  
>  add_task(async function test_update() {
> +  let timerPrecision = Preferences.get("privacy.reduceTimerPrecision");

and here.

::: devtools/server/tests/unit/test_promises_object_creationtimestamp.js:19
(Diff revision 3)
> +ChromeUtils.defineModuleGetter(this, "Preferences",
> +                               "resource://gre/modules/Preferences.jsm");
> +
>  add_task(function* () {
> +  let timerPrecision = Preferences.get("privacy.reduceTimerPrecision");
> +  Preferences.set("privacy.reduceTimerPrecision", false);

here as well

::: netwerk/test/unit/test_race_cache_with_network.js:90
(Diff revision 3)
> +   * the timer works properly by checking that the time was greater than 200ms.
> +   * With a timer precision of 100ms (for example) we will clamp downwards to 200
> +   * and cause the assertion to fail. To resolve this, we hardcode a precision of
> +   * 2us, because it's _really_ racey.
> +   */
> +  Services.prefs.setBoolPref("privacy.reduceTimerPrecision", true);

here?

Comment 78

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218378/diff/11-12/

Comment 79

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218624/diff/10-11/

Comment 80

[Tom Ritter [:tjr]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219236/diff/8-9/

Comment 81

[Tom Ritter [:tjr]]

Comment on attachment 8949996 [details]
Bug 1435296 Remove unneeded script import from test_timeOrigin.html

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

Comment 82

[Tom Ritter [:tjr]]

Comment on attachment 8950103 [details]
Bug 1435296 Address xpcshell test failures from increasing timer precision

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

Comment 83

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218378/diff/12-13/

Comment 84

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218624/diff/11-12/

Comment 85

[Tom Ritter [:tjr]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219236/diff/9-10/

Comment 86

[Tom Ritter [:tjr]]

Comment on attachment 8949996 [details]
Bug 1435296 Remove unneeded script import from test_timeOrigin.html

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

Comment 87

[Tom Ritter [:tjr]]

Comment on attachment 8950103 [details]
Bug 1435296 Address xpcshell test failures from increasing timer precision

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

Comment 88

[Tom Ritter [:tjr]]

Comment on attachment 8947899 [details]
Bug 1435296 Bump the default timer precision resolution to 2ms

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

Comment 89

[Tom Ritter [:tjr]]

Comment on attachment 8949206 [details]
Bug 1435296 Do not apply timer clamping to CSS animations.

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

Comment 90

[Tom Ritter [:tjr]]

Comment on attachment 8948958 [details]
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218378/diff/13-14/

Comment 91

[Tom Ritter [:tjr]]

Comment on attachment 8949262 [details]
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218624/diff/12-13/

Comment 92

[Tom Ritter [:tjr]]

Comment on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219236/diff/10-11/

Comment 93

[Tom Ritter [:tjr]]

Comment on attachment 8949996 [details]
Bug 1435296 Remove unneeded script import from test_timeOrigin.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219274/diff/8-9/

Comment 94

[Tom Ritter [:tjr]]

Comment on attachment 8950103 [details]
Bug 1435296 Address xpcshell test failures from increasing timer precision

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

Comment 95

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a4536966c4
Bump the default timer precision resolution to 2ms r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3171a2575b4e
Do not apply timer clamping to CSS animations. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/312fea6ab87f
Address test failures caused by bumping timer precision to 2 ms r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c07e0f235a9
Update the CSS Animations tests to handle our new Timer Precision decision r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6b4604bbee2
Clean 0ms durations in stub generation r=nchevobbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e184083b2f4
Remove unneeded script import from test_timeOrigin.html r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/744c2e1e71b5
Address xpcshell test failures from increasing timer precision r=baku

Comment 96

[Tom Ritter [:tjr]]

Comment on attachment 8947899 [details]
Bug 1435296 Bump the default timer precision resolution to 2ms

For those following the bug, here is the inbound run: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=744c2e1e71b594975a7981d3b06a37f693528361&selectedJob=161778163






As of this writing we are landing this in inbound. We'd like to get this into beta at the end of the week (Beta 10)

Approval Request Comment
[Feature/Bug causing the regression] / [User impact if declined]: This is a security mitigation for Spectre.

[Is this code covered by automated tests?]: Yes, there are lots of tests affected by this.

[Has the fix been verified in Nightly?]: By the time we are ready to land, it will have hopefully baked for a few days. We have also done manual QA prior to it being in Nightly and users who have toggled Resist Fingerprinting have been affected by this code for months.

[Needs manual test from QE? If yes, steps to reproduce]: I don't think so.

[List of other uplifts needed for the feature/fix]: There may be additional test fixes I land during the week, if so I will list them as Dependencies.

[Is the change risky?] / [Why is the change risky/not risky?]: It is a bit risky. We are affecting how time is processed by the browser, and it has some subtle effects. Many - but not all - of the tests we know were affected are only affected at very high levels of timer clamping (100ms, whereas this patch is only going to 2ms). Many of the tests have been examined carefully to confirm they make sense. Some of them don't have clear causes but seem okay, a few others seem sketchy but have not recurred at the 2ms level. I'm just capturing a high level in this form, we can talk about it more in depth.

[String changes made/needed]: None.


I will probably need to prepare a beta backport which I will do this week.

Comment 97

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, giving this a day or so in Nightly sounds like a good idea, and we can be on the lookout for an updated patch by Wednesday end of the day (to land this for beta 10).  
Julien, fyi since I will be out most of Wednesday.

Comment 98

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/f1a4536966c4
https://hg.mozilla.org/mozilla-central/rev/3171a2575b4e
https://hg.mozilla.org/mozilla-central/rev/312fea6ab87f
https://hg.mozilla.org/mozilla-central/rev/0c07e0f235a9
https://hg.mozilla.org/mozilla-central/rev/e6b4604bbee2
https://hg.mozilla.org/mozilla-central/rev/2e184083b2f4
https://hg.mozilla.org/mozilla-central/rev/744c2e1e71b5

Comment 99

[Ionuț Goldan [:igoldan]]

Noticeable performance improvements thanks to this bug!

== Change summary for alert #11519 (as of Mon, 12 Feb 2018 16:45:32 GMT) ==

Improvements:

  6%  rasterflood_svg windows10-64 pgo e10s stylo     11,335.59 -> 10,618.33
  3%  rasterflood_svg linux64 opt e10s stylo          17,779.47 -> 17,180.83
  3%  rasterflood_svg linux64 pgo e10s stylo          18,671.56 -> 18,147.73

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11519

Comment 100

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8947899 [details]
Bug 1435296 Bump the default timer precision resolution to 2ms

Must fix, baked on Nightly for 2 days, Beta59+

Comment 101

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Tom, since ESR52 is affected, do we need an uplift for ESR52 as well?

Comment 102

[Tom Ritter [:tjr]]

(In reply to Ritu Kothari (:ritu) from comment #101)
> Hi Tom, since ESR52 is affected, do we need an uplift for ESR52 as well?

Bug 1430173

Comment 103

[Tom Ritter [:tjr]]

Beta Backport:

1: Bug 1435296 Bump the default timer precision resolution to 2ms  (applies cleanly from mozreview)
2: Address test failures caused by bumping timer precision to 2 ms  (attached)
3: Bug 1435296 Do not apply timer clamping to CSS animations. (applies cleanly from mozreview)
4: Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision (applies cleanly from mozreview)
5: Bug 1435296 Clean 0ms durations in stub generation r?nchevobbe (applies cleanly from mozreview)
6: Address xpcshell test failures from increasing timer precision (attached)
7: OPTIONAL: Bug 1435296 Remove unneeded script import from test_timeOrigin.html r?baku


#7 is code cleanliness, it really doesn't affect anything. I don't know if it's better to backport such things to match -central or better to keep number of commits to beta as low as possible. Probably the latter so you can omit it.

Comment 104

[Tom Ritter [:tjr]]

Attachment: patch_2_beta.patch

Comment 105

[Tom Ritter [:tjr]]

Attachment: patch_6_beta.patch

Comment 106

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3cba8d67a642
https://hg.mozilla.org/releases/mozilla-beta/rev/96bc5f3e4591
https://hg.mozilla.org/releases/mozilla-beta/rev/576378b59e73
https://hg.mozilla.org/releases/mozilla-beta/rev/9012bf881e43
https://hg.mozilla.org/releases/mozilla-beta/rev/24cb1a9d9b23
https://hg.mozilla.org/releases/mozilla-beta/rev/6ca6a81f6f32

Comment 107

[Tom Ritter [:tjr]]

Attachment: patch_8_beta.patch

Okay, so -beta has different properties available or not available from -central, and I needed to fix up the tests (primarily xpcshell tests but also 1 browser-chrome). The attached patch passes the linter (I hope) and will all succeed (unless I messed up locally, I had them working...)

Does it make sense logically for -beta? Am I strangely using some property one doesn't think is available?

Try Run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54da0a523febc8f6495c88f83d5bfe00068896d9

Comment 108

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failures e.g. in browser/extensions/formautofill/test/unit/test_addressRecords.js, and devtools and XPCshell failures, e.g. services/sync/tests/unit/test_addons_engine.js:

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=aec659045f35f04f7d9ac319665d640a2ccd0a88&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry

Tom is already working on a fix.

Comment 109

[Tom Ritter [:tjr]]

Attachment: patch_8_beta.patch

New patch that uses XPCOMUtils.defineLazyModuleGetter instead of ChromeUtils.defineModuleGetter for -beta.

Tests passed locally. New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03ec70b04bbc024d035c2019553e2af5a9c2617c

Comment 110

[Dave Townsend [:mossop]]

Comment on attachment 8951068 [details] [diff] [review]
patch_8_beta.patch

Review of attachment 8951068 [details] [diff] [review]:
-----------------------------------------------------------------

So many new uses of Preferences.jsm :(

Comment 111

[Tom Ritter [:tjr]]

Attachment: patch_8_beta.patch

Okay, I think this is ready to (re-)land on -beta

Try Run 1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3953df4118f77e76428b2dbeadef72727d26b5f8&selectedJob=162318109
Try Run 2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28b565de42c19d223fde6ebe05e05db76a4d7030

Run 1 had a lint failure in a xpcshell test; Run 2 fixed that.

There are test failures for gfx/tests/mochitest/test_acceleration.html - but those are on the QuantumRender flavor of Win 10, and that isn't being run in beta. 

These are the 7 patches for landing:

1: Bug 1435296 Bump the default timer precision resolution to 2ms  (applies cleanly from mozreview)
2: Address test failures caused by bumping timer precision to 2 ms  (attached)
3: Bug 1435296 Do not apply timer clamping to CSS animations. (applies cleanly from mozreview)
4: Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision (applies cleanly from mozreview)
5: Bug 1435296 Clean 0ms durations in stub generation r?nchevobbe (applies cleanly from mozreview)
6: Address xpcshell test failures from increasing timer precision (attached)
7: <skipping this patch because it's entirely unnecessary>
8: patch_8_beta.patch (attached)

Comment 112

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b8a1c99d4d66
https://hg.mozilla.org/releases/mozilla-beta/rev/392da1c5f6f5
https://hg.mozilla.org/releases/mozilla-beta/rev/1b6ac8a0f791
https://hg.mozilla.org/releases/mozilla-beta/rev/f459e7434f59
https://hg.mozilla.org/releases/mozilla-beta/rev/6bfc2aebde05
https://hg.mozilla.org/releases/mozilla-beta/rev/9e9e611b5ebf
https://hg.mozilla.org/releases/mozilla-beta/rev/ad718c8e5655

Comment 113

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Tom Ritter [:tjr] from comment #96)
> [Is this code covered by automated tests?]: Yes, there are lots of tests
> affected by this.
> [Needs manual test from QE? If yes, steps to reproduce]: I don't think so.

Based on Tom's assessment on manual testing needs and due to the fact that is has automated coverage, marking this as qe-verify-.

Comment 114

[mark]

Firstly, Spectre and Meltdown are indeed extremely serious security issues.
Many vendors, rightfully, doing emergency fixes including temporarily reducing timer precision.

However, I have seriously grave concerns about Mozilla/FireFox's decision to reduce performance.now() precision to 2ms -- there are unanticipated major side effects, including one that will reduce security.

MAJOR SIDE EFFECT 1:

The precision degradation is so massive (even at 60Hz) -- affecting fullscreen WebGL and WebVR majorly -- that I have heard from a friend that a site that plans to automatically detect the 2ms imprecision and automatically prompt the user with step-by-step instructions to fix the imprecision for their site. As a global FireFox setting, this is problematic from a security perspective. A global setting changed.

I think the safer solution is:

"In an absolute emergency: Personally if there's a duress to reduce timer precision, then a temporary permissions API (cameras and fullscreen have more security/privacy fears) to prompt the user for permission to stay at 5us precision. At least during the duration of security studying, until being reverted to maximized precision."

MAJOR SIDE EFFECT 2:

As a past game developer (most standards writers aren't game developers), let me tell you:

Accurate gametimes are necessary for accurate calculations of 3D object positions, especially in full-screen WebGL games.

At sub-refresh-cycles, animations should still use sub-millisecond accuracy to prevent jank, when calculating time-based animations (e.g. calculating object positions based on time) -- that's what 3D graphics often do compensating for fluctuating frame rates, they calculate object world positions based on gametime.

That means 1000 inch per second moving balls (~60mph) can be wrongly mispositioned by 2 inches if the gametime is off by 2 millisecond. That can mean goal/nogoal. Fastballs, baseballs, hockey pucks can go much faster than this. Low-precision gametimes will ruin a lot of HTML5 and WebGL game programming.

Gametimes need sub-millisecond accuracy, especially full-screen WebGL games, because they calculate real-world 3D object positions based on gametime.

That way everything looks correct regardless of framerate. Framerates fluctuate all over the place, so video games using 3D graphics use gametime to calculate the position of everything. 3D object positions are calculated based on gametime. Wrong gametime means everything janks like crazy in full screen WebGL games.

Again, this is for 60 Hz. I'm not even talking about higher refresh rates. 1ms gametime errors create noticeable motion-fluidity flaws in 60Hz full-screen WebGL games!

Things even janks crazily with 5ms and 8ms gametime errors on a 60Hz display. You're in the driver's seat of a 300mph car, in a racing game -- a 2ms gametime error becomes large even on a 60 Hz display. Jank/jerkiness/stutter can get huge even at sub-refresh-cycle levels even on a 60 Hz display, even with sub-refresh-cycle errors.

Also, don't forget that there are advantages to frame rates above refresh rates in reducing latency: https://www.blurbusters.com/faq/benefits-of-frame-rate-above-refresh-rate/

My prediction is FireFox may get a tsunami wave of huge complaints from game designers suddenly hit by major precision reductions in ability to calculate gameworld positions.

MAJOR SIDE EFFECT 3:

Depending on how timer precision is degraded, it potentially eliminates educational motion tests in web browsers. Many scientific tests such as www.testufo.com/ghosting and www.testufo.com/frameskipping (display overclocking) is heavily dependant on perfect frame rate synchronization, and the site is able to detect whenever Chrome misses a frame.

Peer reviewed papers have already been written based on browser motion tests (including www.blurbusters.com/motion-tests/pursuit-camera-paper ...) thanks to browser's ability to achieve perfect refresh rate synchronization)

Even one of my sites, TestUFO, depending on how the site is degraded in upcoming FireFox A/B tests (new versus old browser) -- it may be forced to do something similar to the popup in "MAJOR SIDE EFFECT 1" -- for specific kinds of peer-reviewed scientific motion testing. Is there a way for me to tell users how to whitelist only one site (TestUFO.com) for high-precision timers, without doing it as a global setting? (Thought exercise for W3C)

The TestUFO website already automatically displays a message telling TestUFO users to switch web browsers instead of IE/Edge for 120Hz testing, due to IE/Edge continued violation of Section 7.1.4.2 of HTML 5.2. More than 50% of TestUFO visitors are using a refresh rate other than 60 Hz.

MAJOR SIDE EFFECT 4:

It makes WebVR useless.

Its premise (without creating nausea/headaches) is heavily dependant on accurate gametimes for accurate positions of 3D objects (see MAJOR SIDE EFFECT 2 above).

MAJOR SIDE EFFECTS 5-100

I have a much longer list, but for brevity, I am shortening this email to point out the graveness of FireFox's decision.

CONCLUSION

The approach by the FireFox team should be refined.

If continued short-term emergency mitigation is absolute critical, it should includes a Permissions API (much like Full Screen mode and WebRTC camera permissions) to case-by-case ask the user for permission to use high-precision timers (allowing 5us or 1us precision).

If absolutely necessary, this could even be limited to Organization Validation HTTPS sites, combined with only exclusive same-origin use, even triggered by a click, and only after confirming via a popup Permission API (like for WebRTC), that 5us/1us precision becomes granted to that particular site.

Long-term, these restrictions should be removed once the underlying causes of Spectre/Meltdown becomes solved. However, massive precision reductions, that forces web developers to give instructions to visitors how to configure their web browsers, is a less secure solution.

Thanks,
Mark Rejhon
Founder, Blur Busters / TestUFO
(Past Invited Expert, W3C Web Platform Working Group for HTML 5.2)

Comment 115

[Tom Ritter [:tjr]]

HI Mark, thanks for this feedback. I am curious, is the situation as dire for Chrome and Edge as it is for Firefox? I am trying to understand if there is something different about our implementation compared to theirs that makes it significantly worse for web application programmers while browsers work to address high resolution timing attacks (of which Spectre is just one.) I believe both of their implementations are less expansive than ours, as we adjust all timers (not just performance.now()) and we clamp to 2ms instead of 1ms as they do. Is 1ms an acceptable resolution while 2ms is untenable for the types of applications you describe?

Comment 116

[mark]

(In reply to Tom Ritter [:tjr] from comment #115)
> HI Mark, thanks for this feedback. I am curious, is the situation as dire
> for Chrome and Edge as it is for Firefox? I am trying to understand if there
> is something different about our implementation compared to theirs that
> makes it significantly worse for web application programmers while browsers
> work to address high resolution timing attacks (of which Spectre is just
> one.) I believe both of their implementations are less expansive than ours,
> as we adjust all timers (not just performance.now()) and we clamp to 2ms
> instead of 1ms as they do. Is 1ms an acceptable resolution while 2ms is
> untenable for the types of applications you describe?

1ms is still too imprecise.  

Currently browsers are:
-- Chrome using 100us + 100us pseudorandom jitter (M64)
-- Edge using 20us + 20us pseudorandom jitter
-- Safari using 1ms (same precision as Date.now())

I have not been able to find information about any further precision reductions (did they come up in your meetings)?

From what I understand (from other developers), 100us + 100us pseudorandom jitter is reportedly much harder to do a Spectre/Meltdown attack than a trunctated 2ms hard-edge.  So that may be a better (interim) compromise -- Use 100us instead of 2000us -- *and* add a 100us jitter.   One can create a simulated high-precision timer via precision busywaits from known 2000us hard-edges (trunctated timers), but by adding jitter, you really mess that ability up. 

Even that is something I am really reluctant to tolerate, as there are some game calculations that benefits from sub-50us accuracy, so ultimately, in the long term, there should eventually be 1us precision again (in the post-Spectre/Meltdown mitigation world, or with security permissions).

100us(granularity)+100us(pseudorandom) is more accurate (less jank) for games that calculates positionals from gametime.  Yet from what I am reading, should be harder to execute a Spectre/Meltdown attack from than a non-jittered 2000us(granularity).  The hard-edges of a 2 millisecond trunctation provides exact-interval reference points (just monitor for timer event changes).  By adding (well-seeded) pseudorandom jitter, you add several orders of magnitude difficulty to Spectre/Meltdown attacks, and make 2000us imprecision (as you are doing) unnecessary.  More research is needed probably, but I implore you to find a way to go sub-1ms for games.

An improved version of this concept is a double pseudorandom:

Basically, every (0-100us pseudorandom value of "A") update an internal timer then increment (0-100us pseudorandom of a different pseudorandom value "B").  That way, it never ticks backwards, despite the pseudorandom jitter.  And 33.1782us may have actually passed before the timer incremented by say, 59.197us.  Then 89.3665us later, the timer incremented by 23.2165us.   And so on.  Do all decimal points allowed by the web spec (performance.now supports it) -- the more pseudorandom digits the better.  

The pseudorandom timer increment can be completely independent of the actual real-world time interval between the sudden pseudorandom timer increments.  This double-pseudorandom jitter approach allows you to massively tighten the accuracy.  

By combining these two independent pseudorandoms, you have a much more fine-grained timer, more suitable for games, yet is much harder to do Spectre/Meltdown workarounds than a simple hard-edged 2ms increment every 2ms. 

I think 20us can be made safe by turning it from a non-jittered/single-pseudorandom-jittered approach to a double-pseudorandom-jittered approach (as described above) -- validation will be needed but 2ms is extremely untenable for WebVR / fullscreen 3D FPS browser games.  

Longer term, ultimately, I'd prefer 1us or 0.1us (microsecond) precision to be restored at some point in the future -- e.g. a strong permissions mechanism -- a special sandbox flag that indicates full Spectre/Meltdown safety -- but for emergency mitigation, 20us double-pseudorandom-jittered is a great compromise.

Comment 117

[mark]

Jerry of Duckware just told me something. I learn something new -- Wow.

FireFox 60 with 2ms appears to have a microsecond-accurate hard-edge (performance.now() incrementing by 2ms every exactly 2ms)  That's easier to generate an artificial timer from.  From what I understand 2ms microsecond-accurate-hard-edge is less secure than 100us+100us pseudorandom jitter.   

So you could just do this in FireFox:

var oldTime = performance.now();
while (performance.now() == oldTime);
// Now we're aligned to the exact microsecond here.

That makes Edge/Chrome more-accurate clocks likely more safe than the planned FireFox's degraded 2ms clock.

From this, it is possible to create a software-based accurate clock from known JavaScript performance profiling between two microsecond-accurate timer-increment events.  From this information (e.g. counter increments per second), it is possible to virtualize much more microsecond-accuracy with some clever Javascript programming skills.  

Back in year 2012, I successfully experimented with this -- Look at my October 26th, 2012 answer on StackOverflow (Scroll down to my name "Mark Rejhon") -- https://stackoverflow.com/questions/6233927/microsecond-timing-in-javascript -- I wrote that answer long before performance.now() was invented. 

If you decide to do only a single pseudorandom (not double), remember the jitter should be on the interval, not on the number.  Basically, increment every exact "X" ms, but vary the real-world time between increments.   Exact time between timer-increments is the big problematic part for Spectre/Meltdown.   I now wonder if Apple's approach is as equally flawed as FireFox's microsecond-accurate-edge 2ms degradation.

Comment 118

[Tom Ritter [:tjr]]

clearing ESR flag, tracked in Bug 1430173

Thanks Mark. I had mis-remembered who was at what precision, I knew someone was at 1ms, but I thought it was Chrome/Edge and not Safari!

The double-pseudorandom approach you describe is also known by 'fuzzytime' and I am working on it over in Bug 1425462. I hope to get it into 60 before it moves to Beta. You're correct that having a hard edge allows constructed timers; there are a few other techniques for those as well that we hope to mitigate more expansively in Bug 1432429.

Once I made some good headway on landing the jitter; I'll try to compare attack efficiencies at different times with and without jitter and see what it looks like so we can weigh that in with how to un-break all the use cases you describe..

Comment 119

[mark]

Thank you for listening me out and hope you can quickly validate an alternative fix for FireFox 60 instead of a large 2ms decimation.

I hope your security validation tests confirms that 20us double jitter is sufficient (and at worse, 100us double jitter).   
 
Since performance.now() is floating point -- remember to not trunctate the pseudorandom to microseconds -- let the nanoseconds and beyond also jitter too.  That will add more randomness while allowing smaller jitter (e.g. 20us instead of 100us or worse).

Comment 120

[mark]

P.S. In your next meeting, tell the Safari team about my writings, so that they can undo their 1ms imprecision.

Comment 121

[mark]

Added suggestions of improvements to your fuzz algorithms to Bug 1432429.

Comment 122

[mark]

Ugggg, I forget I cannot my comments on this specific system.   I meant I put my suggestions of fuzz-improvements in Bug 1425462