Closed Bug 1443943 Opened 2 years ago Closed 2 years ago

Only Jitter Timestamps in non-System Principal Code

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

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 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 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)
mozreview clearing flags on me...
Flags: needinfo?(nika)
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 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 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+
Attachment #8957653 - Flags: review?(bugs)
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 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-
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)
checking principal.
I added significantly to the second patch to affect the rest of the performance APIs.
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+
(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 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+
(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.)
Depends on: 1444588
No longer blocks: 1439875
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+
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 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).
(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.
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 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 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+
No longer blocks: 1447685
Depends on: 1447685
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53d478be2796
Ensure redirect SJS' serve the correct content types. r=jya
(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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.