Closed Bug 1407366 (letterboxing) Opened 7 years ago Closed 5 years ago

When privacy.resistFingerprinting=true, dynamically round content dimensions

Categories

(Core :: Window Management, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- ?
firefox67 --- fixed

People

(Reporter: arthur, Assigned: tjr)

References

(Depends on 4 open bugs, Blocks 5 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
Window dimensions are a big source of fingerprintable entropy on the web. Maximized windows reveal available screen width and height (excluding toolbars) and fullscreen windows reveal screen width and height. Non-maxmimized windows can allow strong correlation between two tabs in the same window. While bug 1330882 takes care of new windows, it would be ideal to protect windows continuously even if users resize or maximize their window or enter fullscreen.

For Tor Browser, we experimented with an approach to dynamically round the viewport (content rectangle) dimensions to multiples of 200 x 100. In our prototype, when a user drags the corner of a window, the viewport "snaps" to the largest contained multiple of 200 x 100, leaving a temporary margin of empty gray space in the window chrome. Then, when the user stops resizing (the "resizeend" event), the margin of the window "shrinks" to nothing so that the outer chrome tightly encloses the viewport again.

When the user maximizes the window, the largest possible viewport is used, again a multiple of 200 x 100. Empty gray margins in the chrome part of the window cover the rest of the screen. Similarly, in fullscreen, the viewport is again given dimensions a multiple of 200 x 100, and the chrome areas around it are set to black.

Finally, an extra zoom was applied to the viewport in fullscreen and maximized modes to use as much of the screen as possible and minimize the size of the empty margins. In that case, the window had a "letterbox" (margins at top and bottom only) or "pillbox" (margins at left and right only) appearance. window.devicePixelRatio was always spoofed to 1.0 even when device pixels != CSS pixels.

We haven't yet landed this feature in Tor Browser for at a few reasons:
* There's no true "resizeend" event in Firefox. My approach in pure JS involved emulating a "resizeend" event by looking at the time between "resize" events, but this emulation isn't perfect and it causes somewhat jumpy behavior. I think we'll need a C++ "resizeend" implementation for each platform.
* Tiling window managers on Linux are hard to detect. Any implementation will need to behave appropriately for those.
* Rounding on small screens was a little awkward. We could consider decreasing the quantization to 100 x 50 for small window sizes.

(I will post a link to a video demonstrating our prototype in a future comment.)

An additional opportunity in this model that we didn't yet implement could be to allow users site-specific zoom. This would require managing CSS-pixel dimensions and device-pixel dimensions such that we always spoof window.devicePixelRatio = 1.0 and the CSS-pixel dimensions are properly rounded.

So, for example, suppose the user chooses a zoom level of 1.2 to the window, and suppose that the available device pixel dimensions for the viewport happens to be 1280 x 1000. Then the maximum available CSS-pixel dimensions would by 1066 x 833. In that case, the largest possible CSS-pixel dimensions that are a multiple 200 x 100 would be 1000 x 800. So to achieve those dimensions, we would viewport device-pixel dimensions to be 1200 x 960. (If tabs on the same window had different zoom levels, then it would be necessary to allow margins in the chrome.)
Here's a video of the resizing behavior:
https://www.youtube.com/watch?v=TQxuuFTgz7M
Component: XUL → Window Management
Priority: -- → P5
That sounds so far very useful and might be a better solution for users that do not like the current window behavior privacy.resistFingerprinting provides. Actually it looks like this approach could replace the old one. On making a look at the video always the right and bottom borders are being filled. Personally I would prefer the content window to be horizontally centered and for the vertical alignment I'm currently unsure. Would have an alignment of the content window somehow have an impact on the privacy for this feature? If not users might have different preferences and this could be configurable.

I'm not sure how the zooming part for maximized windows is to be meant. Hopefully it is not something to be felt annoying causing users to not like this feature at all and thus continuing more or less the issue from bug 1401440.
Whiteboard: [fingerprinting]
See Also: → 1450401
Assignee: nobody → tihuang
Priority: P5 → P1
Status: NEW → ASSIGNED
See Also: → 1399279
For achieving dynamical window rounding, I propose that we can dynamically add margins around the browser element to round the content viewport into a certain quantized size. We can implement this by using the CSS margins property. We also want to change the rounding step to 128x100 from 200x100 which fits better with the current screen resolutions, we believe that this change can provide a better user experience of window rounding.

In addition, we will change the name of LanguagePrompt.jsm into RFPHelper.jsm for fitting a general purpose of fingerprinting resistance and implement the mechanism of window rounding into it.
This patch changes the name of LanguagePrompt.jsm to RFPHelper.jsm for
making it more general. 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 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.
This patch implements the window letterboxing when fingerprinting
resistance is enabled. The implementation is based on adding margins
around the browser element to make the content viewport size be always
quantized. Whenever the browser element is resized, the RFPHelper will
add margins around it. But it won't add any margins for the browser
which loads a content with the system principal. In addition, it will
also make the margin area black when a window enters the fullscreen
mode.
Hi Gijs,

What do you think about comment 4? Do you think is this a viable approach?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Tim Huang[:timhuang] from comment #9)
> Hi Gijs,
> 
> What do you think about comment 4? Do you think is this a viable approach?

Yes, in principle this makes sense to me. I think you might want to use padding rather than margin, and I'm out for 2.5 weeks as of EOB Thursday so probably can't help with reviews at this point. But I agree letterboxing will be nicer than the current thing, and it might help remove some complexity from the toplevel window sizing code as described in bug 1476748.
Flags: needinfo?(gijskruitbosch+bugs)
Actually, I've tried using padding before. However, it will confuse the coordinate of the mouse event since the padding is still considering as the content. So the mouse event will be offset by paddings. That's why I chose to use margins instead of padding.
This is a much better approach than setting the window size on startup which wasn't being enforced and not taking into account toolbars, sidebars, theme density and userChrome. Now the window can be maximized to show the full browser chrome while still enforcing the content size.

I did a build to try it out for myself and two things jumped out:

There's a lack of contrast between the page contents and the margins for most light websites. Contrast is important because the margin area is a deadzone where clicking and scrolling don't work, also this is where the page scrollbars are located rather than the edges of the window so they need to be clearly marked. Adding a border (ThreeDShadow) and maybe darkening the margins would help significantly, ideally it would also be darker for the dark theme (stretch goal). Maybe refer to some UI people?

Vertical screen space is at a premium and it hurts to lose it. On Ubuntu 18.04 with a 1080p display I can't get more than 900px height of content, the next available step is 1000px and the closest I can get with disabled titlebar and compact density is 991px. If I use an extension to hide Ubuntu's 27px top bar I can get to 1000px. Perhaps this could be optimized using telemetry to find the most common content size for maximized windows across different systems.
(In reply to Kestrel from comment #12)
> There's a lack of contrast between the page contents and the margins for
> most light websites. Contrast is important because the margin area is a
> deadzone where clicking and scrolling don't work, also this is where the
> page scrollbars are located rather than the edges of the window so they need
> to be clearly marked. Adding a border (ThreeDShadow) and maybe darkening the
> margins would help significantly, ideally it would also be darker for the
> dark theme (stretch goal). Maybe refer to some UI people?

I think adding a border (with a minimum of 1 pixel width) on the most inner pixels of the letterbox (by having a good contrast between letterbox and letterbox-border) should work with any page content.
(In reply to Kestrel from comment #12)
> This is a much better approach than setting the window size on startup which
> wasn't being enforced and not taking into account toolbars, sidebars, theme
> density and userChrome. Now the window can be maximized to show the full
> browser chrome while still enforcing the content size.
> 
> I did a build to try it out for myself and two things jumped out:
> 
> There's a lack of contrast between the page contents and the margins for
> most light websites. Contrast is important because the margin area is a
> deadzone where clicking and scrolling don't work, also this is where the
> page scrollbars are located rather than the edges of the window so they need
> to be clearly marked. Adding a border (ThreeDShadow) and maybe darkening the
> margins would help significantly, ideally it would also be darker for the
> dark theme (stretch goal). Maybe refer to some UI people?
> 

Thanks for trying this feature. The lack of contrast indeed is an issue of usability and I think adding a border would be helpful on this. However, we still need to sort out how this border looks like for a better viewing. In other words, which color should the margin area be as well as the border. 

> Vertical screen space is at a premium and it hurts to lose it. On Ubuntu
> 18.04 with a 1080p display I can't get more than 900px height of content,
> the next available step is 1000px and the closest I can get with disabled
> titlebar and compact density is 991px. If I use an extension to hide
> Ubuntu's 27px top bar I can get to 1000px. Perhaps this could be optimized
> using telemetry to find the most common content size for maximized windows
> across different systems.

Right now, we are using a quantizing step, 128x100, but the step is subject to change. A good step should allow the maximum content size and we are still figuring out what it is.
Question Time!!

- where will you position the viewport? Top center? Top left?
- I assume `privacy.window.maxInnerWidth` and `privacy.window.maxInnerHeight` will be dropped? It is buggy (i.e it still handles toolbar height incorrectly (1 to 3 pixels for some people), and it is not responsive to manual/FS changes). And Firefox will remember the position and size of the last browser window anyway.
- I assume privacy.resistFingerprinting will no longer be used on startup to control some minimum size (see next paragraph), as 1. it's still buggy (see point above re bookmarks toolbar) ... BUT... 2. It would really help reduce permutations if it could (and you fix the toolbar issue).

---

Completely unscientific analysis of steps/permutations on some common screen resolutions

- I have whipped up a spreadsheet (I will email Arthur a copy)
- see attached pic next post
- Figures are for worst case scenario, eg I do not take into account task bars, because they could auto-hide or be vertical rather than horizontal. I only allowed 80pixels for some vertical chrome (drag space, titlebar, menubar, toolbar combos)
- I used the top 10 figures from https://hardware.metrics.mozilla.com/ (excluding no 4 which is "other"). And in my max permutations cell, I ignored no's 10 and 11 (no 11 has more real estate but is only 1.5% of users).
- In scenario 1 (columns J-N) I built in removing some width and height steps, as ridiculously small browser inner windows would be almost non-existant (in these screen resolutions), just to make figures a little more realistic
- In scenario 2 (columns Q-V), *if* a minimum could be enforced (see last point under "Question time" above), and this is problematic given smaller screens, and issues with toolbar heights etc, then permutations drop to almost half or two thirds (depends on the min values). Or you could look at it that most desktop users would not be under this minimum anyway

This is not a complete analysis. It really only takes into account some common desktop res, and there are too many variables (eg users with side bars). But it was fun to do anyway :)

---

Some what if results: (using 1920x1080 as the worst example)

A: 128x100 (ignore first 3 width + 4 height steps) = 72 permutations (i.e max on 1920x1080, on 2560x1440 it is 153, on 1366x768 is it only 3)
B: 200x100 (ignore first 2 width + 4 height steps) = 42 permutations
C: 160x 90 (ignore first 3 width + 4 height steps) = 63 permutations

When we enforce a minimum (or you could look at it that most desktop users would have an inner window at least 800px wide and 600px height), then

A: 128x100 = 45 (40 if min 1000 width)
B: 200x100 = 30 (25 if min 1000 width)
C: 160x 90 = 40 (30 if min 1000 width)

These permutations are way too high for my liking - and that's just looking at essentially desktop. Yes, most desktop users will probably be using FF landscaped, whereas we are looking at permutations. IDK the answer. As Kestrel says, with various chrome (toobar, menubar, drag space, titlebar, various theme density) and add in OS taskbars etc - if the height step is too high it makes it less usable, but if it's too low it ramps up the permutations.
Attached image steps-vs-permutations
see comment 16
(In reply to Simon Mainey from comment #16)
> Question Time!!
> 
> - where will you position the viewport? Top center? Top left?

Right now, We position the viewport at the center for both horizontal and vertical. But, we can change this very easily. So, maybe we can use a pref to control this.

> - I assume `privacy.window.maxInnerWidth` and
> `privacy.window.maxInnerHeight` will be dropped? It is buggy (i.e it still
> handles toolbar height incorrectly (1 to 3 pixels for some people), and it
> is not responsive to manual/FS changes). And Firefox will remember the
> position and size of the last browser window anyway.
> - I assume privacy.resistFingerprinting will no longer be used on startup to
> control some minimum size (see next paragraph), as 1. it's still buggy (see
> point above re bookmarks toolbar) ... BUT... 2. It would really help reduce
> permutations if it could (and you fix the toolbar issue).
> 

In my personal opinion, yes on both, I want to remove those. Just like you said, it is buggy and the code over there is way too complex. It messes up with the other window sizer in Firefox. However, we are still under discussing regarding this.

> ---
> 
> Completely unscientific analysis of steps/permutations on some common screen
> resolutions
> 
> - I have whipped up a spreadsheet (I will email Arthur a copy)
> - see attached pic next post
> - Figures are for worst case scenario, eg I do not take into account task
> bars, because they could auto-hide or be vertical rather than horizontal. I
> only allowed 80pixels for some vertical chrome (drag space, titlebar,
> menubar, toolbar combos)
> - I used the top 10 figures from https://hardware.metrics.mozilla.com/
> (excluding no 4 which is "other"). And in my max permutations cell, I
> ignored no's 10 and 11 (no 11 has more real estate but is only 1.5% of
> users).
> - In scenario 1 (columns J-N) I built in removing some width and height
> steps, as ridiculously small browser inner windows would be almost
> non-existant (in these screen resolutions), just to make figures a little
> more realistic
> - In scenario 2 (columns Q-V), *if* a minimum could be enforced (see last
> point under "Question time" above), and this is problematic given smaller
> screens, and issues with toolbar heights etc, then permutations drop to
> almost half or two thirds (depends on the min values). Or you could look at
> it that most desktop users would not be under this minimum anyway
> 
> This is not a complete analysis. It really only takes into account some
> common desktop res, and there are too many variables (eg users with side
> bars). But it was fun to do anyway :)
> 
> ---
> 
> Some what if results: (using 1920x1080 as the worst example)
> 
> A: 128x100 (ignore first 3 width + 4 height steps) = 72 permutations (i.e
> max on 1920x1080, on 2560x1440 it is 153, on 1366x768 is it only 3)
> B: 200x100 (ignore first 2 width + 4 height steps) = 42 permutations
> C: 160x 90 (ignore first 3 width + 4 height steps) = 63 permutations
> 
> When we enforce a minimum (or you could look at it that most desktop users
> would have an inner window at least 800px wide and 600px height), then
> 
> A: 128x100 = 45 (40 if min 1000 width)
> B: 200x100 = 30 (25 if min 1000 width)
> C: 160x 90 = 40 (30 if min 1000 width)
> 
> These permutations are way too high for my liking - and that's just looking
> at essentially desktop. Yes, most desktop users will probably be using FF
> landscaped, whereas we are looking at permutations. IDK the answer. As
> Kestrel says, with various chrome (toobar, menubar, drag space, titlebar,
> various theme density) and add in OS taskbars etc - if the height step is
> too high it makes it less usable, but if it's too low it ramps up the
> permutations.

This is an insightful analysis, thanks. I agree with you that the change here, 128x100 steps, would have more permutations. In other words, it will expose more maximum entropy bits, which is bad for fingerprinting resistance. On the flip side, a good user experience is also important. Especially for this window rounding feature, because of this is the first thing which users will notice when they turn on fingerprinting resistance. For our current window rounding protection, many people treat it as an issue rather than a protection :(. And we want users can use their screen as much as possible. So, the old 200x100 steps may not serve this purpose very well. We still need to investigate which step is appropriate for both fingerprinting resistance and user experience.

Limiting the minimum window size is beneficial in this case, but I am wondering that would users threat this as a protection or another "bug" for this. We have to handle this carefully since we don't want to push people away from fingerprinting resistance.

Arthur, any thoughts around these questions?
Flags: needinfo?(arthuredelstein)
And while I had slept, another thought popped into my head - a sliding scale of steps e.g. step 128, but above 1280 step in 200 hundreds. And in reverse, under a certain size step even smaller (which is good for tiny screens). Overall this would probably keep the total steps (and permutations) the same, but increase usability
I do think a growing step-size might be a good idea.

On most web pages, screen height is more valuable than screen width. Moving from a 200-pixel horizontal step size to 128 pixels is a sacrifice of 0.64 bits of entropy, with a slight improvement in usability. It occurs to me that we could sacrifice the same number of bits by changing the vertical step size instead, for a greater gain in usability. Namely, instead of 128x100, we could use 200x64. For most web page that would allow users to read more lines of text without vertical scrolling.

It's true that 200x64 is not perfect factor of most screen sizes. But maybe that's less important than getting as much vertical screen space as possible.
Flags: needinfo?(arthuredelstein)
Perfect factors are irrelevant IMO, because chrome (and OS) elements make it so. A growing/sliding step sounds awesome (if I may say so), but should apply to both horizontal and vertical because width is still an issue on smaller screens

Side note: how does a change in orientation affect this, eg if steps are not the same factor, then it would be out. Eg if your screen res is 1280x1000 and you flipped your tablet around and it was now 1000x1280? Is that how it works? I do not have a tablet.

We should get a list of all devices res sizes and build some models based on the two or more w/h demarcations and step sizes to find an optimal result. eg under 800px width or height step (w/h) in 100/64, between 800-1600 step 200s/128s, and over 1600 step 300/180 .. or something.

I still think a minimum is a good idea, especially if you grab the screen res first and only apply the minimum to most users, i.e be able to exclude tiny screens
(In reply to Simon Mainey from comment #21)
> Side note: how does a change in orientation affect this, eg if steps are not
> the same factor, then it would be out. Eg if your screen res is 1280x1000
> and you flipped your tablet around and it was now 1000x1280? Is that how it
> works? I do not have a tablet.

The obvious solution to that is for the steps to change orientation at the same time. If you don't do this then you start to reveal more about the real width/height.
(In reply to Simon Mainey from comment #21)
> Perfect factors are irrelevant IMO, because chrome (and OS) elements make it
> so. A growing/sliding step sounds awesome (if I may say so), but should
> apply to both horizontal and vertical because width is still an issue on
> smaller screens

Perfect factors aren't completely irrelevant, because frequently there are no chrome/OS elements in the horizontal direction. And in full-screen mode (such as watching a YouTube video) it's nice to use whole screen if possible.
(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #23)
> And in full-screen mode (such as watching a YouTube video) it's nice to use whole screen if possible.

A sliding scale could easily include the top 5 or 6 common screen resolutions for full-screen viewing for youtube, netflix and the like - would definitely help uptake

I was thinking about chrome elements affecting the viewport. If you use center/center, then using Find could make the content "jump" (i.e it fits within the current viewport vertically, so any reduction in height makes it reposition). Same goes for toggling on/off menubars, toolbars (although that already initiates vertical movement).

And I'm interested about where the scrollbars appear - inside the viewport, or outside. If inside, the introduction of those (eg expanding a collapsed section (introducing increased with and/or height) can also make the center/center reposition. Or are scrollbars ignored as in the current setup?

I think it might be best to use top/left or at worst, top/center. Just food for thought.

PS: I know that userChrome.css can be used to tweak the "letterboxing" background color, but it would be nice for users to be able to set this from a pref IMO (imagine watching a "dark letterboxed" 2:40 movie on netflix with a grey/white letterboxed viewport.
Hi Jacqueline,

As we discussed offline, we would like to have your UX review for the current implementation.
Here is the document for explanation.
http://bit.ly/2nVWKIz

Specifically and ideally, we hope you can provide feedback on the following items:
1. Margin style
   - The color of the margin border.
   - The line and thickness of the margin border.
   - The color of the margin area.
2. Tooltip displayed when the mouse is hovered to the margin area
   - The current tooltip is “Margins for protecting from browser fingerprinting.”
3. Full screen mode
   - The current proposal is to paint black in the margin area for full screen mode, to make a strong contrast 
     to normal mode (which paints white in the margin area).
4. The step size
   - The current value is 128 x 100 pixel.
Flags: needinfo?(jsavory)
(In reply to Ethan Tseng [:ethan] from comment #25)
> 3. Full screen mode
>    - The current proposal is to paint black in the margin area for full
> screen mode, to make a strong contrast 
>      to normal mode (which paints white in the margin area).

I'd like to add more details regarding this. The original idea of this was proposed by Arthur. But he was pointing to the fullscreen which doesn' have chrome UI, i.e. press F11 in fullscreen mode in Windows. A kiosk mode maybe?

And I misunderstood that and implemented the black margin in fullscreen mode with chrome UI. And we still don't know that whether this is a good idea for either only using the black margin in the kiosk mode or using the black margin in both modes.
I wonder if we could run some study inside Firefox confronting users in that study with different steps sizes/fullscreen handling. Tor Browser using 200x100px was always a stopgap we had because we did not have hard data to decide a good trade-off between usability and size of the fingerprinting set.

I'd especially like a way to see user feedback on the different step sizes/fullscreen handling tested so we can decide what a good trade-off here is that is still acceptable. The lack of acceptability was the major issue back then when we tried the letterboxing approach in a bunch of our alpha builds. Even power users familiar with Tor and Tor Browser asked how they could disable that feature again to get maximum window size they wanted. So, I want to avoid that we do all this thinking and engineering to get the step size right in all scenarios of interest just to learn that users first try to find a knob to turn this off again.

I assume Mozilla has some metrics/process that says a feature X is acceptable if Y% of study participants
can live/are happy with X?
(In reply to Georg Koppen from comment #27)
> I wonder if we could run some study inside Firefox confronting users in that
> study with different steps sizes/fullscreen handling. Tor Browser using
> 200x100px was always a stopgap we had because we did not have hard data to
> decide a good trade-off between usability and size of the fingerprinting set.
> 
> I'd especially like a way to see user feedback on the different step
> sizes/fullscreen handling tested so we can decide what a good trade-off here
> is that is still acceptable. The lack of acceptability was the major issue
> back then when we tried the letterboxing approach in a bunch of our alpha
> builds. Even power users familiar with Tor and Tor Browser asked how they
> could disable that feature again to get maximum window size they wanted. So,
> I want to avoid that we do all this thinking and engineering to get the step
> size right in all scenarios of interest just to learn that users first try
> to find a knob to turn this off again.


It might be possible for us to run a study; yes.  I'm not sure what the receptiveness would be to doing that at this point in the Fusion efforts (since it would really just be for Tor.)  But we can ask.

As far as what values to go with though, we've asked three groups if they have the data, and we're waiting to hear back from 1 (EFF/panopticlick).  We could also add our own telemetry.
Depends on: 1489493
Blocks: 1475825
See Also: → 1450561
Whiteboard: [fingerprinting] → [fingerprinting][fp-triaged]
Lower the priority for now because we don't decide the final solution yet, plus it might require UX support.
Priority: P1 → P2
Attachment #8996253 - Attachment is obsolete: true
Attachment #8996254 - Attachment is obsolete: true
Attachment #8996255 - Attachment is obsolete: true
Attachment #8996256 - Attachment is obsolete: true

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.

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.

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

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

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

Flags: needinfo?(jsavory)

This encompasses the changes requested in the phabricator review.

Attachment #9041571 - Flags: review?(jhofmann)
Attachment #9041572 - Flags: review?(jhofmann)

Ok, thanks, I've already been looking at the patches in phabricator and will wrap this up shortly. Sorry for the delay :)

Whiteboard: [fingerprinting][fp-triaged] → [fingerprinting][fp-triaged][tor 14429]
Attachment #9041571 - Flags: review?(jhofmann) → review+
Attachment #9041572 - Flags: review?(jhofmann) → review+
Comment on attachment 9041573 [details] [diff] [review]
Bug 1407366 - Part 3: Implementing the window letterboxing. r?johannh

Review of attachment 9041573 [details] [diff] [review]:
-----------------------------------------------------------------

Had a bunch of comments but I don't think I need to see this again.

Thank you, this works great!

::: browser/actors/RFPHelperChild.jsm
@@ +7,5 @@
> +
> +const {ActorChild} = ChromeUtils.import("resource://gre/modules/ActorChild.jsm");
> +const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const KPrefLetterboxing = "privacy.resistFingerprinting.letterboxing";

nit: shouldn't this start with a lowercase "k"?

(Personally I prefer PREF_LETTERBOXING but I don't think we have a style guide on that, so it's up to you)

::: browser/components/nsBrowserGlue.js
@@ +292,5 @@
>        ],
>      },
>    },
> +
> +  RFPHelper: {

I think the actor definitions are also listed alphabetically.

::: toolkit/components/resistfingerprinting/RFPHelper.jsm
@@ +13,5 @@
>  const kPrefSpoofEnglish = "privacy.spoof_english";
>  const kTopicHttpOnModifyRequest = "http-on-modify-request";
>  
> +const KPrefLetterboxing = "privacy.resistFingerprinting.letterboxing";
> +const KPrefLetterboxingDimensions =

nit: Again, we should be consistent with the prefixes, so change to lowercase "k" for the two constants above?

@@ +274,5 @@
> +
> +  _handleLetterboxingPrefChanged() {
> +    if (Services.prefs.getBoolPref(KPrefLetterboxing, false)) {
> +      Services.ww.registerNotification(this);
> +      this._attachAllWindows();

Just a side note that we're looking into adding a module that takes care of "Window Management" for you called EveryWindow.jsm. It's actually cool that you need the functionality because "we don't have in-tree consumers" has been the main counter-argument so far.

It's in bug 1416163, once that's done we could rewrite this code.

@@ +290,5 @@
> +      return [];
> +    }
> +
> +    return aPrefValue.split(",").map(item => {
> +      let sizes = item.split("x").map(size => parseInt(size, 10));

It might be worth thinking about / seeing what happens when you input an invalid pref on accident. With the pref getter it's a little hard to tell, so we might want to make sure things don't fail too catastrophically.

@@ +311,5 @@
> +    }
> +
> +    // We should apply no margin around an empty tab or a tab with system
> +    // principal.
> +    if (tab.isEmpty || ssm.isSystemPrincipal(aBrowser.contentPrincipal)) {

nit: I think you can use aBrowser.contentPrincipal.isSystemPrincipal https://searchfox.org/mozilla-central/rev/e00ea598e52bbb35f8c45abf9c2eade17962bb5e/caps/nsIPrincipal.idl#337

@@ +319,5 @@
> +    }
> +  }
> +
> +  _isDimensionRounded(aWidth, aHeight) {
> +    // If the dimension set is not given, we will check that if the content

nit: "we will check that if the content" doesn't make sense in this context, I think. Remove either "that" or "if"?

@@ +332,5 @@
> +    let idx = this._letterboxingDimensions.findIndex(item => {
> +      return (aWidth * aHeight) === (item.width * item.height);
> +    });
> +
> +    return idx !== -1;

You can replace this with

return this._letterboxingDimensions.some(item => { ... });

@@ +337,5 @@
> +  }
> +
> +  /**
> +   * The function will round the given browser by adding margins around the
> +   * content viewport. 

uber-nit: trailing space

@@ +422,5 @@
> +      // to (e.g.) display a striped pattern or similar but it does let us
> +      // choose the color, which we set it to a light grey (for now).
> +      aBrowser.style.borderWidth = `${margins.height}px ${margins.width}px`;
> +      aBrowser.style.borderColor = '#CCC';
> +      aBrowser.style.borderStyle = 'solid';

nit: please use double-quotes instead of single quotes wherever possible
Attachment #9041573 - Flags: review?(jhofmann) → review+
Comment on attachment 9041574 [details] [diff] [review]
Bug 1407366 - Part 4: Adding a test case for testing letterboxing. r?johannh

Review of attachment 9041574 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, see comments!

::: browser/components/resistfingerprinting/test/browser/browser_dynamical_window_rounding.js
@@ +1,1 @@
> +/**

Please add a license header

@@ +18,5 @@
> +  {width: 500,  height: 350},
> +  {width: 300,  height: 170},
> +];
> +
> +function promiseObserver(topic) {

Please replace this with TestUtils.topicObserved

https://searchfox.org/mozilla-central/rev/490ab7f9b84570573a49d7fa018673ce0d5ddf22/testing/modules/TestUtils.jsm#55

::: toolkit/components/resistfingerprinting/RFPHelper.jsm
@@ +16,5 @@
>  const KPrefLetterboxing = "privacy.resistFingerprinting.letterboxing";
>  const KPrefLetterboxingDimensions =
>    "privacy.resistFingerprinting.letterboxing.dimensions";
> +const KPrefLetterboxingTesting =
> +  "privacy.resistFingerprinting.letterboxing.testing";

Again, would be good to be consistent with the "k"
Attachment #9041574 - Flags: review?(jhofmann) → review+
Assignee: tihuang → tom
Attachment #9041573 - Attachment is obsolete: true
Attachment #9042279 - Flags: review?(jhofmann)
Attachment #9041574 - Attachment is obsolete: true
Attachment #9042280 - Flags: review?(jhofmann)

Okay - I addressed your comments and re-requested review because I needed to deal with some test failures.

  1. I needed to change the _isDimensionRounded check to specifically check that we rounded to the intended dimension.
  2. 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.
  3. I needed to correct a different test that never reset the zoom level
  4. 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.
  5. 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

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.

Attachment #9042280 - Attachment is obsolete: true
Attachment #9042280 - Flags: review?(jhofmann)
Attachment #9042534 - Flags: review?(jhofmann)
Attachment #9042534 - Attachment is patch: true
Comment on attachment 9042279 [details] [diff] [review]
Bug 1407366 - Part 3: Implementing the window letterboxing. r?johannh

Review of attachment 9042279 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/resistfingerprinting/RFPHelper.jsm
@@ +286,5 @@
> +  // should be formated as 'width1xheight1, width2xheight2, ...'. For
> +  // example, '100x100, 200x200, 400x200 ...'.
> +  _parseLetterboxingDimensions(aPrefValue) {
> +    if (!aPrefValue || !aPrefValue.match(/^(?:\d+x\d+,\s*)*(?:\d+x\d+)$/)) {
> +      return [];

nit: how about Cu.reportError(`Invalid pref value for ${kPrefLetterboxing}: ${aPrefValue}`);

@@ +417,5 @@
> +
> +  _updateMarginsForTabsInWindow(aWindow) {
> +    let tabBrowser = aWindow.gBrowser;
> +
> +    for (const tab of tabBrowser.tabs) {

nit: let tab

@@ +456,5 @@
> +    aWindow.messageManager
> +           .removeMessageListener(kEventLetterboxingSizeUpdate, this);
> +
> +    // Clear all margins and tooltip for all browsers.
> +    for (const tab of tabBrowser.tabs) {

nit: let tab
Attachment #9042279 - Flags: review?(jhofmann) → review+
Comment on attachment 9042281 [details] [diff] [review]
Bug 1407366 - Part 5: Reset the Zoom in browser_bug1369357_site_specific_zoom_level.js r?johann

Review of attachment 9042281 [details] [diff] [review]:
-----------------------------------------------------------------

So, I don't mind this, but why not just reset the zoom at the start of browser_dynamical_window_rounding.js?
Attachment #9042281 - Flags: review?(jhofmann) → review+
Comment on attachment 9042279 [details] [diff] [review]
Bug 1407366 - Part 3: Implementing the window letterboxing. r?johannh

Review of attachment 9042279 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/resistfingerprinting/RFPHelper.jsm
@@ +371,5 @@
> +          minWaste = waste;
> +        }
> +      }
> +
> +      var result;

another nit: let result
Comment on attachment 9042534 [details] [diff] [review]
Bug 1407366 - Part 4: Adding a test case for testing letterboxing. r?johannh

Review of attachment 9042534 [details] [diff] [review]:
-----------------------------------------------------------------

Phew, those are interesting failures. I don't have any better suggestion than what you did and it's better than disabling the test.

I had a few comments on the logging, though.

::: toolkit/components/resistfingerprinting/RFPHelper.jsm
@@ +25,5 @@
>  const kDefaultHeightStepping = 100;
>  
> +XPCOMUtils.defineLazyGetter(this, "log", () => {
> +  let scope = {};
> +  ChromeUtils.import("resource://gre/modules/Console.jsm", scope);

I recently learned from baku that usage of Console.jsm isn't cool anymore and we should use console.createInstance instead. See https://phabricator.services.mozilla.com/D18496 for an example. You can still do this in this lazy getter, I think, though.

@@ +28,5 @@
> +  let scope = {};
> +  ChromeUtils.import("resource://gre/modules/Console.jsm", scope);
> +  let consoleOptions = {
> +    // Set to 'all' to see debug messages
> +    maxLogLevel: "error",

You should control this via pref to be able to debug on Nightly.
Attachment #9042534 - Flags: review?(jhofmann) → review-
Attachment #9042534 - Attachment is obsolete: true
Attachment #9043854 - Flags: review?(jhofmann)
Comment on attachment 9043854 [details] [diff] [review]
Bug 1407366 - Part 4: Adding a test case for testing letterboxing. r?johannh

Review of attachment 9043854 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: browser/components/resistfingerprinting/test/browser/browser_dynamical_window_rounding.js
@@ +61,5 @@
> +async function test_dynamical_window_rounding(aWindow, aCheckFunc) {
> +  // We need to wait for the updating the margins for the newly opened tab, or
> +  // it will affect the following tests.
> +  let promiseForTheFirstRounding =
> +    TestUtils.topicObserved("test:letterboxing:update-margin-finish", () => true);

nit: you don't have to pass the second argument (the checkFn)

@@ +83,5 @@
> +  for (let {width, height} of TEST_CASES) {
> +    let caseString = "Case " + width + "x" + height + ": ";
> +    // Create a promise for waiting for the margin update.
> +    let promiseRounding =
> +      TestUtils.topicObserved("test:letterboxing:update-margin-finish", () => true);

nit: you don't have to pass the second argument
Attachment #9043854 - Flags: review?(jhofmann) → review+
Comment on attachment 9043854 [details] [diff] [review]
Bug 1407366 - Part 4: Adding a test case for testing letterboxing. r?johannh

>+           * We observed frequent test failures that resulted from receiving an onresize
>+           * event where the browser was resized to an earlier requested dimension. This
>+           * resize event seemed to come from nowhere (and didn't correspond to an ealier
>+           * 'Case').

I must be misunderstanding something here because these sentences appear conflicting to me.  The former indicates the dimensions match an earlier requested dimension, while I infer from the latter that the dimensions don't match an earlier 'Case'.  Are Cases different from requests?

Regardless, it's probably helpful to explain a scenario to expect based on the way the widget code is in the GTK port:

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.

1. Window has size A.

2. Window resize to size B is requested.

3. A fictitious resize event is dispatched with size B.

4. Window resize to size C is requested.

5. A fictitious resize event is dispatched with size C.

6. The size B request is honored.  A resize event is dispatched with size B.

7. 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 may be possible to rework some widget code to avoid the fictitious resize events, but there is likely existing code depending on this that would also need to be modified.

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.

(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.

  1. Window has size A.

  2. Window resize to size B is requested.

  3. A fictitious resize event is dispatched with size B.

  4. Window resize to size C is requested.

  5. A fictitious resize event is dispatched with size C.

  6. The size B request is honored. A resize event is dispatched with size B.

  7. 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:

  1. Test Case 1: A (we call resizeTo)
  2. Test Case 1: B (we await onresize, it occurs, the clientWidth is as we expected)
  3. Test Case 1: C (test:letterboxing:update-margin-finish occurs)
  4. Test Case 1: D (we request content.innerWidth)
  5. Test Case 1: E (we get content.innerWidth, check its value, it's correct, we pass this case)
  6. Test Case 2: A (we call resizeTo)
  7. Test Case 2: B (we await onresize, it occurs, the clientWidth is as we expected)
  8. Test Case 2: C (test:letterboxing:update-margin-finish occurs)
  9. Test Case 2: D (we request content.innerWidth)
  10. Test Case 2: The onresize event handler fires again, but this time the clientWidth is the width value from Test Case 1
  11. 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

Uses waitForCondition

Attachment #9043854 - Attachment is obsolete: true
Attachment #9044810 - Flags: review?(jhofmann)
Comment on attachment 9044810 [details] [diff] [review]
Bug 1407366 - Part 4: Adding a test case for testing letterboxing. r?johannh

Review of attachment 9044810 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great with all the explanations, thanks for being extra thorough!
Attachment #9044810 - Flags: review?(jhofmann) → review+

Carrying forward r+

Attachment #9041572 - Attachment is obsolete: true
Attachment #9045348 - Flags: review+

Carrying forward r+

Attachment #9042279 - Attachment is obsolete: true
Attachment #9045349 - Flags: review+

Carrying forward r+

Attachment #9044810 - Attachment is obsolete: true
Attachment #9045350 - Flags: review+

rebased. Please land the 5 attached patches - not any of the ones from phabricator!

Keywords: checkin-needed

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

Keywords: checkin-needed

Carry forward r+

Attachment #9045349 - Attachment is obsolete: true
Attachment #9045523 - Flags: review+

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

Flags: needinfo?(tom)
Keywords: checkin-needed

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

Keywords: checkin-needed
Depends on: 1538130
Depends on: 1525224
No longer depends on: 1525224

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.

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?

I don't read any mention of that functionnality in release notes even if it's behind a pref ?

(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.

Depends on: 1546832
Blocks: 1548634
Blocks: 1555815
Blocks: 1546832, 1538130
No longer depends on: 1538130, 1546832
See Also: → 1475825
Blocks: 1556000
Blocks: 1556002
Blocks: 1556014
Blocks: 1556016
Blocks: 1556017
Attachment #9040063 - Attachment is obsolete: true
Attachment #9040064 - Attachment is obsolete: true
Attachment #9040065 - Attachment is obsolete: true

Hi,
On Debian Sid with Firefox 67.0.4, there is no privacy.resistFingerprinting.letterboxing pref in about:config.
Thanks

(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.

Alias: letterboxing
Blocks: 1591054
Depends on: 1594455
Depends on: 1595394
Blocks: 1593585

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.

Depends on: 1616046
Depends on: 1640548

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.

Restrict Comments: true
Depends on: 1620347
See Also: → 960875
Depends on: 1744439
Depends on: 1788839
Depends on: 1790988
You need to log in before you can comment on or make changes to this bug.