When privacy.resistFingerprinting=true, dynamically round content dimensions
Categories
(Core :: Window Management, enhancement, P2)
Tracking
()
People
(Reporter: arthur, Assigned: tjr)
References
(Depends on 4 open bugs, Blocks 4 open bugs)
Details
(Whiteboard: [fingerprinting][fp-triaged][tor 14429])
Attachments
(7 files, 19 obsolete files)
335.53 KB,
image/png
|
Details | |
54.69 KB,
image/png
|
Details | |
4.92 KB,
patch
|
tjr
:
review+
|
Details | Diff | Splinter Review |
20.49 KB,
patch
|
tjr
:
review+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
tjr
:
review+
|
Details | Diff | Splinter Review |
8.65 KB,
patch
|
tjr
:
review+
|
Details | Diff | Splinter Review |
15.14 KB,
patch
|
tjr
:
review+
|
Details | Diff | Splinter Review |
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 1•7 years ago
|
||
Updated•7 years ago
|
Comment 2•7 years ago
|
||
Updated•7 years ago
|
Comment 3•7 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 4•6 years ago
|
||
Comment 5•6 years ago
|
||
Comment 6•6 years ago
|
||
Comment 7•6 years ago
|
||
Comment 8•6 years ago
|
||
Comment 9•6 years ago
|
||
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
Comment 12•6 years ago
|
||
Comment 13•6 years ago
|
||
Comment 14•6 years ago
|
||
Comment 15•6 years ago
|
||
Comment 16•6 years ago
|
||
Comment 17•6 years ago
|
||
Comment 18•6 years ago
|
||
Comment 19•6 years ago
|
||
Reporter | ||
Comment 20•6 years ago
|
||
Comment 21•6 years ago
|
||
Comment 22•6 years ago
|
||
Reporter | ||
Comment 23•6 years ago
|
||
Comment 24•6 years ago
|
||
Comment 25•6 years ago
|
||
Comment 26•6 years ago
|
||
Comment 27•6 years ago
|
||
Assignee | ||
Comment 28•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 29•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 30•6 years ago
|
||
Some updates regarding this bug.
- We decided to have this letterboxing feature as an experimental feature. This means it will be hidden behind a pref other than the general RFP pref.
- We would like to see how it looks like for different size of margins area. Say, would it be very terrible if we waste 5% of space in the content area by margins. So, we would add a separate pref which allows controlling the rounded content dimensions.
So far, we haven't decided yet if the letterboxing is the right approach for resolving issues of the current window dimensions protecction in RFP mode. That's the reason why this feature is beind another pref. And we'd like to do some experiments on top of this, so we want to land the basic infrastructure of the letterboxing first and modify it as the experiment goes.
Comment 31•6 years ago
|
||
This patch changes the name of LanguagePrompt.jsm to RFPHelper.jsm.
The RFPHelper is going to not only be responsible for the language
prompt but also for the window letterboxing.
In addition, this patch also changes the place where we do the
uninitialization of the RFPHelper. The RFPHelper won't do anything if we
open a new window and then close it because of RFPHelper will be
uninited during closing and there is no way to bring it back. So, we
move uninit into the nsBrowserGlue._onQuitApplicationGranted(), which
will be called during closing the application.
Comment 32•6 years ago
|
||
This patch implements the window letterboxing. The implementation
is based on adding margins around the browser element to round the
content viewport size. Whenever the browser content is resized, the
RFPHelper will adjust margins around it. But it won't add any margins
for an empty browser or a browser loads a content with the system
principal.
The letterboxing is hidden behind a hidden pref
"privacy.resistFingerprinting.letterboxing." By default, it will use
stepping size 200x100 to round content window. And we can customize
the set of dimensions used for deciding the size of the rounded
content viewport by the pref
"privacy.resistFingerprinting.letterboxing.dimensions". This pref
should be formated as 'width1xheight1, width2xheight2, ...'. We will
find the dimensions which can fit into the real content size and have
the smallest margins to be the rounded content viewport size. For example
, given the set "400x200, 500x300, 800x500" and the real content size
"600x300", we would round the content size into 500x300.
Depends on D18064
Comment 33•6 years ago
|
||
This patch adds a test for ensuring the letterboxing works as we expect.
It will open a tab and resize its window into several different sizes
and to see if the margins are correctly apply. And it will also check
that no margin should apply to a tab with chrome privilege.
Depends on D18065
Assignee | ||
Comment 34•6 years ago
|
||
I'm going to pick this up from Tim while he's on leave.
I've tweaked the patches a little bit and sent in a try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3de8da178672d09a4486173395c5b6339cec9f0
Assignee | ||
Comment 35•6 years ago
|
||
This encompasses the changes requested in the phabricator review.
Assignee | ||
Comment 36•6 years ago
|
||
Assignee | ||
Comment 37•6 years ago
|
||
Assignee | ||
Comment 38•6 years ago
|
||
Comment 39•6 years ago
|
||
Ok, thanks, I've already been looking at the patches in phabricator and will wrap this up shortly. Sorry for the delay :)
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 40•6 years ago
|
||
Comment 41•6 years ago
|
||
Assignee | ||
Comment 42•6 years ago
|
||
Assignee | ||
Comment 43•6 years ago
|
||
Assignee | ||
Comment 44•6 years ago
|
||
Assignee | ||
Comment 45•6 years ago
•
|
||
Okay - I addressed your comments and re-requested review because I needed to deal with some test failures.
- I needed to change the _isDimensionRounded check to specifically check that we rounded to the intended dimension.
- I needed to handle a really weird resize situation I explain in a comment in Patch 4. I didn't solve this at the root; but I found a way to detect when it occured and work around it in the test.
- I needed to correct a different test that never reset the zoom level
- It turns out that using border looks correct but messes with event delivery (it's offset by the border) so I need to use margin like Tim did originally.
- I added a bunch of logging that I used to debug the test that I think would be useful in the future.
I never observed any of these issues when using the pref during browsing.
A diff of the Patch 3 you reviewed to the current Patch 3 is here: https://www.diffchecker.com/CcVKKNIP
A diff of the Patch 4 you reviewed to the current Patch 4 is here: https://www.diffchecker.com/aVqay8tL
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff9f10ddcbe7ef0f8a001b0f447c137e99f267ef
Assignee | ||
Comment 46•6 years ago
|
||
So the previous try run failed on Windows/Mac. It turns out the weird resize bug only triggers on Linux. But on Windows I was seeing off-by-one errors during the test; but my manually resizing the window and using the browser toolbox they were not present...
This try run shows a successful run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d712744c981dcb46454a10251a6a353433cdbe2&selectedJob=227199620 (and I fixed the lint errors too.)
This test is definitely kind of janky. But given that this is an experimental feature we're landing so we can get feedback and test different bucket selections subjectively - and not intended for external use - I'm hopeful it's sufficient at this time. If we were to land this into RFP by 'default' (even though RFP isn't a user-facing pref itself) we could work to clean up the test.
Assignee | ||
Updated•6 years ago
|
Comment 47•6 years ago
|
||
Comment 48•6 years ago
|
||
Comment 49•6 years ago
|
||
Comment 50•6 years ago
|
||
Assignee | ||
Comment 51•6 years ago
|
||
Comment 52•6 years ago
|
||
Comment 53•6 years ago
|
||
Assignee | ||
Comment 54•6 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #53)
Resizes on Linux are asynchronous, but there is code, in Gecko and IIUC in
GTK also, that lies about the resize, indicating that it has happened before
it is complete. That leads to the following possible scenario.
Window has size A.
Window resize to size B is requested.
A fictitious resize event is dispatched with size B.
Window resize to size C is requested.
A fictitious resize event is dispatched with size C.
The size B request is honored. A resize event is dispatched with size B.
The size C request is honored. A resize event is dispatched with size C.
Could that be the scenario observed here?
(I'm not clear from the logs whether or not step 7 eventually happens.)
It's close. Or possibly it's as you described. I think it depends on how much code we have that lies about the resize event. I'll be more explicit about my test code. The following sequence of events happen:
for (let {width, height} of TEST_CASES) {
A) We set up an event handler for onresize, then call aWindow.resizeTo()
B) We await aWindow.onresize firing, and when it does, we only continue (resolve the promise) if the browserContainer.clientWidth is the width we requested
C) We await for the new event test:letterboxing:update-margin-finish to occurs, which I would think will only happen after a real resize; not a fictitious one - but perhaps we're lying more thoroughly than I'd expect
D) We request (async-ly) the content.innerWidth
E) We get it, and perform our test and pass or fail this test case.
}
What I'm seeing is:
- Test Case 1: A (we call resizeTo)
- Test Case 1: B (we await onresize, it occurs, the clientWidth is as we expected)
- Test Case 1: C (test:letterboxing:update-margin-finish occurs)
- Test Case 1: D (we request content.innerWidth)
- Test Case 1: E (we get content.innerWidth, check its value, it's correct, we pass this case)
- Test Case 2: A (we call resizeTo)
- Test Case 2: B (we await onresize, it occurs, the clientWidth is as we expected)
- Test Case 2: C (test:letterboxing:update-margin-finish occurs)
- Test Case 2: D (we request content.innerWidth)
- Test Case 2: The onresize event handler fires again, but this time the clientWidth is the width value from Test Case 1
- Test Case 2: E - We retrieve content.innerWidth and test its value: it is incorrect (it's the value from Test Case 1) and we fail the test (completing step D)
The typical workaround for tests is to wait for the expected results. If
the results do not match expected, then wait again for dimensions to change
and try again. Unfortunately the failure mode is then a timeout, but the
success case works out reasonably nicely if there is an appropriate event on
which to wait.IIUC finding the content dimensions is async, which would mean that merely
waiting for the window to reach the requested size would not be sufficient
because another resize might arrive while the content dimensions are being
evaluated. The test would need to wait until the content dimensions match
what is expected.
I implemented this workaround: I just keep looping until the test succeeds, and I have not experienced a timeout yet. So I think this approach will work. Thank you!!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c416cff078c82ca897a89e5941f603040c99d3e
Assignee | ||
Comment 55•6 years ago
|
||
Talked with johannh; this version uses waitForCondition: https://treeherder.mozilla.org/#/jobs?repo=try&revision=591b2b0300aab22a1ddc1200e261e413c5c30228
Assignee | ||
Comment 56•6 years ago
|
||
Uses waitForCondition
Comment 57•6 years ago
|
||
Assignee | ||
Comment 58•6 years ago
|
||
Carrying forward r+
Assignee | ||
Comment 59•6 years ago
|
||
Carrying forward r+
Assignee | ||
Comment 60•6 years ago
|
||
Carrying forward r+
Assignee | ||
Comment 61•6 years ago
|
||
Carrying forward r+
Assignee | ||
Comment 62•6 years ago
|
||
Carrying forward r+
Assignee | ||
Comment 63•6 years ago
|
||
rebased. Please land the 5 attached patches - not any of the ones from phabricator!
Comment 64•6 years ago
|
||
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5da400636334
Part 1: Rename the LanguagePrompt.jsm to RFPHelper.jsm and changing the place of doing uninitialization. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbae69c2a849
Part 2: Rearrange RFPHelper for expansion r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d089dc2653
Part 3: Implementing the window letterboxing. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/342a7d9308a0
Part 4: Adding a test case for testing letterboxing. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/72a8371c210d
Part 5: Reset the Zoom in browser_bug1369357_site_specific_zoom_level.js r=johann
Comment 65•6 years ago
|
||
Backed out 5 changesets (bug 1407366) for causing multiple build bustages CLOSED TREE
Log failure https://treeherder.mozilla.org/logviewer.html#?job_id=229506688&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=229506664&repo=mozilla-inbound
:tjr could you please take a look?
Assignee | ||
Comment 66•6 years ago
|
||
Carry forward r+
Assignee | ||
Comment 67•6 years ago
|
||
Carry forward r+
Assignee | ||
Comment 68•6 years ago
|
||
Very sorry about that - i had to rebase; and when I did I omitted a file (and caused a lint issue). I updated patches 1 and 3 and this should fix it.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d8d5980168627f61756699db6c04be1fccd980
Comment 69•6 years ago
|
||
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cad0e9929c6
Part 1: Rename the LanguagePrompt.jsm to RFPHelper.jsm and changing the place of doing uninitialization. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1a48e03bb8
Part 2: Rearrange RFPHelper for expansion r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/1490c3e6cef1
Part 3: Implementing the window letterboxing. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ec370629b85
Part 4: Adding a test case for testing letterboxing. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/146c1c01d9dd
Part 5: Reset the Zoom in browser_bug1369357_site_specific_zoom_level.js r=johann
Comment 70•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6cad0e9929c6
https://hg.mozilla.org/mozilla-central/rev/5b1a48e03bb8
https://hg.mozilla.org/mozilla-central/rev/1490c3e6cef1
https://hg.mozilla.org/mozilla-central/rev/9ec370629b85
https://hg.mozilla.org/mozilla-central/rev/146c1c01d9dd
Comment 71•6 years ago
|
||
Is it possible to implement this without actually displaying the letterboxing to the user? As a user, I don't want to give up all that space on my tiny screen.
Comment 72•6 years ago
|
||
Is there any point to this when a user is using an ad-blocker? Is it a good idea to turn resist fingerprinting off if I'm using an ad-blocker anyway?
Maybe firefox shouldn't use it if an adblocker is used?
Comment 73•6 years ago
|
||
I don't read any mention of that functionnality in release notes even if it's behind a pref ?
Comment 74•6 years ago
|
||
(In reply to antistress from comment #73)
I don't read any mention of that functionnality in release notes even if it's behind a pref ?
Firefox 67 stable hasn't been released, so there are no release notes yet. This is also experimental and will likely be removed when it ties into the privacy.resistFingerprinting
. Both prefs are not front-facing and not likely to get any exposure in release notes until then.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 76•5 years ago
|
||
Hi,
On Debian Sid with Firefox 67.0.4, there is no privacy.resistFingerprinting.letterboxing pref in about:config.
Thanks
Comment 77•5 years ago
|
||
(In reply to antistress from comment #76)
Hi,
On Debian Sid with Firefox 67.0.4, there is no privacy.resistFingerprinting.letterboxing pref in about:config.
Thanks
If I create it and restart Firefox, it works.
Assignee | ||
Updated•5 years ago
|
Comment 83•5 years ago
•
|
||
Hi,
On Debian Sid with Firefox 67.0.4, there is no privacy.resistFingerprinting.letterboxing pref in about:config.
Thanks
If I create it and restart Firefox, it works.
Comment 97•4 years ago
|
||
That's a lot of spam. Sorry, everyone, I need to lock this to editbugs-privileged users only. If you've got something to add to this that can materially move this bug forward and you don't have those permissions, please email me and I'll make sure it gets added in.
Description
•