Closed Bug 1435296 Opened 6 years ago Closed 6 years ago

Reduce Timer Resolution to 2ms

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 + fixed
firefox60 + fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(10 files, 2 obsolete files)

Bug 1435296 Bump the default timer precision resolution to 2ms
59 bytes, text/x-review-board-request
ritu
: approval-mozilla-beta+
Details
Bug 1435296 Address test failures caused by bumping timer precision to 2 ms
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1435296 Do not apply timer clamping to CSS animations.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1435296 Update the CSS Animations tests to handle our new Timer Precision decision
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1435296 Clean 0ms durations in stub generation
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1435296 Remove unneeded script import from test_timeOrigin.html
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1435296 Address xpcshell test failures from increasing timer precision
59 bytes, text/x-review-board-request
baku
: review+
Details
patch_2_beta.patch
52.01 KB, patch
Details | Diff | Splinter Review
patch_6_beta.patch
25.19 KB, patch
Details | Diff | Splinter Review
patch_8_beta.patch
13.27 KB, patch
Details | Diff | Splinter Review 
Now that we have some QA Validation, (we think) we're going to reduce the timer resolution to 2ms in beta.
Attachment #8947899 - Flags: review?(overholt)
Attachment #8947899 - Flags: review?(amarchesini)
Comment on attachment 8947899 [details]
Bug 1435296 Bump the default timer precision resolution to 2ms

https://reviewboard.mozilla.org/r/217574/#review223610
Attachment #8947899 - Flags: review?(amarchesini) → review+
Priority: -- → P2
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/905555709846
Bump the default timer precision resolution to 2ms r=baku
Keywords: checkin-needed
Attachment #8949206 - Flags: review?(bbirtles)
Attachment #8948958 - Flags: review?(amarchesini)
Attachment #8949262 - Flags: review?(amarchesini)
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.
Flags: needinfo?(tom)
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
Attachment #8949206 - Flags: review?(bbirtles) → review+
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
Attachment #8948958 - Flags: review?(amarchesini) → review+
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
Attachment #8949262 - Flags: review?(amarchesini) → review+
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 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]
Alright let's try to land this in Nightly.
Keywords: checkin-needed
Is this safe to land since the command detected an ESlint issue?
Flags: needinfo?(tom)
(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?
Flags: needinfo?(tom)
(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.
Flags: needinfo?(tom)
(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.
Flags: needinfo?(tom)
Pushed by ncsoregi@mozilla.com:
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
Keywords: checkin-needed
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
Flags: needinfo?(tom)
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
Attachment #8947899 - Flags: review?(overholt) → review?(amarchesini)
(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.
Flags: needinfo?(tom) → needinfo?(nchevobbe)
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 on attachment 8949932 [details]
Bug 1435296 Clean 0ms durations in stub generation

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

Looks good to me, thanks !
Attachment #8949932 - Flags: review?(nchevobbe) → review+
Flags: needinfo?(nchevobbe)
Comment on attachment 8949996 [details]
Bug 1435296 Remove unneeded script import from test_timeOrigin.html

https://reviewboard.mozilla.org/r/219274/#review225156
Attachment #8949996 - Flags: review?(amarchesini) → review+
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?
Attachment #8950103 - Flags: review?(amarchesini) → review+
Pushed by archaeopteryx@coole-files.de:
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
Keywords: checkin-needed
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.
Attachment #8947899 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(jcristau)
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 on attachment 8947899 [details]
Bug 1435296 Bump the default timer precision resolution to 2ms

Must fix, baked on Nightly for 2 days, Beta59+
Attachment #8947899 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi Tom, since ESR52 is affected, do we need an uplift for ESR52 as well?
Flags: needinfo?(tom)
Flags: needinfo?(jcristau)
(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
Flags: needinfo?(tom)
See Also: → 1430173
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.
Attached patch patch_8_beta.patch (obsolete) — — Splinter Review 
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
Attached patch patch_8_beta.patch (obsolete) — — Splinter Review 
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
Attachment #8951060 - Attachment is obsolete: true
Flags: needinfo?(tom)
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 :(
Attachment #8951068 - Flags: review+
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)
Attachment #8951068 - Attachment is obsolete: true
(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-.
Flags: qe-verify-
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)
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?
(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.
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.
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..
status-firefox-esr52: affected → ---
tracking-firefox-esr52: 59+ → ---
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).
P.S. In your next meeting, tell the Safari team about my writings, so that they can undo their 1ms imprecision.
Added suggestions of improvements to your fuzz algorithms to Bug 1432429.
Ugggg, I forget I cannot my comments on this specific system.   I meant I put my suggestions of fuzz-improvements in Bug 1425462
See Also: → 1440863
See Also: → 1448869
See Also: → 1451790
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: