Closed
Bug 1443943
Opened 6 years ago
Closed 6 years ago
Only Jitter Timestamps in non-System Principal Code
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: tjr, Assigned: tjr)
References
Details
Attachments
(4 files, 1 obsolete file)
We can avoid a slew of debug output and improve performance by only clamping + jittering timestamps that are exposed to web content. (Jitter is the expensive one of the two.)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
Comment on attachment 8957653 [details] Bug 1443943 Do not clamp/jitter event timestamps if it's a system principal Nika, would you know why the pointer chain I'm following in Event.cpp may null out on me, and if the approach I'm taking is okay? I wonder if it's actually the case that if the pointer chain is not in-tact, then we _are_ a System Caller, but this approach is more conservative...
Attachment #8957653 -
Flags: feedback?(nika)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → tom
Assignee | ||
Updated•6 years ago
|
Attachment #8957653 -
Flags: feedback?(nika)
Assignee | ||
Updated•6 years ago
|
Attachment #8957650 -
Flags: review?(amarchesini)
Assignee | ||
Updated•6 years ago
|
Attachment #8957651 -
Flags: review?(amarchesini)
Assignee | ||
Updated•6 years ago
|
Attachment #8957652 -
Flags: review?(amarchesini)
Assignee | ||
Comment 10•6 years ago
|
||
Comment on attachment 8957653 [details] Bug 1443943 Do not clamp/jitter event timestamps if it's a system principal This patch moves some functions around in a way that doesn't seem required. It's not technically required here, but will be needed for Bug 1443943
Attachment #8957653 -
Flags: review?(amarchesini)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8957651 [details] Bug 1443943 Port the performance APIs only to only clamping/jittering on non-System Principal https://reviewboard.mozilla.org/r/226562/#review232540 ::: dom/performance/Performance.cpp:99 (Diff revision 4) > Performance::Now() const > { > TimeDuration duration = TimeStamp::Now() - CreationTimeStamp(); > - return RoundTime(duration.ToMilliseconds()); > + DOMHighResTimeStamp minimallyClamped = RoundTime(duration.ToMilliseconds()); > + > + if (mSystemPrincipal) {}
Attachment #8957651 -
Flags: review?(amarchesini) → review+
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8957652 [details] Bug 1443943 Do not clamp/jitter in the JS Engine if it's system context https://reviewboard.mozilla.org/r/226564/#review232542 You must ask a JS peer to review this patch.
Attachment #8957652 -
Flags: review?(amarchesini)
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8957653 [details] Bug 1443943 Do not clamp/jitter event timestamps if it's a system principal https://reviewboard.mozilla.org/r/226566/#review232544 ::: dom/events/Event.cpp:1101 (Diff revision 4) > > double > -Event::TimeStampImpl() const > +Event::TimeStamp() > { > + bool isSystemCaller = false; > + // Sometimes this chain is not complete: a fair number of unit tests will assert if it is not checked. 80chars max.
Attachment #8957653 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8957653 -
Flags: review?(bugs)
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8957650 [details] Bug 1443943 Move LRUCache Initialization to startup rather than lazily https://reviewboard.mozilla.org/r/226560/#review232548 nsRFPService::RandomMidpoint should be thread-safe, but with this change it's not. What about if sCache is nullified on the main-thread when nsRFPService::RandomMidpoint runs on a different one? Here how I would fix this: 1. in nsRFPService::StartShutdown: lock sLock and nullify sCache. 2. Doing this you don't need ClearOnShutdown() any more. 3. Make LRUCache refcounted. 4. Replace StaticAutoPtr with StaticRefPtr for sCache. 5. In nsRFPService::RandomMidpoint do: RefPtr<LRUCache> cache; { StaticMutexAutoLock lock(sLock) cache = sCache; } if (!cache) { .. we are shutting down. return NS_ERROR_FAILURE; // or any other error. } and use cache instead of sCache in the rest of RandomMidpoint. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:691 (Diff revision 4) > > // Call Update here to cache the values of the prefs and set the timezone. > UpdateRFPPref(); > > + // Create the LRU Cache when we initialize, to avoid accidently trying to > + // cerate it (and call ClearOnShutdown) on a non-main-thread create
Attachment #8957650 -
Flags: review?(amarchesini) → review-
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8957653 [details] Bug 1443943 Do not clamp/jitter event timestamps if it's a system principal https://reviewboard.mozilla.org/r/226566/#review232622 ::: dom/events/Event.cpp:1103 (Diff revision 4) > -Event::TimeStampImpl() const > +Event::TimeStamp() > { > + bool isSystemCaller = false; > + // Sometimes this chain is not complete: a fair number of unit tests will assert if it is not checked. > + // If we can't check, we assume we are not in chrome code. > + if (mPresContext && mPresContext->Document() && mPresContext->Document()->NodePrincipal()) This works only in main thread, not in workers. And even in main thread mOwner should be used. Not every event has prescontext. I'm a bit worried about performance here in general.
Attachment #8957653 -
Flags: review?(bugs) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•6 years ago
|
||
I think I've corrected all the issues noted. (In reply to Andrea Marchesini [:baku] from comment #23) > nsRFPService::RandomMidpoint should be thread-safe, but with this change > it's not. > What about if sCache is nullified on the main-thread when > nsRFPService::RandomMidpoint runs on a different one? > > Here how I would fix this: Thank you for the explicit detail: I struggled for a while in another bug trying to figure out how to avoid shutdown leaks with this; your suggestion worked first try. (In reply to Olli Pettay [:smaug] from comment #24) > This works only in main thread, not in workers. > And even in main thread mOwner should be used. > Not every event has prescontext. > > I'm a bit worried about performance here in general. Fixed. You're worried about the performance of checking the principal; or performance of the entire approach of time jittering? If it's the former, I can check once and store a boolean, assuming the principal doesn't change. If it's the latter, this bug, as well as Bug 1441157, are focused on reducing the cost of jitter.
Flags: needinfo?(nika)
Comment 31•6 years ago
|
||
checking principal.
Assignee | ||
Comment 32•6 years ago
|
||
I added significantly to the second patch to affect the rest of the performance APIs.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 37•6 years ago
|
||
mozreview-review |
Comment on attachment 8957652 [details] Bug 1443943 Do not clamp/jitter in the JS Engine if it's system context https://reviewboard.mozilla.org/r/226564/#review232806
Attachment #8957652 -
Flags: review?(luke) → review+
Assignee | ||
Comment 38•6 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #31) > checking principal. I queued up a Talos run to see if caching the principal shows a performance improvement. (Results still coming in as-of writing.) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=9f375efa6b33&newProject=try&newRevision=843a750871f0&framework=1&showOnlyComparable=1
Comment 39•6 years ago
|
||
mozreview-review |
Comment on attachment 8957650 [details] Bug 1443943 Move LRUCache Initialization to startup rather than lazily https://reviewboard.mozilla.org/r/226560/#review233142 ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:780 (Diff revision 5) > MOZ_ASSERT(NS_IsMainThread()); > > nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > > + if (sCache) { > + StaticMutexAutoLock lock(sLock); Do the opposite: StaticMutexAutoLock lock(sLock); { sCache = nullptr; } you don't need a if() stmt.
Attachment #8957650 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 40•6 years ago
|
||
(In reply to Tom Ritter [:tjr] from comment #38) > (In reply to Olli Pettay [:smaug] from comment #31) > > checking principal. > > I queued up a Talos run to see if caching the principal shows a performance > improvement. (Results still coming in as-of writing.) > > https://treeherder.mozilla.org/perf.html#/ > compare?originalProject=try&originalRevision=9f375efa6b33&newProject=try&newR > evision=843a750871f0&framework=1&showOnlyComparable=1 It doesn't seem like caching the result of the lookup has a real affect on performance. Resolved all other issues, just the last patch needs review (I added it after the first batch of reviews.)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8957650 -
Attachment is obsolete: true
Comment 51•6 years ago
|
||
mozreview-review |
Comment on attachment 8958023 [details] Bug 1443943 Allow internal callers of performance.now() to opt-out of clamping/jittering https://reviewboard.mozilla.org/r/226962/#review233264 r=me with that. ::: dom/performance/Performance.h:67 (Diff revision 5) > > virtual PerformanceStorage* AsPerformanceStorage() = 0; > > void ClearResourceTimings(); > > - DOMHighResTimeStamp Now() const; > + DOMHighResTimeStamp Now(); Keep this const?
Attachment #8958023 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Attachment #8958023 -
Flags: review?(amarchesini)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 55•6 years ago
|
||
mozreview-review |
Comment on attachment 8957652 [details] Bug 1443943 Do not clamp/jitter in the JS Engine if it's system context https://reviewboard.mozilla.org/r/226564/#review233522 r=me with a few nits addressed. Please also disable the `clampAndJitterTime` option for the JavaScript shell (in `SetStandardCompartmentOptions` in js/src/shell/js.cpp). ::: js/src/jsdate.cpp:1352 (Diff revision 9) > > bool > js::date_now(JSContext* cx, unsigned argc, Value* vp) > { > CallArgs args = CallArgsFromVp(argc, vp); > - args.rval().set(TimeValue(NowAsMillis())); > + bool clampAndJitter = JS::CompartmentCreationOptionsRef(js::GetContextCompartment(cx)).clampAndJitterTime(); Style nit: instead of repeating this line of code, please make NowAsMillis take cx as an argument. ::: js/xpconnect/src/Sandbox.cpp:1129 (Diff revision 9) > // [SecureContext] API (bug 1273687). In that case we'd call > // creationOptions.setSecureContext(true). > > + if (principal == nsXPConnect::SystemPrincipal()) { > + creationOptions.setClampAndJitterTime(false); > + } Style nit: Please remove the curly braces on this if-statement. (This is the style in this component -- sorry, there are several dumb differences for the code in js...)
Attachment #8957652 -
Flags: review?(jorendorff) → review+
Comment 56•6 years ago
|
||
mozreview-review |
Comment on attachment 8957652 [details] Bug 1443943 Do not clamp/jitter in the JS Engine if it's system context https://reviewboard.mozilla.org/r/226564/#review233536 ::: js/src/jsapi.h:2079 (Diff revision 9) > > + bool clampAndJitterTime() const { return clampAndJitterTime_; } > + CompartmentCreationOptions& setClampAndJitterTime(bool flag) { > + clampAndJitterTime_ = flag; > + return *this; > + } Please also disable the `clampAndJitterTime` option for the JavaScript shell (in `SetStandardCompartmentOptions` in js/src/shell/js.cpp).
Assignee | ||
Comment 57•6 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #55) > r=me with a few nits addressed. Please also disable the `clampAndJitterTime` > option for the JavaScript shell (in `SetStandardCompartmentOptions` in > js/src/shell/js.cpp). I didn't do this because clamping and jittering is only performed in the js shell if one manually calls setTimeResolution (from Bug 1440539) - we want to support clamping and jittering in the js shell; but it is off by default. Got the rest.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 62•6 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/a5ceb2a28cfd Port the performance APIs only to only clamping/jittering on non-System Principal r=baku https://hg.mozilla.org/integration/autoland/rev/b76d20f277db Do not clamp/jitter in the JS Engine if it's system context r=jorendorff,luke https://hg.mozilla.org/integration/autoland/rev/8d4312a248ec Do not clamp/jitter event timestamps if it's a system principal r=baku https://hg.mozilla.org/integration/autoland/rev/ba24b6cd4de9 Allow internal callers of performance.now() to opt-out of clamping/jittering r=bholley
Keywords: checkin-needed
Comment 63•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a5ceb2a28cfd https://hg.mozilla.org/mozilla-central/rev/b76d20f277db https://hg.mozilla.org/mozilla-central/rev/8d4312a248ec https://hg.mozilla.org/mozilla-central/rev/ba24b6cd4de9
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Updated•6 years ago
|
status-firefox60:
--- → affected
Assignee | ||
Comment 64•6 years ago
|
||
Comment on attachment 8957651 [details] Bug 1443943 Port the performance APIs only to only clamping/jittering on non-System Principal Approval Request Comment [Feature/Bug causing the regression]: The initial version of Jitter that landed (Bug 1425462) would jitter even system principal calls. This was silly and hurt performance. This bug fixes that and improves performance. [User impact if declined]: We might as well turn jitter off since this blocks something that's needed to make it secure. [Is this code covered by automated tests?]: Indirectly, yes. [Has the fix been verified in Nightly?]: Yes, it's baked for a few days [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: Bug 1444588 [Is the change risky?]: No. [Why is the change risky/not risky?]: There is always a risk I got the patch wrong, but it's been reviewed and people seem to think it's good. [String changes made/needed]: None
Attachment #8957651 -
Flags: approval-mozilla-beta?
Comment 65•6 years ago
|
||
Comment on attachment 8957651 [details] Bug 1443943 Port the performance APIs only to only clamping/jittering on non-System Principal timestamp work for beta60
Attachment #8957651 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 66•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/80d6f981383a https://hg.mozilla.org/releases/mozilla-beta/rev/ec9f524f70c5 https://hg.mozilla.org/releases/mozilla-beta/rev/bd233781f767 https://hg.mozilla.org/releases/mozilla-beta/rev/e3104f4386e8
Updated•6 years ago
|
Comment 67•6 years ago
|
||
Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/53d478be2796 Ensure redirect SJS' serve the correct content types. r=jya
Comment 68•6 years ago
|
||
(In reply to Pulsebot from comment #67) > Pushed by cpearce@mozilla.com: > https://hg.mozilla.org/integration/mozilla-inbound/rev/53d478be2796 > Ensure redirect SJS' serve the correct content types. r=jya Off by one error in my commit message for this bug, sorry. This commit isn't for this bug, it's for bug 1443942
Comment 69•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/53d478be2796
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•