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)

Unspecified
Android
defect
Not set
critical

Tracking

(firefox48 fixed, fennec48+)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed
fennec 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)
Flags: needinfo?(liuche)
I see what's happening here - I didn't account for the case where no migration needs to happen!
Flags: needinfo?(liuche)
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+
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)
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.
https://hg.mozilla.org/mozilla-central/rev/f3ff455f8a39
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
tracking-fennec: ? → 48+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.