Closed
Bug 1440539
Opened 7 years ago
Closed 7 years ago
Add Jitter to the JS Shell
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: tjr, Assigned: tjr)
References
Details
Attachments
(1 file)
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•7 years ago
|
||
I will only ask for a flag to disable the jitter during benchmarks.
Comment 2•7 years ago
|
||
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.
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tom
Comment 7•7 years ago
|
||
mozreview-review |
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.
Attachment #8955633 -
Flags: review?(luke) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 8•7 years ago
|
||
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)
Pushed by archaeopteryx@coole-files.de:
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
Keywords: checkin-needed
Comment 10•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•