1975753 - RFP: zoom propagates across domains in the same tab, bypassing disabled zoom.siteSpecific protection

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Thorin [:thorin]]

STR

• RFP enabled
• visit https://example.com - it should be 100%
  • zoom to e.g. 110%
  • same tab visit https://www.webpagetest.org/blank.html
• actual result: blank is 110%
• expected: blank should be 100%

regression (tested by checking FF131 and FF132)

• FF132+ Bug 1914149 Propagate zoom to new browsing contexts unconditionally

archaelogy

• Bug 1369357

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1914149

:emilio, since you are the author of the regressor, bug 1914149, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Do you know what does RFP do differently in this regard? Do you just disable browser.zoom.siteSpecific?

Comment 3

[Thorin [:thorin]]

My understanding of how it worked is

• the pref is not changed, it's hardcoded behind RFP somewhere
  • i.e siteSpecific zoom is ignored (my understanding is they are stored in places)
• zoom persists in a tab whilst on the same site
• zoom is reset when the domain changes in that tab (because it's a new site and there is no zoom specific for it so it resets to 100%)
• this bit gets fuzzy for me
  • a new tab even of the same site is also reset - this still works but I have had reports and replicated some weirdness - not sure if this rest per new tab is by design or a consequence

edit: RFPTarget - https://searchfox.org/mozilla-central/source/toolkit/components/resistfingerprinting/RFPTargets.inc#92

edit2: does this help? https://searchfox.org/mozilla-central/search?q=RFPTarget%3A%3AsiteSpecificZoom&path=&case=false&regexp=false

Comment 4

[Fatih Kilic [:fkilic]]

I think for document zoom our RFP target was affecting this function here https://searchfox.org/mozilla-central/source/browser/components/tabbrowser/content/browser-fullZoom.js#710. I'm not sure how and why it is happening though.

Comment 5

[Fatih Kilic [:fkilic]]

i think adding ShouldRFP(SiteSpecificZoom) to here should fix it though i could be wrong i'll test it

Comment 6

[Fatih Kilic [:fkilic]]

Hmm so the weird thing is, should we RPF if the current or the new context should be exempt from RFP? Like, going from example.com to about:config is fine, but going from about:config to example.com isn't.

Regardless though, I feel like SiteSpecificZoom should be either on or off, no exemption for "safe" documents should exist imo.

Comment 7

[Fatih Kilic [:fkilic]]

Attachment: Bug 1975753 - Don't keep zoom levels if RFPTarget::SiteSpecificZoom is active. r?emilio

Comment 8

[Fatih Kilic [:fkilic]]

What should be the behaviour of RFPTarget::SiteSpecificZoom?

For example

• if you zoom at example.com and then later come back to example.com, should it keep that zoom level?
• if you zoom at example.com and then visit example.org within the same tab, should it keep that zoom level? (by the bug description i think no but just making sure)

Comment 9

[Thorin [:thorin]]

• if you zoom at example.com and then later come back to example.com, should it keep that zoom level?

It didn't before and I'd prefer we kept it that way. AFAICT if you zoom, the value is stored in places (and maybe bfcache or memory?). When the eTLD+1 (or something) changes then it's a new navigation and the cycle starts again (applying siteSpecificZoom which is controlled by RFP), hence this answers the question below. And whilst you the eTLD+1 in the tab remains the same, the current zoom sticks because siteSpecificZoom is not triggered

So if I understood that correctly... when you revisit example.com (and the tab wasn't already example.com) or go to it on a new tab it was always reset because see above

• if you zoom at example.com and then visit example.org within the same tab, should it keep that zoom level? (by the bug description i think no but just making sure)

they're two different sites, so the zoom should reset - something something eTLD+1

---

tl;dr - we didn't do anything special except ignore site specific zooms AFAICT

side note: IDK what happens when you use history nav buttons, I'm more concerned with ensuring every new website, or tab is reset

Comment 10

[Fatih Kilic [:fkilic]]

I see. I think what happened is, currently we try to disable storage of site specific zoom levels by returning false for _isSiteSpecific in browser-fullZoom.js. IIUC, with bug 1914149, we set zoom level for navigations unconditionally. Previously it worked because we didn't have zoom level stored, and didn't inherit/keep zoom levels across navigations, so it wasn't a problem.

Now, we keep the old zoom level for navigations but since we prevent browser-fullZoom.js from applying per site zoom level, we end up not resetting the zoom level per site.

So, we have to somehow reset zoom for navigations in browser-fullZoom.js. I'll look more into it later.

Comment 11

[Fatih Kilic [:fkilic]]

Oh wait so zoom was reset on every navigation before 132? regardless of origin? i tried 130 and it reset it every time despite being in the same origin

Comment 12

[Thorin [:thorin]]

(In reply to Fatih Kilic [:fkilic] from comment #11)

Oh wait so zoom was reset on every navigation before 132? regardless of origin? i tried 130 and it reset it every time despite being in the same origin

IDK what was or is expected. I use RFP enabled as my main daily driver (to experience it all IRL). However I never zoom [well one site I visit weekly'ish and it gets reset the next visit and right click open in new tabs are reset - that's my total experience with it all] and I am at standard/default zoom levels.

https://github.com/arkenfox/user.js/issues/1636 - this is an old thread which I hijacked for this issue. https://github.com/arkenfox/user.js/issues/1636#issuecomment-1445279049 - shows a change in behavior in FF94? FF97? (from previous behavior before that).

• edit: something something about www vs no www ?

My experience and expectation is that

• FPing: change of eTLD+1 should reset
  • this is same tab and new tabs
  • I think newtab reset happens since it changes from an initial about:newtab or about:blank ?
• usability: current tab
  • if same eTLD+1 = should persist otherwise it becomes a usability/annoying issue
• unknown: history back/forward
  • if the FPing and usability is stable then the bfcache should match?

But again, IDK what behavior was or now is expected

Comment 13

[Fatih Kilic [:fkilic]]

In normal firefox, AFAICT, there's two types of zooms we use.

1. Site specific zoom: Default one. If you use site specific zoom, we save your zoom level to content prefs, and load it back. All of this happens in browser-fullZoom.js.
2. Tab level zoom: If you zoom in a tab, no matter which site you are on, the zoom level is kept the same.

With RFP, we disable site specific zoom in browser-fullZoom.js. So, we never save anything. However, we do not set browser.zoom.siteSpecific to false. This causes RFP to never inherit zoom level for the new browsing context. AFAICT the reason that we now persist the zoom level across navigations is the regressing bug. we now always inherit the zoom level (if you are not going back and forth between pages), and let browser-fullZoom.js fix it later if it is different. Since, browser-fullZoom.js thinks we don't use site specific zoom, it doesn't fix it.

If we only wanted to fix this issue for TB, we could remove these lines, and let browser-fullZoom.js save zoom levels per site. Once TB clears user data, zoom levels will be gone. I don't think this is too bad because AFAICT, TB keeps cookies across tabs, so fingerprinting based on zoom isn't a concern.

If we want to fix this for Firefox too, I still think we should remove those lines and let browser-fullZoom.js handle its own thing, but check if the RFPTarget::SiteSpecificZoom is enabled and clear site specific zoom data.

Comment 14

[Fatih Kilic [:fkilic]]

If we want to fix this for Firefox too, I still think we should remove those lines and let browser-fullZoom.js handle its own thing, but check if the RFPTarget::SiteSpecificZoom is enabled and clear site specific zoom data.

Or maybe not. Maybe firefox should just keep the history, because again, if you are not clearing cookies, then fingerprinting isn't much of a concern.

Comment 15

[Fatih Kilic [:fkilic]]

I mean if all we want is site specific zoom that's cleared after the browser is closed, then the normal site specific zoom works fine

Comment 16

[Pier Angelo Vendrame]

(In reply to Fatih Kilic [:fkilic] from comment #13)

If we only wanted to fix this issue for TB, we could remove these lines, and let browser-fullZoom.js save zoom levels per site. Once TB clears user data, zoom levels will be gone. I don't think this is too bad because AFAICT, TB keeps cookies across tabs, so fingerprinting based on zoom isn't a concern.

I agree.
I think our preference would be for the behavior of ESR128: remember per-site zoom, as long as it's cleared at the end of the PBM session, but reset it when changing FPD on the same tab (either to 100%, or to a value already set from memory).
We don't care much about non-PBM for now.

Comment 17

[Thorin [:thorin]]

We don't care much about non-PBM for now.

From TPO's perspective that may not be a priority (and we want this resolved for TB15) - but we (me!) want RFP to work as expected in Firefox as well - we rely on Firefox users to test drive RFP and report issues and have done for years - so all window modes please (or file a follow bug)

Comment 18

[Fatih Kilic [:fkilic]]

so all window modes please

Just to make sure, this doesn't include resetting of zoom levels in normal browsing right? because again, cookies are persisted anyway in normal browsing

Comment 19

[Fatih Kilic [:fkilic]]

Attachment: Bug 1975753 - Force site specific zoom when RFPTarget::SiteSpecificZoom is active. r?timhuang

I wasn't around when RFPTarget::SiteSpecificZoom was implemented, but the current implementation is very confusing.

In Firefox, we have two modes of zoom level. Per tab zoom, and site specific zoom. It is controlled in two (maybe more) places. browser-fullZoom.js and in CanonicalBrowsingContext.cpp.

Our current implementation disables site specific zoom in browser-fullZoom.js by checking the RFP target, but it doesn't modify CanonicalBrowsingContext.cpp. So,CanonicalBrowsingContext still thinks we are using site specific zoom.

Pre-bug1914149 this method worked because we didn't keep/inherit zoom level across navigations. Post-bug1914149, it no longer works because we keep the zoom level across navigations in CanonicalBrowsingContext and let browser-fullZoom.js reset to its correct value back.

The issue is caused because CanonicalBrowsingContext keeps the previous page's zoom level, but browser-fullZoom.js thinks we use tab zoom mode, so it doesn't bother setting the zoom level for the site/page. So, we end up keeping the zoom level.

The solution here I'm suggesting is, doing the opposite of what we are doing in browser-fullZoom.js. So, now we should force SiteSpecificZoom with RFP. The reason I'm suggesting this is because within the same site, even acrss tabs, we persist cookies, so fingerprinting isn't much of concern here. We also don't persist zoom levels in private browsing. So, linking normal to PBM isn't a concern either.

So, in summary,

• If you open the same site, in 100 tabs, all of them will get the same zoom level with this patch (just like default normal Firefox)
• If you are in PBM, the zoom level is NOT persisted.
• If you are in normal browsing, the zoom level is persisted, but so are cookies.

Comment 20

[Thorin [:thorin]]

So, in summary,

• If you open the same site, in 100 tabs, all of them will get the same zoom level with this patch (just like default normal Firefox)

I assume in normal windows and not PB windows which is the next bullet point? What is "same site" ? eTLD+1 and/or partitioning keys?

• If you are in PBM, the zoom level is NOT persisted.

I assume you mean "same site"? So changing the page in the same tab on the same site will reset? That's jarring!

• If you are in normal browsing, the zoom level is persisted, but so are cookies.

So confused - is the zoom stored in a cookie? What do cookies have to do with zoom? If normal mode sanitizes on close does that remove the zoom?

TBH I'm having a hard time following this. It all sounds fine to me - I'll just test it afterwards :)

Comment 21

[Fatih Kilic [:fkilic]]

I assume in normal windows and not PB windows which is the next bullet point?

I mean, if you are in PBM, and you open the same site in 100 tabs, they'll all share the same zoom level.

What is "same site" ?

Full domain. We end up on this line when we call applyZoomToPref. Nothing else is considered.

I assume you mean "same site"? So changing the page in the same tab on the same site will reset? That's jarring!

I meant when you close the browser (or all the PBM tabs), it will not be persisted. So when you close all PBM tabs, and open it back again, it won't load the zoom levels set in the previous session.

So confused - is the zoom stored in a cookie? What do cookies have to do with zoom? If normal mode sanitizes on close does that remove the zoom?

Ah no, I meant cookies are more reliable for tracking people. Since cookies are not reset when you close the browser in normal browsing, zoom level for that site is less of a concern.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1914149

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/213240f79f9a
https://hg.mozilla.org/integration/autoland/rev/44e0d730298b
Force site specific zoom when RFPTarget::SiteSpecificZoom is active. r=timhuang,tabbrowser-reviewers,dao

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a66ce57d3d44
https://hg.mozilla.org/integration/autoland/rev/980e3dd08305
Revert "Bug 1975753 - Force site specific zoom when RFPTarget::SiteSpecificZoom is active. r=timhuang,tabbrowser-reviewers,dao" for causing bc failures @ browser_ext_tabs_zoom.js

Comment 25

[Sandor Molnar[:smolnar]]

Backed out for causing bc failures @ browser_ext_tabs_zoom.js

Backout link: https://hg-edge.mozilla.org/integration/autoland/rev/980e3dd08305692ad454d0c8caf61f8218e2a2a4

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_tabs_zoom.js | Scope should be "per-origin" - Expected: per-origin, Actual: per-tab -

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5cc1a7909a4e
https://hg.mozilla.org/integration/autoland/rev/7de8dcc3a2e1
Force site specific zoom when RFPTarget::SiteSpecificZoom is active. r=timhuang,tabbrowser-reviewers,dao

Comment 27

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7de8dcc3a2e1

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Did you want to nominate this for ESR140 uplift?

Comment 29

[Fatih Kilic [:fkilic]]

I cleared NI by accident. Yes, i'll nominate it now. Thanks!

Comment 30

[Fatih Kilic [:fkilic]]

Attachment: Bug 1975753 - Force site specific zoom when RFPTarget::SiteSpecificZoom is active. r?timhuang

I wasn't around when RFPTarget::SiteSpecificZoom was implemented, but the current implementation is very confusing.

In Firefox, we have two modes of zoom level. Per tab zoom, and site specific zoom. It is controlled in two (maybe more) places. browser-fullZoom.js and in CanonicalBrowsingContext.cpp.

Our current implementation disables site specific zoom in browser-fullZoom.js by checking the RFP target, but it doesn't modify CanonicalBrowsingContext.cpp. So,CanonicalBrowsingContext still thinks we are using site specific zoom.

Pre-bug1914149 this method worked because we didn't keep/inherit zoom level across navigations. Post-bug1914149, it no longer works because we keep the zoom level across navigations in CanonicalBrowsingContext and let browser-fullZoom.js reset to its correct value back.

The issue is caused because CanonicalBrowsingContext keeps the previous page's zoom level, but browser-fullZoom.js thinks we use tab zoom mode, so it doesn't bother setting the zoom level for the site/page. So, we end up keeping the zoom level.

The solution here I'm suggesting is, doing the opposite of what we are doing in browser-fullZoom.js. So, now we should force SiteSpecificZoom with RFP. The reason I'm suggesting this is because within the same site, even acrss tabs, we persist cookies, so fingerprinting isn't much of concern here. We also don't persist zoom levels in private browsing. So, linking normal to PBM isn't a concern either.

So, in summary,

• If you open the same site, in 100 tabs, all of them will get the same zoom level with this patch (just like default normal Firefox)
• If you are in PBM, the zoom level is NOT persisted.
• If you are in normal browsing, the zoom level is persisted, but so are cookies.

Original Revision: https://phabricator.services.mozilla.com/D257497

Comment 31

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Tor Browser devs will have to carry more patches. Also RFP users of ESR version will have a broken zoom RFP protection.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Zoom on a website, set privacy.fingerprintingProtection to true, set privacy.fingerprintingProtection.overrides to "-AllTargets,+SiteSpecificZoom", then clear site data for that website. The site zoom should be reset
• Risk associated with taking this patch: N/A
• Explanation of risk level: N/A
• String changes made/needed: No
• Is Android affected?: no

Comment 32

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/2509e243545d