Closed
Bug 1264262
Opened 8 years ago
Closed 8 years ago
crash in java.lang.IllegalArgumentException: Missing default panels at org.mozilla.gecko.home.HomeConfigPrefsBackend.combineHistoryAndSyncPanels(HomeConfigPrefsBackend.java)
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(firefox48 fixed, fennec48+)
RESOLVED
FIXED
Firefox 48
People
(Reporter: Margaret, Assigned: liuche)
References
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
This bug was filed from the Socorro interface and is report bp-0fa43df1-9b5f-4556-a78e-3e7c62160412. ============================================================= Chenxia, this is our top (unfixed) crash on Nightly. Let's get a patch ASAP. java.lang.IllegalArgumentException: Missing default panels at org.mozilla.gecko.home.HomeConfigPrefsBackend.combineHistoryAndSyncPanels(HomeConfigPrefsBackend.java:179) at org.mozilla.gecko.home.HomeConfigPrefsBackend.maybePerformMigration(HomeConfigPrefsBackend.java:313) at org.mozilla.gecko.home.HomeConfigPrefsBackend.loadConfigFromString(HomeConfigPrefsBackend.java:332) at org.mozilla.gecko.home.HomeConfigPrefsBackend.load(HomeConfigPrefsBackend.java:368) at org.mozilla.gecko.home.HomeConfig.load(HomeConfig.java:1613) at org.mozilla.gecko.home.HomeConfigLoader.loadInBackground(HomeConfigLoader.java:13) at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground$532ebdd5(Unknown Source) at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground$42af7916(Unknown Source) at android.support.v4.content.ModernAsyncTask$2.call(Unknown Source) at java.util.concurrent.FutureTask.run(FutureTask.java:237) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588) at java.lang.Thread.run(Thread.java:818)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(liuche)
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46141/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46141/
Attachment #8741052 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 2•8 years ago
|
||
I see what's happening here - I didn't account for the case where no migration needs to happen!
Flags: needinfo?(liuche)
Reporter | ||
Comment 3•8 years ago
|
||
Comment on attachment 8741052 [details] MozReview Request: Bug 1264262 - crash in java.lang.IllegalArgumentException: Missing default panels at org.mozilla.gecko.home.HomeConfigPrefsBackend.combineHistoryAndSyncPanels(HomeConfigPrefsBackend.java). r=mcomella https://reviewboard.mozilla.org/r/46141/#review42643 ::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java:178 (Diff revision 1) > syncIndex = i; > syncFlags = panelConfig.getFlags(); > } > } > > - if (historyIndex == -1 || syncIndex == -1) { > + if (historyIndex == -1 && syncIndex == -1) { What happens to the logic down below if one of these indices is -1? This assumes that the default config changed to remove both of these panels at the same time, which is true, but it still seems fragile. Could you at least add a comment explaining why we should never see only one of these panels here?
Attachment #8741052 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8741052 [details] MozReview Request: Bug 1264262 - crash in java.lang.IllegalArgumentException: Missing default panels at org.mozilla.gecko.home.HomeConfigPrefsBackend.combineHistoryAndSyncPanels(HomeConfigPrefsBackend.java). r=mcomella Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46141/diff/1-2/
Attachment #8741052 -
Attachment description: MozReview Request: Bug 1264262 - crash in java.lang.IllegalArgumentException: Missing default panels at org.mozilla.gecko.home.HomeConfigPrefsBackend.combineHistoryAndSyncPanels(HomeConfigPrefsBackend.java). r=margaret → MozReview Request: Bug 1264262 - crash in java.lang.IllegalArgumentException: Missing default panels at org.mozilla.gecko.home.HomeConfigPrefsBackend.combineHistoryAndSyncPanels(HomeConfigPrefsBackend.java). r=mcomella
Attachment #8741052 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 5•8 years ago
|
||
Talked this over with mcomella to check my migration logic, and updated this patch to be more explicit. The people this bug affects should be users who got Combined Panels through the initial landing of bug 1220928 and then customized their panels, and have since repeatedly crashed.
Attachment #8741052 -
Flags: review?(michael.l.comella) → review+
Comment on attachment 8741052 [details] MozReview Request: Bug 1264262 - crash in java.lang.IllegalArgumentException: Missing default panels at org.mozilla.gecko.home.HomeConfigPrefsBackend.combineHistoryAndSyncPanels(HomeConfigPrefsBackend.java). r=mcomella https://reviewboard.mozilla.org/r/46141/#review42787 r+ w/ elaborated comment and leaving in the assertion. ::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java:176 (Diff revision 2) > historyFlags = panelConfig.getFlags(); > } else if (type == PanelType.REMOTE_TABS) { > syncIndex = i; > syncFlags = panelConfig.getFlags(); > + } else if (type == PanelType.COMBINED_HISTORY) { > + // These users are already on version 4. Can you elaborate on why these users are already on v4? e.g. "We landed the combined history panel before we landed the migration code. New users got the combined historty panel but were set to version 3. When we landed the migration code, they attempted to update to v4 but v4 assumes you don't already have the combined history panel so we crashed. Thus, if the users already have the combined history panel, we return the existing panels." ::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java (Diff revision 2) > + // These users are already on version 4. > + return jsonPanels; > } > } > > - if (historyIndex == -1 || syncIndex == -1) { You should leave this in – your code below still makes the assumption that this is the case so we should crash if it is not.
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f3ff455f8a39
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Reporter | ||
Updated•8 years ago
|
tracking-fennec: ? → 48+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•