Closed Bug 1306784 Opened 3 years ago Closed 3 years ago

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

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 52
Iteration:
1.6
Tracking Status
firefox49 --- unaffected
fennec 50+ ---
firefox50 + verified
firefox51 + verified
firefox52 + verified

People

(Reporter: kbrosnan, Assigned: ahunt)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MobileAS])

Attachments

(1 file)

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
[Tracking Requested - why for this release]: resets user settings because of bug 1262929
OS: Unspecified → Android
Hardware: Unspecified → All
Sounds like this is caused by our recent panel migration fix (bug 1304777)?
Flags: needinfo?(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.
Flags: needinfo?(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?
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?
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [MobileAS]
Tracking 52+ for this issue related to the panel migration.
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).
Blocks: 1307534
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 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;
Attachment #8797663 - Flags: review?(s.kaspari) → review+
tracking-fennec: ? → 50+
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.)
Pushed by ahunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fad2db198490
Don't set a default panel during fixup migration if all panels are disabled r=sebastian
https://hg.mozilla.org/mozilla-central/rev/fad2db198490
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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?
Flags: needinfo?(ahunt)
Tracking 50+/51+ for this issue related to the panel migration.
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.
Flags: needinfo?(ahunt)
Attachment #8797663 - Flags: approval-mozilla-beta?
Attachment #8797663 - Flags: approval-mozilla-aurora?
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+
Attachment #8797663 - Flags: approval-mozilla-beta?
Attachment #8797663 - Flags: approval-mozilla-beta+
Attachment #8797663 - Flags: approval-mozilla-aurora?
Attachment #8797663 - Flags: approval-mozilla-aurora+
Verified as fixed on Firefox 50 Beta 6
Iteration: --- → 1.6
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.
You need to log in before you can comment on or make changes to this bug.