Closed Bug 1586761 Opened 4 years ago Closed 3 years ago

Check crossOriginIsolated for all nsRFPService::ReduceTimePrecision* callers

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla77
Tracking Flags:
Tracking Status
firefox77 --- fixed

People

(Reporter: annevk, Assigned: tt)

References

(Blocks 2 open bugs)

Details

Attachments

(9 files, 2 obsolete files)

Bug 1586761 - P1 - Expose CrossOriginIsolated to nsIGlobalObject;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1586761 - P2 - Introduce new TimerPrecisionTypes and a set of new Reduce methods to decide the TimerPrecisionType in the nsRFPService;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1586761 - P3 - Use new methods in dom/performance;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1586761 - P4 - Use new methods in dom/file and dom/events;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1586761 - P5 - Use new methods in media
47 bytes, text/x-phabricator-request
Details | Review
Bug 1586761 - P6 - Check CrossOriginIsolated for ReduceTimePrecisionAsUSecsWrapper;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1586761 - P7 - Revert a few incorrect changes on P2;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1586761 - P8 - Set setClampAndJitterTime to false for ProxyAutoConfig to avoid getting the global from a JSContext on the parent process;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1586761 - P9 - Revert a change for performance.now to avoid causing functions in pkcs11f.h to fail;
47 bytes, text/x-phabricator-request
Details | Review

Once bug 1477743 lands we have the infrastructure in place to bring back a high-resolution performance.now() if the correct headers are set and a COOP+COEP process is created. I.e., a process that allows SharedArrayBuffer in postMessage().

This would be good to to do at that point as making folks use SharedArrayBuffer as a workaround seems cumbersome and wasteful.

Tom, apparently there's a single switch in our code protecting high-resolution timers such as performance.now(). That's probably what we want to toggle here. Could you provide some context for whoever ends up working on this?

Flags: needinfo?(tom)

So presently all our code looks like this:

if(!systemPrinciapl) {
   timestamp = nsRFPService::ReduceTimePrecision*(timestamp,  ..., TimerPrecisionType:All);
}

Inside nsRFPService::ReduceTimePrecision* (which call nsRFPService::ReduceTimePrecisionImpl) we check our prefs and decided what to do to the timestamp; in order:

  1. privacy.resistFingerprinting enabled - clamp to 100ms or the value of privacy.resistFingerprinting.reduceTimerPrecision.microseconds - whichever is greater
  2. privacy.reduceTimerPrecision enabled - clamp to the value of privacy.resistFingerprinting.reduceTimerPrecision.microseconds
  3. privacy.reduceTimerPrecision.unconditional enabled - clamp to 5usec
  4. Nothing enabled - do not clamp
  • privacy.resistFingerprinting.reduceTimerPrecision.jitter enabled - perform jitting if (1) or (2) from above is active.

We pass a TimerPrecision Type into the function to indicate that for a few callsites in Animations, we only clamp for RFP mode and nothing else.

For this bug I think what we'll want to do is change

if(!systemPrinciapl) {
   timestamp = nsRFPService::ReduceTimePrecision*(timestamp,  ..., TimerPrecisionType:All);
}

into

auto precisionType = systemContext ? 
                                       TimerPrecisionType:System : 
                                       (COOPCOEPPass ? TimerPrecisionType:HighResAllowed : TimerPrecisionType:Normal)
timestamp = nsRFPService::ReduceTimePrecision*(timestamp, ..., precisionType);
// ::Normal is renamed from ::All
// The ergonomics of this leave a bit to be desired - open to suggestions.

And then inside ReduceTimePrecisionImpl

  1. TimerPrecisionType:System - no clamping
  2. TimerPrecisionType:HighResAllowed - clamp to 5usec if privacy.reduceTimerPrecision.unconditional else no clamping
  3. TimerPrecisionType:Normal / TimerPrecisionType:RFPOnly - the current behavior from above
Flags: needinfo?(tom)

Minor change since last I looked at this: COOPCOEPPass above is now CrossOriginIsolated.

Flags: needinfo?(ttung)
Assignee: nobody → ttung
Status: NEW → ASSIGNED
Flags: needinfo?(ttung)
Attachment #9127366 - Attachment description: Bug 1586761 - WIP for performanc.now; → Bug 1586761 - P1 - Expose CrossOriginIsolated to nsIGlobalObject;
Attachment #9127478 - Attachment description: Bug 1586761 - P2 - Check CrossOriginIsIsolated in performance.now(); → Bug 1586761 - P2 - Introduce new TimerPrecisionTypes and a set of new Reduce methods to decide the TimerPrecisionType in the nsRFPService;

Note that this patch implements a member function CrossOriginIsIsolated in
PerformanceWorker and PerformanceMainThread. In PerformanceMainThread, we need
to cache boolean for CrossOriginIsIsolated() so that we don't need to find the
owning global on every callsites.

Depends on D63293

For the usecases in dom/file, I'm not so sure how to get the global at
callsites, so I let aIsSystemPrincipal and aCrossOriginIsolated to be false for
now.

Depends on D63904

Depends on D63905

Attachment #9127529 - Attachment is obsolete: true
See Also: → 1617872
Attachment #9129742 - Attachment is obsolete: true

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3666d40e119c2c6e35db86242d8e1b421404e10e

if this looks good, I will file a follow-up bug for testing and consider landing these patches.

(In reply to Tom Tung [:tt, :ttung] from comment #16)

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3666d40e119c2c6e35db86242d8e1b421404e10e

if this looks good, I will file a follow-up bug for testing and consider landing these patches.

Not sure if the ni in Phabricator works or not so I needinfo :tjr again here.
:tjr, changing if (!unconditionalClamping && aContextMixin == 0 && aType == TimerPrecisionType::All && timeAsInt < kFeb282008) to if (aContextMixin == 0 && timeAsInt < kFeb282008 && !unconditionalClamping) breaks most of jobs on try.

For instance, if I run "dom/animation/test/mozilla/test_deferred_start.html". It would trigger this call and the current holding time is 0. Since the aContextMixin is 0 in this case, it entered the condtion (aContextMixin == 0 && timeAsInt(which is 0) < kFeb282008).

Should I check whether the TimerPrecisionType is normal or not here? Or, do you have any suggestions for fixing this? I will address that on another patch, then.

Flags: needinfo?(tom)
Blocks: 1621677

Sorry, I typed a response in phab but didn't submit it....

Flags: needinfo?(tom)

While running an xpcshell test (netwerk/test/unit/test_protocolproxyservice.js), I hit this assertion(https://searchfox.org/mozilla-central/rev/7d0c94a0e9a9fe1f83553f49b10128567d21709d/js/xpconnect/wrappers/WrapperFactory.cpp#841-845).

The log for that is: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=292691966&repo=try&lineNumber=3506

I believe this is caused by D64324 and it probably means I cannot access the global object or context on non-main thread.
Jan, I found you have touched the code around, so I guess you might know why I hit this assertion? Could you help me to check if my understanding is correct? Or, if it's incorrect, maybe you have more insight into the meaning of that assertion?

If it's, then one solution I can think of is to let RealmCreationOptions carry the CrossOriginIsolated when it's constructed. Or, do you have any suggestions for fixing it? The goal is to check crossOriginIsolated from the global object.

Thanks in advance!

Flags: needinfo?(jdemooij)

try after applying tjr's comment: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78ea7e7be38d0fc975024f13c70c7a074d6addb5&selectedJob=292861726

So there are two issues so far and I mentioned one in comment#20.

Another is /web-animations/timing-model/animations/setting-the-start-time-of-an-animation.html, I will check this tomorrow.

When we removed IsTimerPrecisionReductionEnabled we also removed the check that looked at the RFP pref.

So in ReduceTimePrecisionImpl you should add

 bool unconditionalClamping = false;
+if (aType == RFP  && !StaticPrefs::privacy_resistfingerprinting) {
+   aType = UnconditionalAKAHighRes;
+}
 if (aType == UnconditionalAKAHighRes || TimerResolution() <= 0) {

(ni? to ensure you see the comment)

Flags: needinfo?(ttung)

(In reply to Tom Ritter [:tjr] (ni for response to sec-[approval|rating|advisories|cve]) from comment #22)

When we removed IsTimerPrecisionReductionEnabled we also removed the check that looked at the RFP pref.

So in ReduceTimePrecisionImpl you should add

 bool unconditionalClamping = false;
+if (aType == RFP  && !StaticPrefs::privacy_resistfingerprinting) {
+   aType = UnconditionalAKAHighRes;
+}
 if (aType == UnconditionalAKAHighRes || TimerResolution() <= 0) {

(ni? to ensure you see the comment)

Ah, that fixes the second issue in comment 21. I will prepare one more patch to make it easier to be reviewed. Thanks!

Flags: needinfo?(ttung)

P2 removed IsTimerPrecisionReductionEnabled and thus removed the check for RFP
pref. While most ReduceTimePrecision* functions are fine with that because
GetTimerPrecisionType checks that, the two ReduceTimePrecision*RFP functions
miss the check.

This patch mainly cover the check for that two functions and rename them to
*RFPOnly since they only use RFP when the pref is on.

Attachment #9133118 - Attachment description: Bug 1586761 - P7 - Revert a few incorrect change on P2; → Bug 1586761 - P7 - Revert a few incorrect changes on P2;

(In reply to Tom Tung [:tt, :ttung] from comment #25)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88ebb117d7c12572b7b1e8184cb3523c63cec8b8

The Gtest failed because it hit the debug assertion I added to ensure the pref for "RFP" enables when it's TimerPrecisionType::RFP. I have a patch to fix it locally.

/css/css-animations/CSSAnimation-startTime.tentative.html failed. I suspect it's because the logic gets change due to P2. I will check the logic in ReduceTimePrecisionImpl again.

(In reply to Tom Tung [:tt, :ttung] from comment #20)

I believe this is caused by D64324 and it probably means I cannot access the global object or context on non-main thread.
Jan, I found you have touched the code around, so I guess you might know why I hit this assertion? Could you help me to check if my understanding is correct? Or, if it's incorrect, maybe you have more insight into the meaning of that assertion?

Sorry for the delay, I didn't notice this NI last week. Unfortunately Treeherder is down right now so I can't look at the log, but as far as I can tell it should be fine to call NativeGlobal on worker threads. It would really help to see a stack trace, I'll keep the NI and look at that when Treeherder is back online...

Oh bleh, it's the ProxyAutoConfig thread :/ Yeah, that's a JS runtime that's completely independent from other DOM machinery so we can't use xpc::NativeGlobal there. Using RealmCreationOptions is probably our safest option here.

Flags: needinfo?(jdemooij)

(In reply to Jan de Mooij [:jandem] from comment #28)

Oh bleh, it's the ProxyAutoConfig thread :/ Yeah, that's a JS runtime that's completely independent from other DOM machinery so we can't use xpc::NativeGlobal there. Using RealmCreationOptions is probably our safest option here.

I see. Thanks for the explanation!

It seems that ProxyAutoConfig is the main process thing which means even if we are in the main thread the crossOriginIsolated is false in theory. Also, if it's independent from DOM machinery, then I guess that time cannot go backward in this case (we were worried about this because timers are clamped by different factors).

Luke, D64324 doesn't take care of all the cases properly. For instance, this can be called on ProxyAutoConfig thread. I will prepare a patch to fix that, but I have some questions:

  • Is this the only case that is not running on the main thread or a worker thread? If it's the only case which we need to take care of additionally, then maybe we can check if it's in the main process or not. Or something like that.
  • Is there a way to check if we can get a global from a JSContext so that we can assume the CrossOriginIsolated is false when the check fails rather than hitting an assertion.
Flags: needinfo?(luke)
  • ProxyAutoConfig is the only other one I'm aware of. (searchfox seems to confirm.)
  • Can you point me to the particular JS API you're using to get a global? In general, JSContexts don't "have a global", but can be queried for the currently-executing-global in various ways, but that can be "none" for a variety of reasons (like no running code).
Flags: needinfo?(luke)

(In reply to Luke Wagner [:luke] from comment #30)

I see. (I just learn that we can know this by checking the callers for JS_NewContext. Thanks!)

  • Can you point me to the particular JS API you're using to get a global? In general, JSContexts don't "have a global", but can be queried for the currently-executing-global in various ways, but that can be "none" for a variety of reasons (like no running code).

I'm using CurrentNativeGlobal(cx) to get a global from given JSContext in D64324 line 607 in toolkit/components/resistfingerprinting/nsRFPService.cpp.

Then, I guess I should probably do:

// toolkit/components/resistfingerprinting/nsRFPService.cpp
  nsCOMPtr<nsIGlobalObject> global = xpc::CurrentNativeGlobal(aCx);
-  MOZ_ASSERT(global);
+ const bool crossOriginIsolated = global ? global->CrossOriginIsolated() : false;
  const auto type = GetTimerPrecisionType(/* aIsSystemPrincipal */ false,
-                                         global->CrossOriginIsolated());
+.                                        crossOriginIsolated

However, I would still hit this assertion. Do you have any suggestion to avoid hitting this debug assertion? (like checking thread or attribute of the JSContext)

Flags: needinfo?(luke)

Maybe you could add something like this:

bool xpc::HasNativeGlobal(JSContext* cx) {
  JSObject* obj = CurrentGlobalOrNull(cx);
  constexpr unsigned flags = JSCLASS_PRIVATE_IS_NSISUPPORTS | JSCLASS_HAS_PRIVATE;
  return (GetObjectClass(obj)->flags & flags) == flags || dom::UnwrapDOMObjectToISupports(obj);
}

Ideally sharing code with the MOZ_ASSERT in NativeGlobal.

(In reply to Jan de Mooij [:jandem] from comment #32)

Maybe you could add something like this:

bool xpc::HasNativeGlobal(JSContext* cx) {
  JSObject* obj = CurrentGlobalOrNull(cx);
  constexpr unsigned flags = JSCLASS_PRIVATE_IS_NSISUPPORTS | JSCLASS_HAS_PRIVATE;
  return (GetObjectClass(obj)->flags & flags) == flags || dom::UnwrapDOMObjectToISupports(obj);
}

Ideally sharing code with the MOZ_ASSERT in NativeGlobal.

Ah, I see. (I wasn't so sure if I can do this since I don't have enough understanding of the code around there) Will provide a patch then. Thanks!

Or it might be simpler to initialize CrossOriginIsolated to false if XRE_IsParentProcess() is true, without calling NativeGlobal in that case?

(In reply to Jan de Mooij [:jandem] from comment #34)

Or it might be simpler to initialize CrossOriginIsolated to false if XRE_IsParentProcess() is true, without calling NativeGlobal in that case?

Yes, I have considered that but that probably depends on whether we will have another exception on the content process. Because it assumes the only exception (which is ProxyAutoConfig) is on the parent process.

That's why I'm asking if there is a preferable way to fix this.

Summary: High-resolution performance.now() → Check crossOriginIsolated for all nsRFPService::ReduceTimePrecision* callers

I think it makes sense for IsCrossOriginIsolated to return false when !JS::CurrentGlobalOrNull(cx); that condition might arise within the content process in corner cases and it makes sense to default to safety.

However, within the parent process, we shouldn't ever be executing untrusted content JS. However, we might be running trusted JS that may legitimately want to use SAB and I think we don't have to guard against Spectre attacks in this situation. So I think it'd be fine to return true when XRE_IsParentProcess().

Flags: needinfo?(luke)
Attachment #9134353 - Attachment description: Bug 1586761 - P8 - Expose a new method HasCurrentNativeGlobal and use it in nsRFPService.cpp; → Bug 1586761 - P8 - Treat crossOriginIsolated as true in nsRFPService when it's in the parent process;

(In reply to Luke Wagner [:luke] from comment #37)

Thanks for the feedback! I update P8 to address the comment.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb2e8d1db3a17a851b1d7ecb7ac719064019914b

Hopefully, the remaining test failures are in toolkit/components/passwordmgr/test/unit/ and I'm investigating why my patches cause them to fail.

(In reply to Tom Tung [:tt, :ttung] from comment #38)

Hopefully, the remaining test failures are in toolkit/components/passwordmgr/test/unit/ and I'm investigating why my patches cause them to fail.

I haven't figured the reason why but it's caused by P3 and the code below:

 DOMHighResTimeStamp Performance::Now() {
   DOMHighResTimeStamp rawTime = NowUnclamped();
-  if (mSystemPrincipal) {
-    return rawTime;
-  }
-
-  return nsRFPService::ReduceTimePrecisionAsMSecs(rawTime,
-                                                  GetRandomTimelineSeed());
+  return nsRFPService::ReduceTimePrecisionAsMSecs(
+      rawTime, GetRandomTimelineSeed(), mSystemPrincipal,
+      CrossOriginIsolated());
 }

I tried to get some information in a failed test (test_LoginManagerParent_doAutocompleteSearch.js) and I got:

  • It's about os.file.copy and the code which uses performace.now()
  • !!mSystemPrincipal is true in Performance::Now, so ReduceTimePrecisionAsMSecs returns the same value as rawTime.
  • I haven't figured out why but if I remove the if-check and returns rawTime by ReduceTimePrecisionAsMSecs as what the change does, it takes longer between performance.mark and performance.measure and that seems to cause the unit test to fail.

(In reply to Tom Tung [:tt, :ttung] from comment #39)

  • I haven't figured out why but if I remove the if-block and returns rawTime by ReduceTimePrecisionAsMSecs as what the change does, it takes longer between performance.mark and performance.measure and that seems to cause the unit test to fail.

Hmm, if I revert the change here, there is no test failure....

Attachment #9134353 - Attachment description: Bug 1586761 - P8 - Treat crossOriginIsolated as true in nsRFPService when it's in the parent process; → Bug 1586761 - P8 - Set setClampAndJitterTime to false for ProxyAutoConfig to avoid getting the global from a JSContext on the parent process;
Priority: -- → P2
Blocks: 1628021
Pushed by ttung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b51663641450
P1 - Expose CrossOriginIsolated to nsIGlobalObject; r=baku
https://hg.mozilla.org/integration/autoland/rev/85317d582166
P2 - Introduce new TimerPrecisionTypes and a set of new Reduce methods to decide the TimerPrecisionType in the nsRFPService; r=tjr
https://hg.mozilla.org/integration/autoland/rev/1e09b1a3a7ee
P3 - Use new methods in dom/performance; r=tjr,baku
https://hg.mozilla.org/integration/autoland/rev/f185ed882616
P4 - Use new methods in dom/file and dom/events; r=baku
https://hg.mozilla.org/integration/autoland/rev/8d554870dce8
P5 - Use new methods in media r=tjr,baku
https://hg.mozilla.org/integration/autoland/rev/886102df9840
P6 - Check CrossOriginIsolated for ReduceTimePrecisionAsUSecsWrapper; r=tjr,luke
https://hg.mozilla.org/integration/autoland/rev/228fb5abeca5
P7 - Revert a few incorrect changes on P2; r=tjr
https://hg.mozilla.org/integration/autoland/rev/8a727b228708
P8 - Set setClampAndJitterTime to false for ProxyAutoConfig to avoid getting the global from a JSContext on the parent process; r=luke,tjr
https://hg.mozilla.org/integration/autoland/rev/6819eae19dfd
P9 - Revert a change for performance.now to avoid causing functions in pkcs11f.h to fail; r=tjr

This is great, thanks all and Tom in particular! I filed https://github.com/w3c/hr-time/issues/89 to get this standardized.

See Also: → 1628753
Regressions: 1804963
You need to log in before you can comment on or make changes to this bug.