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
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
Sounds like this is caused by our recent panel migration fix (bug 1304777)?
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.
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
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).
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 email@example.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
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?
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.
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+
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.