1306784 - History panel is re-enabled if the user had all panels disabled before migration

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Kevin Brosnan [Ex-Mozilla]]

1. Download a build from before the panel reduction
2. In the settings open the home settings
3. Disable every panel

expected: every panel remains disabled
actual: history panel returns to active

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: resets user settings because of bug 1262929

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Sounds like this is caused by our recent panel migration fix (bug 1304777)?

Comment 3

[Andrzej Hunt :ahunt]

I wonder if this is a side-effect of Bug 1251362, where recent tabs was moved into history (as opposed to bug 1304777 which avoided some crashes there).

The recent tabs migration does re-enable the history panel in some specific circumstances, and it's possible that that might have happened here.

Comment 4

[Andrzej Hunt :ahunt]

Actually, it's also possible that our fixup migration code causes this: if no panel is set to default, it sets the history panel as being the default (which overwrites the disabled flag for that panel).

I'm guessing if all panels are disabled, then we don't run into the "no default panel set->crash" situation?

Comment 5

[Andrzej Hunt :ahunt]

Yup, it's a bug in the fixup (ensureDefaultPanelForV5orV8()) which doesn't handle the scenario where all panels are disabled.

I don't think there's any way we can recover the all-panels-disable state for users on nightly/aurora/beta, but we can fix the fixup migration to ensure that release users won't be affected. Does that sound acceptable?

Comment 6

[Marcia Knous [:marcia]]

Tracking 52+ for this issue related to the panel migration.

Comment 7

[Andrzej Hunt :ahunt]

Attachment: Bug 1306784 - Don't set a default panel during fixup migration if all panels are disabled

The fixup migration assumes that there must be a default panel, and sets the history panel as default if not.
This assumption is wrong in the case where no panels are visible: in this case there is no default.
Before this commit, that migration will make the history panel visible for all users who had no
visible panels, with this commit that migration will retain the all-panels-hidden state.

Note: it's impossible for us to recover the all-panels-hidden state for clients that have already
run the migration, however we can at least prevent this issue from happening for users who
have not yet migrated (i.e. in release).

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

Comment 8

[Andrzej Hunt :ahunt]

This patch *should* prevent release users (and users stuck on a version before the migration-crashfix) from being hit by this bug. I'll try to test it more thoroughly on Thursday (currently travelling, and I don't want to kill my battery with the required rebuilds).

Comment 9

[Andrzej Hunt :ahunt]

I've filed Bug 1307534 to add some tests for this case too. (Not sure if we'll need them, but better to implement now than never.)

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8797663 [details]
Bug 1306784 - Don't set a default panel during fixup migration if all panels are disabled

https://reviewboard.mozilla.org/r/83314/#review82206

LGTM. Didn't you recently add a test for the code? Could we add a test case for this case too?

::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java:228
(Diff revision 1)
>          int historyIndex = -1;
>  
> +        // If all panels are disabled, there is no default panel - this is the only valid state
> +        // that has no default. We can use this flag to track whether any visible panels have been
> +        // found.
> +        boolean noEnabledPanelsFound = true;

I'm always a bit confused by booleans that start with "no". Could we flip that? enabledPanelFound = false; or allPanelsDisabled = false;

Comment 11

[Andrzej Hunt :ahunt]

Comment on attachment 8797663 [details]
Bug 1306784 - Don't set a default panel during fixup migration if all panels are disabled

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

Comment 12

[Andrzej Hunt :ahunt]

The tests I've written seem to suggest that my patch works, I'll wait until they're reviewed before landing this bug. (Test in Bug 1307534 - I doubt the tests will be upliftable so I'll keep them in a separate bug to minimise confusion.)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fad2db198490
Don't set a default panel during fixup migration if all panels are disabled r=sebastian

Comment 14

[Phil Ringnalda (:philor)]

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

Comment 15

[Gerry Chang [:gchang]]

Hi :ahunt,
Since this bug also affects 50/51, do you consider to uplift this for 50/51 if this patch is not too risky?

Comment 16

[Gerry Chang [:gchang]]

Tracking 50+/51+ for this issue related to the panel migration.

Comment 17

[Andrzej Hunt :ahunt]

Comment on attachment 8797663 [details]
Bug 1306784 - Don't set a default panel during fixup migration if all panels are disabled

Approval Request Comment
[Feature/regressing bug #]: Bug 1304777 - home panel migration crash fix, leading to this regression. 
[User impact if declined]: User who have all their homepanels disabled will end up with the history panel being re-enabled after migrating from 49 to 50.
[Describe test coverage new/current, TreeHerder]: Manual testing, in addition to automated tests in Bug 1307534.
[Risks and why]: Low-risk: this patch tests for an additional edge-case (all panels disabled), and returns early in that case.
[String/UUID change made/needed]: none.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8797663 [details]
Bug 1306784 - Don't set a default panel during fixup migration if all panels are disabled

Fixes a new regression in 50, Aurora51+, Beta50+

Comment 19

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

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

Comment 20

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

https://hg.mozilla.org/releases/mozilla-beta/rev/f4ae62f16cc7

Comment 21

[u421692]

Verified as fixed on Firefox 50 Beta 6

Comment 22

[Bogdan Surd, QA [:bsurd, NI]]

Device:
 - LG G4 (Android 5.1)
Builds:
 - Beta - 51.0.b1 - Build 3
 - Aurora - 52.0.a2

Hello, I've verified this issue and it's no longer reproducible.