Closed Bug 1089232 Opened 5 years ago Closed 4 years ago

Zoom level is shared across private and non-private window

Categories

(Firefox :: Private Browsing, defect)

36 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox39 --- affected
firefox40 --- affected
firefox41 --- affected
firefox42 --- affected
firefox47 --- fixed

People

(Reporter: daniel.nr01, Assigned: jdm, Mentored)

References

Details

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141025030202

Steps to reproduce:

* Open a private window and navigate to any page
* Navigate to that same page in a regular window
* Zoom in or out in the private window


Actual results:

The zoom level is shared with the non-private window


Expected results:

Nothing from the private window should leak to the other window. 

If the private window is closed before visiting the page in a regular window the page is opened with the default zoom level as expected.
Component: Untriaged → Private Browsing
Confirmed. This leaks that the private browsing session has visited the site in question.
Interesting. Since the pref is stored as a content pref, it should be separate between private and public windows. I'm willing to mentor anybody who wants to look into this; I would start by figuring out what happens in BrowserZoom._applyZoomToPref. Maybe the code that notifies observers isn't checking that the privacy values are the same?
Mentor: josh
Hi Josh! I'd like to take this! 
So, a private browser should apply zoom i.f.f the zoom was set by another private from the same location (the same for the 'normal' context), right? 

Another thing, where could i find more info regarding private browsing code? I'd like to firstly take a look at how things work regarding private browsing (i just found https://wiki.mozilla.org/PrivateBrowsing, but it doesn't say much about the code per se).

Thanks in advance!
Flags: needinfo?(josh)
Great! So, there's not that much information about "private browsing code", per se, since it's basically implemented as an inherited property of windows. It's then up to code that cares about private vs. public to find the relevant context for an operation and use that to choose some set of behaviour. In the case of the zoom-related code, that's a site-specific preference that's stored in a database. We actually have two databases (one private, one public), and the API to set or retrieve these preferences requires a context object that determines whether to use the private or public one. See http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIContentPrefService2.idl#45 for more information about this.
Flags: needinfo?(josh)
There's a bit more information at https://wiki.mozilla.org/Per-window_Private_Browsing/Front-end_implementation_notes, and you can see the long list of work that went into implementing the feature at https://bugzilla.mozilla.org/show_bug.cgi?id=pbngen . The code I mentioned in comment 2 can be found in mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullZoom.js .
Attached patch Test for the given bug (obsolete) — Splinter Review
Tests: normal tab zoom <--leaks--> private tab zoom. Currently fails, as expected.
Great start!
Thanks Josh! 

Got back to the bug, from what i observed taking a look at the callstack, when i change the zoom in a normal tab and then see the change happening in the private, `onContentPrefSet` is called, then `onContentPrefChanged` (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullZoom.js#135) without setting the `isNextPrefCHangeInternal` flag, so it proceeds to check if the group passed matches the domain. 
I was thinking about adding and extra check so that we could validate if context also matches, but i can't figure out how to obtain the context from the setter (in this case, get the context from the normal tab) - is it possible to know which was the browser that originated that (so that i could perform `this._loadContextFromBrowser(orig_browser)`) or is this expensive and we should actually pass `context.usePrivateBrowsing` bool in the message) ? 

Thanks again!
Flags: needinfo?(josh)
Also, i thought that when getting content preferences passing a context arg to the getter this check (if the current equals the one that was supplied to the setter) would be made by default (when setting a preference we're passing context as well ..). Am i missing something?
Flags: needinfo?(josh)
Attached patch bug1089232-zoom_level_leak.patch (obsolete) — Splinter Review
- updates test_contentPrefs with tests for the change in nsIContentPrefService and  ContentPrefInstance.
Attachment #8566487 - Attachment is obsolete: true
Attached image strange-call.png
Hi Josh! Thanks for pointing that. So, i've made some changes but i've been facing a strange as shown in the image attached: `onContentPrefSet` is called with the four args (including 'aIsPrivate'), but on the callee signature the `aIsPrivate` is not present. Then, when it calls `_onContentPrefChanged` the parameter 'aIsPrivate' is not passed. I thought that this could be a problem regarding the change in the 'idl', but i wasn't able to go much further on this :( Do you have any thoughts ?

Thanks!
Flags: needinfo?(josh)
That's... weird. Did you rebuild with a full `./mach build`?
Flags: needinfo?(josh)
Did a full build last night and now it works like a charm, thanks! Regarding the bug, zoom 'leaks' in two forms:
- immediately (ok, done)
- tab switching && window creation

The first was fixed by using that idea of passing the 'isPrivate' arg, but in the second case it seems a bit deeper issue: in 'onLocationChange' there's a call to 'getCachedByDomainAndName', which takes in consideration both private and non-private cache when in private mode (the _getCached part: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm#220-222 where it is retrieved from both of the caches, and the _get which then sets the _cache https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm#142-145). 

Do you think altering this behavior would be in the scope of this bug? Any ideas on other ways of tackling this?
Thanks!
Flags: needinfo?(josh)
Sorry for the silence, I forgot about this request. Hmm, this is stretching my understanding of how the content prefs are supposed to work, so I'm going to bring in someone more knowledgeable.

Drew, can you answer comment 14?
Flags: needinfo?(josh) → needinfo?(adw)
Is the problem that ContentPrefService2.prototype._get unconditionally updates the cache, and the cache doesn't have a concept of privacy?  So if you're in a private window and _get updates the cache, and then you close that window, the private pref (and its domain) remains in the cache?

Hmm... is that really a problem though?  If an item were added to the cache in this way, then it must also be present in the permanent, non-private store.  Someone could tell that you visited the domain, but since the domain is also present in the non-private store, they wouldn't know whether you had done so in a private window.

Maybe I don't understand the problem, or I'm not seeing why what I just described is a problem?
Flags: needinfo?(adw)
¡Hola Josh!

I'm confirming this one as you volunteered to mentor this bug on https://bugzilla.mozilla.org/show_bug.cgi?id=1089232#c2

What's needed to move this forward?

¡Gracias!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(josh)
Ok, state of the world here. I ran this patch locally, and it fixes the case where changing the zoom level in a private window leaks to the non-private window. It does not fix loading a page in a non-private window for the first time that is currently open in a private window (ie. no prior cached prefs), but I couldn't find any other ways to leak the zoom level from the private window through switching tabs as mentioned earlier.
Flags: needinfo?(josh)
I lied, it does fix the first load problem. I think we should go ahead and review this patch as it stands.
Attachment #8567341 - Attachment is obsolete: true
Assignee: nobody → josh
Status: NEW → ASSIGNED
Attachment #8705346 - Flags: review?(adw)
Comment on attachment 8705346 [details] [diff] [review]
Updates nsContentPrefService to take an extra isPrivate argument

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

Thanks, r+ but please see the comments.

::: dom/interfaces/base/nsIContentPrefService.idl
@@ +10,4 @@
>  interface nsILoadContext;
>  interface mozIStorageConnection;
>  
>  [scriptable, uuid(746c7a02-f6c1-4869-b434-7c8b86e60e61)]

Need to change the uuid, right?

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +253,5 @@
>      if (context && context.usePrivateBrowsing) {
>        this._pbStore.set(group, name, value);
>        this._schedule(function () {
>          cbHandleCompletion(callback, Ci.nsIContentPrefCallback2.COMPLETE_OK);
> +        this._cps._notifyPrefSet(group, name, value, context);

Am I reading this wrong or are you passing a load context for the new argument when you should be passing a boolean?

@@ +326,5 @@
>          if (ok)
>            this._cache.setWithCast(group, name, value);
>          cbHandleCompletion(callback, reason);
>          if (ok)
> +          this._cps._notifyPrefSet(group, name, value, context);

Here too.
Attachment #8705346 - Flags: review?(adw) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d3455d9c8a44d1d5f1464d70ed0e46b2d322782
Bug 1089232 - Updates nsContentPrefService to take an extra isPrivate argument. r=adw
https://hg.mozilla.org/mozilla-central/rev/5d3455d9c8a4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
I've managed to reproduce this bug on Nightly 36.0a1 (2014-10-25) on Linux, 64 Bit.

This Bug is now verified as fixed on Latest Firefox Beta 47.0b3

Build ID: 20160505125249
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
OS: Linux 3.19.0-58-generic x86-64
QA Whiteboard: [testday-20160506]
You need to log in before you can comment on or make changes to this bug.