Check crossOriginIsolated for all nsRFPService::ReduceTimePrecision* callers
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox77 | --- | fixed |
People
(Reporter: annevk, Assigned: tt)
References
(Blocks 2 open bugs)
Details
Attachments
(9 files, 2 obsolete files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
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.
Reporter | ||
Comment 1•4 years ago
|
||
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?
Comment 2•4 years ago
|
||
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:
- privacy.resistFingerprinting enabled - clamp to 100ms or the value of privacy.resistFingerprinting.reduceTimerPrecision.microseconds - whichever is greater
- privacy.reduceTimerPrecision enabled - clamp to the value of privacy.resistFingerprinting.reduceTimerPrecision.microseconds
- privacy.reduceTimerPrecision.unconditional enabled - clamp to 5usec
- 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
- TimerPrecisionType:System - no clamping
- TimerPrecisionType:HighResAllowed - clamp to 5usec if privacy.reduceTimerPrecision.unconditional else no clamping
- TimerPrecisionType:Normal / TimerPrecisionType:RFPOnly - the current behavior from above
Reporter | ||
Comment 3•4 years ago
|
||
Minor change since last I looked at this: COOPCOEPPass above is now CrossOriginIsolated.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
Depends on D63238
Assignee | ||
Comment 6•4 years ago
|
||
Assignee | ||
Comment 7•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 8•4 years ago
|
||
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
Assignee | ||
Comment 9•4 years ago
|
||
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
Assignee | ||
Comment 10•4 years ago
|
||
Depends on D63905
Updated•4 years ago
|
Assignee | ||
Comment 11•4 years ago
|
||
Assignee | ||
Comment 12•4 years ago
|
||
Assignee | ||
Comment 13•4 years ago
|
||
Assignee | ||
Comment 14•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 15•4 years ago
|
||
Assignee | ||
Comment 16•4 years ago
|
||
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.
Assignee | ||
Comment 17•4 years ago
|
||
I probably misinterpret this suggection, try before change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74a8bfa22e7086c13584c47453f177c5f2f0adda
Assignee | ||
Comment 18•4 years ago
|
||
(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.
Comment 19•4 years ago
|
||
Sorry, I typed a response in phab but didn't submit it....
Assignee | ||
Comment 20•4 years ago
|
||
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!
Assignee | ||
Comment 21•4 years ago
|
||
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.
Comment 22•4 years ago
|
||
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)
Assignee | ||
Comment 23•4 years ago
|
||
(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!
Assignee | ||
Comment 24•4 years ago
|
||
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.
Updated•4 years ago
|
Assignee | ||
Comment 25•4 years ago
|
||
Assignee | ||
Comment 26•4 years ago
|
||
(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.
Comment 27•4 years ago
|
||
(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...
Comment 28•4 years ago
|
||
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.
Assignee | ||
Comment 29•4 years ago
|
||
(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.
![]() |
||
Comment 30•4 years ago
|
||
- 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).
Assignee | ||
Comment 31•4 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #30)
- ProxyAutoConfig is the only other one I'm aware of. (searchfox seems to confirm.)
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
)
Comment 32•4 years ago
•
|
||
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.
Assignee | ||
Comment 33•4 years ago
|
||
(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!
Comment 34•4 years ago
|
||
Or it might be simpler to initialize CrossOriginIsolated to false if XRE_IsParentProcess() is true, without calling NativeGlobal in that case?
Assignee | ||
Comment 35•4 years ago
•
|
||
(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.
Assignee | ||
Comment 36•4 years ago
|
||
Depends on D66734
Assignee | ||
Updated•4 years ago
|
![]() |
||
Comment 37•4 years ago
|
||
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()
.
Updated•4 years ago
|
Assignee | ||
Comment 38•4 years ago
|
||
(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.
Assignee | ||
Comment 39•4 years ago
•
|
||
(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 usesperformace.now()
!!mSystemPrincipal
istrue
inPerformance::Now
, soReduceTimePrecisionAsMSecs
returns the same value asrawTime
.- I haven't figured out why but if I remove the if-check and returns
rawTime
byReduceTimePrecisionAsMSecs
as what the change does, it takes longer betweenperformance.mark
andperformance.measure
and that seems to cause the unit test to fail.
Assignee | ||
Comment 40•4 years ago
|
||
(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
byReduceTimePrecisionAsMSecs
as what the change does, it takes longer betweenperformance.mark
andperformance.measure
and that seems to cause the unit test to fail.
Hmm, if I revert the change here, there is no test failure....
Updated•4 years ago
|
Reporter | ||
Updated•4 years ago
|
Assignee | ||
Comment 41•4 years ago
|
||
Depends on D67446
Comment 42•4 years ago
|
||
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
Comment 43•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b51663641450
https://hg.mozilla.org/mozilla-central/rev/85317d582166
https://hg.mozilla.org/mozilla-central/rev/1e09b1a3a7ee
https://hg.mozilla.org/mozilla-central/rev/f185ed882616
https://hg.mozilla.org/mozilla-central/rev/8d554870dce8
https://hg.mozilla.org/mozilla-central/rev/886102df9840
https://hg.mozilla.org/mozilla-central/rev/228fb5abeca5
https://hg.mozilla.org/mozilla-central/rev/8a727b228708
https://hg.mozilla.org/mozilla-central/rev/6819eae19dfd
Reporter | ||
Comment 44•4 years ago
|
||
This is great, thanks all and Tom in particular! I filed https://github.com/w3c/hr-time/issues/89 to get this standardized.
Description
•