1355426 - When closing tabs, select the next tab before working on closing the tab

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[Florian Quèze [:florian]]

Closing tabs is janky, and sometimes very slow (seconds), see bug 1344302. I think we could hide this perceived slowness by selecting immediately the next tab, so that the user can start interacting with the browser again immediately.

Then we can finish closing the previous tab in the background. In the unfortunate case where closing the previous tab requires showing a beforeunload prompt to the user, we'll have to switch the tab selection again, and that may cause some flickering, but I think users are likely to blame this interruption on the site rather than on the browser.

We may want to leave a few ms for the tab we are closing to show the prompt before we switch the selected tab (especially if switching the selected tab requires scrolling the tabstrip).

Comment 1

[Dão Gottwald [:dao]]

Bug 1336763 aside, what's slow in _beginRemoveTab? We already call _blurTab directly after _beginRemoveTab.

A varying amount of work can hide behind the TabClose event. However, changing the order of that would also be an API change.

Comment 2

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [::dao] from comment #1)
> Bug 1336763 aside,

I think that's the biggest part.

> what's slow in _beginRemoveTab?

I'll need to look at newer profiles again (preferably on low end hardware) to give you a good answer, but from looking at https://perf-html.io/public/ea9fdeb90607e2179cf0ce6831177544ec32e75a/calltree/?callTreeFilters=prefixjs-yUvyV0yV3yV4yV5&jsOnly&thread=0
we have:
- sdk overhead (hopefully will be removed before 57)
- permit unload
- SessionStore stuff
- and finally some devtools and telemetry stuff

We also really need to optimize updateCurrentBrowser (bug 1349822).

Comment 3

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

Hey mmucci, when you have a sec, can you update the Photon Tracking spreadsheet and pin this one on me?

Comment 4

[Marco Mucci [:MarcoM]]

(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #3)
> Hey mmucci, when you have a sec, can you update the Photon Tracking
> spreadsheet and pin this one on me?

Thanks Mike, done.

Comment 5

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

So I started poking at this a bit more today.

Some findings:

1) tabbrowser's _blurTab looks like it already tries to do this. It's called when removing a tab here: http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/browser/base/content/tabbrowser.xml#2875 and here's the implementation of _blurTab: http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/browser/base/content/tabbrowser.xml#3156-3190

2) It's possible that _blurTab can occur sooner in the tab removal process. I'll try fiddling with that next.

Comment 6

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

Attachment: Bug 1355426 - Make tabbrowser binding call blurTab earlier when removing tab.

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

Comment 7

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

This is a pretty naive patch. I can't personally tell if it's making tab closing feel faster. I'll try to do some local measurements (since I don't believe any Talos tests would capture this). In the meantime, I'll push to try to see what automation thinks.

Comment 8

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

So I did some semi-scientific local measurements on my MBP with this patch, and it does seem to yield a small but significant improvement.

I used the Performance Timing API to add markers to removeTab, and the point where we finally switch a tab when the layers are ready in the async tab switcher. I also added a measure for those two markers. Then I opened up 11 tabs at mozilla.org, made sure they were all loaded, made my selected tab the first tab, ensured the markers / measurements were clear, and then closed each tab. When I was done, I gathered up my measurements.

Here's the patch I used in case anybody wants to repeat the experiment:

diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -2825,16 +2825,17 @@
         []
       </field>

       <method name="removeTab">
         <parameter name="aTab"/>
         <parameter name="aParams"/>
         <body>
           <![CDATA[
+            window.performance.mark("removeTabStart");
             if (aParams) {
               var animate = aParams.animate;
               var byMouse = aParams.byMouse;
               var skipPermitUnload = aParams.skipPermitUnload;
             }

             // Telemetry stopwatches may already be running if removeTab gets
             // called again for an already closing tab.
@@ -4337,16 +4338,22 @@
                   }
                 }

                 // This doesn't necessarily exist if we're a new window and haven't switched tabs yet
                 if (this.lastVisibleTab)
                   this.lastVisibleTab._visuallySelected = false;

                 this.visibleTab._visuallySelected = true;
+                window.performance.mark("removeTabEnd");
+                try {
+                  window.performance.measure("removeTab", "removeTabStart", "removeTabEnd");
+                } catch(e) {
+                  Components.utils.reportError(e);
+                }
               }

               this.lastVisibleTab = this.visibleTab;
             },

             assert(cond) {
               if (!cond) {
                 dump("Assertion failure\n" + Error().stack);


And here are the results.

Without patch:

Measurement (in ms):
41.39499999998952
33.63500000000931
34.13000000000466
59.47500000000582
34.77499999999418
34.01000000000931
35.36499999999069
62.460000000020955
33.95999999999185
35.44000000000233
39.889999999984866

Average: 40.41227272727304
Median: 35.44000000000233


With patch:

Measurement (in ms):
33.87000000000262
38.75
32.05500000000029
31.340000000003783
39.24500000000262
32.150000000001455
32.68999999999505
38.20999999999913
32.125
38.36000000000058
33.154999999998836

Average: 34.72272727272767
Median: 33.87000000000262


This isn't too surprising - we're causing the window of time between requesting the next tab, and the start of the removeTab call to shrink. This seems to gain us a few milliseconds.

So it's not a huge win, but it's not nothing. If automation likes it, I think we should land it.

Comment 9

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

Attachment: Bug 1355426 - Update browser_hide_removing.js to wait for TabClose instead of TabSelect.

browser_hide_removing.js assumes that when a tab is closed, the next tab to be displayed will
fire its TabSelect event _after_ the closed tab has been marked as "closed". Bug 1355426 makes
it so that the tab selection occurs earlier, so this test has been updated to use the TabClose
event instead.

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

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Redirected to Dao since I think his comments in comment 1 point to him already having an opinion on the changes here and I want to give him a chance to review this.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8892128 [details]
Bug 1355426 - Make tabbrowser binding call blurTab earlier when removing tab.

https://reviewboard.mozilla.org/r/163116/#review168844

::: browser/base/content/tabbrowser.xml:2859
(Diff revision 1)
>              if (!animate &&
>                  aTab.closing) {
>                this._endRemoveTab(aTab);
>                return;
>              }
>  

_endRemoveTab will call _blurTab again (it has to), so you can move your _blurTab call at least after the above block.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8892128 [details]
Bug 1355426 - Make tabbrowser binding call blurTab earlier when removing tab.

https://reviewboard.mozilla.org/r/163116/#review168864

::: browser/base/content/tabbrowser.xml:2850
(Diff revision 1)
>                TelemetryStopwatch.start("FX_TAB_CLOSE_TIME_ANIM_MS", aTab);
>                TelemetryStopwatch.start("FX_TAB_CLOSE_TIME_NO_ANIM_MS", aTab);
>              }
>              window.maybeRecordAbandonmentTelemetry(aTab, "tabClosed");
>  
> +            this._blurTab(aTab);

When testing the patch, I noticed that closing a tab that uses beforeunload makes the UI flicker because we select another tab, update the UI, and then obviously have to re-select the original tab for the prompt. This seems like a pretty big side effect.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

DevTools has a way of knowing what event listeners are on an object. Could we use that same API to know if a beforeunload event listener is attached and then skip this fast path in that case?

Comment 14

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #12)
> When testing the patch, I noticed that closing a tab that uses beforeunload
> makes the UI flicker because we select another tab, update the UI, and then
> obviously have to re-select the original tab for the prompt. This seems like
> a pretty big side effect.

Typo... I meant bad rather than big. I understand that beforeunload isn't the common case and we shouldn't over-optimize for it at the cost of slowing down non-beforeunload tabs, but I'm not convinced this patch makes the right trade-off.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/dom/events/nsIEventListenerService.idl is the API I have in mind.

Here's how DevTools uses it, http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/devtools/server/event-parsers.js#168-219

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8892214 [details]
Bug 1355426 - Update browser_hide_removing.js to wait for TabClose instead of TabSelect.

https://reviewboard.mozilla.org/r/163158/#review168936

Comment 17

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

(In reply to Dão Gottwald [::dao] from comment #12)
> When testing the patch, I noticed that closing a tab that uses beforeunload
> makes the UI flicker because we select another tab, update the UI, and then
> obviously have to re-select the original tab for the prompt. This seems like
> a pretty big side effect.

Thanks for the thorough testing, Dao. Will take a look at this.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> DevTools has a way of knowing what event listeners are on an object. Could
> we use that same API to know if a beforeunload event listener is attached
> and then skip this fast path in that case?

We have something like that for the remote browser case, and since that's what we're going to be shipping by default for 57, maybe we can optimize for that directly.

Comment 18

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

Comment on attachment 8892128 [details]
Bug 1355426 - Make tabbrowser binding call blurTab earlier when removing tab.

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

Comment 19

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

Comment on attachment 8892214 [details]
Bug 1355426 - Update browser_hide_removing.js to wait for TabClose instead of TabSelect.

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

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8892128 [details]
Bug 1355426 - Make tabbrowser binding call blurTab earlier when removing tab.

https://reviewboard.mozilla.org/r/163116/#review169020

::: browser/base/content/tabbrowser.xml:2927
(Diff revision 2)
> +                skipPermitUnload = !browser.frameLoader.tabParent ||
> +                                   !browser.frameLoader.tabParent.hasBeforeUnload;

nice!

Comment 21

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

Comment on attachment 8892128 [details]
Bug 1355426 - Make tabbrowser binding call blurTab earlier when removing tab.

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

Comment 22

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

Comment on attachment 8892214 [details]
Bug 1355426 - Update browser_hide_removing.js to wait for TabClose instead of TabSelect.

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

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 8892128 [details]
Bug 1355426 - Make tabbrowser binding call blurTab earlier when removing tab.

https://reviewboard.mozilla.org/r/163116/#review169140

::: browser/base/content/tabbrowser.xml:2920
(Diff revision 3)
>            <![CDATA[
>              if (aTab.closing ||
>                  this._windowIsClosing)
>                return false;
>  
> +            let skipPermitUnload = aSkipPermitUnload;

I think this is slightly cleaner:

let checkPermitUnload = !aSkipPermitUnload &&
                        !aAdoptedByTab &&
                        aTab.linkedPanel &&
                        !aTab._pendingPermitUnload;

::: browser/base/content/tabbrowser.xml:2923
(Diff revision 3)
>                return false;
>  
> +            let skipPermitUnload = aSkipPermitUnload;
>              var browser = this.getBrowserForTab(aTab);
>  
> +            if (!skipPermitUnload) {

and:

  if (checkPermitUnload && browser.isRemoteBrowser) {
    checkPermitUnload = browser.frameLoader.tabParent &&
                        browser.frameLoader.tabParent.hasBeforeUnload;
  }

::: browser/base/content/tabbrowser.xml:2955
(Diff revision 3)
>                  return false;
>                }
>              }
>  
> +            if (!aAdoptedByTab) {
> +              this._blurTab(aTab);

Is the !aAdoptedByTab check actually needed? I don't think it should be..

Comment 24

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

Comment on attachment 8892128 [details]
Bug 1355426 - Make tabbrowser binding call blurTab earlier when removing tab.

https://reviewboard.mozilla.org/r/163116/#review169140

> Is the !aAdoptedByTab check actually needed? I don't think it should be..

You're right - I didn't need this, but I think without it, I end up racing inside browser_bug495058.js, which is (for some reason) waiting for MozAfterPaint in the non-remote browser case. I'll re-jig that test in a separate patch in this series.

Comment 25

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

Comment on attachment 8892128 [details]
Bug 1355426 - Make tabbrowser binding call blurTab earlier when removing tab.

https://reviewboard.mozilla.org/r/163116/#review169140

> and:
> 
>   if (checkPermitUnload && browser.isRemoteBrowser) {
>     checkPermitUnload = browser.frameLoader.tabParent &&
>                         browser.frameLoader.tabParent.hasBeforeUnload;
>   }

I agree, this is much cleaner. Thank you!

Comment 26

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

Attachment: Bug 1355426 - Fix browser_bug495058.js to not wait for MozAfterPaint in non-remote browser case.

I'm not sure why the non-remote browser case needed to wait for MozAfterPaint. In
the remote browser case, we do wait for MozAfterPaint in the content area before
focusing the URL bar, but in the non-e10s case, we just focus it right away.

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

Comment 27

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

Comment on attachment 8892128 [details]
Bug 1355426 - Make tabbrowser binding call blurTab earlier when removing tab.

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

Comment 28

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

Comment on attachment 8892214 [details]
Bug 1355426 - Update browser_hide_removing.js to wait for TabClose instead of TabSelect.

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

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8892214 [details]
Bug 1355426 - Update browser_hide_removing.js to wait for TabClose instead of TabSelect.

https://reviewboard.mozilla.org/r/163158/#review170022

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8893022 [details]
Bug 1355426 - Fix browser_bug495058.js to not wait for MozAfterPaint in non-remote browser case.

https://reviewboard.mozilla.org/r/164034/#review170024

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30d54e94a67e
Make tabbrowser binding call blurTab earlier when removing tab. r=dao
https://hg.mozilla.org/integration/autoland/rev/f418889f0d19
Update browser_hide_removing.js to wait for TabClose instead of TabSelect. r=jaws
https://hg.mozilla.org/integration/autoland/rev/39dde16cec1d
Fix browser_bug495058.js to not wait for MozAfterPaint in non-remote browser case. r=jaws

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/30d54e94a67e
https://hg.mozilla.org/mozilla-central/rev/f418889f0d19
https://hg.mozilla.org/mozilla-central/rev/39dde16cec1d

Comment 33

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [::dao] from comment #14)
> (In reply to Dão Gottwald [::dao] from comment #12)
> > When testing the patch, I noticed that closing a tab that uses beforeunload
> > makes the UI flicker because we select another tab, update the UI, and then
> > obviously have to re-select the original tab for the prompt. This seems like
> > a pretty big side effect.
> 
> Typo... I meant bad rather than big. I understand that beforeunload isn't
> the common case and we shouldn't over-optimize for it at the cost of slowing
> down non-beforeunload tabs, but I'm not convinced this patch makes the right
> trade-off.

beforeunload is the case where this bug had the biggest potential for a perceived performance improvement. What I see very often when closing tabs is that some tabs have beforeunload event listeners that force a round trip to the (busy/slow) content process before we can actually close the tab. Showing the beforeunload prompt is a rare case; waiting for the content process due to a beforeunload listener that won't show a prompt is common (for me at least).

I was hoping this bug would fix the common case, and cause flicker in the uncommon case, in a way that would make the user think the site is getting in the way: the perception should be "this site is a pain" rather than "Firefox is slow to close tabs."

If we want to avoid the beforeunload flicker when the content process happen to be very responsive, I think we could wait 50ms or 100ms before switching the selected tab when there's a beforeunload event listener.

Comment 34

[Adrian Florinescu [:aflorinescu]]

I will mark this bug as verified for the common tab closing scenarios and handle verification of the more complex scenarios containing beforeunload event listeners + busy content processes into bug 1388387.

verified on:
Ubuntu 16.04 x64, OSX 10.12, Windows 10 x64 
57.0a1 20170820100343