816607 - SessionStore closes all tabs one by one, triggering synchronous reflow each time

Status: RESOLVED WORKSFORME

(Firefox :: Session Restore, defect)

Description

[Mike Hommey [:glandium]]

See this profile: http://bit.ly/WxccDR

This comes from this code in restoreWindow in SessionStore.jsm:
    // when overwriting tabs, remove all superflous ones
    if (aOverwriteTabs && newTabCount < openTabCount) {
      Array.slice(tabbrowser.tabs, newTabCount, openTabCount)
           .forEach(tabbrowser.removeTab, tabbrowser);
    }

Comment 1

[(no longer active)]

The global PB mode will hopefully go away soon (perhaps Firefox 20) so this is not worth fixing I don't think.  That said, this is really a sessionstore bug.  PB is only a consumer of the setBrowserState API which does this.

Comment 2

[(dormant account)]

(In reply to Ehsan Akhgari [:ehsan] from comment #1)
> The global PB mode will hopefully go away soon (perhaps Firefox 20) so this
> is not worth fixing I don't think.  That said, this is really a sessionstore
> bug.  PB is only a consumer of the setBrowserState API which does this.

We should fix this in sessionstore, there are likely to be other consumers of this. No reason to keep bad code around.

Comment 3

[(no longer active)]

(In reply to comment #2)
> (In reply to Ehsan Akhgari [:ehsan] from comment #1)
> > The global PB mode will hopefully go away soon (perhaps Firefox 20) so this
> > is not worth fixing I don't think.  That said, this is really a sessionstore
> > bug.  PB is only a consumer of the setBrowserState API which does this.
> 
> We should fix this in sessionstore, there are likely to be other consumers of
> this. No reason to keep bad code around.

There are no other consumers on m-c (besides tests of course), but there are definitely a bunch of add-ons which use this API.  <https://mxr.mozilla.org/addons/search?string=setBrowserState>

Comment 5

[:Gavin Sharp [email: [e-mail]]]

The code tries to re-use the current window, I guess potentially to avoid to much visual noise when resetting a session. That seems misguided - it would be simpler to just always use new windows, to avoid having to close many tabs individually.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Per-window private browsing landed so this is not an issue anymore.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> Per-window private browsing landed so this is not an issue anymore.

The API still has this problem, right? I think we should fix it even if it isn't used by PB.

Comment 8

[(no longer active)]

(In reply to comment #7)
> (In reply to Tim Taubert [:ttaubert] from comment #6)
> > Per-window private browsing landed so this is not an issue anymore.
> 
> The API still has this problem, right? I think we should fix it even if it
> isn't used by PB.

It is currently unused outside of the tests.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

If we don't think the API is worth maintaining we should just remove it - but I don't think we're going to do that, because it's useful to add-ons and it's already been exposed to them. So we should maintain it.

Comment 10

[(no longer active)]

(In reply to comment #9)
> If we don't think the API is worth maintaining we should just remove it - but I
> don't think we're going to do that, because it's useful to add-ons and it's
> already been exposed to them. So we should maintain it.

Sigh.  There are indeed a few add-ons which use it: <https://mxr.mozilla.org/addons/search?string=setBrowserState>

Comment 11

[Steven MacLeod [:smacleod]]

Attachment: Patch - Make restoreWindow create tabs all at once

This is a preliminary patch which has |restoreWindow()| create and insert tabs all at once. On large sessions with lots of tabs I see about a 50% reduction in FX_SESSION_RESTORE_RESTORE_WINDOW_MS (From ~5000ms to ~2500ms).

Comment 12

[Dão Gottwald [:dao]]

(In reply to Steven MacLeod [:smacleod] from comment #11)
> Created attachment 784724 [details] [diff] [review]
> Patch - Make restoreWindow create tabs all at once

Doesn't sound like it belongs in this bug ("setBrowserState closes all tabs one by one").

Comment 13

[:Gavin Sharp [email: [e-mail]]]

OK, per IRL discussion with Tim and Steven, re-summarizing this bug to more accurately fit the problem we should address here. The inefficient tab closing is not isolated to setBrowserState, it also affects the other SessionStore paths. And the "inefficiently closing tabs" issue is different enough that it makes sense to track it separately from "inefficiently opening tabs" problem Steven's patch is solving, for which he'll file a separate bug.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 784724 [details] [diff] [review]
Patch - Make restoreWindow create tabs all at once

>+            let t = this._createTab(aURI, this.tabContainer,
>+                                    {animate: animate});

_createTab doesn't expect this second argument.

>             if (animate) {
>-              mozRequestAnimationFrame(function () {
>-                this.tabContainer._handleTabTelemetryStart(t, aURI);
>-
>-                // kick the animation off
>-                t.setAttribute("fadein", "true");
>-
>-                // This call to adjustTabstrip is redundant but needed so that
>-                // when opening a second tab, the first tab's close buttons
>-                // appears immediately rather than when the transition ends.
>-                if (this.tabs.length - this._removingTabs.length == 2)
>-                  this.tabContainer.adjustTabstrip();
>-              }.bind(this));
>+              this._animateTab(t, aURI);
>             }

>+      <method name="_animateTab">
>+        <parameter name="aTab"/>
>+        <parameter name="aURI"/>
>+        <body>
>+          <![CDATA[
>+            mozRequestAnimationFrame(function () {
>+              this.tabContainer._handleTabTelemetryStart(aTab, aURI);
>+
>+              // kick the animation off
>+              aTab.setAttribute("fadein", "true");
>+
>+              // This call to adjustTabstrip is redundant but needed so that
>+              // when opening a second tab, the first tab's close buttons
>+              // appears immediately rather than when the transition ends.
>+              if (this.tabs.length - this._removingTabs.length == 2)
>+                this.tabContainer.adjustTabstrip();
>+            }.bind(this));
>+          ]]>
>+        </body>
>+      </method>

Unrelated change?

>+            // inject the new DOM nodes
>+            this.tabContainer.appendChild(tabsFragment);
>+            this.mPanelContainer.appendChild(browsersFragment);
>+            this.tabContainer.updateVisibility();
>+
>+            for (let i = 0; i < tabs.length; i++) {
>+              let t = tabs[i], b = browsers[i];
>+
>+              // wire up a progress listener for the new browser object.
>+              let tabListener = this.mTabProgressListener(t, b, true);
>+              const filter = Components.classes["@mozilla.org/appshell/component/browser-status-filter;1"]
>+                                       .createInstance(Components.interfaces.nsIWebProgress);
>+              filter.addProgressListener(tabListener, Components.interfaces.nsIWebProgress.NOTIFY_ALL);
>+              b.webProgress.addProgressListener(filter, Components.interfaces.nsIWebProgress.NOTIFY_ALL);
>+              this.mTabListeners[startPosition + i] = tabListener;
>+              this.mTabFilters[startPosition + i] = filter;
>+
>+              b.droppedLinkHandler = handleDroppedLink;
>+
>+              // Dispatch a new tab notification.  We do this once we're
>+              // entirely done, so that things are in a consistent state
>+              // even if the event listener opens or closes tabs.
>+              let evt = document.createEvent("Events");
>+              evt.initEvent("TabOpen", true, false);
>+              t.dispatchEvent(evt);

You're dispatching TabOpen for the individual tabs at a point where you already added various other tabs that didn't get the TabOpen event yet. That's basically breaking the API.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(this patch was moved to bug 900803)

Comment 16

[Steven MacLeod [:smacleod]]

Removing myself as assignee as I'm no longer working on this

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

This is in the same state as bug 900803 and doesn't seem to be actionable enough to be put in the backlog.

Comment 18

[Dão Gottwald [:dao]]

Comment 5 seems actionable.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #18)
> Comment 5 seems actionable.

Well, yes. The question is whether the average user has enough tabs in a single window so that the overhead of closing and opening a new window is an acceptable tradeoff. If you have a window with let's say 10 tabs and you restore one with 2 tabs then we'll close 8 of them. The average user won't ever hit this case, esp. not at startup or even when undo-closing a window so this doesn't seem like something worth spending time on.

Comment 20

[Tobias B. Besemer [:BesTo] (QA)]

A close-/shutdown-bug can't block an startup-bug, or?

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 12 votes.
:dao, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 22

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Comment 23

[:Gijs (he/him)]

Sync style reflows when closing a tab are still there, and I can't find another extant bug for this, so I filed bug 1822532 for it.

AFAICT we don't really use overwriteTabs anymore, except on startup session restore - in which case there is only 1 tab in the window, which is selected, and doesn't get removed. So I'm going to close this out. If there is still a perf issue here we should have a new bug with more specifics on how to repro and what exactly the issue is.