669272 - Session Restore moves windows around unnecessarily

Status: RESOLVED INCOMPLETE

(Firefox :: Session Restore, defect)

Description

[Jez Ng [:int3]]

STR:

1. Set Preferences -> When Firefox starts -> Show my home page, with about:home as the home page
2. Open two firefox windows on different parts of the screen. Let us label the the two positions as A and B.
3. Quit firefox with the last focused window being located at position A.
4. Reopen firefox, and click on the 'Restore Previous Session' button that appears.

Actual Results:
When firefox is reopened, the new window is positioned at A. When the 'Restore Previous Session' button is clicked, that window is moved to B, and another window is opened at A. This new window grabs the focus.

Expected Results:
When firefox is reopened, the new window is positioned at A. When the 'Restore Previous Session' button is clicked, another window is opened at B. The window at A retains the focus throughout.

The current behavior produces a noticeable 'flicker' as the window at A is moved away and a new one created in its place. Also, since the user cannot effectively interact with the browser until the focus stops changing, and since creating new windows is a relatively slow step, this makes the restore process feel slower.

This behavior is also observed if one selects the preference 'Show my windows and tabs from last time'.

Comment 1

[Vlad [QA]]

WFM on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110705 Firefox/7.0a1.

When I've restored the session, the windows retained their original position and focus.

Also, could you see if the issue occurs if using Firefox in safe mode:
http://support.mozilla.com/kb/Safe+Mode

Could you see if the issue occurs if using an empty testing profile? (Don't install any addons into it)
http://support.mozilla.com/kb/Basic+Troubleshooting#w_8-make-a-new-profile

Comment 2

[Jez Ng [:int3]]

Attachment: Patch v1

Comment 3

[Jez Ng [:int3]]

Attachment: Patch v1

Comment 4

[Vlad [QA]]

Considering comment!, setting resolution to Resolved WFM.

Comment 5

[Jez Ng [:int3]]

Vlad: Sorry for not replying with more details -- yes, the issue occurs with a clean profile. The issue might not occur 100% of the time though -- I think there's the possibility that the windows indeed get restored without unnecessary movement; however, the code definitely doesn't guarantee that this happens. My patch should fix that.

Comment 6

[Vlad [QA]]

Ok. Waiting for the patch.
I have tried on the latest nightly and is still WFM.

Comment 7

[Jez Ng [:int3]]

The patch has already been submitted and is awaiting review. See above.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 545754 [details] [diff] [review]
Patch v1

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

Just a couple nits.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1474,5 @@
>      // the lastWindow.
>      let lastWindow = this._getMostRecentBrowserWindow();
>      let canUseLastWindow = lastWindow &&
>                             !lastWindow.__SS_lastSessionWindowID;
> +    let lastSessionFocusedWindow = null;

My gut wants to say that we don't actually need lastSessionFocusedWindow, but we do.

@@ +1518,5 @@
>          //        in _preWindowToRestoreInto will prevent most (all?) Panorama
>          //        weirdness but we will still merge other extData.
>          //        Bug 588217 should make this go away by merging the group data.
>          this.restoreWindow(windowToUse, { windows: [winState] }, canOverwriteTabs, true);
> +        if (i == 0) lastSessionFocusedWindow = windowToUse;

Nit: 2 line that if

@@ +1528,4 @@
>        }
>        else {
> +        let win = this._openWindowWithState({ windows: [winState] });
> +        if (i == 0) lastSessionFocusedWindow = win;

Nit: 2 line that if

@@ +2485,5 @@
>      var winData;
> +    if (!root.selectedWindow) {
> +      root.selectedWindow = 0;
> +    } else {
> +      root.windows.unshift(root.windows.splice(root.selectedWindow - 1, 1)[0]);

Add a comment here that this makes sure the selected window gets restored first

Comment 9

[Jez Ng [:int3]]

Attachment: Patch v2

Patch with comments addressed.

Comment 10

[Ed Morley [:emorley]]

In my queue :-)
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=feaf94ae787f

Comment 11

[Ed Morley [:emorley]]

Attempt 2 (one of the other checkin-neededs broke that try push):
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=189de6fe4c76

Comment 12

[Ed Morley [:emorley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5312c652c469

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/5312c652c469

Comment 14

[Vlad [QA]]

Hi guys.
I am trying to verify this on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110907 Firefox/9.0a1

Every thing works as expected except the fact that after step 4 from the description, I don't have the 'Restore Previous Session' button in the page, only in the menu. If have try this on a clean profile.

Is this the intended behavior?

Thanks

Comment 15

[u279076]

(In reply to Vlad [QA] from comment #14)
> Hi guys.
> I am trying to verify this on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6;
> rv:9.0a1) Gecko/20110907 Firefox/9.0a1
> 
> Every thing works as expected except the fact that after step 4 from the
> description, I don't have the 'Restore Previous Session' button in the page,
> only in the menu. If have try this on a clean profile.
> 
> Is this the intended behavior?
> 
> Thanks

This bug is more about the behaviour of windows when restored. Use whatever method you can to restore the session. It's not clear to me whether step 4 refers to the Restore Session link in the home page or the Restore Previous Session menuitem in the History menu.

Comment 16

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Vlad [QA] from comment #14)
> Is this the intended behavior?

No, but it's not directly related to this bug. It may be bug 682944.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #15)
> It's not clear to me whether step 4
> refers to the Restore Session link in the home page or the Restore Previous
> Session menuitem in the History menu.

For the record, it doesn't actually matter.

Comment 17

[Vlad [QA]]

Considering comment14, comment15 and comment16, setting resolution to Verified Fixed.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Backed out:

https://hg.mozilla.org/integration/fx-team/rev/66295fcfee13

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Back out is now on m-c:

https://hg.mozilla.org/mozilla-central/rev/66295fcfee13

Comment 20

[Wayne Mery (:wsmwk)]

jez, anyone,
do you still see this?  or Bug 1235231?
personally, I haven't. on Windows anyway

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Hey mikedeboer - do we know if this is still an issue? Given comment 20, perhaps we should close this as INCOMPLETE unless anybody can still reproduce it.

Comment 22

[Mike de Boer [:mikedeboer]]

You're right, let's get it from the list.

Comment 23

[Mike de Boer [:mikedeboer]]

s/get/remove/