1488951 - Disable FastBlock after a certain number of seconds after navigation start

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[François Marier [:francois]]

In order to put an upper bound on the amount of post-load blocking that FastBlock could do, let's put a limit after which it will disable itself.

Comment 1

[François Marier [:francois]]

Attachment: Bug 1488951 - Put a limit on how long FastBlock runs. r?mayhemer,ehsan

Comment 2

[François Marier [:francois]]

I used 20 seconds as the default limit based on the fact that it's greater than the average webpage load time regardless of country and category:

https://www.machmetrics.com/speed-blog/average-page-load-times-websites-2018/
https://analytics.googleblog.com/2012/04/global-site-speed-overview-how-fast-are.html

Here's how I manually tested this:

1. Enable FastBlock Set the limit to 15 seconds (default)
2. Load https://techcrunch.com/ with devtools console open (Errors and Warnings only).
3. Wait for the page to finish loading.
4. Confirm that a bunch of tracker domains are blocked.
5. Scroll down to the bottom of the page.
6. Clear the devtools console messages.
7. Click on "Load More".
8. Confirm that trackers are not blocked anymore (nothing in the console).

Comment 3

[(no longer active)]

Comment on attachment 9006742 [details]
Bug 1488951 - Put a limit on how long FastBlock runs. r?mayhemer,ehsan

:Ehsan Akhgari has approved the revision.

Comment 4

[François Marier [:francois]]

Honza, just putting this on your Bugzilla radar in case you missed the Phabricator notification. We're hoping to uplift into Beta 63 after it lands in Nightly.

Comment 5

[François Marier [:francois]]

Try looks good with limit=0: https://treeherder.mozilla.org/#/jobs?repo=try&revision=16389dd1579c3e44d6aadfc0054a5d13c164c071

and with limit=20s: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5806d47eff5a2f154c988f0b358dc252b3159cc2

Comment 6

[Honza Bambas (:mayhemer)]

I'm not sure I understand how the motivation for this bug and its execution tune together.  I've seen some complain that fastblock after the 5 seconds limit causes some breakage when interacting.  Does the same happen with TP on?  If not, then this bug is definitely WONTFIX.  Because after the default 5 seconds for FB, we should behave EXACTLY the same as when TP is on.  Hence, we have already made necessary precautions to not break interaction while still blocking trackers.

Comment 7

[François Marier [:francois]]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> I've seen some complain that fastblock after the 5 seconds limit causes
> some breakage when interacting.  Does the same happen with TP on?

Yes, it's the same.

The idea behind FastBlock is that it's intended purely as a perf feature to speed up the initial page load. As much as possible, we don't want to affect the page after it has loaded. That means that if a site delays loading ads until after the page has finished loading, we don't want to block those ads.

Ehsan and I discussed that and came up with three different ways in which we could disable FastBlock after a page load:

1. disable it on user interaction (bug 1485492)
2. disable it once the load event fires (bug 1488974)
3. disable it after a certain amount of time (this bug)

We don't know yet which of these will end up working and so we've decided to do all three and then measure them.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 9006742 [details]
Bug 1488951 - Put a limit on how long FastBlock runs. r?mayhemer,ehsan

Honza Bambas (:mayhemer) has approved the revision.

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to François Marier [:francois] from comment #7)
> (In reply to Honza Bambas (:mayhemer) from comment #6)

Thanks for making this clear for me.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e97cfb1a3f88
Put a limit on how long FastBlock runs. r=mayhemer,Ehsan

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3aa3df7274f4
Backed out changeset e97cfb1a3f88 by bhearsum`s request.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3199cd1e46de
Put a limit on how long FastBlock runs. r=mayhemer,Ehsan. CLOSED TREE

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/3199cd1e46de

Comment 14

[François Marier [:francois]]

Comment on attachment 9006742 [details]
Bug 1488951 - Put a limit on how long FastBlock runs. r?mayhemer,ehsan

Approval Request Comment
[Feature/Bug causing the regression]: FastBlock
[User impact if declined]: We will not be able to toggle a parameter in FastBlock that can reduce breakage.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes, I have manually tested it following the steps in comment 2.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is only enabled in Nightly. It is disabled in Beta and release (it's set to 0).
[String changes made/needed]: None

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9006742 [details]
Bug 1488951 - Put a limit on how long FastBlock runs. r?mayhemer,ehsan

Low-risk patch as it is a feature disabled on beta, uplift approved for 63 beta 6 thanks.

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dbf69e2a3a0e

Comment 17

[Daniel Bodea [:danibodea]]

I have reproduced the issue in Nightly v64.0a1 (2018-09-05) and I can confirm the fix in Nightly v64.0a1 (2018-09-13).
Awaiting beta build for uplift.

Comment 18

[Daniel Bodea [:danibodea]]

This issue does not seem to be fixed in Beta v63.0b6. Considering this, I have re-reproduced it in Nightly v64.0a1 from 2018-09-05 and re-confirmed the fix in Nightly v64.0a1 from 2018-09-16.

I have used the following steps to reproduce:
1. Set pref "browser.contentblocking.ui.enabled" to true;
2. Set pref "browser.fastblock.enabled" to true;
3. Set pref "browser.fastblock.timeout" to 15000 (15000miliseconds=15seconds until fastblock stops blocking);
4. Open browser and browser console;
5. Load page "https://techcrunch.com/";
(A pop-up will appear and a confirmation will be needed in order to load the page)
6. Confirm that some tracker domains are being blocked (If not, reload the page and hit the "Load more" button from the bottom of the page before the 15 seconds have elapsed to confirm that some tracker domains get blocked);
7. Wait for the 15 seconds to pass, clear the browser console and hit the "Load more" button again;
Expected: There aren't any slow loading tracker domains being blocked (errors/warnings in the browser console);
Actual: There ARE errors about slow loading tracker domains being blocked in the browser console.

@François Marier: Can you have a look at this? Is there something wrong with my STR or something else?

Comment 19

[François Marier [:francois]]

(In reply to Bodea Daniel [:danibodea] from comment #18)
> 3. Set pref "browser.fastblock.timeout" to 15000 (15000miliseconds=15seconds
> until fastblock stops blocking);

This is the problem. The pref you want to change is browser.fastblock.limit.

You can leave browser.fastblock.timeout to its default value.

Comment 20

[Daniel Bodea [:danibodea]]

This issue has been re-verified on Nightly v64.0a1 (2018-09-18) and verified in Beta v63.0b6 on Windows 10 and Ubuntu 16.04.
The issue is fixed and the uplift is successful. 

Thank you.