Closed Bug 669272 Opened 8 years ago Closed Last year

Session Restore moves windows around unnecessarily

Categories

(Firefox :: Session Restore, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED INCOMPLETE
Firefox 9

People

(Reporter: int3, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p5])

Attachments

(1 file, 2 obsolete files)

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'.
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
Attached patch Patch v1 (obsolete) — Splinter Review
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #545753 - Attachment is obsolete: true
Attachment #545754 - Flags: review?(paul)
Considering comment!, setting resolution to Resolved WFM.
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
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.
Assignee: nobody → jezreel
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WORKSFORME → ---
Ok. Waiting for the patch.
I have tried on the latest nightly and is still WFM.
The patch has already been submitted and is awaiting review. See above.
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
Attachment #545754 - Flags: review?(paul) → review+
Attached patch Patch v2Splinter Review
Patch with comments addressed.
Attachment #545754 - Attachment is obsolete: true
Keywords: checkin-needed
Attempt 2 (one of the other checkin-neededs broke that try push):
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=189de6fe4c76
http://hg.mozilla.org/mozilla-central/rev/5312c652c469
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
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
(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.
(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.
Flags: in-testsuite-
Considering comment14, comment15 and comment16, setting resolution to Verified Fixed.
Status: RESOLVED → VERIFIED
Depends on: 694378
Depends on: 712763
Backed out:

https://hg.mozilla.org/integration/fx-team/rev/66295fcfee13
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
jez, anyone,
do you still see this?  or Bug 1235231?
personally, I haven't. on Windows anyway
Assignee: jezreel → nobody
Blocks: ss-perf
Status: REOPENED → NEW
Whiteboard: [fxperf]
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.
Flags: needinfo?(mdeboer)
Whiteboard: [fxperf] → [fxperf:p5]
You're right, let's get it from the list.
Status: NEW → RESOLVED
Closed: 8 years agoLast year
Flags: needinfo?(mdeboer)
Resolution: --- → INCOMPLETE
s/get/remove/
You need to log in before you can comment on or make changes to this bug.