Honour the exception list for tracking protection in the exact same way that we used to up to and including in Firefox 62

VERIFIED FIXED in Firefox 63

Status

()

defect
P1
major
VERIFIED FIXED
9 months ago
9 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({regression})

unspecified
Firefox 64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 unaffected, firefox63blocking verified, firefox64+ verified)

Details

Attachments

(2 attachments)

STR:

1. Enable blocking all detected trackers always by going to Content Blocking in Preferences, selected Always under Trackers.
2. Open www.reddit.com in a new non-private window.
3. From the Control Centre, select Disable Blocking For This Site.
4. Open a private window, and load www.reddit.com there as well.

Actual results:

  * Trackers aren't blocked in the private window (no messages indicating any trackers are blocked gets logged to the browser console.)


Expected results:

  * Trackers should be blocked in the private window.

5. Remove the exception added in step 3 by clicking that button again.
6. Now in the private window, add an exception for reddit by following a similar instruction to step 3.
7. Reload the non-private tab.

Actual results:

  * Trackers aren't blocked in the non-private window (no messages indicating any trackers are blocked gets logged to the browser console.)

Expected results:

  * Trackers should be blocked in the non-private window.



In very simple terms, before this regression, the exception lists used to be checked independently, the normal list for normal windows and the private list for private windows.  After the regression, we are checking both lists for both types of windows.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Priority: -- → P1
Duplicate of this bug: 1498219
The first 4 steps show that the normal list is honored in private mode.  If I recall correctly, Francois said that that is intended or desired behavior.  (Though, I would be perfectly fine if the normal list was not honored in private mode.)

Steps 5-7 show that the private list is honored in normal mode.  That is a problem - we shouldn't leak anything from private mode into regular mode.
Note that whatever behavior we decide for Tracking Protection, we also decide for Third Party Cookies.  Both should do the same thing.  If product or UX have thoughts on this, they should chime in right away.
Bryan mentioned earlier that the "UI lies".  I see what he means now.  If you disable protection in private mode, then go to normal mode and visit the same page, the protection is still disabled.  But worse - the shield with the strike through is not in the url bar to show users that protection is disabled.

Here is a nice test case:
https://senglehardt.com/test/cookie_restrictions/simple_iframe.html

1. Turn on Tracking Protection Always (All Detected Trackers set to Always)
2. Open private mode.  Go to https://senglehardt.com/test/cookie_restrictions/simple_iframe.html.  The tracker iframe does not load (there is a blank box on the page).
3. In the private mode tab, click the shield and Disable temporarily.  The tracker iframe loads on the page.  The url bar shows a shield with a strike through.
4.  Open a normal mode tab.  Go to https://senglehardt.com/test/cookie_restrictions/simple_iframe.html.  

Actual Result: The tracker iframe loads on the page, indicating that Tracking Protection is disabled.  Even though the exception was made in private mode, it is honored in normal mode too.  Moreover, the Shield with the strike through is not in the URL Bar.  There is no Enable/Disable protection buttons in Control Center.

The shield with a strikethrough should be in the URL bar because we are using an exception and allowing tracking.

The consequence is that the user doesn't know that there are any trackers on the page.  If there were, the user would expect for them to be blocked.  But actually, there are trackers on the page and they are being allowed.  Also, we are leaking private mode site visits into regualr mode.

Note that in about:preferences#privacy->Content Blocking->Exceptions, there is no entry for https://senglehardt.com.  So that is good, since it is harder to discover the private mode data.

Expected result: The tracker iframe doesn't load on the page, since Tracking Protection shouldn't be disabled in normal mode.  The URL bar should show the shield since trackers were detected and blocked on the page.

(The above scenario also works in the opposite way - start in normal mode and then go to private mode.  In that case, the Exceptions section of preferences does get populated, which is fine.  But the Shield UI problems still exist.)
Flags: needinfo?(bbell)
(In reply to Tanvi Vyas[:tanvi] from comment #3)
> The first 4 steps show that the normal list is honored in private mode.  If
> I recall correctly, Francois said that that is intended or desired behavior.
> (Though, I would be perfectly fine if the normal list was not honored in
> private mode.)

Francois is wrong about that.  I just added the following comment on the code review which I will paste here again to repeat the same thing here once again:

That isn't the behavior in Firefox 62.  I just retested to confirm.

STR:

1. In a fresh profile, enable Tracking Protection.
2. In a new tab, go to www.reddit.com.
3. From the Control Centre, click on "Disable for This Site".
4. Open a new private window and go to www.reddit.com.
5. Open the web console in the private window and verify that you get messages like this in the console:

The resource at “https://www.googletagservices.com/tag/js/gpt.js” was blocked because tracking protection is enabled.

If what you are asking for is for us to *change* the behavior that we had up to and including Firefox 62, please note that we *also* need to change the UI for that to make sense.  Otherwise the Control Centre UI in the STR above in the private window will incorrectly make it appear as if there is no exception being applied to www.reddit.com in a private window.  Since this patch is trying to fix a regression, I would like to revert things back to our old behavior so that it can get uplifted.  Any changes in behavior should happen in follow-up bugs.

> Steps 5-7 show that the private list is honored in normal mode.  That is a
> problem - we shouldn't leak anything from private mode into regular mode.

Both are problems.  This bug is about restoring the behavior to the Firefox 62 behavior, changing the summary to clarify that.  For changes to behavior or other things, please file follow-ups.
Summary: The Content Blocking exception lists for private and non-private windows are mixed now → Honour the exception list for tracking protection in the exact same way that we used to up to and including in Firefox 62
(In reply to Tanvi Vyas[:tanvi] from comment #5)
> Bryan mentioned earlier that the "UI lies".  I see what he means now.  If
> you disable protection in private mode, then go to normal mode and visit the
> same page, the protection is still disabled.  But worse - the shield with
> the strike through is not in the url bar to show users that protection is
> disabled.
> 
> Here is a nice test case:
> https://senglehardt.com/test/cookie_restrictions/simple_iframe.html
> 
> 1. Turn on Tracking Protection Always (All Detected Trackers set to Always)
> 2. Open private mode.  Go to
> https://senglehardt.com/test/cookie_restrictions/simple_iframe.html.  The
> tracker iframe does not load (there is a blank box on the page).
> 3. In the private mode tab, click the shield and Disable temporarily.  The
> tracker iframe loads on the page.  The url bar shows a shield with a strike
> through.
> 4.  Open a normal mode tab.  Go to
> https://senglehardt.com/test/cookie_restrictions/simple_iframe.html.  

FWIW this is basically a copy of the second part of the STR in comment 0.  :-)

> Actual Result: The tracker iframe loads on the page, indicating that
> Tracking Protection is disabled.  Even though the exception was made in
> private mode, it is honored in normal mode too.  Moreover, the Shield with
> the strike through is not in the URL Bar.  There is no Enable/Disable
> protection buttons in Control Center.
> 
> The shield with a strikethrough should be in the URL bar because we are
> using an exception and allowing tracking.

Not at all, exactly the opposite.  In reality it is the UI that is correct here and our behavior at the UI level never changed after bug 1484876.  Gecko is behaving incorrectly.  (But of course the user would have no way of knowing what's going on since Gecko isn't behaving as it should be, which is in the above case, blocking trackers in the normal window.

(Again, if you'd like to change the old behavior, that's an entirely different matter and off-topic for this bug.  Here we're only talking about fixing a regression, nothing more.)

> The consequence is that the user doesn't know that there are any trackers on
> the page.  If there were, the user would expect for them to be blocked.  But
> actually, there are trackers on the page and they are being allowed.

I don't know what you mean by this part.  The UI shows the correct state with regard to the presence of trackers on the page.  For example the shield icon still appears as it should.  It is only the exception state that is incorrect...

> Also,
> we are leaking private mode site visits into regualr mode.

I don't see how that can happen.  Can you please explain?  In step 4 in your case, all that senglehardt.com can tell is that trackers are being loaded for it, which is the normal mode of operation, so it can't tell whether that happened because TP was turned off in Firefox, or whether it was turned on and there was an exception recorded in PB mode that crept its way into normal mode.

> Note that in about:preferences#privacy->Content Blocking->Exceptions, there
> is no entry for https://senglehardt.com.  So that is good, since it is
> harder to discover the private mode data.

Yes, we have never ever showed private mode exceptions in that UI.  Bug 1484876 made no change about that.

> Expected result: The tracker iframe doesn't load on the page, since Tracking
> Protection shouldn't be disabled in normal mode.  The URL bar should show
> the shield since trackers were detected and blocked on the page.
> 
> (The above scenario also works in the opposite way - start in normal mode
> and then go to private mode.  In that case, the Exceptions section of
> preferences does get populated, which is fine.  But the Shield UI problems
> still exist.)

And that is the first part of the STR in comment 0.
I'm happy that we noticed this bug. With that said, I'm not convinced we need to rush a fix in the next couple of days. We are doing what the user asked us to do, whitelisting pages, but we're under-reporting that to users when that user changes between private and normal browsing windows. It's a distinction that would require a very vigilant user to notice. Since we're not countering the users intent, we can fix this in 64.
Flags: needinfo?(bbell)
(In reply to bbell from comment #8)
> I'm happy that we noticed this bug. With that said, I'm not convinced we
> need to rush a fix in the next couple of days. We are doing what the user
> asked us to do, whitelisting pages, but we're under-reporting that to users
> when that user changes between private and normal browsing windows. It's a
> distinction that would require a very vigilant user to notice. Since we're
> not countering the users intent, we can fix this in 64.

There may still be a privacy leak here which we haven't quite discovered yet.  I think we should fix this more from a viewpoint of the separation of private and non-private sessions rather than the tracking protection allow list itself.
Ehsan, Tanvi, Bryan, I would like to highlight that from a release management perspective, we build RC2 tomorrow (early morning PST) so if this bug is a serious regression that needs fixing before we ship 63, we need a patch uplifted to the release branch as soon as possible so as that we can include it in RC2, thanks.
I'll prepare a patch in the next hour.
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/762299961b3f
Ensure that we only check the corresponding Content Blocking exception list when testing whether a top-level document is on the Content Blocking allow list r=francois
Comment on attachment 9017718 [details]
Bug 1499549 - Ensure that we only check the corresponding Content Blocking exception list when testing whether a top-level document is on the Content Blocking allow list

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1484876

User impact if declined: See comment 0

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment 0

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The patch is pretty small scoped and well understood.  It's quite safe and was written with the possibility of uplifting to mozilla-beta/release in mind.

String changes made/needed: None
Attachment #9017718 - Flags: approval-mozilla-beta?
Comment on attachment 9017718 [details]
Bug 1499549 - Ensure that we only check the corresponding Content Blocking exception list when testing whether a top-level document is on the Content Blocking allow list

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String changes made/needed:
Attachment #9017718 - Flags: approval-mozilla-beta? → approval-mozilla-release?
This is a blocker for 63 AFAIK.
(In reply to :Ehsan Akhgari from comment #7)
> (In reply to Tanvi Vyas[:tanvi] from comment #5)
> > Bryan mentioned earlier that the "UI lies".  I see what he means now.  If
> > you disable protection in private mode, then go to normal mode and visit the
> > same page, the protection is still disabled.  But worse - the shield with
> > the strike through is not in the url bar to show users that protection is
> > disabled.
> > 
> > Here is a nice test case:
> > https://senglehardt.com/test/cookie_restrictions/simple_iframe.html
> > 
> > 1. Turn on Tracking Protection Always (All Detected Trackers set to Always)
> > 2. Open private mode.  Go to
> > https://senglehardt.com/test/cookie_restrictions/simple_iframe.html.  The
> > tracker iframe does not load (there is a blank box on the page).
> > 3. In the private mode tab, click the shield and Disable temporarily.  The
> > tracker iframe loads on the page.  The url bar shows a shield with a strike
> > through.
> > 4.  Open a normal mode tab.  Go to
> > https://senglehardt.com/test/cookie_restrictions/simple_iframe.html.  
> 
> FWIW this is basically a copy of the second part of the STR in comment 0. 
> :-)

Yes, I know.  I added these because the senglehardt domain makes it clear when the cookie is blocked and when it isn't, so it may be an easier test case to look at without opening developer tools.  Also, I added parts about the UI experience.

> 
> > Actual Result: The tracker iframe loads on the page, indicating that
> > Tracking Protection is disabled.  Even though the exception was made in
> > private mode, it is honored in normal mode too.  Moreover, the Shield with
> > the strike through is not in the URL Bar.  There is no Enable/Disable
> > protection buttons in Control Center.
> > 
> > The shield with a strikethrough should be in the URL bar because we are
> > using an exception and allowing tracking.
> 
> Not at all, exactly the opposite.  In reality it is the UI that is correct
> here and our behavior at the UI level never changed after bug 1484876. 
> Gecko is behaving incorrectly.  (But of course the user would have no way of
> knowing what's going on since Gecko isn't behaving as it should be, which is
> in the above case, blocking trackers in the normal window.

I just mean that the UI isn't portraying what is happening in that page load, whatever the reason.
We need QE to verify the fix, Cornel we will want to test both in nightly and rc2 tomorrow
Flags: qe-verify+
Flags: needinfo?(cornel.ionce)
(In reply to Tanvi Vyas[:tanvi] from comment #17)
> (In reply to :Ehsan Akhgari from comment #7)
> > (In reply to Tanvi Vyas[:tanvi] from comment #5)
> > > Bryan mentioned earlier that the "UI lies".  I see what he means now.  If
> > > you disable protection in private mode, then go to normal mode and visit the
> > > same page, the protection is still disabled.  But worse - the shield with
> > > the strike through is not in the url bar to show users that protection is
> > > disabled.
> > > 
> > > Here is a nice test case:
> > > https://senglehardt.com/test/cookie_restrictions/simple_iframe.html
> > > 
> > > 1. Turn on Tracking Protection Always (All Detected Trackers set to Always)
> > > 2. Open private mode.  Go to
> > > https://senglehardt.com/test/cookie_restrictions/simple_iframe.html.  The
> > > tracker iframe does not load (there is a blank box on the page).
> > > 3. In the private mode tab, click the shield and Disable temporarily.  The
> > > tracker iframe loads on the page.  The url bar shows a shield with a strike
> > > through.
> > > 4.  Open a normal mode tab.  Go to
> > > https://senglehardt.com/test/cookie_restrictions/simple_iframe.html.  
> > 
> > FWIW this is basically a copy of the second part of the STR in comment 0. 
> > :-)
> 
> Yes, I know.  I added these because the senglehardt domain makes it clear
> when the cookie is blocked and when it isn't, so it may be an easier test
> case to look at without opening developer tools.  Also, I added parts about
> the UI experience.

Good point, yes that is a better test page.

> > > Actual Result: The tracker iframe loads on the page, indicating that
> > > Tracking Protection is disabled.  Even though the exception was made in
> > > private mode, it is honored in normal mode too.  Moreover, the Shield with
> > > the strike through is not in the URL Bar.  There is no Enable/Disable
> > > protection buttons in Control Center.
> > > 
> > > The shield with a strikethrough should be in the URL bar because we are
> > > using an exception and allowing tracking.
> > 
> > Not at all, exactly the opposite.  In reality it is the UI that is correct
> > here and our behavior at the UI level never changed after bug 1484876. 
> > Gecko is behaving incorrectly.  (But of course the user would have no way of
> > knowing what's going on since Gecko isn't behaving as it should be, which is
> > in the above case, blocking trackers in the normal window.
> 
> I just mean that the UI isn't portraying what is happening in that page
> load, whatever the reason.

Indeed.
https://hg.mozilla.org/mozilla-central/rev/762299961b3f
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Comment on attachment 9017718 [details]
Bug 1499549 - Ensure that we only check the corresponding Content Blocking exception list when testing whether a top-level document is on the Content Blocking allow list

We need this fix in 63 as its a blocker, should be included in RC2. Beta63+
Attachment #9017718 - Flags: approval-mozilla-release? → approval-mozilla-release+
Severity: normal → major
(In reply to :Ehsan Akhgari from comment #15)
> Created attachment 9017980 [details] [diff] [review]
> Patch (for release)

Ehsan, can you confirm that this is the one we should uplift and not the one you requested uplift approval before?
Flags: needinfo?(ehsan)
(In reply to Pascal Chevrel:pascalc from comment #22)
> (In reply to :Ehsan Akhgari from comment #15)
> > Created attachment 9017980 [details] [diff] [review]
> > Patch (for release)
> 
> Ehsan, can you confirm that this is the one we should uplift and not the one
> you requested uplift approval before?

Yes.
Flags: needinfo?(ehsan)
I reproduced this issue using Fx 64.0a1 (2018-10-15)on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 64.0a1 (2018-10-16) and Fx 63.0-build2, on Windows 10 x64, Ubuntu 16.04 LTS x64 and macOS 10.13.4.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(cornel.ionce)
You need to log in before you can comment on or make changes to this bug.