1369357 - Making Firefox not to use site specific zoom level when 'privacy.resistFingerprinting' is true

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

[Tim Huang[:timhuang]]

For fingerprinting resistance, we want to disable site specific zoom level wehn 'privacy.resistFingerprinting' is true since this is a fingerprintable vector. The desired behavior here is like turning 'browser.zoom.siteSpecific' to false.

Comment 1

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1369357 - Enabling privacy.resistFingerprinting implies disabling browser.zoom.siteSpecific

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

Comment 2

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1369357 - Add test case

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

Comment 3

[Chung-Sheng Fu [:cfu]]

Conditions (browser.zoom.siteSpecific == true) become ((privacy.resistFingerprinting == false) && (browser.zoom.siteSpecific == true)).

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8878971 [details]
Bug 1369357 - Enabling privacy.resistFingerprinting implies disabling browser.zoom.siteSpecific

https://reviewboard.mozilla.org/r/150228/#review155088

::: browser/base/content/browser-fullZoom.js:29
(Diff revision 1)
>    // Stores initial locations if we receive onLocationChange
>    // events before we're initialized.
>    _initialLocations: new WeakMap(),
>  
>    get siteSpecific() {
>      return this._siteSpecificPref;

How about this getter checks if this._siteSpecificPref === undefined and sets it in that case? We can then stop assigning this._siteSpecificPref in init, and observe can set it back to undefined.

::: browser/base/content/browser-fullZoom.js:34
(Diff revision 1)
>      return this._siteSpecificPref;
>    },
>  
> +  getSiteSpecificPref: function FullZoom_getSiteSpecificPref() {
> +    return !gPrefService.getBoolPref("privacy.resistFingerprinting")
> +        && gPrefService.getBoolPref("browser.zoom.siteSpecific");

The intentation is weird here. Move && to the end of the previous line instead?

Comment 5

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8878971 [details]
Bug 1369357 - Enabling privacy.resistFingerprinting implies disabling browser.zoom.siteSpecific

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

Comment 6

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8879009 [details]
Bug 1369357 - Add test case

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

Comment 7

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8878971 [details]
Bug 1369357 - Enabling privacy.resistFingerprinting implies disabling browser.zoom.siteSpecific

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

Comment 8

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8879009 [details]
Bug 1369357 - Add test case

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

Comment 9

[Chung-Sheng Fu [:cfu]]

(In reply to Dão Gottwald [::dao] from comment #4)
> Comment on attachment 8878971 [details]
> Bug 1369357 - Enabling privacy.resistFingerprinting implies disabling
> browser.zoom.siteSpecific
> 
> https://reviewboard.mozilla.org/r/150228/#review155088
> 
> ::: browser/base/content/browser-fullZoom.js:29
> (Diff revision 1)
> >    // Stores initial locations if we receive onLocationChange
> >    // events before we're initialized.
> >    _initialLocations: new WeakMap(),
> >  
> >    get siteSpecific() {
> >      return this._siteSpecificPref;
> 
> How about this getter checks if this._siteSpecificPref === undefined and
> sets it in that case? We can then stop assigning this._siteSpecificPref in
> init, and observe can set it back to undefined.

Nice idea.
I updated the patch and please take a look again.

> 
> ::: browser/base/content/browser-fullZoom.js:34
> (Diff revision 1)
> >      return this._siteSpecificPref;
> >    },
> >  
> > +  getSiteSpecificPref: function FullZoom_getSiteSpecificPref() {
> > +    return !gPrefService.getBoolPref("privacy.resistFingerprinting")
> > +        && gPrefService.getBoolPref("browser.zoom.siteSpecific");
> 
> The intentation is weird here. Move && to the end of the previous line
> instead?

I also fixed the styling issue in ImageDocument.cpp.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8878971 [details]
Bug 1369357 - Enabling privacy.resistFingerprinting implies disabling browser.zoom.siteSpecific

https://reviewboard.mozilla.org/r/150228/#review155580

::: browser/base/content/browser-fullZoom.js:32
(Diff revision 3)
>  
>    get siteSpecific() {
> +    if (this._siteSpecificPref === undefined) {
> +      this._siteSpecificPref =
> +          !gPrefService.getBoolPref("privacy.resistFingerprinting") &&
> +          gPrefService.getBoolPref("browser.zoom.siteSpecific");

nit: reduce indentation by two spaces in the two lines above

::: browser/base/content/browser-fullZoom.js:110
(Diff revision 3)
> +          case "privacy.resistFingerprinting":
> +            // fall through
>            case "browser.zoom.siteSpecific":
> -            this._siteSpecificPref =
> -              gPrefService.getBoolPref("browser.zoom.siteSpecific");
> +            // Get the preferences in the getter once this._siteSpecificPref === undefined.
> +            // Set this._siteSpecificPref back to undefined when the preferences change.
> +            // In this way, we can stop assigning this._siteSpecificPref in init.

Documenting what we've stopped doing is a bit weird. How about:

// Let the siteSpecific getter update _siteSpecificPref on next access.

Or even simpler:

// Invalidate pref cache

Comment 11

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8878971 [details]
Bug 1369357 - Enabling privacy.resistFingerprinting implies disabling browser.zoom.siteSpecific

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

Comment 12

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8879009 [details]
Bug 1369357 - Add test case

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

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8879009 [details]
Bug 1369357 - Add test case

https://reviewboard.mozilla.org/r/150288/#review156122

::: browser/components/resistfingerprinting/test/browser/browser_bug1369357_site_specific_zoom_level.js:97
(Diff revision 4)
> +  tab2Zoom = ZoomManager.getZoomForBrowser(tab2.linkedBrowser);
> +
> +  isnot(tab2Zoom, tab1Zoom, "privacy.resistFingerprinting is true, site-specific zoom level should be disabled");
> +
> +  await FullZoomHelper.removeTabAndWaitForLocationChange(tab1);
> +  await FullZoomHelper.removeTabAndWaitForLocationChange(tab2);

Why wait for the location change notification here? Can't you just remove the tabs and be done with the test?

Comment 14

[Arthur Edelstein [:arthur]]

Comment on attachment 8878971 [details]
Bug 1369357 - Enabling privacy.resistFingerprinting implies disabling browser.zoom.siteSpecific

https://reviewboard.mozilla.org/r/150228/#review156520

Comment 15

[Arthur Edelstein [:arthur]]

Comment on attachment 8879009 [details]
Bug 1369357 - Add test case

https://reviewboard.mozilla.org/r/150288/#review156522

::: browser/components/resistfingerprinting/test/browser/browser_bug1369357_site_specific_zoom_level.js:25
(Diff revision 4)
> +    BrowserTestUtils.loadURI(tab.linkedBrowser, url);
> +
> +  return loaded;
> +}
> +
> +// Copied from browser/base/content/test/general/head.js

Rather than copying all this code, would it make sense to put these tests in browser/base/content/test ?

Comment 16

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8879009 [details]
Bug 1369357 - Add test case

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

Comment 17

[Chung-Sheng Fu [:cfu]]

I refined the test by using basic BrowserTestUtils APIs.
So code copied from browser/base/content/test/general/head.js can be removed.
Please take a look again.
Thanks.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8879009 [details]
Bug 1369357 - Add test case

https://reviewboard.mozilla.org/r/150288/#review156638

::: browser/components/resistfingerprinting/test/browser/browser_bug1369357_site_specific_zoom_level.js:21
(Diff revision 5)
> +  tab1Zoom = ZoomManager.getZoomForBrowser(tab1.linkedBrowser);
> +
> +  tab2 = await BrowserTestUtils.openNewForegroundTab(gBrowser, testPage);
> +  tab2Zoom = ZoomManager.getZoomForBrowser(tab2.linkedBrowser);
> +
> +  isnot(tab2Zoom, tab1Zoom, "privacy.resistFingerprinting is true, site-specific zoom level should be disabled");

Please do the oppositve check before setting privacy.resistFingerprinting to ensure the test works as expected.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8879009 [details]
Bug 1369357 - Add test case

https://reviewboard.mozilla.org/r/150288/#review157440

Comment 20

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8879009 [details]
Bug 1369357 - Add test case

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

Comment 21

[Chung-Sheng Fu [:cfu]]

Now the test will open 3 tabs with the same URL.
Zoom level will be changed in the 1st tab.
Then open the 2nd tab with privacy.resistFingerprinting = false, and the zoom level should be the same as the 1st tab.
At last, set privacy.resistFingerprinting = true and open the 3rd tab.  The zoom level should be reset and not equal to the 1st and 2nd tab.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8879009 [details]
Bug 1369357 - Add test case

https://reviewboard.mozilla.org/r/150288/#review157954

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b0caf5d6f14
Enabling privacy.resistFingerprinting implies disabling browser.zoom.siteSpecific r=arthuredelstein,dao
https://hg.mozilla.org/integration/autoland/rev/720a9d8fb641
Add test case r=arthuredelstein,dao

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b0caf5d6f14
https://hg.mozilla.org/mozilla-central/rev/720a9d8fb641

Comment 25

[Tanvir Rahman]

I have reproduced this Bug with Nightly 55.0a1 (2017-06-01) on Windows 10, 64 Bit!

The bug's fix is now verified on latest Nightly 56.0a1

Build ID 	20170706060058
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 26

[Ethan Tseng [:ethan]]

(In reply to Tanvir Rahman from comment #25)
> The bug's fix is now verified on latest Nightly 56.0a1
> Build ID 	20170706060058
> User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0)
> Gecko/20100101 Firefox/56.0

Thank you for verifying this.

Comment 27

[Tom Grabowski [:TomGrab]]

Verified successfully on Mac OS 10.12.6 with Nightly 58.0a1 (2017-10-21) (64-bit)


Verification steps:

Test case 1:
1. Go to arstechnica.com
2. Set the zoom level of the page to 133%
3. Make sure "browser.zoom.siteSpecific" parameter is set to true
4. Make sure "privacy.resistFingerprinting" parameter is set to false
5. Open new tab and go to arstechnica.com

Expected result:
The page opens with zoom level 133%

Actual result:
The page opens with zoom level 133%


Test case 2:
1. Go to arstechnica.com
2. Set the zoom level of the page to 133%
3. Make sure "browser.zoom.siteSpecific" parameter is set to false
4. Make sure "privacy.resistFingerprinting" parameter is set to false
5. Open new tab and go to arstechnica.com

Expected result:
The page opens with zoom level 100%

Actual result:
The page opens with zoom level 100%


Test case 3:
1. Go to arstechnica.com
2. Set the zoom level of the page to 133%
3. Make sure "browser.zoom.siteSpecific" parameter is set to false
4. Make sure "privacy.resistFingerprinting" parameter is set to true
5. Open new tab and go to arstechnica.com

Expected result:
The page opens with zoom level 100%

Actual result:
The page opens with zoom level 100%


Test case 4:
1. Go to arstechnica.com
2. Set the zoom level of the page to 133%
3. Make sure "browser.zoom.siteSpecific" parameter is set to true
4. Make sure "privacy.resistFingerprinting" parameter is set to true
5. Open new tab and go to arstechnica.com

Expected result:
The page opens with zoom level 100%

Actual result:
The page opens with zoom level 100%