1261008 - Nightly does not launch custom homepage after killing app in task manager and android settings

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u549602]

Environment: Nightly 48.0a1
Device: Xiaomi mi i4  (Android 5.1.1);
Build: Nightly 48.0a1 (2016-03-30);

Steps to reproduce:
1. Install fresh build of Nightly
2. Set a custom home page 
3. Quit app from task manager or by Android "Force stop" setting
4. Restart Nightly 

Expected result:
The home page displayed should be the one that was set at step 2

Actual result:
Nightly does not recognize the Quit action from task manager/Android setting and the home page displayed is about:home

Please check https://www.youtube.com/watch?v=VS58W_dB_pU&feature=youtu.be for further details

Comment 1

[:Margaret Leibovic]

Re-summarizing to better explain what I think this is about.

Can you help us narrow down a regression range?

Comment 2

[u549602]

Hei Margaret, regression range found, it can be noticed that in the 48.0a1 build from 8th of march this issue could not be reproduced but in the 9th of march, this issue started reproducing with a crash after restarting the app (while homepage was changed - as described in the repro-steps above), no crash report generated though.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bdde3fedb45be528f985a0aff373b37b010fb927&tochange=af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86

Comment 3

[:Margaret Leibovic]

Sebastian or Mike, can one of you own this?

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Hmm, we only load the homepage if shouldRestore is false, which is kind of strange:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1462-1471

If you disable session restore then we load the homepage :)

Comment 5

[Steve Sutton]

I'm running 47, on Android, and I have restore tabs set to "Don't restore...." and a homepage set.  When I start firefox I get bookmarks (since I disabled all of the homepage panels, I actually get a blank page).

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1261008 - Do not restore tabs pointing to about:home and with no history.

If the browser is set to always restore the session (default) then we will never
start with an empty session. Even if the user closes all tabs we will re-create
a new tab pointing to about:home. So we will always at least restore this tab on
the next startup. As a consequence we will never open the 'homepage' on app start
because we will never have a non-empty session.

With this patch we won't restore tabs that point to about:home and do not contain
any other history. As a result we might restore an empty session and load the
homepage or about:home (depending on configuration) in a new tab.

In case we decide to not restore the currently selected tab, we just select the
first restored tab if there's any.

Review commit: https://reviewboard.mozilla.org/r/62668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62668/

Comment 8

[Jan Henning [:JanH]]

Hmm, this is overlapping with bug 1284017, since this will be another case where a SessionRestoreException will be triggered, yet we don't want to report this as a corrupt sessionstore.js in telemetry.

How soon do you plan on uplifting this, especially to Beta? If it's more than a few days, would it be possible to land bug 1284017 first for easier uplifting and include the necessary compatibility changes (I can sketch a possible solution, unless you've got a better idea) as part of this bug?

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jan Henning [:JanH] from comment #8)
> How soon do you plan on uplifting this, especially to Beta? If it's more
> than a few days, would it be possible to land bug 1284017 first for easier
> uplifting and include the necessary compatibility changes (I can sketch a
> possible solution, unless you've got a better idea) as part of this bug?

I would like to land and uplift this pretty soon. Because right now the "homepage" feature is basically not working (since we switch to default to "restore all").
I'm not sure why bug 1284017 is blocking this? Can't we land this either way - as long as the second one takes care of not sending the telemetry for this case?
But if we can get this in by next week then I'm okay with waiting some days.

Comment 10

[Jan Henning [:JanH]]

No, I just wanted to avoid getting into a situation where you'd land your patch first, but then I'd want to uplift before you, because I think that in that case both of us would have to provide separate patches for uplifting to incorporate the necessary changes. But if you want to uplift pretty soon, then I can just land on top of you and do the uplifting likewise.

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 8768407 [details]
Bug 1261008 - Do not restore tabs pointing to about:home and with no history.

https://reviewboard.mozilla.org/r/62668/#review59434

Yikes, this is making my life slightly more complicated for bug 1284017, but that's the way things are... :-)

Other than that, this seems okay to me, except I'd possibly select a more adjacent tab - but that's no blocker.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1707
(Diff revision 1)
> +                            if (sessionTab.isSelected()) {
> +                                // Unfortunately this tab is the selected tab. Let's just try to select
> +                                // the first tab. If we haven't restored any tabs so far then remember
> +                                // to select the next tab that gets restored.
> +
> +                                if (!Tabs.getInstance().selectFirstTab()) {

Although selecting the first tab is probably slightly easier, might it not be more intuitive (from an UX perspective) to select an adjacent tab (which at this point would be the last tab available in mOrder)?

Your call though - fix it or drop it as you wish.

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:268
(Diff revision 1)
> +    public boolean selectFirstTab() {
> +        if (mOrder.isEmpty()) {
> +            return false;
> +        }
> +
> +        selectTab(mOrder.get(0).getId());

See above, maybe `mOrder.size() - 1` or something like that would be better?

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/62668/#review59434

> See above, maybe `mOrder.size() - 1` or something like that would be better?

Yeah, this is a good idea. We won't be able to select the perfect/right tab, but if the user has a lot of tabs open then selecting one close to the previous one will be helpful to navigate in the tabs tray.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/62668/#review59730

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:263
(Diff revision 1)
>          // Pass a message to Gecko to update tab state in BrowserApp.
>          GeckoAppShell.notifyObservers("Tab:Selected", String.valueOf(tab.getId()));
>          return tab;
>      }
>  
> +    public boolean selectFirstTab() {

Oh, and looking at the other methods this should be synchronized too.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8768407 [details]
Bug 1261008 - Do not restore tabs pointing to about:home and with no history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62668/diff/1-2/

Comment 15

[Andrzej Hunt :ahunt]

Comment on attachment 8768407 [details]
Bug 1261008 - Do not restore tabs pointing to about:home and with no history.

https://reviewboard.mozilla.org/r/62668/#review60032

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/e48c07cdf008f3bf2ebc5bb60d295391a4914163
Bug 1261008 - Do not restore tabs pointing to about:home and with no history. r=janh,ahunt

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e48c07cdf008

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8768407 [details]
Bug 1261008 - Do not restore tabs pointing to about:home and with no history.

Approval Request Comment

[Feature/regressing bug #]: This bug existed since we implemented the homepage feature but it's only triggered if restoring tabs is set to "always". This is the default since bug 1219343.

[User impact if declined]: Since we always have a session to restore with at least one tab (about:home) we never open the homepage if the user has set one.

[Describe test coverage new/current, TreeHerder]: Local testing.

[Risks and why]: I consider the risk to be low because the patch is small and focused. However we are altering the session store and breaking it is a rather unpleasant experience. We could wait some additional days to see if there are obvious problems in Nightly.

[String/UUID change made/needed]: -

Comment 19

[Gerry Chang [:gchang]]

Hi Sebastian,
If we let this ride the train on 49/50 and won't fix in 48 beta, will this a big impact for user given this is not a regression?

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Gerry Chang [:gchang] from comment #19)
> If we let this ride the train on 49/50 and won't fix in 48 beta, will this a
> big impact for user given this is not a regression?

Firefox 48 is the first version that has the tab pref set to "always restore" by default, see bug 1219343. If we are not going to uplift this to 48 then we are breaking the homepage feature for users that have set one (Supported since Firefox 44).

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8768407 [details]
Bug 1261008 - Do not restore tabs pointing to about:home and with no history.

In order not to break the homepage feature for users that have set one, take it in 48 beta 8 and aurora.

Comment 22

[Gerry Chang [:gchang]]

This needs to be verified in beta.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce26d5834678

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This has conflicts on beta:
$ hg graft -r ce26d5834678
grafting 352467:ce26d5834678 "Bug 1261008 - Do not restore tabs pointing to about:home and with no history. r=janh,ahunt a=gchang"
merging mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
merging mobile/android/base/java/org/mozilla/gecko/Tabs.java
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/GeckoApp.java! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Guessing it's because https://hg.mozilla.org/releases/mozilla-aurora/rev/d4f569803fec needs to get uplifted?

Comment 25

[Carsten Book [:Tomcat]]

in risk of missing beta 9

Comment 26

[Jan Henning [:JanH]]

Bug 905223 doesn't need uplifting - it modified bits of @Override onTabRead, but nothing that really interferes with this patch - I think the conflicts are just the merge routine being picky about neighbouring lines having been modified [1].

[1] two variables being turned into final variables, and moving tab.updateTitle() into a UI thread runnable.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Jan, can you help? Thanks

Comment 28

[Sylvestre Ledru [:Sylvestre]]

(help to write the patch)

Comment 29

[Jan Henning [:JanH]]

Unfortunately I'm away until Thursday.

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

I'll write the patch.

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1261008-homepage-BETA.patch

Modified patch for BETA.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7955d606935c

Comment 33

[u549602]

Verified as fixed on all branches(48.0b9, Aurora 49.0a2/2016-07-18 and Nightly 50.0a1/2016-07-18) on a Samsung Galaxy S6 Edge (Android 6.0.1) and on a Samsung Galaxy Tab S2(Android 5.0.2)