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

REOPENED
Unassigned

Status

()

defect
REOPENED
7 years ago
Last year

People

(Reporter: glandium, Unassigned)

Tracking

({perf})

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1][fxperf:p3])

Attachments

(1 obsolete attachment)

Reporter

Description

7 years ago
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

7 years ago
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.
Component: Private Browsing → Session Restore

Comment 2

7 years ago
(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

7 years ago
(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>
Duplicate of this bug: 695823
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.
Assignee: enndeakin → nobody
Per-window private browsing landed so this is not an issue anymore.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
(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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Enabling private browsing closes all tabs one by one, triggering synchronous reflow each time → setBrowserState closes all tabs one by one, triggering synchronous reflow each time

Comment 8

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

6 years ago
(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>
Assignee: nobody → smacleod
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).
Attachment #784724 - Flags: feedback?(ttaubert)
(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").
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.
Summary: setBrowserState closes all tabs one by one, triggering synchronous reflow each time → SessionStore closes all tabs one by one, triggering synchronous reflow each time
Attachment #784724 - Flags: feedback?(ttaubert)
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.
(this patch was moved to bug 900803)
Attachment #784724 - Attachment is obsolete: true
Removing myself as assignee as I'm no longer working on this
Assignee: smacleod → nobody
Flags: firefox-backlog?
OS: Linux → All
Hardware: x86_64 → All
This is in the same state as bug 900803 and doesn't seem to be actionable enough to be put in the backlog.
Flags: firefox-backlog? → firefox-backlog-
Comment 5 seems actionable.
Flags: needinfo?(ttaubert)
(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.
Flags: needinfo?(ttaubert)
A close-/shutdown-bug can't block an startup-bug, or?
No longer blocks: 582005
Whiteboard: [Snappy:P1] → [Snappy:P1][fxperf]
Whiteboard: [Snappy:P1][fxperf] → [Snappy:P1][fxperf:p3]
You need to log in before you can comment on or make changes to this bug.