Cut Performance and timestamps in general over to fine-grained RFP checks
Categories
(Core :: DOM: Performance APIs, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox109 | --- | fixed |
People
(Reporter: tjr, Assigned: tjr)
References
(Blocks 1 open bug, Regressed 1 open bug)
Details
Attachments
(32 files, 4 obsolete files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
I had thought this would be a simple patch, but as I got more and more into it I realized how expansive it was.
The goal, in general, is to cut over all ReduceTimePrecision*
callsites over to a new signature that takes in a shouldResistFingerprinting
boolean to allow fine-grained control of whether we should use RFP's timer precision, or Firefox's general timer precision.
This boolean will be provided by fine-grained calls based on the context of the call, using the newer fine-grained nsContentUtils::ShouldResistFingerprinting() functions.
Edit: See also comment 29
Assignee | ||
Comment 1•3 years ago
|
||
This will allow us to invoke the Timer Precision callback with this
boolean.
Assignee | ||
Comment 2•3 years ago
|
||
This will be needed when we set the Javascript Realm options
for Worklets.
Depends on D151283
Assignee | ||
Comment 3•3 years ago
|
||
Depends on D151297
Assignee | ||
Comment 4•3 years ago
|
||
Along the way we remove the calls to JS::SetTimeResolutionUsec.
This function is used to set values that are only used when
we don't have a ReuceTimerPrecision callback, which we always do
(as confirmed by coverage.) This in turns lets us remove some
other code.
Depends on D151298
Assignee | ||
Comment 5•3 years ago
|
||
This will be needed for the various Timestamp related
members of Animations.
Depends on D151299
Assignee | ||
Comment 6•3 years ago
|
||
Depends on D151300
Assignee | ||
Comment 7•3 years ago
|
||
Depends on D151301
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D151302
Assignee | ||
Comment 9•3 years ago
|
||
Depends on D151303
Assignee | ||
Comment 10•3 years ago
|
||
Depends on D151304
Assignee | ||
Comment 11•3 years ago
|
||
The System Principal boolean was only used for RFP purposes.
We can repurpose the same boolean for our fine-grained RFP
needs, and populate the boolean not in the Performance ctor
but rather in the CreateFor methods that will populate it
based on the context of their construction.
Depends on D151305
Assignee | ||
Comment 12•3 years ago
|
||
Depends on D151306
Assignee | ||
Comment 13•3 years ago
|
||
Depends on D151307
Assignee | ||
Comment 14•3 years ago
|
||
Depends on D151308
Updated•3 years ago
|
Assignee | ||
Comment 15•3 years ago
|
||
Depends on D151283
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 16•3 years ago
|
||
Depends on D151299
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 17•3 years ago
|
||
Depends on D151309
Assignee | ||
Comment 18•3 years ago
|
||
This way we can use it in more tests.
Depends on D156505
Assignee | ||
Comment 19•3 years ago
|
||
Depends on D156506
Assignee | ||
Comment 20•3 years ago
|
||
Depends on D156507
Assignee | ||
Comment 21•3 years ago
|
||
Rename browser_navigator_header.sjs to file_navigator_header.sjs
This way all support files for the test begin with file_ and only
test files begin with browser_
Depends on D156508
Assignee | ||
Comment 22•3 years ago
|
||
This commit moves the test description from the test file
to the shared infrastructure.
It also adds two parameters to the set of shared tests:
extraData and extraPrefs. extraData is data passed through
to the validation function (mostly) unchanged and unused.
extraPrefs are prefs that applied (and removed) before executing
the test.
Depends on D156509
Assignee | ||
Comment 23•3 years ago
|
||
Depends on D156510
Assignee | ||
Comment 24•3 years ago
|
||
Depends on D156511
Assignee | ||
Comment 25•3 years ago
|
||
Depends on D156512
Assignee | ||
Comment 26•3 years ago
|
||
Depends on D156513
Assignee | ||
Comment 27•3 years ago
|
||
We now need to check if a value is rounded to the RFP
value (16.67ms) or the baseline RTP value (default 1 ms).
To do this we pass both precisions in extraData.
We also need to communicate the expected behavior of rounding,
that is if RFP should apply for this test set. Typically we
would know this based on the expectedValues which are supplied
to the testFunction (as in the navigator test) - but here we
don't know the expected values ahead of time - we're testing if
timer values are properly rounded. Therefore we need to add
a boolean (in each test) that indicates if RFP is expected to
apply for this test.
Depends on D156514
Assignee | ||
Comment 28•3 years ago
|
||
Depends on D156515
Assignee | ||
Comment 29•3 years ago
•
|
||
Welcome everyone to this very large patchset you have been flagged for review on. I appreciate your attention.
As mentioned in the first comment: this patchset grew and grew until it eventually came to encompass all calls to the ReduceTimePrecision set of functions which means it affects virtually every dom-exposed timestamp everywhere.
To start at the beginning. In Bug 1450398 we (that is to say I, voluneteering on behalf of Tor) are pursuing a goal of allowing Resist Fingerprinting (RFP, privacy.resistFingerprinting, Tor's pref) to be disabled on a per-site basis, while also allowing it to be restricted to PBM and to not affect Web Extensions. It's a slow migration, and all behind a testing preference privacy.resistFingerprinting.testGranularityMask
, so nothing is directly affecting Tor or FF users who have enabled RFP today.
The starting point of this bug was to cut over the Performance objects so they didn't round the timestamp when they didn't need to. I quickly realized that actually I would need to cut over all the ReduceTimePrecision functions. Presently, they take two bools IsSystemPrincipal and CrossOriginIsolated that help them determine their behavior. As part of this cutover, these functions needed to now consider three bools: ShouldResistFingerprinting (because no longer is this a global pref, but now a context-sensitive decision where we pass in the appropriate behavior), IsSystemPrincipal, and CrossOriginIsolated. That's a lot of bools, so I opted to make it a bitfield.
I would also need to throw a ShouldRFP boolean into a JS Context, as JS will call into the ReduceTimePrecision functions also. This means Workers, Worklets, and the normal page JS Realm get some attention as well.
So the bulk of the patches for everyone besides Tim are of the form "Figure out if we should resist fingerprinting in this context by using the new ShouldRFP functions and then use a new helper function GetRTPCallerType()
to call the ReduceTimePrecision function". You will probably want to peek at D156504 to see how that helper function works so you can be sure I'm doing the right thing in your particular area of expertise.
I have try nearly completely green, I'm just doing some double checking that these are true intermittents not related to the patch.
As far as testing goes, all tests that previously tested that timestamps are clamped appropriately should still pass. Even better if they tested Cross Origin Isolation or RFP. Tests that did RFP sometimes needed to be refactored, in particular this test which tests performance.now, event timestamp, JS' Date, audiocontext, and File's timestamp was completely rewritten to test things under third party iframes respecting RFP being exempted on different domains related to the frame/framer - so all that is pretty well tested.
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Updated•3 years ago
|
Assignee | ||
Comment 32•3 years ago
|
||
For the less common subclasses we will hardcode a choice to always obey
RFP if the pref is enabled.
Assignee | ||
Comment 33•3 years ago
|
||
This does not remove mShouldResistFingerprinting from
WorkerPrivate->mLoadInfo. This is because in between
mLoadInfo getting initialized (including where shouldRFP gets
set) and when the worker's global scope is constructed, we
need to keep track of whether we shouldRFP or not. We could
make this single bool an outparam of GetLoadInfo to later
pass to GetOrCreateGlobalScope, but that seems noisy.
Depends on D157562
Assignee | ||
Comment 34•3 years ago
|
||
This centralizes the logic in one place.
Depends on D151299
Assignee | ||
Comment 35•3 years ago
|
||
Depends on D151309
Assignee | ||
Comment 36•3 years ago
|
||
Depends on D156516
Assignee | ||
Comment 37•3 years ago
|
||
Depends on D157567
Assignee | ||
Comment 38•3 years ago
|
||
Depends on D157564
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 39•3 years ago
|
||
I'm not going to land this patch, instead I'm going
to rebase it into the others like it was always this
way. But I wanted to get your r+ on it this way because
I thought that'd be easier to understand.
tl;dr: RTPCallerType on Workers (and probably Worklets)
was broken because they were off main thread.
Basically I was getting test failures that were not directly
attributed to this, but rather exposed the fact that RTPCaller
type wasn't working in workers. I eventually (took a looooong
time) realized that was because nsIGlobalObject::RTPCallerType
was using PrincipalOrNull() and if you're off main thread -
that's null.
So I let RTPCallerType be overrideen in subclasses, and override
it for Workers and Worklets. I tried to not duplicate code with
a second function, but it couldn't be used for Workers.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 40•3 years ago
|
||
Comment 41•3 years ago
|
||
Backed out for causing bp-hybrid bustages on nsIPrincipal.h.
Failure log: https://treeherder.mozilla.org/logviewer?job_id=397864954&repo=autoland
Backout link: https://hg.mozilla.org/integration/autoland/rev/e1a6c7adbe777a98a3bbe914a677a7768367b0dd
Comment 42•3 years ago
|
||
Comment 43•3 years ago
|
||
Backed out for causing build bustages at nsIGlobalObject.h.
Backout link: https://hg.mozilla.org/integration/autoland/rev/9e2c53fb252cbb56938e3e30d01f2380e48d4fdf
Failure log: https://treeherder.mozilla.org/logviewer?job_id=397951386&repo=autoland&lineNumber=5128
Comment 44•3 years ago
|
||
Comment 45•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d73ee2e40c93
https://hg.mozilla.org/mozilla-central/rev/91dd29cbf16d
https://hg.mozilla.org/mozilla-central/rev/bd4251cc135f
https://hg.mozilla.org/mozilla-central/rev/d51ead218a89
https://hg.mozilla.org/mozilla-central/rev/1a27e6f8f9d8
https://hg.mozilla.org/mozilla-central/rev/32fdf6f014f7
https://hg.mozilla.org/mozilla-central/rev/a8e623b228ae
https://hg.mozilla.org/mozilla-central/rev/aba9097fa0f1
https://hg.mozilla.org/mozilla-central/rev/4bc2e3050d75
https://hg.mozilla.org/mozilla-central/rev/866c3dfbcc33
https://hg.mozilla.org/mozilla-central/rev/8aff2d2c67eb
https://hg.mozilla.org/mozilla-central/rev/61a0ae3e24bd
https://hg.mozilla.org/mozilla-central/rev/5c5f879f5a0f
https://hg.mozilla.org/mozilla-central/rev/58335f848eed
https://hg.mozilla.org/mozilla-central/rev/c89df8e0e89d
https://hg.mozilla.org/mozilla-central/rev/264391f1a7fe
https://hg.mozilla.org/mozilla-central/rev/7c10596dbad5
https://hg.mozilla.org/mozilla-central/rev/6005bf5a4eb7
https://hg.mozilla.org/mozilla-central/rev/5e3f387086a8
https://hg.mozilla.org/mozilla-central/rev/12ca06bc5608
https://hg.mozilla.org/mozilla-central/rev/1d48979d8fb2
https://hg.mozilla.org/mozilla-central/rev/e1058e60dc91
https://hg.mozilla.org/mozilla-central/rev/6bd6ae7136e8
https://hg.mozilla.org/mozilla-central/rev/f1472a04734c
https://hg.mozilla.org/mozilla-central/rev/cdacd3a4029e
https://hg.mozilla.org/mozilla-central/rev/dae0d3984525
https://hg.mozilla.org/mozilla-central/rev/d775da74592f
https://hg.mozilla.org/mozilla-central/rev/d1a12c6b46fb
https://hg.mozilla.org/mozilla-central/rev/9028c4700078
https://hg.mozilla.org/mozilla-central/rev/c4adefaadbb3
https://hg.mozilla.org/mozilla-central/rev/1635299da306
https://hg.mozilla.org/mozilla-central/rev/3ea8716ecf69
Assignee | ||
Updated•3 years ago
|
Description
•