Closed Bug 1369357 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox56 --- verified

People

(Reporter: timhuang, Assigned: cfu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fingerprinting][tor][fp:m2])

Attachments

(2 files)

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.
Priority: -- → P2
Priority: P2 → P1
Assignee: nobody → cfu
Summary: Making Firefox not to use size specific zoom level when 'privacy.resistFingerprinting' is true → Making Firefox not to use site specific zoom level when 'privacy.resistFingerprinting' is true
Attachment #8878971 - Flags: review?(dao+bmo)
Attachment #8878971 - Flags: review?(arthuredelstein)
Attachment #8879009 - Flags: review?(dao+bmo)
Attachment #8879009 - Flags: review?(arthuredelstein)
Conditions (browser.zoom.siteSpecific == true) become ((privacy.resistFingerprinting == false) && (browser.zoom.siteSpecific == true)).
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?
(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 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
Attachment #8878971 - Flags: review?(dao+bmo) → review+
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 on attachment 8878971 [details]
Bug 1369357 - Enabling privacy.resistFingerprinting implies disabling browser.zoom.siteSpecific

https://reviewboard.mozilla.org/r/150228/#review156520
Attachment #8878971 - Flags: review?(arthuredelstein) → review+
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 ?
Attachment #8879009 - Flags: review?(arthuredelstein) → review+
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 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 on attachment 8879009 [details]
Bug 1369357 - Add test case

https://reviewboard.mozilla.org/r/150288/#review157440
Attachment #8879009 - Flags: review?(dao+bmo) → review-
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 on attachment 8879009 [details]
Bug 1369357 - Add test case

https://reviewboard.mozilla.org/r/150288/#review157954
Attachment #8879009 - Flags: review?(dao+bmo) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8b0caf5d6f14
https://hg.mozilla.org/mozilla-central/rev/720a9d8fb641
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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
QA Whiteboard: [bugday-20170705]
(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.
Status: RESOLVED → VERIFIED
Depends on: 1377820
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%
See Also: → 1609880
You need to log in before you can comment on or make changes to this bug.