893061 - When restoring a session, "about:home" is displayed in the selected tab before the tab is restored

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Johan C]

I believe this shouldn't happen?
Feels unnecessary to load "about:home" page if it's not supposed to be displayed in the tab. This potentially slows down startup?

STR:
  1. Open at least one tab (e.g. gmail.com).
  2. Set "When Nightly starts:" to "Show my windows ands tabs from last time"
  3. restart Nightly.

Actual:
  The "about:home"-page is displayed for a few seconds before gmail.com is restored.

Expected:
  One blank tab is displayed until the tab(s) can be restored.

Preferred?:
  Do not display one blank tab before restoring several tabs? (This might be more suited for a different bug, but I think it's a bit ugly to display one blank tabs before restoring the session. Especially if more than one tab is restored.)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: prevent the default homepage from loading if we're going to restore a session

This might have been broken ever since we started reading the session file asynchronously. Maybe since we use OS.File or the SessionWorker to do that. The problem is that the async read almost always finishes after the first browser windows has been opened. The domwindowopened observer in nsSessionStartup.js can therefore not null window.arguments[0] in time.

This is an optimization and we need the session file contents to do that. We should therefore null 'uriToLoad' at the latest point possible, shortly before loading the URL. If the session file has been read until then, great. If not we should just bail out and let the homepage load.

Comment 2

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

Comment on attachment 780029 [details] [diff] [review]
prevent the default homepage from loading if we're going to restore a session

This looks a lot cleaner!

One uber-nit:

windows.some(w => w.tabs.some(t => !t.pinned))

 will likely be more efficient than 

!windows.every(w => w.tabs.every(t => t.pinned))

 in the common case?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Oh, right. Good catch, indeed!

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: prevent the default homepage from loading if we're going to restore a session, v2

Addressed Gavin's comment.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 780038 [details] [diff] [review]
prevent the default homepage from loading if we're going to restore a session, v2

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+    if (!window.arguments || !window.arguments[0]) {
>+      return null;
>+    }

>+    if (uri == defaultArgs && sessionStartup.willOverrideHomepage) {
>+      return null;
>+    }

nit: not sure if it's common to omit braces around early returns, breaks etc., but local prevailing style is to omit braces around single lines anyway.

>@@ -23,16 +23,28 @@ interface nsISessionStartup: nsISupports
>   readonly attribute jsval state;
> 
>   /**
>    * Determine if session should be restored
>    */
>   boolean doRestore();

Should this comment also be changed like in nsSessionStartup.js?

>+  _doRestore: function () {
>     return this._sessionType == Ci.nsISessionStartup.RECOVER_SESSION ||
>            this._sessionType == Ci.nsISessionStartup.RESUME_SESSION;
>   },

Rename to _willRestore or something? _doRestore seems confusing, because it doesn't actually restore something.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #5)
> nit: not sure if it's common to omit braces around early returns, breaks
> etc., but local prevailing style is to omit braces around single lines
> anyway.

Ok.

> >   /**
> >    * Determine if session should be restored
> >    */
> >   boolean doRestore();
> 
> Should this comment also be changed like in nsSessionStartup.js?

Yeah, good idea.

> >+  _doRestore: function () {
> >     return this._sessionType == Ci.nsISessionStartup.RECOVER_SESSION ||
> >            this._sessionType == Ci.nsISessionStartup.RESUME_SESSION;
> >   },
> 
> Rename to _willRestore or something? _doRestore seems confusing, because it
> doesn't actually restore something.

Yeah I don't really like the name as well. I'll change it for the internal method and maybe we can get rid of the external method in the future anyway... I hope.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/8aa1d0e0025d

Comment 8

[Dão Gottwald [:dao]]

When exactly did this break? Does the patch need to be uplifted?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

As bug 342471 originally implemented it, I wonder if this has been broken for a really long time? Ever since we started using NetUtils.asyncCopy() to read the session file? Maybe it started to fail when we started using OS.File. Or when we introduced the SessionWorker...

Comment 10

[Dão Gottwald [:dao]]

Should we just uplift it to aurora then, just in case?

Comment 11

[Dão Gottwald [:dao]]

https://hg.mozilla.org/mozilla-central/rev/8aa1d0e0025d

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #10)
> Should we just uplift it to aurora then, just in case?

Yeah, let's do that.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 780038 [details] [diff] [review]
prevent the default homepage from loading if we're going to restore a session, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not sure, it's a long-standing regression.
User impact if declined: Homepage appears shortly before restoring a session. Can lead to a slightly slower startup and flickering.
Testing completed (on m-c, etc.): Will be in today's Nightly.
Risk to taking this patch (and alternatives if risky): Low risk. The alternative would be to just let it ride the trains.
String or IDL/UUID changes made by this patch: Added an attribute to nsISessionStartup.

Comment 14

[Dão Gottwald [:dao]]

Apparently the issue fixed here is also what caused the first Ctrl+N in any session to open a new window with a blank page rather than the home page. I saw this constantly on Windows and Linux and I'm fairly sure this isn't a super-old regression.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Oh, that's interesting and great. I was about to investigate this because it started to really bother me. It's not super-old but has been around for quite some time, afaict.

It all makes sense now as the blank page was caused by the nsISessionStartup domwindowopened observer for the "first" window that never got called. Thanks for bringing this up!

The issue mentioned by Dão in the previous comment is a really visible one and I think that would be another good reason for an Aurora uplift.

Comment 16

[bhavana bajaj [:bajaj]]

Comment on attachment 780038 [details] [diff] [review]
prevent the default homepage from loading if we're going to restore a session, v2

Given the user impact in comment# 25, looks good to uplift.
Also looks like UUID change is taken care to avoid addon compat issues.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/67d7fe31fa0f

Comment 19

[Johan C]

This is still happening, or it has regressed again.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

This might still happen at startup if your machine is really slow and it takes a long time to load the session from the sessionstore.js file. After all this is just an optimization. If we read the session in time before we load 'about:home' we can figure out if there's a reason to not load it (because we're going to restore something).

Comment 21

[Johan C]

(In reply to Tim Taubert [:ttaubert] from comment #20)
> This might still happen at startup if your machine is really slow and it
> takes a long time to load the session from the sessionstore.js file. After
> all this is just an optimization. If we read the session in time before we
> load 'about:home' we can figure out if there's a reason to not load it
> (because we're going to restore something).

My machine is far from slow, and the patch did work for a few days.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Can you find the first Nightly build that doesn't work for you anymore since that patch landed?

Comment 23

[Johan C]

(In reply to Tim Taubert [:ttaubert] from comment #22)
> Can you find the first Nightly build that doesn't work for you anymore since
> that patch landed?

I was mistaken. I've backtracked to the 2013-07-27 build and the start page does appear sometimes. I guess I wasn't attentive enough on subsequent restart after the patch landed.

While this is for another bug entirely; if at all possible, would it not be preferable if the session was restored earlier? Is there no way of telling if there are tabs to restore without having to load the entire session file at start-up? Firefox has for a long time/(always?) loaded a single tab before it restores the previous session.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Johan Charlez from comment #23)
> I was mistaken. I've backtracked to the 2013-07-27 build and the start page
> does appear sometimes. I guess I wasn't attentive enough on subsequent
> restart after the patch landed.

Yeah, I can reproduce it on my Windows machine, too. Sometimes the session file is read first, sometimes not.

> While this is for another bug entirely; if at all possible, would it not be
> preferable if the session was restored earlier? Is there no way of telling
> if there are tabs to restore without having to load the entire session file
> at start-up? Firefox has for a long time/(always?) loaded a single tab
> before it restores the previous session.

Not with the current session "storage", i.e. a json file. We start reading the session file early on a background thread to minimize the time it takes to paint the first window. I agree that it doesn't make sense to load about:home if we don't need it but making the window wait for the session file to be read would certainly be worse and make Firefox feel slower.

Comment 25

[Manoj]

You could use a transition effect to inform the user that Firefox is doing something when there is a session to load. Loading about:home and then switching to the page that actually should have been loaded is not an intuitive user experience.

Manoj

Comment 26

[Ioana (away)]

I couldn't reproduce on any machines the part of this issue where about:home is displayed before the session is loaded. As big a session I had, it always loaded immediately and fast (I don't have access to old machines, where this would have more chances of working worst). I did reproduce quite often the part where Ctrl/Cmd + N opens about:blank instead of about:home though.

I also tried to reproduce this issue on Firefox 24 beta 7 (Win7, Mac OSX 10.7.5, Ubuntu 13.04), but it didn't reproduce once.

According to the above comments, this is still an issue, but it has definitely improved (it is more difficult to reproduce it).

Comment 27

[Ioana (away)]

Also verified on Firefox 25:
Mac OS X 10.8.4 - 20130912004004
Ubuntu 13.04 - 20130911004006
Windows 7 - 20130912004004