1432509 - Short tab-switch spinners displayed when switching to a tab that is still warming but not yet rendered.

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

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

STR (at least for me on macOS):

0) Enable tab warming by setting browser.tabs.remote.warmup.enabled to true
1) Open the the "Sample page" attachment in two different tabs and wait until loaded
2) With one of the Sample pages selected, hover over the other and click it soon after (you have to do this very quickly).

ER:

The tab switch should behave the same as with tab warming disabled - the tab should not render until the layers are ready; or we should display a spinner only after 400ms.

AR:

If timed correctly, hovering and then quickly clicking results in a tab switch spinner right away, which produces a flashing effect.

Comment 1

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

Attachment: Bug 1432509 - Allow async tab switcher to load requested tabs that are still warming.

This patch makes the following changes:

1. When layers are ready for a tab, we remove it from the warming list. We used to
   do this once a tab was requested, but it actually makes more sense to keep the
   "warming" state for the tab around until the layers have arrived.
2. In postActions, when choosing which tab to try to load layers for next,
   allow for the possibility that the requested tab is being warmed (and
   therefore in STATE_LOADING). If the tab is warming, loadRequestedTab
   won't re-request layers, but will at least create the loadTimer which
   is necessary so that updateDisplay knows that we're _trying_ to load
   the warmed tab, and that we should at least wait for the loadTimer to
   finish before showing the tab switch spinner.

Review commit: https://reviewboard.mozilla.org/r/216916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216916/

Comment 2

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

Hey dao, if you're feeling a bit reluctant to review this because of how complex the async tab switcher is, I completely understand. I've written some documentation in bug 1432863 that you might want to read _before_ reviewing this, if you need to reinforce your mental model of how this all works.

Comment 3

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

Also, for context, here's why this bug was appearing:

The async tab switcher wasn't prepared to handle the case where the requested tab to be displayed was already going to be in STATE_LOADING (which is the case if a tab is warming, but hasn't finished rendering or uploading its layers yet).

This meant that in postActions, when determining _which_ tab to show, it'd fallback to showing the requested tab right away - and since that tab was in STATE_LOADING instead of STATE_LOADED, the switcher determined that showing the tab spinner made sense. This is where the short spinners were coming from.

This patch makes it so that if switching to a warming tab in STATE_LOADING, we treat it as if its in STATE_UNLOADED, and create the loadTimer for it. The loadTimer is needed so that updateDisplay knows to keep showing the _previous_ tab until either the layers for the selected tab are ready, _or_ the loadTimer goes off (after which we'll show the spinner).

This gets rid of the short spinners.

Comment 4

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

dao seems pretty bogged down. I'll see if Gijs has some cycles to review this.

Comment 5

[:Gijs (out for now; he/him)]

Comment on attachment 8947155 [details]
Bug 1432509 - Allow async tab switcher to load requested tabs that are still warming.

https://reviewboard.mozilla.org/r/216916/#review223776

Fascinating. I have some questions on the approach here, so I'm going to clear review for now so that I can wrap my head around it better. Which may then feed into the docs thing (which I'm glad I reviewed before this one).

Besides the ones from the patch context below, can you explain to this newbie-to-tab-warming how we're able to change when we unwarm or load warmed tabs, without affecting the logic for maximum number of warming tabs? It looks like that checking now lives in postActions, but I don't quite follow why it's safe to make the changes we make here without messing with that.

::: browser/base/content/tabbrowser.xml:4436
(Diff revision 1)
>              setTabState(tab, state) {
>                this.setTabStateNoAction(tab, state);

Should this just short-circuit if `state == this.tabState.get(tab)` ? Is that a bad idea?

I'm asking because in principle it's a bit scary that essentially this patch is saying "it's OK to make this code run repeatedly with the same argument (ie STATE_LOADING) . And this does call some XPCOM APIs + some XBL setters wrapping some more XPCOM APIs... which doesn't necessarily seem like an amazing plan perf-wise.

::: browser/base/content/tabbrowser.xml:4810
(Diff revision 1)
>                    this.loadTimer = null;
>                  }
>                }
>  
>                // If we're not loading anything, try loading the requested tab.
>                let requestedState = this.getTabState(this.requestedTab);

I know it's more wordy (and I'll take other suggestions), but IMO `stateOfRequestedTab` would be clearer than `requestedState`, which implies that we're trying to *get* to the `requestedState` rather than the `stateOfRequestedTab` being the *current* state of the tab.

Or at least, when I first read this block in light of the commit message, I was very confused...

::: browser/base/content/tabbrowser.xml:4811
(Diff revision 1)
>                if (!this.loadTimer && !this.minimizedOrFullyOccluded &&
>                    (requestedState == this.STATE_UNLOADED ||
> -                   requestedState == this.STATE_UNLOADING)) {
> +                   requestedState == this.STATE_UNLOADING ||
> +                   this.warmingTabs.has(this.requestedTab))) {

Putting my comment for `setTabState` differently: why do it this way rather than just checking `requestedState == this.STATE_LOADING` ? Could we do this for all LOADING tabs? Why (not) ?

Comment 6

[:Gijs (out for now; he/him)]

Also, separately, please can we have a bug to move this tab switcher thing to its own jsm? tabbrowser.xml is big enough as it is, and afaict the code is now just over 1000 lines. It deserves its own file.

Comment 7

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

(In reply to :Gijs from comment #6)
> Also, separately, please can we have a bug to move this tab switcher thing
> to its own jsm? tabbrowser.xml is big enough as it is, and afaict the code
> is now just over 1000 lines. It deserves its own file.

A solid idea. Filed as bug 1436361.

Comment 8

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

Comment on attachment 8947155 [details]
Bug 1432509 - Allow async tab switcher to load requested tabs that are still warming.

https://reviewboard.mozilla.org/r/216916/#review223776

I'm not sure I fully follow the question. `postActions` fires after every possible state change, and scans the background tabs for tabs that have finished warming (are in `STATE_LOADED`) to see if it exceeds the max - and if so, does an eviction.

Before, we were removing the tab from the warming set when it was _requested_ by the user, where the switch starts. Now, instead, we mark the tab as no longer warming once the layers are ready. That way, a warmed tab can be switched to, and we can detect that case and use the loadTimer mechanism to delay updating the UI until the layers finally arrive (if they haven't already). That's how we avoid the short spinners.

I'm not really sure I understand the question though, and I'm less sure I just answered it. Could you please re-phrase it or give me an example to work with to help me understand?

> Should this just short-circuit if `state == this.tabState.get(tab)` ? Is that a bad idea?
> 
> I'm asking because in principle it's a bit scary that essentially this patch is saying "it's OK to make this code run repeatedly with the same argument (ie STATE_LOADING) . And this does call some XPCOM APIs + some XBL setters wrapping some more XPCOM APIs... which doesn't necessarily seem like an amazing plan perf-wise.

Yeah, I think that makes sense. nsITabParent already does work to ensure that repeated calls to renderLayers don't result in more messages being sent, but for docShellIsActive (for both remote and non-remote browsers), repeated calls might do things. Let me see what happens if we short-circuit here.

> I know it's more wordy (and I'll take other suggestions), but IMO `stateOfRequestedTab` would be clearer than `requestedState`, which implies that we're trying to *get* to the `requestedState` rather than the `stateOfRequestedTab` being the *current* state of the tab.
> 
> Or at least, when I first read this block in light of the commit message, I was very confused...

That's a good idea - I'll rename the variable.

> Putting my comment for `setTabState` differently: why do it this way rather than just checking `requestedState == this.STATE_LOADING` ? Could we do this for all LOADING tabs? Why (not) ?

I think it's because we want to run `loadRequestedTab` on `STATE_LOADING` tabs _only_ if they're warming (with this patch, warming implies `STATE_LOADING`). We _don't_ want to run `loadRequestedTab` if the tab is in `STATE_LOADING` but _not_ being warmed.

That's because of how the tab switch spinner logic works. If we reach this point in `postActions` and the `requestedTab` is in `STATE_LOADING`, the indication that we're supposed to be showing the spinner is because the `loadTimer` doesn't exist (since it destroys itself after it fires).

Comment 9

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

Comment on attachment 8947155 [details]
Bug 1432509 - Allow async tab switcher to load requested tabs that are still warming.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216916/diff/1-2/

Comment 10

[:Gijs (out for now; he/him)]

Comment on attachment 8947155 [details]
Bug 1432509 - Allow async tab switcher to load requested tabs that are still warming.

https://reviewboard.mozilla.org/r/216916/#review225200

Thanks for the explanations! This makes (more) sense to me now. Ship it!

Comment 11

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

(In reply to :Gijs from comment #10)
> Comment on attachment 8947155 [details]
> Bug 1432509 - Allow async tab switcher to load requested tabs that are still
> warming.
> 
> https://reviewboard.mozilla.org/r/216916/#review225200
> 
> Thanks for the explanations! This makes (more) sense to me now. Ship it!

Thanks! I know this isn't the funnest code to review - I really appreciate it.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cead139b13a7
Allow async tab switcher to load requested tabs that are still warming. r=Gijs

Comment 13

[Dorel Luca [:dluca]]

Backed out changeset cead139b13a7 (bug 1432509) for Browser chrome failures on docshell/test/navigation/browser_bug343515.js and many more. CLOSED TREE

Log of the failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=161705910&repo=autoland&lineNumber=4862

Backout changeset:
https://hg.mozilla.org/integration/autoland/rev/642c48b909df945bf79829de6bd09155116b7234

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cead139b13a730262a52c5decb81e707137e50f4&selectedJob=161705910

Comment 14

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

Thanks, investigating.

Comment 15

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

Comment on attachment 8947155 [details]
Bug 1432509 - Allow async tab switcher to load requested tabs that are still warming.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216916/diff/2-3/

Comment 16

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

Comment on attachment 8947155 [details]
Bug 1432509 - Allow async tab switcher to load requested tabs that are still warming.

https://reviewboard.mozilla.org/r/216914/#review229422

::: browser/base/content/tabbrowser.xml:4446
(Diff revision 3)
> +              if (state == this.getTabState(tab)) {
> +                return;
> +              }

This was why my last patch was failing in automation - there was a real bug here.

When initializing the tab switch spinner, one of the first things we do is to put the state of the initial tab in the tabState map. That initial tab is usually in the STATE_LOADING or (more often) STATE_LOADED state because otherwise we'd probably still have a tab switch spinner existing and we wouldn't need to init a new one (and if it was STATE_UNLOADING or STATE_UNLOADED, it would mean that this _wasn't_ the initially selected tab since we'd swiched away. OR something has gone horribly wrong).

When setting state, this little conditional checks to see if the state we're setting to already matches the one we have on record, and if so, it bails out.

The problem is that getTabState is _lazy_ so that if there's a tab for which we've never evaluated state, we'll do it when getTabState is called. The problem is that getTabState only returns that value - it doesn't store it in the tabState map.

So the bug was that we were initting the tab switcher, the initially selected tab was in STATE_LOADED, we tried to set the state of it internally to STATE_LOADED. Then we hit this conditional, getTabState said "Yeah, this thing is already in STATE_LOADED", and we bail out early. But we _don't write the state to tabState_.

That meant that postActions, when going through the list of states to check would skip over the initial tab. And that's why the test was failing.

So now, getTabState, after lazily evaluating a tab's state, stashes it in the tabState map.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19e03e264707
Allow async tab switcher to load requested tabs that are still warming. r=Gijs

Comment 18

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/19e03e264707