1550662 - Reloading all the tabs can make FF unresponsive when switching Content Blocking modes

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[Vicky Chin [:vchin]]

STR:

1. Open a bunch of tabs and navigate to different pages
2. Change content blocking from standard to strict (or vice versa)
3. Follow prompt to reload all tabs

Results:
All tabs will reload at the same time. The responsiveness of Firefox gets considerably worse the more tabs you have open.

A few questions:

1. Is it possible to only reload the tabs that are currently loaded and only force reload the other tabs when the user goes to use it? (Or after the loaded tabs are done with a small number of concurrent reloads.)

2. If we must reload all the tabs right away can we limit the number of concurrent reloads happening at the same time?

Comment 1

[Vicky Chin [:vchin]]

Reloading tabs with opened google docs is particularly bad but I notice gdocs tabs also do not have the content blocking symbol on them. Does that mean there is no content blocking for those sites? Do they need to be reloaded then?

Comment 2

[Honza Bambas (:mayhemer)]

We are reloading all tabs from here:
https://searchfox.org/mozilla-central/rev/116bd975c30746ddefc3d20e6947d1871469354f/browser/components/preferences/in-content/privacy.js#1126

This is crazy :) Did you forget about the first implementation of session restore long years ago? We were trying to load all tabs after start, at once. Soon there were extremely popular addons preventing it. These days we reload tabs only when they are activated.

I think we need something similar here as well. I would tend to implement some kind of a more general mechanism to "invalidate" all open tabs to be reloaded on activation.

Question is what to do for new network and maybe storage access requests that a page can do from timer/event handlers when new (more strict) content blocking is now in effect. Can we block them based on new policies dynamically?

If not that, then at least a queue to reload only one or two tabs at a time.

Comment 3

[Honza Bambas (:mayhemer)]

Baku, any idea about the "Question is what to do..." from the previous comment? Any ideas how this could be solved in a different way?

Comment 4

[Andrea Marchesini [:baku]]

We can move "trackingprotection" and "trackingprotection-pb" permissions into CookieSettings and we will have a consistent configuration per document/worker/channel/tab. This check is currently done here:

https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/toolkit/components/antitracking/AntiTrackingCommon.cpp#1686-1709

I can help on this part, but I would like johannh to answer about the dynamic reload.

Comment 5

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
This seems bad enough that we shouldn't just ship it like this.

I'd prioritize this P1 but it needs an owner. Hopefully Baku or Johann can help.

Comment 6

[Johann Hofmann [:johannh]]

(In reply to :Gijs (he/him) from comment #5)

[Tracking Requested - why for this release]:
This seems bad enough that we shouldn't just ship it like this.

I completely disagree. This is based on user interaction and we clearly tell the user that clicking that button will reload all their tabs. Changing content blocking preferences is not a common thing to do, and most users have a very low number of tabs (https://mzl.la/2w0OdYT). I was leaning between P5 or WONTFIX for this originally.

Question is what to do for new network and maybe storage access requests that a page can do from timer/event handlers when new (more strict) content blocking is now in effect. Can we block them based on new policies dynamically?

We moved away from this on purpose, see https://groups.google.com/forum/#!topic/mozilla.dev.platform/pyz-grWRe5s/discussion

Reloading tabs with opened google docs is particularly bad but I notice gdocs tabs also do not have the content blocking symbol on them. Does that mean there is no content blocking for those sites? Do they need to be reloaded then?

Yes, we're refreshing the cookie settings and they surely use cookies.

I'd prioritize this P1 but it needs an owner. Hopefully Baku or Johann can help.

To be clear, I'm not against optimizing this, but I don't think we will have time to work on it. The strategies outlined in comment 0 seem fine but surely there are more urgent performance issues we should focus on, instead.

Comment 7

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #6)

(In reply to :Gijs (he/him) from comment #5)

[Tracking Requested - why for this release]:
This seems bad enough that we shouldn't just ship it like this.

I completely disagree. This is based on user interaction and we clearly tell the user that clicking that button will reload all their tabs.

Comments in bug 1543037 suggested people are still surprised.

Do we warn with a modal dialog like we do for opening more than N bookmarks in tabs etc.? We've historically done so, and that might be a cheap way to make this better...

Changing content blocking preferences is not a common thing to do,

But common enough to offer the reload button at all? Also seems like this is the type of thing power users would be more likely to do than other users, which conflicts with your next point...

and most users have a very low number of tabs (https://mzl.la/2w0OdYT). I was leaning between P5 or WONTFIX for this originally.

The contention here is that the "very low" number makes the problem better. I mean, more tabs are worse, obviously. But for small numbers of tabs, it might be OK on our (powerful) hardware, it likely won't be much better on more average hardware and internet connections.

Anyway, perhaps we don't need to track this (esp. as we can't fix for 67 anymore), but this seems like a pretty severe papercut in a new feature that we should just fix.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Instead of reloading all tabs immediately, could we discard all browsers, so that we reload upon selection?

I believe that'd be a matter of calling gBrowser.discardBrowser() (defined here) with each tab that we'd normally have unloaded.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Or maybe Erica has time to look at this?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Hi Ethan,

Do you think anybody on your team would have time to look at this?

Comment 11

[Ethan Tseng [:ethan]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #10)

Hi Ethan,
Do you think anybody on your team would have time to look at this?

Thank you for making me be aware of this issue. Keep the needinfo on and I'll follow up this week.

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

I can look into this.

Comment 13

[Julien Cristau [:jcristau]]

As a P3 I'll stop tracking this for 68.

Comment 14

[Johann Hofmann [:johannh]]

Thanks!

Comment 15

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1550662 - Make "Reload All Tabs" simply discard browsers when switching Content Blocking modes. r=mconley,johannh

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

This probably needs some UI feedback for the user as well, to indicate that the tabs will be reloaded when they're selected.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2524189c4082
Make "Reload All Tabs" simply discard browsers when switching Content Blocking modes. r=mconley,johannh

Comment 18

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2524189c4082