Closed
Bug 1488951
Opened 6 years ago
Closed 6 years ago
Disable FastBlock after a certain number of seconds after navigation start
Categories
(Core :: Networking, defect, P1)
Core
Networking
Tracking
()
VERIFIED
FIXED
mozilla64
People
(Reporter: francois, Assigned: francois)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
ehsan.akhgari
:
review+
mayhemer
:
review+
pascalc
:
approval-mozilla-beta+
|
Details |
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.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
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•6 years ago
|
||
Comment on attachment 9006742 [details]
Bug 1488951 - Put a limit on how long FastBlock runs. r?mayhemer,ehsan
:Ehsan Akhgari has approved the revision.
Attachment #9006742 -
Flags: review+
Assignee | ||
Comment 4•6 years ago
|
||
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.
Flags: needinfo?(honzab.moz)
Assignee | ||
Comment 5•6 years ago
|
||
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•6 years ago
|
||
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.
Flags: needinfo?(honzab.moz) → needinfo?(francois)
Assignee | ||
Comment 7•6 years ago
|
||
(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.
Flags: needinfo?(francois)
Comment 8•6 years ago
|
||
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.
Attachment #9006742 -
Flags: review+
Comment 9•6 years ago
|
||
(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•6 years ago
|
||
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e97cfb1a3f88
Put a limit on how long FastBlock runs. r=mayhemer,Ehsan
Comment 11•6 years ago
|
||
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3aa3df7274f4
Backed out changeset e97cfb1a3f88 by bhearsum`s request.
Comment 12•6 years ago
|
||
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3199cd1e46de
Put a limit on how long FastBlock runs. r=mayhemer,Ehsan. CLOSED TREE
Comment 13•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee | ||
Comment 14•6 years ago
|
||
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
Attachment #9006742 -
Flags: approval-mozilla-beta?
Comment 15•6 years ago
|
||
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.
Attachment #9006742 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 16•6 years ago
|
||
bugherder uplift |
status-firefox63:
--- → fixed
Updated•6 years ago
|
Flags: qe-verify+
Comment 17•6 years ago
|
||
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•6 years ago
|
||
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?
Flags: needinfo?(francois)
Assignee | ||
Comment 19•6 years ago
|
||
(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.
Flags: needinfo?(francois)
Comment 20•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•