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

VERIFIED FIXED in Firefox 57

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: florian, Assigned: mconley)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-performance])

Attachments

(3 attachments)

Reporter

Description

2 years ago
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).
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.
Reporter

Comment 2

2 years ago
(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).
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-performance]
Reporter

Updated

2 years ago
No longer blocks: photon-performance-triage
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
Assignee

Updated

2 years ago
Flags: needinfo?(mconley)
Hey mmucci, when you have a sec, can you update the Photon Tracking spreadsheet and pin this one on me?
Assignee: nobody → mconley
Flags: needinfo?(mconley) → needinfo?(mmucci)
(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.
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Flags: needinfo?(mmucci)
Priority: P2 → P1
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
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.
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.
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 hidden (mozreview-request)
Attachment #8892128 - Flags: review?(jaws) → review?(dao+bmo)
Attachment #8892214 - Flags: review?(jaws) → review?(dao+bmo)
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

2 years ago
mozreview-review
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.
Attachment #8892128 - Flags: review?(dao+bmo) → review+

Comment 12

2 years ago
mozreview-review
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.
Attachment #8892128 - Flags: review+
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?
(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 16

2 years ago
mozreview-review
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
Attachment #8892214 - Flags: review?(dao+bmo)
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

2 years ago
mozreview-review
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..
Attachment #8892128 - Flags: review?(dao+bmo) → review+
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Assignee

Comment 24

2 years ago
mozreview-review-reply
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.
Assignee

Comment 25

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

2 years ago
mozreview-review
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
Attachment #8892214 - Flags: review?(jaws) → review+

Comment 30

2 years ago
mozreview-review
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
Attachment #8893022 - Flags: review?(jaws) → review+

Comment 31

2 years ago
Pushed by mconley@mozilla.com:
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
Reporter

Comment 33

2 years ago
(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.
Assignee

Updated

2 years ago
Blocks: 1388387
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

2 years ago
Depends on: 1429659
No longer depends on: 1429659
Reporter

Updated

Last year
Depends on: 1444886
You need to log in before you can comment on or make changes to this bug.