342471 - [sessionstore] urlbar of first tab focused after restoring a session if homepage is about:blank

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[logan]

If browser.startup.homepage is about:blank, the urlbar of the first tab is focused when restoring a session.

http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#1001

1001   var element;
1002   if (gIsLoadingBlank && gURLBar && !gURLBar.hidden && !gURLBar.parentNode.parentNode.collapsed)
1003     element = gURLBar;
1004   else
1005     element = content;

<dietrich_> logan: adding a !SessionStartup.doRestore() to that line fixes it

Comment 1

[Simon Bünzli]

This has been introduced when splitting the SessionStore service in two. SessionStoreService._isCmdLineEmpty (which removes "about:blank" from the window's arguments) was supposed to be called before BrowserStartup.

The problem with Dietrich's suggestion is however that we want this behavior only for the very first window. It seems to me that nsSessionStartup will have to wait for the first browser window (observing domwindowopened) and clean it's first argument as needed. Should we do that, we could also initialize the SessionStore directly from SessionStartup...

If we don't, please remember me to file a bug to clean up _isCmdLineEmpty (which should only check .arguments[0] and compare it against nsIBrowserHandler::defaultArgs).

Comment 2

[Simon Bünzli]

Attachment: fix

This patch removes the url(s) to load from the first browser window's arguments before they are retrieved. It also cleans up the test for whether a window is supposed to load the default homepage(s) by comparing the only the first argument (the others are for charset, POSTDATA, and further arguments) with nsIBrowserHandler.defaultArgs (which returns the url(s) to be loaded into a clean window in nsBrowserContentHandler.js).

Comment 3

[Simon Bünzli]

*** Bug 344456 has been marked as a duplicate of this bug. ***

Comment 4

[Simon Bünzli]

Without this patch, the user's home pages will start loading before SessionStore replaces them.

Comment 5

[Nickolay_Ponomarev]

Does this patch prevent the homepage overrides (e.g. "upgraded" page) from loading? Is it intentional/desired?

Comment 6

[logan]

Do you need to set a homepage for 3?

Comment 7

[Simon Bünzli]

(In reply to comment #5)
> Does this patch prevent the homepage overrides (e.g. "upgraded" page) from
> loading? Is it intentional/desired?

No, the homepage overrides should continue loading as expected (since they're only returned once for defaultArgs - which defaults to the normal homepage(s) on subsequent accesses).

Comment 8

[Nickolay_Ponomarev]

Comment on attachment 227772 [details] [diff] [review]
fix

>+  /**
>+   * Prevents the first browser window to load the homepage(s)
>+   * @returns bool
>+   */
>+  _isBrowserWindow: function sss_isBrowserWindow(aWindow) {
> [...]
>+    if (aWindow.arguments && aWindow.arguments[0] &&
>+        aWindow.arguments[0] == defaultArgs)
>+      aWindow.arguments[0] = null;

I definitely wouldn't expect a function named _isBrowserWindow to do things like this to the window it's called on :) Also in the jsdoc comment s/to load/from loading/, right?

Comment 9

[Simon Bünzli]

(In reply to comment #8)
> I definitely wouldn't expect a function named _isBrowserWindow to do things
> like this to the window it's called on :)

I'll refactor the code into a more general _onWindowLoaded which will include the check for the browser window, remove the defaultArgs from the first browser window and remove the observer when we're done.

> Also in the jsdoc comment s/to
> load/from loading/, right?

Right.

Comment 10

[Simon Bünzli]

Attachment: fix

Comment 11

[Mike Connor [:mconnor]]

This cleans up a lot of strange cases, we should get this in place.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 229789 [details] [diff] [review]
fix


>+  /**
>+   * Removes the default arguments from the first browser window
>+   * (and removes removes the "domwindowopened" observer afterwards)
>+   */
>+  _onWindowOpened: function sss_onWindowOpened(aWindow) {
>+    var wType = aWindow.document.documentElement.getAttribute("windowtype");
>+    if (wType != "navigator:browser")
>+      return;

nit: remove one of the "removes" :)

>Index: browser/base/content/browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>retrieving revision 1.660
>diff -u -8 -d -p -r1.660 browser.js
>--- browser/base/content/browser.js	18 Jul 2006 23:24:59 -0000	1.660
>+++ browser/base/content/browser.js	19 Jul 2006 07:41:03 -0000
>@@ -1096,34 +1096,24 @@ function delayedStartup()
>   if (!getBoolPref("ui.click_hold_context_menus", false))
>     SetClickAndHoldHandlers();
> #endif
> 
>   // Initialize the microsummary service by retrieving it, prompting its factory
>   // to create its singleton, whose constructor initializes the service.
>   Cc["@mozilla.org/microsummary/service;1"].getService(Ci.nsIMicrosummaryService);
> 
>-  // initialize the session-restore service
>-  var ssEnabled = true;
>-  var prefBranch = Cc["@mozilla.org/preferences-service;1"].
>-                   getService(Ci.nsIPrefBranch);
>-  try {
>-    ssEnabled = prefBranch.getBoolPref("browser.sessionstore.enabled");
>-  } catch (ex) {}
>-
>-  if (ssEnabled) {
>-    var wType = window.document.documentElement.getAttribute("windowtype");
>-    if (wType == "navigator:browser") {
>-      try {
>-        var ss = Cc["@mozilla.org/browser/sessionstore;1"].
>-                 getService(Ci.nsISessionStore);
>-        ss.init(window);
>-      } catch(ex) {
>-        dump("nsSessionStore could not be initialized: " + ex + "\n");
>-      }
>+  // initialize the session-restore service (in case it's not already running)
>+  if (document.documentElement.getAttribute("windowtype") == "navigator:browser") {
>+    try {
>+      var ss = Cc["@mozilla.org/browser/sessionstore;1"].
>+               getService(Ci.nsISessionStore);
>+      ss.init(window);
>+    } catch(ex) {
>+      dump("nsSessionStore could not be initialized: " + ex + "\n");
>     }
>   }
> 
>   // browser-specific tab augmentation
>   AugmentTabs.init();
> 
>   // set up history menu
>   HistoryMenu.init();

At first I thought removing the pref check was a problem. However, I think the number of dependencies on the service make it not possible to turn off entirely. The original intent of the pref was to allow extensions to usurp the restoration functionality. However, they could flip the resume_from_crash pref to stop restoration while allowing the service to run. I'll file a follow-up bug to address the future of that pref.

r=me

Comment 13

[Simon Bünzli]

Attachment: fix (nit fixed)

Comment 14

[Simon Bünzli]

(In reply to comment #12)
> At first I thought removing the pref check was a problem. However, I think the
> number of dependencies on the service make it not possible to turn off
> entirely.

Removing the pref check in browser.js is a no-op since that same check is repeated in nsSessionStore.init where it IMO belongs. And I don't see any dependencies which would justify not allowing to disable SessionStore completely yet.

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to comment #14)
> (In reply to comment #12)
> > At first I thought removing the pref check was a problem. However, I think the
> > number of dependencies on the service make it not possible to turn off
> > entirely.
> 
> Removing the pref check in browser.js is a no-op since that same check is
> repeated in nsSessionStore.init where it IMO belongs. And I don't see any
> dependencies which would justify not allowing to disable SessionStore
> completely yet.
> 

Hm, yeah I guess extensions are able to hook in to all the right places to provide existing functionality. If an extension removes or disables functionality, then users can evaluate whether the trade-off was worth it or not.

Comment 16

[Dietrich Ayala (:dietrich)]

Checked in on trunk.

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 229902 [details] [diff] [review]
fix (nit fixed)

Problem: Aside from the focus edge-case, the user's homepage starts loading before the session is restored. This is visually disconcerting as well as a waste of resources.

Solution: Handle window arguments at the proper time (regressed in the fix for bug 337320).

Risk: Low

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229902 [details] [diff] [review]
fix (nit fixed)

a=drivers. Please land on the MOZILLA_1_8_BRANCH.

Comment 19

[Michael Kohler [:mkohler]]

VERIFIED with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090616 Minefield/3.6a1pre ID:20090616030754

Comment 20

[u279076]

in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7788