1330882 - When privacy.resistFingerprinting = true, set new windows to rounded dimensions [tor 19459]

Status: REOPENED

(Core :: XUL, defect, P3)

Description

[Arthur Edelstein [:arthur]]

In Tor Browser (where privacy.resistFingerprinting is enabled), when a new window is opened, the dimensions of the content are always set to 1000x1000, or, if enough screen space is not available, to the nearest 200x100. This reduces the entropy in screen-size and window-size that can otherwise be used for fingerprinting or linking across sites.

We'd like to propose uplifting this patch: torpat.ch/19459

We also have code for auto-quantizing the window dimensions after it is resized by the user but we'll keep that to a separate ticket because it is more complex and probably more controversial as well...

Comment 1

[Thorin [:thorin]]

Can an extra preference to disable auto-sizing be implemented eg privacy.resistFingerprinting.windowRounding so that Firefox users (as compared to TBB users) can disable this functionality, but TBB can easily switch it on?

I use resistFingerprinting but resize my window (allowing for non inner window elements) to match common screen resolutions. From the scratch pad I can enter window.resizeTo(1614,946) to achieve 1366x768 or window.resizeTo(1528,978) for 1280x800 etc. I don't go fullscreen, I don't resize, and I don't change (show/hide) my sidebar/toolbars/etc. Its static.

https://trac.torproject.org/projects/tor/ticket/14098

I am but one user, but surely, when hiding in the Firefox set of users, we would want common screen resolutions, not round hundreds. I would rather, as a non-Tor browser and being in the Firefox subset, be returning more common or realistic values. I also think a lot of power users would not like their browser being resized on them. And additionally, the current Tor dimensions of tall and skinny (on wide desktops) does not suit all. Regardless of which strategy (using a small set of rounded hundreds or an even smaller set of common screen resolutions) is better (and I do think that at the end of the day, some enforced set resolutions is the only way to gain significant numbers to hide in), surely at this stage, a toggle would be desirable - especially if you are to not drive away users of privacy.resistFingerprinting. A single extra pref at this stage would allow some flexibility.

Comment 2

[Thorin [:thorin]]

(In reply to Simon Mainey from comment #1)
> https://trac.torproject.org/projects/tor/ticket/14098

Sorry, wrong ticket link. I can't find the discussion I previously read about screen resolutions vs rounded hundreds. I understand the rounded hundreds argument for TOR users, but I also believe an even smaller set of common screen resolutions is more practical/usable, and is even better to hide in, for Firefox users.

Comment 3

[Arthur Edelstein [:arthur]]

(In reply to Simon Mainey from comment #1)
> Can an extra preference to disable auto-sizing be implemented eg
> privacy.resistFingerprinting.windowRounding so that Firefox users (as
> compared to TBB users) can disable this functionality, but TBB can easily
> switch it on?
> 
> I use resistFingerprinting but resize my window (allowing for non inner
> window elements) to match common screen resolutions. From the scratch pad I
> can enter window.resizeTo(1614,946) to achieve 1366x768 or
> window.resizeTo(1528,978) for 1280x800 etc. I don't go fullscreen, I don't
> resize, and I don't change (show/hide) my sidebar/toolbars/etc. Its static.

Hi Simon -- in the proposed patch (https://torpat.ch/19459) there are two prefs, privacy.window.maxInnerWidth and privacy.window.maxInnerHeight, that let you set the startup size of the window. Does that address your concerns? It also helps you avoid having to use resizeTo().

> https://trac.torproject.org/projects/tor/ticket/14098
> 
> I am but one user, but surely, when hiding in the Firefox set of users, we
> would want common screen resolutions, not round hundreds. I would rather, as
> a non-Tor browser and being in the Firefox subset, be returning more common
> or realistic values. I also think a lot of power users would not like their
> browser being resized on them. And additionally, the current Tor dimensions
> of tall and skinny (on wide desktops) does not suit all. Regardless of which
> strategy (using a small set of rounded hundreds or an even smaller set of
> common screen resolutions) is better (and I do think that at the end of the
> day, some enforced set resolutions is the only way to gain significant
> numbers to hide in), surely at this stage, a toggle would be desirable -
> especially if you are to not drive away users of
> privacy.resistFingerprinting. A single extra pref at this stage would allow
> some flexibility.

I'm not sure there is a set of common screen resolutions that makes sense. We are enforcing the content window dimensions, not the screen dimensions, so taskbars, menubars, the window chrome, etc., affect the size available for content. I think the dimensions you are setting your window to are likely to increase your fingerprintability rather than decrease it.

I am open to ideas to improving this feature, but because window dimensions provide a lot of fingerprinting entropy, I think it's important that privacy.resistFingerprinting includes this feature.

Comment 4

[Thorin [:thorin]]

(In reply to Arthur Edelstein [:arthuredelstein] from comment #3)
> Hi Simon -- in the proposed patch (https://torpat.ch/19459) there are two
> prefs, privacy.window.maxInnerWidth and privacy.window.maxInnerHeight, that
> let you set the startup size of the window. Does that address your concerns?
> It also helps you avoid having to use resizeTo().

That's just it. I don't use resizeTo(). Firefox remembers it's last settings. I have set it once, catering for all non-inner window items, and I haven't had to change it since (since the resist.Fingerprinting pref landed). What I have fits nicely on my monitor, allows me to have a sidebar, extra toolbars, etc - tweaks for  theme and padding, whatever I want. And everybody will have different needs. Also, all my reported dimensions are the same. See http://ip-check.info/?lang=en for a test to measure inner, browser and usable-screen measurements, and panotoclick for screen = all are the same Screen (1280 x 800 pixels 24 bit color depth). It is easy enough to retrieve dimensions, do the math and resize to get the right measurement.

If privacy.window.maxInnerWidth and privacy.window.maxInnerHeight actually means I can set them as eg 1200 and 800, and the inner window is set exactly as that (not some MAX figure as the pref name suggests), then that would do the trick, I guess, as it would have the same effect.

> I'm not sure there is a set of common screen resolutions that makes sense.
> We are enforcing the content window dimensions, not the screen dimensions,
> so taskbars, menubars, the window chrome, etc., affect the size available
> for content. I think the dimensions you are setting your window to are
> likely to increase your fingerprintability rather than decrease it.
> 
> I am open to ideas to improving this feature, but because window dimensions
> provide a lot of fingerprinting entropy, I think it's important that
> privacy.resistFingerprinting includes this feature.

I can't find the tor ticket thread I read a few months ago, but the two sides of the discussion went ..
- round hundred combinations eg 8 heights (or more), 8 widths (or more depending on your code which currently favors portrait rather than landscape etc) = 64 combinations total (not saying that's how your code would handle it, and I would assume the set is much smaller)
- "common" screen resolutions: just pick 5 of them (or 10 even, or less: I think someone argued for just 2 at some point) = 5 total eg 1920x1080, 1366x768, 1280x800, 1440x900, 1600x900 etc that cover 95% of end user's needs. Eg my monitor's usable screen is approx 1680x800 (i.e minus the double height task bar), so after code calculates the non inner window pieces, it can easy work out that I can use 2 of those settings: 1280x800 and 1366x768. It would then know from Telemetry (see point below) that the second option is most common.
- privacy.window.maxInnerWidth and privacy.window.maxInnerHeight can be used for the 5% who need it to override.
- surely 10 is smaller set than 64
- As a Firefox subset (not a TOR subset), surely all those not using the privacy.resistFingerprinting for screen will leak a "common" resolution (see next point on telemetry data). Obviously some code out there may only use this one measure, but most will likely use multiple measurements, so the key is to make all Firefox users with privacy.resistFingerprinting as a set, use as few dimensions as possible.
- https://metrics.mozilla.com/firefox-hardware-report/  Firefox telemetry common display resolutions (Sept 2016). 33% 1366x768 , 20% 1920x1080, etc

I think we'll go round in circles with this. I definitely want the end result to be ENFORCEMENT of browser inner window dimensions for Firefox users with privacy.resistFingerprinting, as the aim is to reduce entropy in this subset. What TBB does I don't mind/care. But for Firefox, I would rather there were more options, albeit a LIMITED set, so some users can have widescreen/landscape and some users can have portrait, and so it caters for users with different setups (such as my big fat side bar). I am sure a limited set of 10 resolutions or less could be worked out - in round hundreds (it would pass the test of time, as who knows what the future will bring in manufacturing).

In fact, why not just let users set one of eg six pre-defined resolutions with no exceptions eg ("privacy.window.maxDimensions", 1) where
1= 1500x800 (more width and allows for a sidebar)
2= 1000x800 (currently what TBB throws at me on startup, portrait)
3+4= next sizes down

NO idea if every real world scenario would fit in that set, but surely the smallest size would cover all, or fallback to the old code? Just throwing some ideas at you from an end user perspective who would HATE "my" Firefox to lose any usability (side bars etc) due to a lack of discussion or options, or be forced to too narrow a width for viewing. There has to be some flexibility or people will be turned off. Thanks :)

Comment 5

[Arthur Edelstein [:arthur]]

(In reply to Simon Mainey from comment #4)

> If privacy.window.maxInnerWidth and privacy.window.maxInnerHeight actually
> means I can set them as eg 1200 and 800, and the inner window is set exactly
> as that (not some MAX figure as the pref name suggests), then that would do
> the trick, I guess, as it would have the same effect.

Yes. The "max" part of the name refers to the behavior that, if the screen is too small to accomodate the target sizes, then it chooses smaller sizes with the same quantization.

> > I'm not sure there is a set of common screen resolutions that makes sense.
> > We are enforcing the content window dimensions, not the screen dimensions,
> > so taskbars, menubars, the window chrome, etc., affect the size available
> > for content. I think the dimensions you are setting your window to are
> > likely to increase your fingerprintability rather than decrease it.
> > 
> > I am open to ideas to improving this feature, but because window dimensions
> > provide a lot of fingerprinting entropy, I think it's important that
> > privacy.resistFingerprinting includes this feature.
>
> NO idea if every real world scenario would fit in that set, but surely the
> smallest size would cover all, or fallback to the old code? Just throwing
> some ideas at you from an end user perspective who would HATE "my" Firefox
> to lose any usability (side bars etc) due to a lack of discussion or
> options, or be forced to too narrow a width for viewing. There has to be
> some flexibility or people will be turned off. Thanks :)

Your feedback is welcome, and I agree that it's worth investigating further what set of sizes we should permit. Also, after this patch lands, we can always revise the set of allowed sizes again as we get user feedback.

Comment 6

[Thorin [:thorin]]

Note: privacy.resistFingerprinting is enabled.

Whoer (https://whoer.net/#extended)
results:
- height 800 
- width 1280 
- availHeight 800 
- availWidth 1280 
- window size 1268x800 (1280x800) 

JoDonym (http://ip-check.info/?lang=en)
"inner", "browser" and "usable-screen" : all 1280x800

Panoptoclick (https://panopticlick.eff.org/)
"Screen Size": 1280x800

Browserspy (http://browserspy.dk/screen.php)
"Screen" : 1280x800

Whoer.net is taking the vertical scroll bar of 12 pixels into account. Is this an entropy issue? Such as blocked content from one user resulting in no scroll bar, and others getting it. Do scroll bar widths vary with themes?

===
Arthur, with a little more thought, I agree that round hundreds are best (we can't predict the future). So please ignore the using "common" screen resolutions. A select few round-hundred combinations can cover all screens (they don't HAVE to use the most height or width available, we can have an upper and lower bound) - it would certainly be lower than the dozen or two screen resolutions floating around (not that you would use them all). Just MORE-but-ENFORCED (or enforced at a later date) options for end users would be ideal.

Comment 7

[Tim Huang[:timhuang]]

Attachment: Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

Review commit: https://reviewboard.mozilla.org/r/117404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117404/

Comment 8

[Tim Huang[:timhuang]]

Attachment: Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

Review commit: https://reviewboard.mozilla.org/r/117406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117406/

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843814 [details]
Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

https://reviewboard.mozilla.org/r/117404/#review119178

I'd like to see those changes.

Not really about this bug but, how does this size rounding work with fullscreen? Do we not support fullscreen when the fingerprint pref is set or what?

::: xpfe/appshell/nsXULWindow.cpp:1072
(Diff revision 1)
> +  int32_t availForContentWidthCSS =
> +    std::min(maxInnerWidth, NSToIntRound((0.95 * availWidth - chromeWidth) /
> +                                         devicePerCSSPixels));
> +  int32_t availForContentHeightCSS =
> +    std::min(maxInnerHeight, NSToIntRound((0.95 * availHeight - chromeHeight) /
> +                                          devicePerCSSPixels));

0.95 is rather magical. Why is that needed? Especially with width, where content width is usually pretty much the same as chrome width.

::: xpfe/appshell/nsXULWindow.cpp:1079
(Diff revision 1)
> +    NSToIntRound(devicePerCSSPixels *
> +                 (availForContentWidthCSS - (availForContentWidthCSS % 200)));
> +  int32_t targetContentHeight =
> +    NSToIntRound(devicePerCSSPixels *
> +                 (availForContentHeightCSS - (availForContentHeightCSS % 100)));
> +

This needs some good comment, why 200 and 100 are being used there.

::: xpfe/appshell/nsXULWindow.cpp:1128
(Diff revision 1)
>      }
>  
>      if (gotSize) {
>        SetSpecifiedSize(specWidth, specHeight);
>      }
>  

This code would be easier to understand if there was 'else' after if (ShouldResistFingerprinting).
Now one needs to know that ResizeToRoundedDimensions sets mIgnoreXULSize and mIgnoreXULSizeMode.

Also, could we call ResizeToRoundedDimensions to
ForceRoundedDimensions or something to hint that it may do also other thing than just resize.
(setting mIgnoreXULSize = true; and mIgnoreXULSizeMode = true; is quite major side effect)

Comment 10

[Tim Huang[:timhuang]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #9)
> Comment on attachment 8843814 [details]
> Bug 1330882 - Part 1: Making new windows to be rounded size when
> fingerprinting resistance is enabled.
> 
> https://reviewboard.mozilla.org/r/117404/#review119178
> 
> I'd like to see those changes.
> 
> Not really about this bug but, how does this size rounding work with
> fullscreen? Do we not support fullscreen when the fingerprint pref is set or
> what?
> 

I think this bug is targeting to enforce newly opened windows to be a rounded size after fingerprinting resistance is enabled. So, this patch here does not handle with fullscreen or resizing. The work of fullscreen or resizing will be a separate bug and how to do it is under discussion. One possible solution is that window.innerWidth and innerHeight reports a nearest rounded value after resize or maximize windows. Another one is that automatically quantize the window size after resizing or maximizing, fullscreen might be disabled in this case.

Arthur, what is the way that Tor browser deal with fullscreen? 

> ::: xpfe/appshell/nsXULWindow.cpp:1072
> (Diff revision 1)
> > + int32_t availForContentWidthCSS =
> > + std::min(maxInnerWidth, NSToIntRound((0.95 * availWidth - chromeWidth) /
> > + devicePerCSSPixels));
> > + int32_t availForContentHeightCSS =
> > + std::min(maxInnerHeight, NSToIntRound((0.95 * availHeight - chromeHeight) /
> > + devicePerCSSPixels));
> 
> 0.95 is rather magical. Why is that needed? Especially with width, where
> content width is usually pretty much the same as chrome width.
> 

Arthur, could you answer this for me? And I agree that width doesn't have to use this magical number.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8843815 [details]
Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/117406/#review119220

Almost there, but I'd like to take a look at the final patch.

::: browser/components/sessionstore/SessionStore.jsm:210
(Diff revision 1)
> + * A global value to tell that fingerprinting resistance is enabled or not.
> + * If it's enabled, the session restore won't restore the window's size and
> + * size mode.
> + * This value is controlled by preference privacy.resistFingerprinting.
> + */
> +var gIsResistFingerPrinting = false;

nit: can you rename this to `gResistFingerPrintingEnabled` ?

::: browser/components/sessionstore/SessionStore.jsm:698
(Diff revision 1)
>      this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);
> +
> +    try {
> +      gIsResistFingerPrinting = Services.prefs.getBoolPref("privacy.resistFingerprinting");
> +    } catch (ex) {
> +      if (ex.result != Cr.NS_ERROR_UNEXPECTED) {

When will this happen? Are you not initializing this pref in firefox.js or perhaps even all.js?

::: browser/components/sessionstore/SessionStore.jsm:704
(Diff revision 1)
> +        Cu.reportError(ex);
> +      }
> +    }
> +    Services.prefs.addObserver("privacy.resistFingerprinting", () => {
> +      gIsResistFingerPrinting = Services.prefs.getBoolPref("privacy.resistFingerprinting");
> +    }, false);

I *think* `false` is the default. Can you make this work like all the other pref observers in this class:
`Services.prefs.addObserver("privacy.resistFingerprinting", this);` and do the variable setting in `onPrefChange` below.

::: browser/components/sessionstore/SessionStore.jsm:3867
(Diff revision 1)
>        if (aSizeMode != "maximized" || win_("sizemode") != "maximized") {
>          aWindow.resizeTo(aWidth, aHeight);
>        }
>      }
> -    if (aSizeMode && win_("sizemode") != aSizeMode)
> +    if (aSizeMode && win_("sizemode") != aSizeMode &&
> +        !gIsResistFingerPrinting)

nit: 80 chars is not a hard limit; if it improves legibility of the code we pragmatically allow an upper limit of 120 chars. IOW: you can leave this (and probably the one above too) on the same line!

Comment 12

[Tim Huang[:timhuang]]

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Comment on attachment 8843815 [details]
> Bug 1330882 - Part 2: Disallow the session restore to modify window size
> when fingerprinting resistance is enabled.
> 
> ::: browser/components/sessionstore/SessionStore.jsm:698
> (Diff revision 1)
> > this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);
> > +
> > + try {
> > + gIsResistFingerPrinting = Services.prefs.getBoolPref("privacy.resistFingerprinting");
> > + } catch (ex) {
> > + if (ex.result != Cr.NS_ERROR_UNEXPECTED) {
> 
> When will this happen? Are you not initializing this pref in firefox.js or
> perhaps even all.js?
> 

Yes, this pref 'privacy.resistFingerprinting' has not been added into firefox.js or all.js. But, we are planning to add this pref in a separate bug. And that bug will remove these try-catch code sections.

Comment 13

[Mike de Boer [:mikedeboer]]

(In reply to Tim Huang[:timhuang] from comment #12)
> Yes, this pref 'privacy.resistFingerprinting' has not been added into
> firefox.js or all.js. But, we are planning to add this pref in a separate
> bug. And that bug will remove these try-catch code sections.

Well, adding this pref to either of these files is a low bar and trivial to do. I'd prefer it if you'd just add them here, but if you'd like to track that work somewhere else, please CC me on the bug :)

Comment 14

[Arthur Edelstein [:arthur]]

(In reply to Tim Huang[:timhuang] from comment #10)

> Arthur, what is the way that Tor browser deal with fullscreen? 

In fullscreen, we have a separate patch in torbutton (an extension that is bundled with Tor Browser) that introduces empty margins at top and bottom or left and right. It also applies a zoom so that the screen is filled as much as possible. We also have a patch that quantizes the content bounds while the user resizes the window (e.g., by dragging the corner). But our patches need work, and I think it is good to keep them separate.

> > 0.95 is rather magical. Why is that needed? Especially with width, where
> > content width is usually pretty much the same as chrome width.
> 
> Arthur, could you answer this for me? And I agree that width doesn't have to
> use this magical number.

Yes, that's true. In fact in Tor Browser we are working to get rid of 0.95 from both width and height. But that  requires a bug fix for 581863 that I am working on here: https://trac.torproject.org/projects/tor/ticket/20905

But I agree, for now you can get rid of the 0.95 factor from the width.

Comment 15

[Arthur Edelstein [:arthur]]

Comment on attachment 8843815 [details]
Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/117406/#review119656

::: browser/components/sessionstore/SessionStore.jsm:210
(Diff revision 1)
> + * A global value to tell that fingerprinting resistance is enabled or not.
> + * If it's enabled, the session restore won't restore the window's size and
> + * size mode.
> + * This value is controlled by preference privacy.resistFingerprinting.
> + */
> +var gIsResistFingerPrinting = false;

I would suggest calling this gResistFingerprintingEnabled (lowercase p). We have been treating fingerprinting as a single word in all other references.

::: browser/components/sessionstore/SessionStore.jsm:698
(Diff revision 1)
>      this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);
> +
> +    try {
> +      gIsResistFingerPrinting = Services.prefs.getBoolPref("privacy.resistFingerprinting");
> +    } catch (ex) {
> +      if (ex.result != Cr.NS_ERROR_UNEXPECTED) {

I agree with Mike that we should give this pref a default value.

Comment 16

[Arthur Edelstein [:arthur]]

Comment on attachment 8843814 [details]
Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

https://reviewboard.mozilla.org/r/117404/#review119658

::: commit-message-32dcd:1
(Diff revision 1)
> +Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled. r?smaug

Would be nice to refer here to tor #19459 :)

::: browser/base/content/browser.js:1173
(Diff revision 1)
>      // their tasks BEFORE the browser window is shown. SessionStore uses it to
>      // restore tabs into windows AFTER important parts like gMultiProcessBrowser
>      // have been initialized.
>      Services.obs.notifyObservers(window, "browser-window-before-show", "");
>  
> +    let isResistFingerPrinting = false;

Call this `isResistFingerprintingEnabled` (lowercase p).

Comment 17

[Tim Huang[:timhuang]]

Comment on attachment 8843814 [details]
Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117404/diff/1-2/

Comment 18

[Tim Huang[:timhuang]]

Comment on attachment 8843815 [details]
Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117406/diff/1-2/

Comment 19

[Tim Huang[:timhuang]]

Comment on attachment 8843815 [details]
Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/117406/#review119220

> I *think* `false` is the default. Can you make this work like all the other pref observers in this class:
> `Services.prefs.addObserver("privacy.resistFingerprinting", this);` and do the variable setting in `onPrefChange` below.

We still have to give a `false` here since it will throw an exception if we don't give it.

Comment 20

[Tim Huang[:timhuang]]

Attachment: Bug 1330882 - Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled.

Review commit: https://reviewboard.mozilla.org/r/118498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118498/

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8843815 [details]
Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/117406/#review120458

Well, since Gijs rs'd the patch that adds the pref, can you land that one first and this patch without the try...catch? With that, r=me. Thanks!

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8845319 [details]
Bug 1330882 - Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/118498/#review120718

::: browser/components/resistfingerprinting/test/browser/.eslintrc.js:5
(Diff revision 1)
> +"use strict";
> +
> +module.exports = {
> +  "extends": [
> +    "../../../../../testing/mochitest/browser.eslintrc.js"

I have no idea what this is.
I assume it is right

::: browser/components/resistfingerprinting/test/browser/browser_roundedWindow.js:11
(Diff revision 1)
> +const { classes: Cc, Constructor: CC, interfaces: Ci, utils: Cu } = Components;
> +
> +const TEST_DOMAIN = "http://example.net/";
> +const TEST_PATH = TEST_DOMAIN + "browser/browser/components/resistFingerprinting/test/browser/";
> +
> +let desireWidth  = 1000;

should it be desired, not desire


Why initialize the values here, when they are overridden later.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8845319 [details]
Bug 1330882 - Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/118498/#review120728

::: browser/components/resistfingerprinting/test/browser/browser_roundedWindow.js:53
(Diff revision 1)
> +      is(content.innerWidth, obj.desireWidth,
> +        "The window.innerWidth has a correct rounded value");
> +      is(content.innerHeight, obj.desireHeight,
> +        "The window.innerHeight has a correct rounded value");
> +    }
> +  );

I think there should be tests for window.open() usage too, when it is opening windows with different sizes.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

How does this all work when one does something like
var w = window.open("", "", "width=10000");
w.onresize = function() { console.log(w.innerWidth); }

Please add some tests at least for window.open handling.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843814 [details]
Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

https://reviewboard.mozilla.org/r/117404/#review120722

::: xpfe/appshell/nsXULWindow.h:94
(Diff revision 2)
>     NS_IMETHOD EnsureContentTreeOwner();
>     NS_IMETHOD EnsurePrimaryContentTreeOwner();
>     NS_IMETHOD EnsurePrompter();
>     NS_IMETHOD EnsureAuthPrompter();
> +   NS_IMETHOD ForceRoundedDimensions();
> +   NS_IMETHOD GetAvailScreenSize(int32_t* availWidth, int32_t* availHeight);

arguments should be in form aName

::: xpfe/appshell/nsXULWindow.cpp:1001
(Diff revision 2)
>        wwatch->GetNewAuthPrompter(ourWindow, getter_AddRefs(mAuthPrompter));
>    }
>    return mAuthPrompter ? NS_OK : NS_ERROR_FAILURE;
>  }
> +
> +NS_IMETHODIMP nsXULWindow::GetAvailScreenSize(int32_t* availWidth, int32_t* availHeight)

ditto

::: xpfe/appshell/nsXULWindow.cpp:1071
(Diff revision 2)
> +                                           1000);
> +  int32_t availForContentWidthCSS =
> +    std::min(maxInnerWidth, NSToIntRound((availWidth - chromeWidth) /
> +                                         devicePerCSSPixels));
> +  int32_t availForContentHeightCSS =
> +    std::min(maxInnerHeight, NSToIntRound((0.95 * availHeight - chromeHeight) /

Why this magical 0.95? Please add a comment

Comment 26

[Tim Huang[:timhuang]]

It looks like that the window.open() can still change its size after fingerprinting resistance is enabled. I will update the test case and write a new patch to address this issue.

Comment 27

[Tim Huang[:timhuang]]

Attachment: Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

This patch makes window.open() ignores 'width' and 'height' in its window features for content windows when fingerprinting is enabled, so the window.open() can only open a rounded window. The chrome window, however, can still specify window features when using window.open().

Review commit: https://reviewboard.mozilla.org/r/118948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118948/

Comment 28

[Tim Huang[:timhuang]]

Comment on attachment 8843814 [details]
Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117404/diff/2-3/

Comment 29

[Tim Huang[:timhuang]]

Comment on attachment 8843815 [details]
Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117406/diff/2-3/

Comment 30

[Tim Huang[:timhuang]]

Comment on attachment 8845319 [details]
Bug 1330882 - Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118498/diff/1-2/

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

What about one opening a new window and resizing afterwards using window.innerWidth = <new value> or such?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/118948/#review120990

I'd rather see this feature break less the web than more, so rounding the size to some reasonable would be good.
And perhaps not even round, but just add upper-limits for the size - similar limits what the other patch gives for newly opened windows.

::: toolkit/components/windowwatcher/nsWindowWatcher.cpp:2481
(Diff revision 1)
>      // at the widget level, or whatever), let's re-fetch the scale factor for
>      // wherever it really ended up
>      treeOwnerAsWin->GetUnscaledDevicePixelsPerCSSPixel(&scale);
>    }
> -  if (aSizeSpec.SizeSpecified()) {
> +  if (aSizeSpec.SizeSpecified() &&
> +      (aIsCallerChrome || !nsContentUtils::ShouldResistFingerprinting())) {

Isn't this too strict? Why prevent all the sizes?
I mean, why not just round the size?

Also, does this affect whether the new window is opened in a new tab or new window?

Couple of lines above we already do checks for too small windows. Couldn't that round the size?

Comment 33

[Tim Huang[:timhuang]]

(In reply to Olli Pettay [:smaug] from comment #31)
> What about one opening a new window and resizing afterwards using
> window.innerWidth = <new value> or such?

I don't think we have to limit this behavior here since the window size which is set by a user is what matters browser fingerprinting. IOW, if a website opens a new window then setting its size through window.innerWidth or innerHeight does not reveal anything about a user because this value is given by the website but not by the user.

And we still remain the capability that users can customize their window size by dragging the window corner or scripts like window.innerWidth even if the fingerprinting resistance is on, which is consistent with Tor browser. Maybe we have to disable this for preventing increasing entropy, but it needs more discussions and not in the scope of this bug.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Tim Huang[:timhuang] from comment #33)
> (In reply to Olli Pettay [:smaug] from comment #31)
> > What about one opening a new window and resizing afterwards using
> > window.innerWidth = <new value> or such?
> 
> I don't think we have to limit this behavior here since the window size
> which is set by a user is what matters browser fingerprinting. IOW, if a
> website opens a new window then setting its size through window.innerWidth
> or innerHeight does not reveal anything about a user because this value is
> given by the website but not by the user.
> 

What you mean? The actual window size changes when window.innerWidth is set to a new value.
How is that not revealing the exactly the same information as opening a new window?
First set innerWidth to very high value and after that read what its current value is and you now know
what the size of the screen is.


> And we still remain the capability that users can customize their window
> size by dragging the window corner
That is fine, but I'm not talking about it :)

> or scripts like window.innerWidth even if
> the fingerprinting resistance is on, which is consistent with Tor browser.
> Maybe we have to disable this for preventing increasing entropy, but it
> needs more discussions and not in the scope of this bug.
Well, I don't then quite understand what Tor is trying to do.
Maybe Arthur could clarify that?

Comment 35

[Tim Huang[:timhuang]]

(In reply to Olli Pettay [:smaug] from comment #34)
> (In reply to Tim Huang[:timhuang] from comment #33)
> > (In reply to Olli Pettay [:smaug] from comment #31)
> > > What about one opening a new window and resizing afterwards using
> > > window.innerWidth = <new value> or such?
> > 
> > I don't think we have to limit this behavior here since the window size
> > which is set by a user is what matters browser fingerprinting. IOW, if a
> > website opens a new window then setting its size through window.innerWidth
> > or innerHeight does not reveal anything about a user because this value is
> > given by the website but not by the user.
> > 
> 
> What you mean? The actual window size changes when window.innerWidth is set
> to a new value.
> How is that not revealing the exactly the same information as opening a new
> window?
> First set innerWidth to very high value and after that read what its current
> value is and you now know
> what the size of the screen is.
> 

I think your concern is right here. But, IIUC, the window.innerWidth/Height is a replaceable attributes [1] if it is a content window, so a normal web cannot use this way to acquire available screen size. Only chrome window can change size through innerWidth/innerHeight. So, are you saying that we should make chrome window cannot change size when fingerprinting resistance is on, or, at least, window size should be rounded after setting.  

[1] http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#14616

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Web pages definitely can change the size of a window.
Something like
data:text/html,<a href='' onclick='window.w = window.open("about:blank", "", "width=400"); window.w.onload = function() { window.w.onresize=function() { console.log(window.w.innerWidth); }; window.w.innerWidth = 5000 }' >test</a>

Comment 37

[Stephan Sokolow]

Some environments (typically tiling window managers, but also the "Window Rules" feature in KDE's KWin in "force" mode) overrule attempts by programs to enforce dimensions (eg. ignoring Vim's request to snap its dimensions to multiples of the character cell size), so you'll want to take that into consideration.

In the worst case (appearance-wise), I'd do what Vim does and reconcile the difference by padding out the viewport with empty "window background" on the bottom and right edges.

(In my case, I turn off window decorations to ensure that the top row of pixels on the screen gets events routed to the tab bar, so it'd be nice if I could have Firefox retain that ability while mitigating fingerprinting by simply snapping the viewport size to what would be more typical for users with 1280x1024 monitors, then distributing the leftover space somewhere else in the window.)

Comment 38

[Thorin [:thorin]]

see the discussion at https://trac.torproject.org/projects/tor/ticket/9881

BY setting
* browser.link.open_newwindow.restriction -> 0
* dom.disable_window_move_resize -> true
* full-screen-api.enabled -> false
This mitigates "malicious" windows sizes

Note: you can still right click a link and open in a new window (been a while, I would have to double check).

Comment 39

[Arthur Edelstein [:arthur]]

(In reply to Olli Pettay [:smaug] from comment #34)
> (In reply to Tim Huang[:timhuang] from comment #33)
> > or scripts like window.innerWidth even if
> > the fingerprinting resistance is on, which is consistent with Tor browser.
> > Maybe we have to disable this for preventing increasing entropy, but it
> > needs more discussions and not in the scope of this bug.
> Well, I don't then quite understand what Tor is trying to do.
> Maybe Arthur could clarify that?

We have a separate patch in torbutton (part of Tor Browser; see https://trac.torproject.org/14429) that enforces "quantized" innerWidth and innerHeight for content. So when the user resizes a window, the window "snaps" to a multiple of 100 x 100 pixels.

However this patch is quite complex and needs some polishing before it can be uplifted to Firefox. In the meantime we think the patch Tim is uplifting in this ticket provides a good partial benefit. The size of the screen is not revealed unless the user maximizes the window. And tabs can't be easily correlated by size unless the user manually resizes the window to something unique.

(In reply to Olli Pettay [:smaug] from comment #36)
> Web pages definitely can change the size of a window.
> Something like
> data:text/html,<a href='' onclick='window.w = window.open("about:blank", "",
> "width=400"); window.w.onload = function() { window.w.onresize=function() {
> console.log(window.w.innerWidth); }; window.w.innerWidth = 5000 }' >test</a>

That's an interesting demonstration, but I think it doesn't reveal the size of the screen, because a large window like that simply expands beyond the screen's boundaries.

Comment 40

[Arthur Edelstein [:arthur]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/118948/#review120990

I do think there is a benefit to rounding in that it prevents multiple tabs on the new window from being easily correlated by contentWindow.innerWidth/Height. We could round the window size to the nearest 200 x 100 larger than the specified size. An upper limit also probably makes sense.

Comment 41

[Arthur Edelstein [:arthur]]

Comment on attachment 8845319 [details]
Bug 1330882 - Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/118498/#review121226

Looks good.

Comment 42

[Arthur Edelstein [:arthur]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/118948/#review121228

Comment 43

[Stephan Sokolow]

(In reply to Arthur Edelstein [:arthuredelstein] from comment #39)
> That's an interesting demonstration, but I think it doesn't reveal the size
> of the screen, because a large window like that simply expands beyond the
> screen's boundaries.

While I doubt it's a significant enough slice of the user base to matter, I should mention that there do exist situations where that's not entirely true.

It's been years, but I know that one of the window managers I experimented with in the past would clamp and move windows to ensure that the resize handle couldn't pass out of the bounds of the screen or get covered up by an always-on-top panel.

In that context, all you'd need to do is specify some ridiculously large size and then read off what you actually got.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Arthur Edelstein [:arthuredelstein] from comment #39)

> (In reply to Olli Pettay [:smaug] from comment #36)
> > Web pages definitely can change the size of a window.
> > Something like
> > data:text/html,<a href='' onclick='window.w = window.open("about:blank", "",
> > "width=400"); window.w.onload = function() { window.w.onresize=function() {
> > console.log(window.w.innerWidth); }; window.w.innerWidth = 5000 }' >test</a>
> 
> That's an interesting demonstration, but I think it doesn't reveal the size
> of the screen, because a large window like that simply expands beyond the
> screen's boundaries.

On my machines that limits the width of the window to 1920, which is the actual size

Comment 45

[Tim Huang[:timhuang]]

(In reply to Olli Pettay [:smaug] from comment #44)
> On my machines that limits the width of the window to 1920, which is the
> actual size

Same for my system. So I think that we have to have some restriction for setting window.innerWidth/innerHeight. If someone sets them with a value which is greater than the maximum rounded size, the innerWidth/Height should be set to the maximum rounded size. If the value is less than the maximum rounded size, we should round the size to the nearest upper 200x100.

Comment 46

[Arthur Edelstein [:arthur]]

(In reply to Tim Huang[:timhuang] from comment #45)
> (In reply to Olli Pettay [:smaug] from comment #44)
> > On my machines that limits the width of the window to 1920, which is the
> > actual size
> 
> Same for my system. So I think that we have to have some restriction for
> setting window.innerWidth/innerHeight. If someone sets them with a value
> which is greater than the maximum rounded size, the innerWidth/Height should
> be set to the maximum rounded size. If the value is less than the maximum
> rounded size, we should round the size to the nearest upper 200x100.

Makes sense to me. Thanks to both of you for finding this issue.

Comment 47

[Tim Huang[:timhuang]]

Attachment: Bug 1330882 - Part 5: Add more test cases for rounded windows test.

This patch adds two more test cases, browser_roundedWindow_open.js and
browser_roundedWindow_windowSetting.js. The browser_roundedWindow_open.js tests
the window.open() with window features, it will test window.open() with numbers
of window features to see that whether the opend window is correctly rounded.

The browser_roundedWindow_windowSetting.js tests the setting of
innerWidth/Height and outerWidth/Height. To see that the window is correctly
rounded or not after the setting.

This patch also adds a head.js and rename the browser_roundedWindow.js to
browser_roundedWindow_newWindow.js. The head.js carries two helper functions
that calculate the maximum available content size and the chrome UI size of
the pop up window.

Review commit: https://reviewboard.mozilla.org/r/120900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120900/

Comment 48

[Tim Huang[:timhuang]]

Comment on attachment 8843814 [details]
Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117404/diff/3-4/

Comment 49

[Tim Huang[:timhuang]]

Comment on attachment 8843815 [details]
Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117406/diff/3-4/

Comment 50

[Tim Huang[:timhuang]]

Comment on attachment 8845319 [details]
Bug 1330882 - Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118498/diff/2-3/

Comment 51

[Tim Huang[:timhuang]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118948/diff/1-2/

Comment 52

[Arthur Edelstein [:arthur]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/118948/#review123192

Can more of the code in this patch be refactored to share code with the other patches? I'm not 100% sure, but it seems to me the code for rounding both new windows and SetInnerWidth should be almost entirely sharable. (Sorry if I'm wrong about this.) It would be nice to combine in terms of consistency and future maintenance.

::: dom/base/nsContentUtils.cpp:2167
(Diff revision 2)
>    return !isChrome && sPrivacyResistFingerprinting;
>  }
>  
> +/* static */
> +void
> +nsContentUtils::CalcMaximumInnerWindowSizeForResistFingerprinting(int32_t  aChromeWidth,

I find this name confusing. Maybe call this `CalcAvailableInnerWindowSizeForResistingFingerprinting`?

::: dom/base/nsContentUtils.cpp:2189
(Diff revision 2)
> +  // height. It is not necessary for the width since the content width
> +  // is usually pretty much the same as the chrome width.
> +  availContentWidth = std::min(sPrivacyMaxInnerWidth,
> +                                (aScreenWidth - aChromeWidth));
> +  availContentHeight = std::min(sPrivacyMaxInnerHeight,
> +                                NSToIntRound(0.95 * aScreenHeight - aChromeHeight));

It occurs to me now that we could change the correction from `0.95 *`  to `-40 +` because the correction shouldn't really depend on available screen size. Instead it depends on the height of the titlebar on system windows. In my experience, on GTK this height is 26 pixels, so 40px should be pretty safe for most systems.

Also the correction is not needed at all on Mac or Windows, because they correctly include decorations in the outerHeight. So maybe use `%ifdef MOZ_WIDGET_GTK` for the correction?

Comment 53

[Arthur Edelstein [:arthur]]

Comment on attachment 8847962 [details]
Bug 1330882 - Part 5: Add more test cases for rounded windows test.

https://reviewboard.mozilla.org/r/120900/#review123200

Looks great.

::: commit-message-3c023:6
(Diff revision 1)
> +Bug 1330882 - Part 5: Add more test cases for rounded windows test. r?smaug,arthuredelstein
> +
> +This patch adds two more test cases, browser_roundedWindow_open.js and
> +browser_roundedWindow_windowSetting.js. The browser_roundedWindow_open.js tests
> +the window.open() with window features, it will test window.open() with numbers
> +of window features to see that whether the opend window is correctly rounded.

Typo: opened

::: browser/components/resistfingerprinting/test/browser/head.js:8
(Diff revision 1)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +
> +// This function calculates the maximum available window size and returns them

Nit: "window dimensions" or "window sizes"

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/118948/#review123504

Better to review this after the feedback from Arthur has been addressed.

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8847962 [details]
Bug 1330882 - Part 5: Add more test cases for rounded windows test.

https://reviewboard.mozilla.org/r/120900/#review123506

Comment 56

[Tim Huang[:timhuang]]

Comment on attachment 8843814 [details]
Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117404/diff/4-5/

Comment 57

[Tim Huang[:timhuang]]

Comment on attachment 8843815 [details]
Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117406/diff/4-5/

Comment 58

[Tim Huang[:timhuang]]

Comment on attachment 8845319 [details]
Bug 1330882 - Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118498/diff/3-4/

Comment 59

[Tim Huang[:timhuang]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118948/diff/2-3/

Comment 60

[Tim Huang[:timhuang]]

Comment on attachment 8847962 [details]
Bug 1330882 - Part 5: Add more test cases for rounded windows test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120900/diff/1-2/

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/118948/#review123870

::: dom/base/nsGlobalWindow.cpp:14643
(Diff revision 3)
>    if (!ValueToPrimitive<int32_t, eDefault>(aCx, aValue, &value)) {
>      aError.Throw(NS_ERROR_UNEXPECTED);
>      return;
>    }
>  
> +  if (nsContentUtils::ShouldResistFingerprinting(GetDocShell())) {

I wonder if all this should go to the actual methods dealing with size changes, that way we wouldn't need to do rather ugly string comparisons.
But I guess this might be a tad simpler after all.

::: dom/base/nsGlobalWindow.cpp:14692
(Diff revision 3)
> +          int32_t* targetContentWidth = nullptr;
> +          int32_t* targetContentHeight = nullptr;
> +          int32_t unused = 0, screenWidth = 0, screenHeight = 0;
> +          int32_t chromeWidth = 0, chromeHeight = 0;
> +          int32_t inputWidth = 0, inputHeight = 0;
> +

Nit, each variable definition goes to its own line.

Comment 62

[Tim Huang[:timhuang]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118948/diff/3-4/

Comment 63

[Tim Huang[:timhuang]]

Comment on attachment 8847962 [details]
Bug 1330882 - Part 5: Add more test cases for rounded windows test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120900/diff/2-3/

Comment 64

[Tim Huang[:timhuang]]

Comment on attachment 8843814 [details]
Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117404/diff/5-6/

Comment 65

[Tim Huang[:timhuang]]

Comment on attachment 8843815 [details]
Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117406/diff/5-6/

Comment 66

[Tim Huang[:timhuang]]

Comment on attachment 8845319 [details]
Bug 1330882 - Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118498/diff/4-5/

Comment 67

[Tim Huang[:timhuang]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118948/diff/4-5/

Comment 68

[Tim Huang[:timhuang]]

Comment on attachment 8847962 [details]
Bug 1330882 - Part 5: Add more test cases for rounded windows test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120900/diff/3-4/

Comment 69

[Arthur Edelstein [:arthur]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

Thanks, Tim!

Comment 70

[Tim Huang[:timhuang]]

Comment on attachment 8843814 [details]
Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117404/diff/6-7/

Comment 71

[Tim Huang[:timhuang]]

Comment on attachment 8843815 [details]
Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117406/diff/6-7/

Comment 72

[Tim Huang[:timhuang]]

Comment on attachment 8845319 [details]
Bug 1330882 - Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118498/diff/5-6/

Comment 73

[Tim Huang[:timhuang]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118948/diff/5-6/

Comment 74

[Tim Huang[:timhuang]]

Comment on attachment 8847962 [details]
Bug 1330882 - Part 5: Add more test cases for rounded windows test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120900/diff/4-5/

Comment 75

[Tim Huang[:timhuang]]

Hi, Arthur. I don't know why your r+ in comment 69 doesn't show up on the review board. And the new version of the patch 'part 4' is only for rebasing. So you can either ignore the review request or give it r+ directly.

Comment 76

[Tim Huang[:timhuang]]

Try looks good.

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

Comment 77

[Ryan VanderMeulen [:RyanVM]]

This was autolanded successfully, but pulsebot was down at the time and the bug never got updated.

Comment 78

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad5841073c68
Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459). r=arthuredelstein,smaug
https://hg.mozilla.org/integration/autoland/rev/792b96eb03a0
Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled. r=arthuredelstein,mikedeboer
https://hg.mozilla.org/integration/autoland/rev/08657a598ae9
Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled. r=arthuredelstein,smaug
https://hg.mozilla.org/integration/autoland/rev/0fa4f1c4f1ee
Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled. r=smaug
https://hg.mozilla.org/integration/autoland/rev/937027c942ae
Part 5: Add more test cases for rounded windows test. r=arthuredelstein,smaug

Comment 79

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ad5841073c68
https://hg.mozilla.org/mozilla-central/rev/792b96eb03a0
https://hg.mozilla.org/mozilla-central/rev/08657a598ae9
https://hg.mozilla.org/mozilla-central/rev/0fa4f1c4f1ee
https://hg.mozilla.org/mozilla-central/rev/937027c942ae

Comment 80

[Arthur Edelstein [:arthur]]

Comment on attachment 8845798 [details]
Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled.

https://reviewboard.mozilla.org/r/118948/#review127778

Comment 81

[Dão Gottwald [:dao]]

Comment on attachment 8843814 [details]
Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459).

https://reviewboard.mozilla.org/r/117404/#review148656

::: browser/base/content/browser.js:1193
(Diff revision 7)
> +      document.documentElement.setAttribute("sizemode", "normal");
> +    } else if (!document.documentElement.hasAttribute("width")) {
>        const TARGET_WIDTH = 1280;
>        const TARGET_HEIGHT = 1040;
>        let width = Math.min(screen.availWidth * .9, TARGET_WIDTH);
>        let height = Math.min(screen.availHeight * .9, TARGET_HEIGHT);

This code is about the very first browser window opened in a fresh profile. How does disabling this code help resist fingerprinting? Without it, what dimensions is that window going to have?

Comment 82

[Tim Huang[:timhuang]]

Without this code, users have a small screen resolution, less than 1280x1040, will open a maximized window when opening in a fresh profile. A maximized window can reveal your screen resolution which is a fingerprintable factor. So, we try to not open a maxmized window when fingerprinting resistance is enabled.

Actually, 'privacy.resistFingerprinting' is false by default for Firefox, so this is not our use case. However, TorBrowser relies on this to prevent opening a maxmized window for certain users because they enable it by default.

Comment 83

[Hugo Osvaldo Barrera]

This approach seems to only work for composing/floating window managers, but has no effect on tiling window managers, since there's no clear concept of window size there.

I think the extra space should be somehow filled in somehow on non-floating WMs.

Comment 84

[Stephan Sokolow]

Whatever approach is used to fill in the extra space in situations where the WM overrules Firefox, I'd appreciate if it weren't too difficult to tweak Firefox's toolbar layout to convert as much of that padding as possible into usable toolbar space.

(And I define "usable" as being "multiple rows", not "make the icons bigger so you can't fit as many")

Comment 85

[Tom Grabowski [:TomGrab]]

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #0)
> In Tor Browser (where privacy.resistFingerprinting is enabled), when a new
> window is opened, the dimensions of the content are always set to 1000x1000,
> or, if enough screen space is not available, to the nearest 200x100. 

I did some testing with Nightly on Mac OS, and the results are not as expected.
58.0a1 (2017-10-20) (64-bit)

Here are some examples:
1. Screen resolution: 1680x1050
Expected new windows size: 1000x1000
Actual new window dimensions: 1000x800

2. Screen resolution: 1344x840
Expected: 1000x800
Actual: 600x500

3. Screen resolution: 1024x640
Expected: 900x600
Actual: 600x500

4. Screen resolution: 1280x1024
Expected: 1000x1000
Actual: 1000x800

5. Screen resolution: 1050x1680
Expected: 1000x1000
Actual: 1000x1100

Tim Huang, could you please comment on this? Maybe I misunderstood what "the nearest 200x100" mean.

Comment 86

[Tim Huang[:timhuang]]

(In reply to Tom Grabowski [:TomGrab] from comment #85)
> (In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from
> comment #0)
> > In Tor Browser (where privacy.resistFingerprinting is enabled), when a new
> > window is opened, the dimensions of the content are always set to 1000x1000,
> > or, if enough screen space is not available, to the nearest 200x100. 
> 
> I did some testing with Nightly on Mac OS, and the results are not as
> expected.
> 58.0a1 (2017-10-20) (64-bit)
> 
> Here are some examples:
> 1. Screen resolution: 1680x1050
> Expected new windows size: 1000x1000
> Actual new window dimensions: 1000x800
> 
> 2. Screen resolution: 1344x840
> Expected: 1000x800
> Actual: 600x500
> 
> 3. Screen resolution: 1024x640
> Expected: 900x600
> Actual: 600x500
> 
> 4. Screen resolution: 1280x1024
> Expected: 1000x1000
> Actual: 1000x800
> 

These four situations are expected since we need to subtract the hight of chrome elements when we calculate the window size. And system utilities, like the dock of MACOS, could make the available window size less. Given that, the actual available window size for content would be smaller than the real screen resolution. 


> 5. Screen resolution: 1050x1680
> Expected: 1000x1000
> Actual: 1000x1100
> 

This seems wrong to me, I will take a look, thanks.

Comment 87

[Tim Huang[:timhuang]]

Hi Tom,

I have tested my MAC with a monitor with 1080x1920 resolution, and the result shows that it gives me a 1000x1000 content window when fingerprinting resistance is enabled. That's to say the protection works correctly on my platform. So, I am wondering that the result you gave for the fifth test is the outer window size or the inner window size. The protection supposes to work for the inner window, the content's window. You can use the https://browserleaks.com/javascript to see the screen resolution as well as the available window size for the content window.

Comment 88

[Tom Grabowski [:TomGrab]]

(In reply to Tim Huang[:timhuang] from comment #87)
> Hi Tom,
> 
> I have tested my MAC with a monitor with 1080x1920 resolution, and the
> result shows that it gives me a 1000x1000 content window when fingerprinting
> resistance is enabled. That's to say the protection works correctly on my
> platform. So, I am wondering that the result you gave for the fifth test is
> the outer window size or the inner window size. The protection supposes to
> work for the inner window, the content's window. You can use the
> https://browserleaks.com/javascript to see the screen resolution as well as
> the available window size for the content window.

My outer window was 1000x1100. The 100 pixel is the UI at the top, I guess. 
I'm going to run a few more tests, as I just got my hands on another machine with some external screens. I will post here if I see any issues.

Comment 89

[Hugo Osvaldo Barrera]

I can't get this working, not even close. I'm on ArchLinux, using Xorg+i3, firefox 56.0.1. Step to repro:

* Create a new profile (--no-remote --profilemanager).
* Set privacy.resistFingerprinting to true.
* Restart Firefox (just in case!).
* Visit https://browserleaks.com/javascript:
  * Result is 1920×1088 24-bit TrueColor (working area: 1912×1088)

I can consistently reproduce this on different machines. These values are definitely not rounded to anything not-unique.

Comment 90

[Tom Grabowski [:TomGrab]]

(In reply to Hugo Osvaldo Barrera from comment #89)
> I can't get this working, not even close. I'm on ArchLinux, using Xorg+i3,
> firefox 56.0.1. Step to repro:
> 
> * Create a new profile (--no-remote --profilemanager).
> * Set privacy.resistFingerprinting to true.
> * Restart Firefox (just in case!).
> * Visit https://browserleaks.com/javascript:
>   * Result is 1920×1088 24-bit TrueColor (working area: 1912×1088)
> 
> I can consistently reproduce this on different machines. These values are
> definitely not rounded to anything not-unique.

Hugo,

Perhaps the problem is that you are using Firefox 56.0.1. I'm not sure whether this feature is fully implemented there.
Could you try downloading DevEdition Firefox and running it there?  You can use this link: https://www.mozilla.org/en-US/firefox/developer/  (Dev Edition will create its own independent profile, so no need to worry about creating profiles manually - you can use this browser independently from your 56.0.1)

Also, once you start your browser and set privacy.resistFingerprinting to true, open a new window by clicking on "New Window" (in DevEdition it should be in the drop-down menu on the right). See if that newly opened window has the expected size.

Thanks,
Tom

Comment 91

[Hugo Osvaldo Barrera]

Similar results on firefox-dev:

* Ran firefox-dev (58.0b1 (64-bit)).
* Set privacy.resistFingerprinting to true.
* Restart Firefox (just in case!).
* Visit https://browserleaks.com/javascript:
  * Result is 1920×1086 24-bit TrueColor (working area: 1912×1086)

I guess the 2px difference is probably due to the chrome changes in the dev release, but it's still isn't working as expected.

Comment 92

[Hani Yacoub, Desktop QA]

I tested this issue on Ubuntu 16.04 on The latest nightly using these steps:

- Go to about:config
- Turn privacy.resistFingerprinting parameter to true
- Open new browser window by clicking on "New Window"

I registered these results on the following resolutions:

1. 3840 x 2160 (16:9)
   New window size = 1000 x 1000 24-bit TrueColor (Working Area 990 x 1000)
2. 2028 x 1280 (16:10)
   New window size = 1000 x 1000 24-bit TrueColor (Working Area 990 x 1000)
3. 1920 x 1080 (16:9)
   New window size = 1000 x 900 24-bit TrueColor (Working Area 990 x 900)
4. 1600 x 1200 (4:3)
   New window size = 1000 x 1000 24-bit TrueColor (Working Area 990 x 1000)
5. 1280 x 1024 (5:4)
   New window size = 1000 x 800 24-bit TrueColor (Working Area 990 x 800)
6. 1280 x 720 (16:9)
   New window size = 1000 x 500 24-bit TrueColor (Working Area 990 x 500)


The test cases I have didn't match my monitors resolutions, so I tested using my monitor resolutions.
Could you please tell my if these results are the desirable once?

Thanks.

Comment 93

[Tim Huang[:timhuang]]

Hi Hani,

I would say that they are desirable results in terms of the window size. However, it kinds a little bit unexpected for the result of the working area, they are supposed to be 1000 in these cases. May I ask you do you have some customizations alongside the content window? If you do, this could be the reason why there is a 10px decrease in the working area.

Comment 94

[Hani Yacoub, Desktop QA]

I have the default setting on Ubuntu 16.04.
Could you please tell me more what kind of customizations I could have?

Comment 95

[Thorin [:thorin]]

@Tim - AFAIK, the "working area" at https://browserleaks.com/javascript is simply the inner window minus the scrollbar(s) - if you click "more" you can see the values are generated from div.clientWidth and div.clientHeight

On Windows 7 the (vertical) scrollbar removes 17 pixels. Hugo above has 8 pixels difference. Hani on Ubuntu 16.04 (GNOME? KDE?) it removes 10 pixels (on Ubuntu 14.04 with GNOME it removed 13 pixels - see https://trac.torproject.org/projects/tor/ticket/22137 )

See Bug 1397996, and if you go to https://www.hackerfactor.com/blog/index.php?/archives/761-Exploiting-the-TOR-Browser.html and scroll down to the "Test results for your browser" you can see what I mean. If you look at the JS for that it uses innerHeight and clientHeight and math.

Comment 96

[Tim Huang[:timhuang]]

Thanks for pointing this out, Simon.

So, I think the 10px is for the scroll bar, but not the customizations.

Comment 97

[Hugo Osvaldo Barrera]

I'd assume that the rounding is done *after* taking things like scrollbar size into consideration. Otherwise this'll still leak fingerprintable data (especially when the system theme is a custom one with an uncommonly sized scrollbar).

Comment 98

[Thorin [:thorin]]

^^ Not every page generates a scrollbar. There are also two possible srollbars (vertical, horizontal). The sizing is also done before the browser is displayed or anything loaded. Bug 1397996 may one day address uniformity of scrollbars across platforms

Comment 99

[Hugo Osvaldo Barrera]

Maybe always show a scrollbar (at least when this setting is active), and make just the display area fixed?

What I mean is, if we're making the display area 1000x1000px, keep the scrollbars _outside_ that area, and also always show them (kinda like `overflow: scroll` would? This fixes both part of this issue and 1397996.

Comment 100

[Thorin [:thorin]]

(In reply to Hugo Osvaldo Barrera from comment #91)
> Similar results on firefox-dev:
> 
> * Ran firefox-dev (58.0b1 (64-bit)).
> * Set privacy.resistFingerprinting to true.
> * Restart Firefox (just in case!).
> * Visit https://browserleaks.com/javascript:
>   * Result is 1920×1086 24-bit TrueColor (working area: 1912×1086)

note: previous result by Hugo on 56 was working area (1912x1088)

What is your screen resolution? To me this sounds like you are grabbing the results from https://browserleaks.com/javascript AFTER you have maximized firefox or put it into full screen.

Comment 101

[Hugo Osvaldo Barrera]

I'm not using a composing window manager, so there's not concept of maximized/minimized full windows; just windows.

My screen resolution is 2880x1800, and Xorg is configured at 160dpi.

Comment 102

[Thorin [:thorin]]

@Arthur et al: Just throwing some ideas out there - floating scrollbars, floating findbar - not just for privacy.resistFingerprinting, but in all modes. Here is an (old) floating scrollbars (I have no idea if it still works) - https://github.com/ardiman/userChrome.js/tree/master/floatingscrollbar. Perhaps we could create two new tickets for these so at least it's tracked, and add it to the Tor Uplift Project?

Comment 103

[Georg Koppen]

(In reply to Simon Mainey from comment #102)
> @Arthur et al: Just throwing some ideas out there - floating scrollbars,
> floating findbar - not just for privacy.resistFingerprinting, but in all
> modes. Here is an (old) floating scrollbars (I have no idea if it still
> works) -
> https://github.com/ardiman/userChrome.js/tree/master/floatingscrollbar.

Interesting. Sounds like a good idea to me to explore that approach.

> Perhaps we could create two new tickets for these so at least it's tracked,
> and add it to the Tor Uplift Project?

Fine with me. Some of this is tracked in our https://trac.torproject.org/projects/tor/ticket/16456 although this one is a more generic *bar-is-changing-the-reported-screen-size-ticket. It seems there is already a ticket (1418537) for the bookmark issue, good.

Comment 104

[Thorin [:thorin]]

> Interesting. Sounds like a good idea to me to explore that approach.

OK, I explored it - see https://bugzilla.mozilla.org/show_bug.cgi?id=1397996#c2 .. now what? :)

Comment 106

[Tom Ritter [:tjr]]

Bumping this to P3 and tagging for eventual investigate into corner cases that aren't working.

Comment 107

[surely.you.jest]

Ok, so one way to resist fingerprinting is to have a small set of fixed window sizes, and there can be endless discussion about what window sizes are best... but what about going the other way, and randomizing the window size for every new window? For example, perhaps Firefox could check the maximum possible window width and height on that screen, then set each new window width and height to a random value between that and 30% less than that. This would add a heck of a lot of entropy when trying to track a user between sessions.

You could take it even further by randomizing the window dimensions by a few pixels each time the user switches tabs or navigates to a new site, although that's probably overkill (and annoying).

And instead of worrying about the user resizing the window, accept it as an additional source of randomness! Every time I manually resize a window, it ends up with slightly different dimensions, making fingerprinting more difficult.

Comment 108

[Mos At]

Want to assure a consistent size? Force a maximized window. Not follow in Tor's backwards footsteps.
1080/1200/1440/2160 scaled 125% (a quite popular setup)? there goes your math, firefox is now setting an annoying tiny window.

Comment 109

[Stephan Sokolow]

(In reply to Mos At from comment #108)

Want to assure a consistent size? Force a maximized window. Not follow in Tor's backwards footsteps.
1080/1200/1440/2160 scaled 125% (a quite popular setup)? there goes your math, firefox is now setting an annoying tiny window.

People resize their taskbars/panels and add/move them to alter which screen edges they're on. That's the whole reason Tor does what it does. The OS will force inconsistent limitations on the application window if you maximize it.

Comment 110

[Thorin [:thorin]]

People resize...

... not only that (the actual available screen res for the firefox window to fit into: and there are other factors see [1]), but the firefox chrome will vary between users (density settings, os/distro, custom browser themes can change padding, if toolbar/menubar/sidebar etc are open to name bu a few)

Since screen/available-screen/outer-window are tied to inner-window .. the only option here is to control the inner window and you can only do that by setting an exact size

That said, I believe we should be able to do two things

• 1. decouple screen+available-screen from inner+outer: i.e screen+available-screen = the same (spoofed: maybe on a sliding/stepped scale): most/all FPing scripts do not bother with inner window as it is too unstable
• 2. increase the default max width for usability. This needs analysis, but AFAICT wouldn't add any entropy

[1] https://old.reddit.com/r/TOR/comments/ebwcte/does_changing_the_window_size_still_reduce_my/fb7mjmp/ - Matt Traudt

Comment 111

[Hugo Osvaldo Barrera]

Honestly, this feature need to be implemented by adding padding inside the window (this is actually what Tor does).

Many environments/desktops won't allow the window to resize itself. For example, Tiling window managers will give the window a fixed amount of space, and won't allow the window to make itself any smaller -- since the idea of becoming smaller doesn't make sense on tiling window managers.

Also, making the window maximised will leak the exact screen resolution every time. Combined with the size of the browser's chrome, this is likely to generate plenty of identifiable information.

Padding inside the window makes sure that:

• Resizing is not a problem.
• It works in things like tiling WMs and other contexts where resizing by the client application is diallowed.
• There's no leak due to the size of toolbar & chrome.

Also, if it's what Tor does, then it's apparently already implemented in a way that works.

Comment 112

[Thorin [:thorin]]

^ hidden pref: privacy.resistFingerprinting.letterboxing - Bug 1407366 since FF65+

Resizing is still a problem. Letterboxing just mitigates the possible combos of width/height. Inner window (and thus screen) entropy is still there. The solution is to decouple screen metrics from chrome metrics. Then screen metrics in could be limited to four or five common results - stepping based on current inner window. Inner becomes less important (scripts only want screen, not inner), but we still want to protect it: so we still open at usable default sizes (needs some analysis: increase the width for starters), we still use letterboxing: and those who disable letterboxing, at screen metrics are very limited

Comment 113

[Thorin [:thorin]]

I think we can close this, yes?

FWIW we plan to propose uplifting a bunch of newwin, letterboxing, betterboxing patches, in new issues if need be. And to address some other issues not covered yet on bugzilla (i.e open new bugzillas). This issue is not it, it's just (old) noise