Last Comment Bug 771504 - Nightly is focusing last app tab instead of homepage
: Nightly is focusing last app tab instead of homepage
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Andres Hernandez [:andreshm]
:
Mentors:
http://www.youtube.com/watch?v=iL7590...
: 786965 787125 (view as bug list)
Depends on:
Blocks: 681005
  Show dependency treegraph
 
Reported: 2012-07-06 07:04 PDT by Peter Henkel [:Terepin]
Modified: 2012-09-24 13:22 PDT (History)
10 users (show)
andres: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed


Attachments
Patch v1 (7.35 KB, patch)
2012-07-11 14:51 PDT, Andres Hernandez [:andreshm]
ttaubert: feedback+
Details | Diff | Review
Patch v2 (7.21 KB, patch)
2012-07-24 16:40 PDT, Andres Hernandez [:andreshm]
ttaubert: feedback+
Details | Diff | Review
Patch v3 (7.19 KB, patch)
2012-07-26 15:31 PDT, Andres Hernandez [:andreshm]
ttaubert: review+
Details | Diff | Review
Patch v4 (7.22 KB, patch)
2012-07-26 15:54 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Review
Patch v5 (9.25 KB, patch)
2012-07-31 14:10 PDT, Andres Hernandez [:andreshm]
ttaubert: review+
Details | Diff | Review

Description Peter Henkel [:Terepin] 2012-07-06 07:04:36 PDT
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.
Comment 1 Peter Henkel [:Terepin] 2012-07-06 07:05:59 PDT
I suspect bug 681005 as a culprit, since it started yesterday. I might be wrong, though.
Comment 2 Tim Taubert [:ttaubert] 2012-07-06 07:07:46 PDT
Can you please post some more detailed steps to reproduce? Which tab was focused when you closed the browser?
Comment 3 Peter Henkel [:Terepin] 2012-07-06 07:38:35 PDT
For example this one. I closed Nightly, start it again and last app tab was focused. In my case it's Twitter.
Comment 4 Tim Taubert [:ttaubert] 2012-07-06 07:42:56 PDT
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?
Comment 5 Peter Henkel [:Terepin] 2012-07-06 07:43:40 PDT
I had this on Windows 7 as well.
Comment 6 Tim Taubert [:ttaubert] 2012-07-06 07:53:06 PDT
Cannot reproduce on Windows 7.
Comment 7 Peter Henkel [:Terepin] 2012-07-06 07:54:30 PDT
Is there a freeware software I can use to record video from desktop?
Comment 8 Andres Hernandez [:andreshm] 2012-07-06 13:48:25 PDT
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.
Comment 9 juan becerra [:juanb] 2012-07-06 14:13:24 PDT
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
Comment 10 Peter Henkel [:Terepin] 2012-07-07 09:38:58 PDT
As you can see, Nightly will always focus last app tab, rather than first regular tab.
Comment 11 Peter Henkel [:Terepin] 2012-07-09 11:17:23 PDT
Yeah, YouTube is probably better. Sorry.
Comment 12 Andres Hernandez [:andreshm] 2012-07-11 14:51:19 PDT
Created attachment 641209 [details] [diff] [review]
Patch v1

Updated the restore order code, now it's simpler to understand and fix this issue.
Comment 13 Tim Taubert [:ttaubert] 2012-07-24 06:04:17 PDT
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;
Comment 14 Andres Hernandez [:andreshm] 2012-07-24 16:40:37 PDT
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.
Comment 15 Tim Taubert [:ttaubert] 2012-07-25 00:24:29 PDT
(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.
Comment 16 Tim Taubert [:ttaubert] 2012-07-25 04:12:05 PDT
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.
Comment 17 Andres Hernandez [:andreshm] 2012-07-26 15:31:16 PDT
Created attachment 646367 [details] [diff] [review]
Patch v3

I'll requested a manual test, since startups can't be simulate on test.
Comment 18 Tim Taubert [:ttaubert] 2012-07-26 15:39:41 PDT
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.
Comment 19 Andres Hernandez [:andreshm] 2012-07-26 15:41:10 PDT
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.
Comment 20 Tim Taubert [:ttaubert] 2012-07-26 15:41:36 PDT
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! :)
Comment 21 Andres Hernandez [:andreshm] 2012-07-26 15:54:10 PDT
Created attachment 646380 [details] [diff] [review]
Patch v4

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8ed2515fb628
Comment 22 Andres Hernandez [:andreshm] 2012-07-26 15:56:24 PDT
> I read it wrong, it's a valid optimization. Feel free to tell me when I'm
> wrong! :)

Sure! The previous patch is updated.
Comment 23 Peter Henkel [:Terepin] 2012-07-26 16:28:23 PDT
(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.
Comment 24 Andres Hernandez [:andreshm] 2012-07-27 08:49:35 PDT
Try finished with oranges. I think non related to our bug.

See: https://tbpl.mozilla.org/?tree=Try&rev=8ed2515fb628
Comment 25 Tim Taubert [:ttaubert] 2012-07-27 10:17:49 PDT
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.
Comment 26 Andres Hernandez [:andreshm] 2012-07-27 12:51:31 PDT
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
Comment 27 Tim Taubert [:ttaubert] 2012-07-27 15:31:17 PDT
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.
Comment 28 Andres Hernandez [:andreshm] 2012-07-31 14:10:00 PDT
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
Comment 29 Tim Taubert [:ttaubert] 2012-08-01 02:18:10 PDT
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.
Comment 30 Tim Taubert [:ttaubert] 2012-08-01 08:24:41 PDT
Yep, looks good. Great work!
Comment 31 Andres Hernandez [:andreshm] 2012-08-01 09:33:11 PDT
We wait for the in-litmus test first or can I mark it as checkin-needed ?
Comment 32 Tim Taubert [:ttaubert] 2012-08-01 09:38:50 PDT
(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.
Comment 33 Tim Taubert [:ttaubert] 2012-08-01 10:25:42 PDT
Will check this in.
Comment 34 Tim Taubert [:ttaubert] 2012-08-01 10:28:08 PDT
https://hg.mozilla.org/integration/fx-team/rev/fcb3934935e2
Comment 35 Tim Taubert [:ttaubert] 2012-08-02 04:22:14 PDT
https://hg.mozilla.org/mozilla-central/rev/fcb3934935e2
Comment 36 Virgil Dicu [:virgil] [QA] 2012-08-30 02:14:31 PDT
*** Bug 786965 has been marked as a duplicate of this bug. ***
Comment 37 Dão Gottwald [:dao] 2012-08-30 04:21:02 PDT
Why wasn't this uplifted to aurora? Can it still be uplifted to beta now?
Comment 38 Dão Gottwald [:dao] 2012-08-30 04:21:47 PDT
Alternatively, can bug 681005 be backed out from mozilla-beta?
Comment 39 Matthias Versen [:Matti] 2012-08-30 14:28:45 PDT
*** Bug 787125 has been marked as a duplicate of this bug. ***
Comment 40 Michal Stanke (Mozilla.cz) [:MikkCZ] 2012-08-31 05:15:30 PDT
It would be quite unpleasant, if this problem stays inside the final release - it harms all users using pinned tabs (quite huge number).
Comment 41 Alex Keybl [:akeybl] 2012-08-31 16:21:47 PDT
My preference would be to back out bug 681005. Tracking for release of FF16.
Comment 42 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-03 12:41:38 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/a3790585b2a2

Note You need to log in before you can comment on or make changes to this bug.