1034534 - [Session Restore] Restore windows one by one

Status: NEW

(Firefox :: Session Restore, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

For the moment, we open all windows at once. I suspect that we could have better user-perceived results by changing the algorithm to

1/ restore the first window exclusively (where the first window is the one with the most recent `lastAccessed` tab);
2/ only once the first window is usable restore the second window;
3/ etc.

It is my hope that this will put users in front of a responsive browser faster, plus it will jank less during restoration. The drawback, of course, is that  windows accessed least recently will be longer to restore.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Not working on it atm, feel free to steal.

Comment 2

[Hans-Peter Jansen]

You all seem to suffer from a subset of https://bugzilla.mozilla.org/show_bug.cgi?id=372650 that nags Firefox power users since nearly a decade! 

I'm not kidding, this issue will celebrate its 9th birthday on the 5th of March this year (2016).

Comment 3

[palswim]

(In reply to Hans-Peter Jansen from comment #2)
> You all seem to suffer from a subset of
> https://bugzilla.mozilla.org/show_bug.cgi?id=372650 that nags Firefox power
> users since nearly a decade! 

Not quite. While Firefox may have an issue restoring windows to their proper locations or sizes, this bug deals with opening Firefox windows synchronously or asynchronously. These things happen at different parts of the code, so neither bug is a subset of the other.

Comment 4

[palswim]

Looking at SessionStore.jsm (in browser/components/sessionstore/), it appears we have a race condition between the restoreWindows function loop and the openWindow function that it calls. We want some way to open the next window after the shell finishes the creation of the previous window. Just to clarify, we do open the windows in order, but we leave it to the shell to handle the actual creation of the windows.

I found the event "SSWindowRestored", but I think that happens after we've finished the restoration of the tabs within the window, which would happen too late (slowly) for some users. I couldn't locate an event that would represent the completion of the shell window creation; does one exist or should I emit a new event in the initializeWindow function (or elsewhere)?

Comment 5

[Markus.N]

I have noticed a few things that argue for the race condition you mentioned.

Background info: I'm using Firefox on Arch Linux and I have multiple profiles (over 10) with at least 5 windows with 5 to 50 tabs each.

Most times it works well, and the windows open in correct order.

But when an add-on update occurs during start (and the addon's info page is opened in an additional tab), things start to screw up. The behaviour is completely timing-dependent. The info page opens in a random window, it looks like it uses the window that was just opened at the moment when the add-on has finished installing and triggers opening the info page. When opening this additional tab is fast, the window order often remains correct. But when it takes only a bit longer (because the internet connection or the server is slow at the moment), things go out of control. And as soon as one window gets out of timing, all subsequent windows are also affected.

An additional problem could be the fact that opening the addon's info page ignores my setting "Don't load tabs until selected".

My proposal for a fix is:

* At first, retrieve only the window count and positios from the session, ignore content.
* Open the windows one after another (synchroneously) without any content and place them to their coordinates. As no content is added, this should be fast enough for the most users not noticing at all that there are empty windows.
* After the windows are open, start to populate them with the tabs.
* Also delay all additional tabs (like the add-on info pages) until all windows are opened and placed. It could even be necessary to delay _everything_ _else_ until windows are opened and placed.
* Populating the windows with their content could possibly go in reverse order. This way, the last opened window (that will most probably displayed on top) will get it's content first. This also helps hiding the two-step approach from the user. And when he starts switching to another window, chances are good that it's also already populated.

Comment 6

[palswim]

Attachment: Proposed patch which delays each separate window opening

I added a quick-and-dirty fix for the session restore in that I added a 400ms delay to each window opening. I would prefer an approach with event handling, but I couldn't find the right event to which to attach.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8811072 [details] [diff] [review]
Proposed patch which delays each separate window opening

Review of attachment 8811072 [details] [diff] [review]:
-----------------------------------------------------------------

I assume you meant to r? instead of r+, right?

::: browser/components/sessionstore/SessionStore.jsm
@@ +3292,5 @@
> +           let window = self._openWindowWithState({ windows: [winData] });
> +           if (setFocus) {
> +             self.windowToFocus = window;
> +           }
> +        }, (400 * (w - 1)), this, (w == (root.selectedWindow - 1))); // 400ms delay

I doubt that 400ms guarantees anything. It might take longer to restore a single window.

Comment 8

[palswim]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #7)
> I doubt that 400ms guarantees anything. It might take longer to restore a
> single window.

The timeout doesn't guarantee anything, true, which is why I would prefer to have an event to handle. A single window will definitely take longer to restore, but we only need to open the window before the next window, not complete the restoration.

In fact, we could probably reduce the delay to 200ms or less. We've always had a race condition in the window restoration, but the introduction of a couple of lines of code after FF 41 took enough time to cause the erroneous window ordering.

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8811072 [details] [diff] [review]
Proposed patch which delays each separate window opening

Review of attachment 8811072 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for posting a patch here!

To guarantee that you've seen a window opened, you can use the following function:

```js
/**
 * @param win (optional)
 *        The window we should wait to have "domwindowopened" sent through
 *        the observer service for. If this is not supplied, we'll just
 *        resolve when the first "domwindowopened" notification is seen.
 * @return {Promise}
 *         A Promise which resolves when a "domwindowopened" notification
 *         has been fired by the window watcher.
 */
function domWindowOpened(win) {
  return new Promise(resolve => {
    function observer(subject, topic, data) {
      if (topic == "domwindowopened" && (!win || subject === win)) {
        let observedWindow = subject.QueryInterface(Ci.nsIDOMWindow);
        Services.ww.unregisterNotification(observer);
        resolve(observedWindow);
      }
    }
    Services.ww.registerNotification(observer);
  });
}
```

Which is about the fastest method to use from JS to know when any window has opened.

Also, please consider that you're adding asynchronous code/ behavior to the `restoreWindows` method, which may not be a problem per sé, but you do need to be aware of possible race conditions.

Comment 10

[Mike de Boer [:mikedeboer]]

One thing you might want to look into using is `Task.spawn` https://dxr.mozilla.org/mozilla-central/search?q=Task.spawn&redirect=true (without using yield as an iterator, but as a function taking a Generator and returning a Promise).

Comment 11

[palswim]

Attachment: Patch which introduces a wait upon the "domwindowopened" event

This patch does not contain code which I intend for Firefox to include and distribute. I have been using this patch every time I update Firefox to modify the SessionStore.jsm file from the omni.ja resource file (you also must remove the version in the omni.ja cache at omni.ja/jsloader/resource/app/modules/sessionstore/SessionStore.jsm).

Know of these issues before you apply this patch:
 - The windows will restore their size, but not their state (minimized, etc.) correctly.
 - I updated Firefox to 49, 50, 50.1 after I developed this patch and noticed a memory leak that would render Firefox very sluggish after about a week. I couldn't say whether this came from my patch, an extension, or Firefox itself, though I would lean toward an extension causing the problem.

For me, I preferred these aforementioned issues to the issue of non-sequential window restoration, so that's why I continue to use this patch.

Comment 12

[Markus.N]

While you're at it, you could fix another related issue:

When you start firefox using a profile with multiple windows, the info pages from updated addOn's appear in random windows. Probably it's always the last one that was opened before the addOn decided to open it's info page.

I think it would be good to wait for the session restore to be completed before opening any tabs that were not part of the session. You could even put them in a new window.

Comment 13

[Bao Quan [:beekill]]

Attachment: one-by-one.v1.patch

The patch here will restore a window after event SSWindowRestored of previous window is fired.

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8881210 [details] [diff] [review]
one-by-one.v1.patch

I understand this approach, but would like to wait until bug 1034036 is resolved as I think that has a higher priority and bigger impact.

Comment 15

[palswim]

Following some of the discussions of bug 1235231 and bug 1034036, we don't actually have to restore windows synchronously if we can use an OS API to re-order them after we finish opening them. So, restoring windows in a different order than we had the windows when we closed them is the issue, and restoring them one-by-one is one possible solution. Perhaps we can close this bug and focus the attention on bug 1235231 (after we fix the issues with the regression from the solution for bug 1034036, of course). In that case, bug 1235231 would not depend on this bug, but only bug 1034036.

Comment 16

[palswim]

(In reply to Mike de Boer [:mikedeboer] from comment #14)
> I understand this approach, but would like to wait until bug 1034036 is
> resolved as I think that has a higher priority and bigger impact.

It sounds like the resolution for Bug 1034036 is coming slower than we thought. If we have a potential resolution for this issue, couldn't we at least work to resolve this issue if we find that this fix is fairly straightforward?

Comment 17

[User Dderss]

So, no chance of solving the bug?

It blocks the random session windows restore bug, so it is VERY annoying.

Comment 18

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

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

For more information, please visit auto_nag documentation.

Comment 19

[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.