1440539 - Add Jitter to the JS Shell

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

In Bug 1425462 we're adding a callback so the JS Engine relies on the ResistFingerprinting Component to centralize the logic time precision reduction and jittering.

Up until now we've had time precision reduction done in the JS Engine also. But we aren't adding jitter logic in the JS Engine in Bug 1425462.  In this bug we're going to add a non-cryptographyically secure jitter algorithm exclusively for the JS shell. This will give us behavior parity between the browser and the shell for testing; although obviously not security-parity.

Comment 1

[Nicolas B. Pierron [:nbp]]

I will only ask for a flag to disable the jitter during benchmarks.

Comment 2

[Lars T Hansen [:lth]]

We have some tests that observe the time and may sensitive to jitter, so we should be on the lookout for issues.  The atomics tests (for better and worse) use sleep() and similar methods to coordinate actions among threads; it would be helpful if sleep(), which is only a shell function, did not have significant jitter - it's dicey enough as it is.

Comment 3

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1440539 Support time jitter in the JS Shell, and expose a function to enable it

This adds jittering to the already existing logic for time clamping. It also exposes a
testing function allowing those interested to enable time clamping or time clamping and
jittering.

Neither (clamping nor jittering) is enabled by default.

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

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8955633 [details]
Bug 1440539 Support time jitter in the JS Shell, and expose a function to enable it

https://reviewboard.mozilla.org/r/224726/#review230752

::: js/src/builtin/TestingFunctions.cpp:4940
(Diff revision 1)
> +SetTimeResolution(JSContext* cx, unsigned argc, Value* vp)
> +{
> +   CallArgs args = CallArgsFromVp(argc, vp);
> +   RootedObject callee(cx, &args.callee());
> +
> +   args.rval().setUndefined();

nit: Could you move this to right before the 'return true' to match the general style of setting the return value right before the return?  (Note: args.rval() is ignored on 'return false')

::: js/src/builtin/TestingFunctions.cpp:4943
(Diff revision 1)
> +   RootedObject callee(cx, &args.callee());
> +
> +   args.rval().setUndefined();
> +
> +   if (args.length() < 2) {
> +      ReportUsageErrorASCII(cx, callee, "Wrong number of arguments");

nit: can you use args.requireAtLeast() instead?

::: js/src/jsdate.cpp:1326
(Diff revision 1)
> +            // with trying to prevent an attacker from calculating the midpoint themselves.
> +            // So we use a very simple, very fast CRC with a hardcoded seed.
> +
> +            uint64_t midpoint = *((uint64_t*)&clamped);
> +            midpoint ^= 0x0F00DD1E2BAD2DED; // XOR in a 'secret'
> +            // MurmurHash3 from chromium//third_party/WebKit/Source/platform/TimeClamper.cpp

Searchfoxing murmur just now, it seems we have a few implementations lying around.  Could we perhaps hoist one of them into mfbt (or use something already in mfbt)?

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 8955633 [details]
Bug 1440539 Support time jitter in the JS Shell, and expose a function to enable it

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

Comment 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 8955633 [details]
Bug 1440539 Support time jitter in the JS Shell, and expose a function to enable it

https://reviewboard.mozilla.org/r/224726/#review230752

> Searchfoxing murmur just now, it seems we have a few implementations lying around.  Could we perhaps hoist one of them into mfbt (or use something already in mfbt)?

It could, but I am loathe to do so in this bug. Technically we are not using MurmurHash3, but one of its internal functions. Even if we abstract Murmur in mfbt, exposing an internal function would be odd.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8955633 [details]
Bug 1440539 Support time jitter in the JS Shell, and expose a function to enable it

https://reviewboard.mozilla.org/r/224726/#review230794

Fair enough.

Comment 8

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1021640878958d55db50a684012e61fa87d2106e -d b8e64a94e635: rebasing 450473:102164087895 "Bug 1440539 Support time jitter in the JS Shell, and expose a function to enable it r=luke" (tip)
merging js/src/builtin/TestingFunctions.cpp
merging js/src/jsdate.cpp
warning: conflicts while merging js/src/jsdate.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a66754828b42
Support time jitter in the JS Shell, and expose a function to enable it. r=luke

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a66754828b42