1480443 - Enable FastBlock by default on Nightly

Status: VERIFIED FIXED

(Core :: Networking: HTTP, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

With the Content Blocking UI going live in Nightly right now (bug 1476217) we probably want to flip the FastBlock pref as well to get some early input.

Comment 1

[François Marier [:francois]]

When we enable fastblock by default in Nightly, we should also reset the privacy.trackingprotection.introCount pref to make sure that users see the new onboarding.

Comment 2

[Johann Hofmann [:johannh]]

(In reply to François Marier [:francois] from comment #1)
> When we enable fastblock by default in Nightly, we should also reset the
> privacy.trackingprotection.introCount pref to make sure that users see the
> new onboarding.

Note that there's no new onboarding yet, so we should do that in a separate bug.

Comment 3

[Gary Chen [:xeonchen]]

Ethan will handle this

Comment 4

[Ethan Tseng [:ethan]]

(In reply to Gary Chen [:xeonchen] from comment #3)
> Ethan will handle this

Cheers. This would be my good first bug to practice Phabricator. ;)

Comment 5

[Ethan Tseng [:ethan]]

Attachment: Bug 1480443 - Enable FastBlock by default on Nightly

Comment 6

[François Marier [:francois]]

Comment on attachment 9001669 [details]
Bug 1480443 - Enable FastBlock by default on Nightly

François Marier [:francois] has approved the revision.

Comment 7

[Ethan Tseng [:ethan]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8abe49b7e34f2d950f084647ede4dd040e20eb0b

This patch breaks some test cases. For example, browser/components/preferences/in-content/tests/browser_contentblocking.js
has the assumption that FastBlock is disabled by default and checks if it successfully modified the pref value.

We need to fix these test failures.

Comment 8

[Ethan Tseng [:ethan]]

Attachment: Bug 1480443 - Part 2: Fix test case failure of Content Blocking

Comment 9

[Ethan Tseng [:ethan]]

Attachment: Bug 1480443 - Part 3: Disable FastBlock in URL Classifier tests

Comment 10

[Dimi Lee [:dimi]]

Comment on attachment 9004210 [details]
Bug 1480443 - Part 3: Disable FastBlock in URL Classifier tests

Dimi Lee[:dimi][:dlee] has approved the revision.

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 9003834 [details]
Bug 1480443 - Part 2: Fix test case failure of Content Blocking

Johann Hofmann [:johannh] has approved the revision.

Comment 12

[Ethan Tseng [:ethan]]

The updated try result (with Part 2 and 3 patches):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90f57ed4db47f9450f9614a310a24af170412081

There is still at least one failure which seems to be caused by the patch of this bug.
TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/mochitest/test_classifier.html | Test timed out.

Comment 13

[(no longer active)]

Given the fact that having a feature like fastblock turned on can turn any test which has tracking resources orange intermittently if the machine runs too slowly, you might want to consider disabling the pref globally for all tests in https://searchfox.org/mozilla-central/source/testing/profiles/common/user.js and only enabling it specifically for the tests that actually want to turn fastblock on.  That file is intended for preferences that are intended to be set globally across both correctness test harnesses as well as Talos.

Comment 14

[Johann Hofmann [:johannh]]

Hm, but is it really a good idea to disable a pref that will be set for users globally in our tests? At least with browser chrome mochitests we try to stay as close to the real thing as possible, in my view. Note that you can set prefs in test .ini files: https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/browser/base/content/test/performance/browser.ini#7

This leaves the option of disabling these prefs by directory instead.

Comment 15

[Johann Hofmann [:johannh]]

(In reply to François Marier [:francois] from comment #1)
> When we enable fastblock by default in Nightly, we should also reset the
> privacy.trackingprotection.introCount pref to make sure that users see the
> new onboarding.

Please note that the onboarding ui tour situation was already handled in separate bugs and there is nothing we need to do here, **especially** NOT resetting that pref, which would cause all our users to have to see the old TP onboarding again.

Thanks :)

Comment 16

[(no longer active)]

(In reply to Johann Hofmann [:johannh] from comment #14)
> Hm, but is it really a good idea to disable a pref that will be set for
> users globally in our tests?

Ideally, no.  But this isn't a binary decision, i.e., disabling fastblock for everything doesn't mean not testing it, we *should* have specific tests for fastblock, turning the pref on for everything is a very poor replacement for that.

> At least with browser chrome mochitests we try
> to stay as close to the real thing as possible, in my view. Note that you
> can set prefs in test .ini files:
> https://searchfox.org/mozilla-central/rev/
> 55da592d85c2baf8d8818010c41d9738c97013d2/browser/base/content/test/
> performance/browser.ini#7

I think the choice we are going to have to is whether to keep fastblock enabled and add some intermittent test failures to every single test that creates a tracking channel, or disable fastblock globally and opt into it for the tests that really want to test the feature.  Based on my experience I think the second option in the long term is going to waste a lot of developer and sheriff time and result in a lot of one off "disable fast block for this test" patches being written and landing.

My $0.02.

Comment 17

[Ethan Tseng [:ethan]]

The latest try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1c8a019f3af209ca5b66375b49b6b3761a5b76c

Comment 18

[Ethan Tseng [:ethan]]

(In reply to :Ehsan Akhgari from comment #16)
> (In reply to Johann Hofmann [:johannh] from comment #14)
> > Hm, but is it really a good idea to disable a pref that will be set for
> > users globally in our tests?
> Ideally, no.  But this isn't a binary decision, i.e., disabling fastblock
> for everything doesn't mean not testing it, we *should* have specific tests
> for fastblock, turning the pref on for everything is a very poor replacement
> for that.

Ehsan and Johann, thanks for your input!

From our offline discussion, we have made a conclusion NOT to disable FastBlock globally for all the tests. As Johann said in comment 14, this is more close to the real setting (FastBlock is enabled by default on Nightly). Also, we can avoid using hacky solutions to fix test failures in Content Blocking.

The current patches only disable FastBlock in certain url-classifier tests which would fail when FastBlock is on. I will file a follow-up bug to disable FastBlock for all url-classifier tests so FB won't interfere with any URL Classifier/Tracking Protection tests which would create annotating channels.

Comment 19

[François Marier [:francois]]

Comment on attachment 9004210 [details]
Bug 1480443 - Part 3: Disable FastBlock in URL Classifier tests

François Marier [:francois] has approved the revision.

Comment 20

[Ethan Tseng [:ethan]]

Try result of the latest patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d06df01e584a0e94e407055b5991568a65a175cf

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e01b236471e
Enable FastBlock by default on Nightly. r=francois
https://hg.mozilla.org/integration/autoland/rev/6280d28ae6af
Part 2: Fix test case failure of Content Blocking. r=johannh
https://hg.mozilla.org/integration/autoland/rev/1082415e378f
Part 3: Disable FastBlock in URL Classifier tests. r=dimi,francois

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5e01b236471e
https://hg.mozilla.org/mozilla-central/rev/6280d28ae6af
https://hg.mozilla.org/mozilla-central/rev/1082415e378f

Comment 23

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed, I verified using Fx 64.0a1 (2018-09-19) on Windows 10 x64, mac 10.14 and Ubuntu 14.04 LTS x64.