If a user disables tracking protection for a site (via the shield doorhanger) while in Private Browsing mode, that exception will stick around and apply even outside of Private Browsing. This privacy leak allows someone that a particular website was visited while in Private Browsing. Monica notes that if we fix that then it means that exceptions in PB will always be temporary, which could be annoying in the context of bug 1143530.

I'm not sure this is a bug. Other state the the user deliberately sets in private browsing (such as bookmarks) also persist once the session ends.

In Fx42, we'll aim for this: - TP exceptions added while in Private Browsing will expire when the Private Browsing session ends

This might be useful: https://bugzilla.mozilla.org/show_bug.cgi?id=967812

As far as I know, we don't have a separate permission manager for private browsing. Ehsan can you confirm? It would pretty simple to set TP exceptions to expire the current browser session ends (session exceptions never persist to disk). This is not the same as expiring when the private browsing session ends, though.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #6) > As far as I know, we don't have a separate permission manager for private > browsing. Ehsan can you confirm? That's correct. Note that bug 967812 doesn't save us here either. > It would pretty simple to set TP exceptions to expire the current browser > session ends (session exceptions never persist to disk). This is not the > same as expiring when the private browsing session ends, though. We should do exactly what we do for other similar things in private browsing: Handle last-pb-context-exited which gets fired when the last private window is closed, and clear the in-memory information that tells us which TP exceptions exist at that point. And we should not store this information in the permission manager, that won't ever be useful and it will do the wrong thing by default. That way, if you close a private window and reopen one and go to the same website, you will need to reset the exception again, but that is consistent with all of the other temporary information that we usually remember for private windows for the duration of the browsing session.

This morning with the developers, I suggested that our approach should consider that we may bring TP to normal mode in the future and would persist exceptions there while clearing them from PBM on session close. If it's the same amount of work, we may want to allow the current mechanism of persistence to remain in normal mode, avoiding future rework while being faithful to PBM. The scope for 42 is just PBM though, so if it's considerable more work, we solve strictly for PBM.

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #7) > We should do exactly what we do for other similar things in private > browsing: Handle last-pb-context-exited which gets fired when the last > private window is closed, and clear the in-memory information that tells us > which TP exceptions exist at that point. And we should not store this > information in the permission manager, that won't ever be useful and it will > do the wrong thing by default. If I understand this correctly then, you suggest that we change nsChannelClassifier::ShouldEnableTrackingProtection to not consult the permission manager, but a different in-memory structure that gets cleared when the last private window is closed? And in the tracking protection in normal mode case, persist that structure on shutdown?

This is my understanding: I think we're fine to use the permission manager outside of PB windows but entries added to the allow list in a PB window should be added to a separate in-memory structure that only gets used for PB windows. The one question I'm not 100% sure of the answer to is whether we should also look at the existing permission manager entries while in a PB window. e.g. if I allow tracking on www.mozilla.org in a regular window, should tracking be allowed in a PB window.

If the goal is to persist these for non-private mode in the future as comment 8 suggests (which is a fine goal, and is my preference here), we shouldn't change what nsChannelClassifier::ShouldEnableTrackingProtection does in the case of a non-private window, and use the in-memory table for the private window case, and clear our that table on last-pb-context-exited. (In reply to Matthew N. [:MattN] from comment #10) > The one question I'm not 100% sure of the answer to is whether we should > also look at the existing permission manager entries while in a PB window. > e.g. if I allow tracking on www.mozilla.org in a regular window, should > tracking be allowed in a PB window. That is a great question. This is mostly a UX issue, but we try to create the conceptual model of loading stuff in a private window in a "clean slate", which makes me want to suggest that we should not fall back to testing the permission in the case of private windows at all. This is worth bringing up with the UX team though.

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #11) > (In reply to Matthew N. [:MattN] from comment #10) > > The one question I'm not 100% sure of the answer to is whether we should > > also look at the existing permission manager entries while in a PB window. > > e.g. if I allow tracking on www.mozilla.org in a regular window, should > > tracking be allowed in a PB window. > > That is a great question. This is mostly a UX issue, but we try to create > the conceptual model of loading stuff in a private window in a "clean > slate", which makes me want to suggest that we should not fall back to > testing the permission in the case of private windows at all. This is worth > bringing up with the UX team though. My personal guess would be that TP exceptions should work similarly to history: it's all accessible from PB mode but anything that is added while in PB does not persist. That way, if you whitelist a site in regular mode because it's broken when using TP, then it would not require you to do the same thing again every time you go into PB mode and visit that site.

Stealing from François with permission.

(In reply to François Marier [:francois] from comment #12) > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, > needinfo? me!) from comment #11) > > (In reply to Matthew N. [:MattN] from comment #10) > > > The one question I'm not 100% sure of the answer to is whether we should > > > also look at the existing permission manager entries while in a PB window. > > > e.g. if I allow tracking on www.mozilla.org in a regular window, should > > > tracking be allowed in a PB window. > > > > That is a great question. This is mostly a UX issue, but we try to create > > the conceptual model of loading stuff in a private window in a "clean > > slate", which makes me want to suggest that we should not fall back to > > testing the permission in the case of private windows at all. This is worth > > bringing up with the UX team though. > > My personal guess would be that TP exceptions should work similarly to > history: it's all accessible from PB mode but anything that is added while > in PB does not persist. > > That way, if you whitelist a site in regular mode because it's broken when > using TP, then it would not require you to do the same thing again every > time you go into PB mode and visit that site. That's fine by me, I don't feel strongly either way.

This is my work-in-progress that seems to work well in my testing so far. Ehsan, I'm not sure if you are available for review/feedback requests, but I assume you would know who to pass this on to.

Comment on attachment 8636599 [details] [diff] [review] WIP Review of attachment 8636599 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsChannelClassifier.cpp @@ +170,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > +#ifdef DEBUG > + if (exists) { > + LOG(("nsChannelClassifier[%p]: Allowlisting channel[%p] for %s", this, Nit: it would be nice to mention in the debug message that it came from the PB in-memory list (as opposed to the one in the permission manager).

Comment on attachment 8636599 [details] [diff] [review] WIP Review of attachment 8636599 [details] [diff] [review]: ----------------------------------------------------------------- (Yep, I can help review this stuff!) The ideas in this patch are all good, but there are two issues with it: 1. Please don't put this stuff inside the permission manager, that component shouldn't have logic specific to one of its consumers. I would expect this code to live in the channel classifier. 2. This doesn't work well with multiple content processes, since each one will have its own separate copy of this list which may be out of sync with the rest of the processes. I'm not sure if that is something that we need to be worried about now or not, but please check with the e10s folks. If we need to handle that case, you should probably store the list in the parent process.

(In reply to François Marier [:francois] from comment #16) > Nit: it would be nice to mention in the debug message that it came from the > PB in-memory list (as opposed to the one in the permission manager). Good point, fixed locally. (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #17) > 1. Please don't put this stuff inside the permission manager, that component > shouldn't have logic specific to one of its consumers. I would expect this > code to live in the channel classifier. > > 2. This doesn't work well with multiple content processes, since each one > will have its own separate copy of this list which may be out of sync with > the rest of the processes. I'm not sure if that is something that we need > to be worried about now or not, but please check with the e10s folks. If we > need to handle that case, you should probably store the list in the parent > process. I asked in #e10s and it looks like shipping support for more than 1 content process is not going to happen any time soon and since this bug is important for 42, I think we don't have to worry about it just yet. On the other hand, I was told that the permission manager runs in the parent process, so the arrangement in the current patch would do the right thing in that case. I'm not certain, but it looks like the channel classifier runs in both processes (so your comment holds for that case), but it's not exposed to JS AFAICT, so it can't be called from browser-trackingprotection.js like I do with the permission manager. So my problem with your first suggestion is how to call the new methods from JS if I move them to nsChannelClassifier. There is also nsIURIClassifier and various places in the url-classifier directory that look promising, but I don't know enough about the relationships between these entities or the general architecture of our safe browsing components to tell if those would work (or if that's what you meant).

(In reply to Panos Astithas [:past] from comment #18) > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, > needinfo? me!) from comment #17) > > 1. Please don't put this stuff inside the permission manager, that component > > shouldn't have logic specific to one of its consumers. I would expect this > > code to live in the channel classifier. > > > > 2. This doesn't work well with multiple content processes, since each one > > will have its own separate copy of this list which may be out of sync with > > the rest of the processes. I'm not sure if that is something that we need > > to be worried about now or not, but please check with the e10s folks. If we > > need to handle that case, you should probably store the list in the parent > > process. > > I asked in #e10s and it looks like shipping support for more than 1 content > process is not going to happen any time soon and since this bug is important > for 42, I think we don't have to worry about it just yet. OK, fair! > On the other hand, I was told that the permission manager runs in the parent > process, so the arrangement in the current patch would do the right thing in > that case. I'm not certain, but it looks like the channel classifier runs in > both processes (so your comment holds for that case), but it's not exposed > to JS AFAICT, so it can't be called from browser-trackingprotection.js like > I do with the permission manager. The permission manager also runs in both processes. > So my problem with your first suggestion is how to call the new methods from > JS if I move them to nsChannelClassifier. There is also nsIURIClassifier and > various places in the url-classifier directory that look promising, but I > don't know enough about the relationships between these entities or the > general architecture of our safe browsing components to tell if those would > work (or if that's what you meant). How about defining a new interface, such as nsIPrivateBrowsingTrackingProtectionWhitelist essentially with the three methods that you were adding in your previous patch, and implement that interface in JS or C++ (whichever you prefer) and just using do_GetService to instantiate that service in the channel classifier code? I guess that code could also live in toolkit/components/url-classifier.

This version uses a new JS service that implements the nsIPrivateBrowsingTrackingProtectionWhitelist interface. I tested in both desktop and Android and it appears to work well. A try run was green, too: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a04335b64fd8 Margaret can you review the small Fennec change and Ehsan the rest? I'm working on writing a new test that I'll post as a separate patch in this bug, but this should be ready for review.

Comment on attachment 8637906 [details] [diff] [review] Patch v2 Review of attachment 8637906 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks great! I have a few nits below. FWIW, I also reviewed the Fennec changes. It would be super nice if you would factor out the common logic between browser/ and mobile/ into PrivateBrowsingUtils.jsm. Bonus points for doing it in this bug, but I don't want to force you to do that. :-) But at least please file a follow-up for that. ::: netwerk/base/nsChannelClassifier.cpp @@ +173,5 @@ > + bool exists = false; > + rv = pbmtpWhitelist->ExistsInPBTrackingAllowlist(topWinURI, &exists); > + NS_ENSURE_SUCCESS(rv, rv); > + > +#ifdef DEBUG Why only in debug builds? It seems useful to log this in non-debug builds as well. @@ +185,5 @@ > + mIsAllowListed = true; > + *result = false; > + } else { > + *result = true; > + } Nit: I would get rid of the #ifdef above, move the mIsAllowList assignment into the above condition, and then just do: |*result = !exists;|. ::: toolkit/components/url-classifier/nsIPrivateBrowsingTrackingProtectionWhitelist.idl @@ +20,5 @@ > + * the operation is essentially a no-op. > + * > + * @param uri the uri to add to the list > + */ > + void addToPBTrackingAllowlist(in nsIURI uri); Nit: let's not make these method names so verbose. addToAllowList, removeFromAllowList and existsInAllowList sounds like acceptable alternate names to me! ::: toolkit/components/url-classifier/nsPrivateBrowsingTrackingProtectionWhitelist.js @@ +43,5 @@ > + removeFromPBTrackingAllowlist(uri) { > + let length = this._allowlist.length; > + for (let i = 0; i < length; i++) { > + if (this._allowlist[i] === uri.spec) { > + this._allowlist.splice(i, 1); Shouldn't you break out of the loop here? Also, it would be better if you find the index using indexOf rather than searching through the array manually. @@ +65,5 @@ > + } > + } > +}; > + > +this.NSGetFactory = XPCOMUtils.generateNSGetFactory([PrivateBrowsingTrackingProtectionWhitelist]); I think you should make this a singleton, by using generateSingletonFactory. ::: toolkit/components/url-classifier/nsURLClassifier.manifest @@ +3,5 @@ > component {ca168834-cc00-48f9-b83c-fd018e58cae3} nsUrlClassifierListManager.js > contract @mozilla.org/url-classifier/listmanager;1 {ca168834-cc00-48f9-b83c-fd018e58cae3} > component {9111de73-9322-4bfc-8b65-2b727f3e6ec8} nsUrlClassifierHashCompleter.js > contract @mozilla.org/url-classifier/hashcompleter;1 {9111de73-9322-4bfc-8b65-2b727f3e6ec8} > +component {a319b616-c45d-4037-8d86-01c592b5a9af} nsPrivateBrowsingTrackingProtectionWhitelist.js Nit: please drop the ns prefix in the name of this file.

Comment on attachment 8637906 [details] [diff] [review] Patch v2 Review of attachment 8637906 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting to Chenxia.

All good suggestions, thanks!

Ehsan reviewed the Fennec changes, so an additional review is not strictly necessary, but if Chenxia has any comments, I'm happy to address them.

This test verifies that whitelisting sites works across tabs in the same PB window, but the list is not persistent across PB windows.

Comment on attachment 8638628 [details] [diff] [review] Add a test for bug 1145503 Review of attachment 8638628 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks great! ::: browser/base/content/test/general/browser_trackingUI_5.js @@ +30,5 @@ > +} > + > +function testTrackingPage(window) { > + info("Tracking content must be blocked"); > + ok (!TrackingProtection.container.hidden, "The container is visible"); Style nit: no space before ( please! @@ +73,5 @@ > + browser = privateWin.gBrowser; > + let tab = browser.selectedTab = browser.addTab(); > + > + TrackingProtection = browser.ownerGlobal.TrackingProtection; > + Services.prefs.setBoolPref(PB_PREF, true); Please use SpecialPowers.pushPrefEnv() which takes care of clearing the pref for you as well!

Oops, copy & paste fail, thanks for the quick review!

Try looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a3e66cd8b4a

https://hg.mozilla.org/integration/fx-team/rev/7449a9371810 https://hg.mozilla.org/integration/fx-team/rev/39ba9a407b24

https://hg.mozilla.org/mozilla-central/rev/7449a9371810 https://hg.mozilla.org/mozilla-central/rev/39ba9a407b24

This patch makes products without MOZ_URL_CLASSIFIER defined unable to compile. MOZ_URL_CLASSIFIER is usually turned on by MOZ_SAFE_BROWSING. Should we either: - file a follow-up to keep builds without MOZ_URL_CLASSIFIER to compile. - accept that everyone needs MOZ_URL_CLASSIFIER, and remove it as a flag.

(In reply to [:fabrice] NOT READING BUGMAIL UNTIL JULY 27 from comment #31) > This patch makes products without MOZ_URL_CLASSIFIER defined unable to > compile. MOZ_URL_CLASSIFIER is usually turned on by MOZ_SAFE_BROWSING. > Should we either: > - file a follow-up to keep builds without MOZ_URL_CLASSIFIER to compile. > - accept that everyone needs MOZ_URL_CLASSIFIER, and remove it as a flag. Bug 1188158 fixes this.

I tested using François' test page here: https://people.mozilla.org/~fmarier/test_tp.html Looks good on Nightly 42.0a1, 2015-08-05.