Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Nightly is focusing last app tab instead of homepage

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Session Restore
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Terepin, Assigned: andreshm)

Tracking

({regression})

Trunk
Firefox 17
regression
Points:
---
Bug Flags:
in-litmus ?

Firefox Tracking Flags

(firefox15 unaffected, firefox16+ fixed)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0
Build ID: 20120705030540

Steps to reproduce:

I fired up Nightly.


Actual results:

Last app tab showed up.


Expected results:

Homepage should showed up.

I tried this in Safe mode.
(Reporter)

Comment 1

5 years ago
I suspect bug 681005 as a culprit, since it started yesterday. I might be wrong, though.
Blocks: 681005
Can you please post some more detailed steps to reproduce? Which tab was focused when you closed the browser?
(Reporter)

Comment 3

5 years ago
For example this one. I closed Nightly, start it again and last app tab was focused. In my case it's Twitter.
Not sure if this is maybe related to Windows 8? I'm not seeing this on Linux, last focused tab is focused on startup again. Can anyone else with Windows 8 confirm?
(Reporter)

Comment 5

5 years ago
I had this on Windows 7 as well.
Cannot reproduce on Windows 7.
(Reporter)

Comment 7

5 years ago
Is there a freeware software I can use to record video from desktop?
(Assignee)

Comment 8

5 years ago
I can reproduce it in Mac with the nightly build in http://nightly.mozilla.org/. The homepage tab is not being focus, but the other tabs (pinned or not) are not being loaded. 
However, it works all fine with my local compiled build with latest mozilla-central code.
Peter, I've used Jing in the past, but there are several options for screencasting: 

http://en.wikipedia.org/wiki/Comparison_of_screencasting_software

I was not able to confirm this on Windows 8 Release Preview Build 8400
(Reporter)

Comment 10

5 years ago
As you can see, Nightly will always focus last app tab, rather than first regular tab.
(Reporter)

Comment 11

5 years ago
Yeah, YouTube is probably better. Sorry.
(Assignee)

Updated

5 years ago
Assignee: nobody → andres
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 12

5 years ago
Created attachment 641209 [details] [diff] [review]
Patch v1

Updated the restore order code, now it's simpler to understand and fix this issue.
Attachment #641209 - Flags: review?(ttaubert)
Comment on attachment 641209 [details] [diff] [review]
Patch v1

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

Thanks for cleaning this up! Looks a lot better now. Still, we can have this a little easier. All the magic happening regarding the selected tab and its index should be simplified, that's quite hard to read. I described below how this could be done. All tests are passing locally. If you see a problem with the suggested code, please say so!

Also, we need a test. There should already be an existing test where you could just add this to. Alternatively you could of course just create a new one. I'll leave that up to you.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2843,5 @@
> +    let selectedTab;
> +    let selectedTabData;
> +    if (aSelectedTab-- && aTabs[aSelectedTab]) {
> +      selectedTab = aTabs.splice(aSelectedTab, 1)[0];
> +      selectedTabData = aTabData.splice(aSelectedTab, 1)[0];

I think we should just do:

> let selectedTab;
> if (aSelectedTab > 0 && aTabs[aSelectedTab]) {
>   selectedTab = aTabs[aSelectedTab];
> }

We shouldn't and don't have to modify aSelectedTab that way and can keep the selected tab in aTabs. So we don't need to keep track of the selected tab's index but can just use indexOf().

@@ +2848,5 @@
> +    }
> +
> +    // Store the pinned tabs and hidden tabs.
> +    let pinnedTabsCount = aTabData.filter(function (aData) aData.pinned).length;
> +    let hiddenTabsCount = aTabData.filter(function (aData) aData.hidden).length;

I don't think there's a perf advantage here if we iterate twice over aTabData. We can just remove this and the condition below.

@@ +2853,5 @@
> +    let pinnedTabs = [];
> +    let pinnedTabsData = [];
> +    let hiddenTabs = [];
> +    let hiddenTabsData = [];
> +    if (aTabs.length > 1 && (pinnedTabsCount || hiddenTabsCount)) {

Without pinnedTabsCount and hiddenTabsCount we can just remove this. If (aTabs.length == 0) we'll just not enter the loop.

@@ +2859,5 @@
>          if (aTabData[t].pinned) {
> +          pinnedTabs.unshift(aTabs.splice(t, 1)[0]);
> +          pinnedTabsData.unshift(aTabData.splice(t, 1)[0]);
> +          if (selectedTab && aSelectedTab > t)
> +            aSelectedTab--;

These two lines can be removed.

@@ +2864,5 @@
> +        } else if (aTabData[t].hidden) {
> +          hiddenTabs.unshift(aTabs.splice(t, 1)[0]);
> +          hiddenTabsData.unshift(aTabData.splice(t, 1)[0]);
> +          if (selectedTab && aSelectedTab > t)
> +            aSelectedTab--;

These two lines as well.

@@ +2870,5 @@
>        }
>      }
>  
> +    // Optimize the visible tabs only if there is a selected tab.
> +    if (selectedTab && aSelectedTab > 1) {

This must be then:

> if (selectedTab) {
>   let selectedTabIndex = aTabs.indexOf(selectedTab);
>   if (selectedTabIndex > 0) {

@@ +2875,5 @@
> +      let scrollSize = aTabBrowser.tabContainer.mTabstrip.scrollClientSize;
> +      let tabWidth = aTabs[0].getBoundingClientRect().width;
> +      // Max visible tabs includes the store selected tab that currently is
> +      // not in the tabs array. 
> +      let maxVisibleTabs = Math.ceil(scrollSize / tabWidth) - 1;

let maxVisibleTabs = Math.ceil(scrollSize / tabWidth);

(The selected tab is included in aTabs.)

@@ +2877,5 @@
> +      // Max visible tabs includes the store selected tab that currently is
> +      // not in the tabs array. 
> +      let maxVisibleTabs = Math.ceil(scrollSize / tabWidth) - 1;
> +
> +      if (maxVisibleTabs < aTabs.length && aSelectedTab > 1) {

if (maxVisibleTabs < aTabs.length)

@@ +2882,3 @@
>          let firstVisibleTab = 0;
> +        let nonVisibleTabsCount = aTabs.length - maxVisibleTabs;
> +        if (nonVisibleTabsCount > aSelectedTab) {

if (nonVisibleTabsCount >= selectedTabIndex) {

@@ +2882,5 @@
>          let firstVisibleTab = 0;
> +        let nonVisibleTabsCount = aTabs.length - maxVisibleTabs;
> +        if (nonVisibleTabsCount > aSelectedTab) {
> +          // Selected tab is leftmost since we scroll to it when possible.
> +          firstVisibleTab = aSelectedTab;

firstVisibleTab = selectedTabIndex;

@@ +2901,5 @@
> +    // Load the selected tab to the first position and select it.
> +    if (selectedTab) {
> +      aTabs.unshift(selectedTab);
> +      aTabData.unshift(selectedTabData);
> +      aTabBrowser.selectedTab = selectedTab;

This needs to be now:

> let index = aTabs.indexOf(selectedTab);
> aTabs = aTabs.splice(index, 1).concat(aTabs);
> aTabData = aTabData.splice(index, 1).concat(aTabData);
> aTabBrowser.selectedTab = selectedTab;
Attachment #641209 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 14

5 years ago
Created attachment 645577 [details] [diff] [review]
Patch v2

(In reply to Tim Taubert [:ttaubert] from comment #13)
> Comment on attachment 641209 [details] [diff] [review]
> Patch v1
> 
> If you see a problem with the suggested code, please say so!

I think it works fine. The case that could fail was when the selected tab is a pinned tab. Since, it's moved to the stored pinned tabs. But it's handled when optimizing the visible tabs with the following condition:

let selectedTabIndex = aTabs.indexOf(selectedTab);
if (selectedTabIndex > 0) {

> Also, we need a test. There should already be an existing test where you
> could just add this to. Alternatively you could of course just create a new
> one. I'll leave that up to you.

The test browser/components/sessionstore/test/browser_480148.js already handles these ordering cases. Do we need more tests?

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +2843,5 @@
> > +    let selectedTab;
> > +    let selectedTabData;
> > +    if (aSelectedTab-- && aTabs[aSelectedTab]) {
> > +      selectedTab = aTabs.splice(aSelectedTab, 1)[0];
> > +      selectedTabData = aTabData.splice(aSelectedTab, 1)[0];
> 
> I think we should just do:
> 
> > let selectedTab;
> > if (aSelectedTab > 0 && aTabs[aSelectedTab]) {
> >   selectedTab = aTabs[aSelectedTab];
> > }
> 
> We shouldn't and don't have to modify aSelectedTab that way and can keep the
> selected tab in aTabs. So we don't need to keep track of the selected tab's
> index but can just use indexOf(). 

We need to subtract one, in order to get the index in aTabs. Because, aSelectedTab is the index starting from 1 not 0. 0 means no selected tab and 1 the first tab. 
 
> This needs to be now:
> 
> > let index = aTabs.indexOf(selectedTab);
> > aTabs = aTabs.splice(index, 1).concat(aTabs);
> > aTabData = aTabData.splice(index, 1).concat(aTabData);
> > aTabBrowser.selectedTab = selectedTab;

We still need to verify that there is a selected tab. Otherwise do nothing. 
Also, I added a condition to switch the selected tab only if index > 0.
Attachment #641209 - Attachment is obsolete: true
Attachment #645577 - Flags: review?(ttaubert)
(In reply to Andres Hernandez [:andreshm] from comment #14)
> > Also, we need a test. There should already be an existing test where you
> > could just add this to. Alternatively you could of course just create a new
> > one. I'll leave that up to you.
> 
> The test browser/components/sessionstore/test/browser_480148.js already
> handles these ordering cases. Do we need more tests?

Well, when we broke the affected code that led to this bug we didn't know because there were no tests for it. So we should at least add another combination to browser_480148.js and make sure it fails without this patch applied.

> We need to subtract one, in order to get the index in aTabs. Because,
> aSelectedTab is the index starting from 1 not 0. 0 means no selected tab and
> 1 the first tab. 

I know, thanks to your comment. I just think we should probably not keep doing it that way as this special handling is only needed for storage reasons. The code is much easier to read if I don't have to subtract one in my mind every time I read it.

> We still need to verify that there is a selected tab. Otherwise do nothing.

Yes, I didn't mean to remove the condition, sorry I should've added it to the code sample.

> Also, I added a condition to switch the selected tab only if index > 0.

Hmmm. I think we can't assume that the first tab is always the currently selected tab. If we're overwriting a current session then the selected tab might actually be arbitrary.
Keywords: regression
Component: Untriaged → Session Restore
OS: Windows 8 → All
Hardware: x86_64 → All
Version: 16 Branch → Trunk
Comment on attachment 645577 [details] [diff] [review]
Patch v2

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

Thank you, this looks good to me. Holding off r+ until we have a test case that reproduces the issue and fails without the patch applied.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2840,5 @@
>      aTabBrowser, aTabs, aTabData, aSelectedTab) {
> +
> +    // Store the selected tab. Need to substract one to get the index in aTabs.
> +    let selectedTab;
> +    if (aSelectedTab-- && aTabs[aSelectedTab]) {

if (aSelectedTab > 0 ...) {

No need to modify the argument if we don't use it anywhere.

@@ +2892,5 @@
> +
> +    // Load the selected tab to the first position and select it.
> +    if (selectedTab) {
> +      let selectedTabIndex = aTabs.indexOf(selectedTab);
> +      if (selectedTabIndex > 0) {

I think we can't assume the the first tab is always the selected tab. This might only be true after startup but not when overwriting an existing/active session.
Attachment #645577 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 17

5 years ago
Created attachment 646367 [details] [diff] [review]
Patch v3

I'll requested a manual test, since startups can't be simulate on test.
Attachment #645577 - Attachment is obsolete: true
Attachment #646367 - Flags: review?(ttaubert)
Comment on attachment 646367 [details] [diff] [review]
Patch v3

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

Great work, thank you! Please push it to try so we can land it afterwards.
Attachment #646367 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 19

5 years ago
These are the STR:
1. Open Firefox.
2. Select 'Show my home page' in the 'When Firefox starts:' pref.
3. Open 3 pinned tabs and a couple of regular tabs.
4. Select a pinned tab.
5. Restart Firefox.

Expected result: The homepage should be opened and selected. The pinned tabs should opened but not selected.
Flags: in-litmus?
One last thing, can you please revert this change I requested:

(In reply to Tim Taubert [:ttaubert] from comment #16)
> > +    // Load the selected tab to the first position and select it.
> > +    if (selectedTab) {
> > +      let selectedTabIndex = aTabs.indexOf(selectedTab);
> > +      if (selectedTabIndex > 0) {
> 
> I think we can't assume the the first tab is always the selected tab. This
> might only be true after startup but not when overwriting an existing/active
> session.

I read it wrong, it's a valid optimization. Feel free to tell me when I'm wrong! :)
(Assignee)

Comment 21

5 years ago
Created attachment 646380 [details] [diff] [review]
Patch v4

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8ed2515fb628
Attachment #646367 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
> I read it wrong, it's a valid optimization. Feel free to tell me when I'm
> wrong! :)

Sure! The previous patch is updated.
(Reporter)

Comment 23

5 years ago
(In reply to Andres Hernandez [:andreshm] from comment #19)
> These are the STR:
> 1. Open Firefox.
> 2. Select 'Show my home page' in the 'When Firefox starts:' pref.
> 3. Open 3 pinned tabs and a couple of regular tabs.
> 4. Select a pinned tab.
> 5. Restart Firefox.
> 
> Expected result: The homepage should be opened and selected. The pinned tabs
> should opened but not selected.

Small corrections:
The selected tab is irrelevant, since after new start homepage should be always focused. 
Plus, after regular restart the previously selected tab should be focused again after startup. 
If you know what I mean.
(Assignee)

Comment 24

5 years ago
Try finished with oranges. I think non related to our bug.

See: https://tbpl.mozilla.org/?tree=Try&rev=8ed2515fb628
Unfortunately, the try server was reset and I think that's why I can't see any results here. Are you sure they were unrelated? We can always push to try again if you're unsure.
(Assignee)

Comment 26

5 years ago
These are the errors plus an intermittent.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug595601.js | Test timed out 

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug595601.js | restoring 3 tabs concurrently - Got 0, expected 3
I can't find any bugs related to an intermittently failing test for bug 595601. If that's really the case we need to look into why that suddenly started to fail.
(Assignee)

Comment 28

5 years ago
Created attachment 647676 [details] [diff] [review]
Patch v5

Debugging and looking at test, I found that the test was not pointing to the correct selected tab. 

Local tests are working.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=afc1990cbade
Attachment #646380 - Attachment is obsolete: true
Attachment #647676 - Flags: review?(ttaubert)
Comment on attachment 647676 [details] [diff] [review]
Patch v5

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

Good catch! What a stupid mistake. The try run looks very promising but let's wait for the Win XP builds to finish.
Attachment #647676 - Flags: review?(ttaubert) → review+
Yep, looks good. Great work!
(Assignee)

Comment 31

5 years ago
We wait for the in-litmus test first or can I mark it as checkin-needed ?
(In reply to Andres Hernandez [:andreshm] from comment #31)
> We wait for the in-litmus test first or can I mark it as checkin-needed ?

You can mark it as checkin-needed now.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Will check this in.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/fcb3934935e2
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/fcb3934935e2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 17
Duplicate of this bug: 786965
Why wasn't this uplifted to aurora? Can it still be uplifted to beta now?
status-firefox15: --- → unaffected
status-firefox16: --- → affected
tracking-firefox16: --- → ?
Alternatively, can bug 681005 be backed out from mozilla-beta?
Duplicate of this bug: 787125
It would be quite unpleasant, if this problem stays inside the final release - it harms all users using pinned tabs (quite huge number).
My preference would be to back out bug 681005. Tracking for release of FF16.
tracking-firefox16: ? → +
https://hg.mozilla.org/releases/mozilla-beta/rev/a3790585b2a2
status-firefox16: affected → disabled
status-firefox16: disabled → fixed
You need to log in before you can comment on or make changes to this bug.