1339884 - Crash in java.lang.NullPointerException: at org.mozilla.gecko.prompts.PromptInput$DateTimeInput.getValue(PromptInput.java) on Android 4.x

Status: VERIFIED FIXED

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

Description

[Jan Henning [:JanH]]

This bug was filed from the Socorro interface and is 
report bp-19be837a-7fc7-49f6-91d4-3dcc82170215.
=============================================================
STR:
1. Go to http://www.swr3.de/musik/playlisten/-/id=47424/cf=42/a0bmc9/index.html
2. Click on the date input field.
3. Kaboom.

Judging from crash-stats, only API level 19 and below (i.e. Android 4.x) is affected.

Comment 1

[Jing-wei Wu [:jwu]]

This crash is raised by android.widget.CalendarView that the selected time isn't between maximum and minimum time.
Check the crash stack below that I reproduced on Android 4.1 device:

E/GeckoPromptService: Error showing prompt inputs
                      java.lang.IllegalArgumentException: Time not between Fri Dec 02 00:00:00 GMT+08:00 2016 and Thu Mar 02 00:00:00 GMT+08:00 2017
                          at android.widget.CalendarView.goTo(CalendarView.java:1103)
                          at android.widget.CalendarView.setMinDate(CalendarView.java:761)
                          at org.mozilla.gecko.widget.DateTimePicker.<init>(DateTimePicker.java:306)
                          at org.mozilla.gecko.prompts.PromptInput$DateTimeInput.getView(PromptInput.java:182)
                          at org.mozilla.gecko.prompts.Prompt.wrapInput(Prompt.java:370)
                          at org.mozilla.gecko.prompts.Prompt.addInputs(Prompt.java:394)
                          at org.mozilla.gecko.prompts.Prompt.create(Prompt.java:191)
                          at org.mozilla.gecko.prompts.Prompt.show(Prompt.java:134)
                          at org.mozilla.gecko.prompts.Prompt.show(Prompt.java:108)
                          at org.mozilla.gecko.prompts.PromptService.handleMessage(PromptService.java:48)
                          at org.mozilla.gecko.EventDispatcher$2.run(EventDispatcher.java:323)
                          at android.os.Handler.handleCallback(Handler.java:615)
                          at android.os.Handler.dispatchMessage(Handler.java:92)
                          at android.os.Looper.loop(Looper.java:137)
                          at android.app.ActivityThread.main(ActivityThread.java:4745)
                          at java.lang.reflect.Method.invokeNative(Native Method)
                          at java.lang.reflect.Method.invoke(Method.java:511)
                          at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:786)
                          at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
                          at dalvik.system.NativeStart.main(Native Method)

It looks like that CalendarView has some bugs on API 19 and below because:
1. For boundary check, it only checks date(DAY_OF_YEAR and YEAR), but
2. When building the view, it checks time(in millisecond)

One situation that crash will occur:
1. When doing boundary check, CalendarView checks date: mSelectedDate and mMaxDate have same date, early return without any modification.
2. When building the view, CalendarView checks time: if the time of mSelectedDate is larger than mMaxDate, IllegalArgumentException is thrown.

Comment 2

[Jing-wei Wu [:jwu]]

The mSelectedDate is created in android.widget.CalendarView and initialized to System.currentTimeMillis().

Comment 3

[Jing-wei Wu [:jwu]]

Attachment: Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

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

Comment 4

[Jan Henning [:JanH]]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

https://reviewboard.mozilla.org/r/116564/#review118166

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:290
(Diff revision 1)
>          // then display a calendar.
>          if (mState == PickersState.DATE || mState == PickersState.DATETIME) {
>              mCalendar = new CalendarView(context);
>              mCalendar.setVisibility(GONE);
>  
> +            // If mMaxDate/mMinDate has same date with selectedDate, set it to upper/lower bound of selectedDate. See Bug 1339884.

Question: Is this something that we should do in any case, or is it just purely working around an Android bug? If the latter, maybe this should be made conditional on the appropiate `Versions.featureNNPlus` condition, so we don't forget to remove it again when we stop supporting these Android versions.

Comment 5

[Jing-wei Wu [:jwu]]

I don't prefer to check API version because I think adjust mMaxDate to the end of the date(23:59:59,999) is more reasonable since it is the upper bound of the date. Similar thought on mMinDate.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

@grisha: Can you take this review from me?

Comment 7

[:Grisha Kruglov]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

https://reviewboard.mozilla.org/r/116564/#review120642

Let me know if I'm misunderstanding something about your patch - it's the first time I'm looking at this code or _any_ Calendar code for that matter.

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:280
(Diff revision 1)
>  
>          // mTempDate will either be a site-supplied value, or today's date otherwise. CalendarView implementations can
>          // crash if they're supplied an invalid date (i.e. a date not in the specified range), hence we need to set
>          // a sensible default date here.
>          if (mTempDate.before(mMinDate) || mTempDate.after(mMaxDate)) {
>              mTempDate.setTimeInMillis(mMinDate.getTimeInMillis());

This looks like a bug to me. Why are we setting `mTempDate` to min value IF it's after the max value? shouldn't we do something like this instead?
```
if (tempDate < minDate) {
  tempDate = minDate;
} else if (tempDate > maxDate) {
  tempDate = maxDate;
}
```

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:290
(Diff revision 1)
>          // then display a calendar.
>          if (mState == PickersState.DATE || mState == PickersState.DATETIME) {
>              mCalendar = new CalendarView(context);
>              mCalendar.setVisibility(GONE);
>  
> +            // If mMaxDate/mMinDate has same date with selectedDate, set it to upper/lower bound of selectedDate. See Bug 1339884.

In addition to the bug number, can you provide a comment for why this is necessary? Having an in-line explanation is much easier to read through than having to go to a bug and look for explanations there.

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:292
(Diff revision 1)
>              mCalendar = new CalendarView(context);
>              mCalendar.setVisibility(GONE);
>  
> +            // If mMaxDate/mMinDate has same date with selectedDate, set it to upper/lower bound of selectedDate. See Bug 1339884.
> +            final Calendar selectedDate = Calendar.getInstance();
> +            selectedDate.setTimeInMillis(mCalendar.getDate());

I'm confused how this is supposed to work.

First, you _just_ initialized mCalendar above, so I would expect it to return current date from getDate(). Or am I missing something?

Second, I don't think CalendarView view actually includes time information, right? E.g. wouldn't getDate always return you the very beginning of any date, e.g. midnight of the date?

So, Calendar.getInstance() returns you a current date/time (which you're calling 'selectedDate' for some reason I don't yet understand). Which you then reset to beginning of a current date. 

Then you check if maxDate is the same _day_ as the current day, and if so, you set it to the end of current day. And the same for min date, check if it's the same current date, and if so - reset it to the beginning of current date.

Shouldn't you instead check against `mTempDate`, as opposed to current time as you do?

Comment 8

[Jing-wei Wu [:jwu]]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

https://reviewboard.mozilla.org/r/116564/#review120642

> This looks like a bug to me. Why are we setting `mTempDate` to min value IF it's after the max value? shouldn't we do something like this instead?
> ```
> if (tempDate < minDate) {
>   tempDate = minDate;
> } else if (tempDate > maxDate) {
>   tempDate = maxDate;
> }
> ```

The code here is:

    if (mTempDate.before(mMinDate) || mTempDate.after(mMaxDate)) {
        mTempDate.setTimeInMillis(mMinDate.getTimeInMillis());
    }

The mTempDate is the selected date for CalendarView. The IF block gives the boundary check to see if the selected date is before the minimal or after the maximal date.

In usual case mTempDate should be between mMinDate and mMaxDate. When it's out of range, it's value becomes invalid and has no meaning anymore.

I think it's okay to always set it to mMinDate. Because the purpose of the modification is just to prevent IllegalArgumentException occurs.

> In addition to the bug number, can you provide a comment for why this is necessary? Having an in-line explanation is much easier to read through than having to go to a bug and look for explanations there.

Sorry for the unclear comments. I'll resend a patch with more detailed explanation.

> I'm confused how this is supposed to work.
> 
> First, you _just_ initialized mCalendar above, so I would expect it to return current date from getDate(). Or am I missing something?
> 
> Second, I don't think CalendarView view actually includes time information, right? E.g. wouldn't getDate always return you the very beginning of any date, e.g. midnight of the date?
> 
> So, Calendar.getInstance() returns you a current date/time (which you're calling 'selectedDate' for some reason I don't yet understand). Which you then reset to beginning of a current date. 
> 
> Then you check if maxDate is the same _day_ as the current day, and if so, you set it to the end of current day. And the same for min date, check if it's the same current date, and if so - reset it to the beginning of current date.
> 
> Shouldn't you instead check against `mTempDate`, as opposed to current time as you do?

The short answers for the questions:

1. Partially yes, CalendarView is initialized to 'current time' when it's created.
2. No and No. Although there's no place to edit time in CalendarView, it still keeps time information(at least on Android 4.x). And the return value of getDate() it the time when CalendarView is created, not the beggining of the date. 


I try to give a detailed explanation how this issue would occur:

The CalendarView is initializd to current time when it's created. And the time information is stored in a private variable called mSelectedDate.

    private final Calendar mSelectedDate = Calendar.getInstance();
    
The methods setDate/setMaxDate/setMinDate in CalendarView only care about date information, it early returns if it the date information unchanged.

    public void setMinDate(long minDate) {
        mTempDate.setTimeInMillis(minDate);
        if (isSameDate(mTempDate, mMinDate)) {
            return;
        }
        mMinDate.setTimeInMillis(minDate);
        ...
        
But for boundary check, CalendarView checks the whole time information.

    if (mSelectedDate.before(mMinDate) || mSelectedDate.after(mMaxDate)) {
        throw new IllegalArgumentException("Time not between " + mMinDate.getTime()
                + " and " + mMaxDate.getTime());
    }
    ...

So if mSelectedDate and mMinDate(or mMaxDate) is same 'date' but the 'time' of mSelectedDate is before mMinDate(or after mMaxDate), IllegalArgumentException will be thrown!


The first idea to fix this issue is to edit the selected date(mTempDate in DateTimePicker) to make sure it's always after mMinDate and before mMaxDate. But setDate() is the only entry to modify and it doesn't care about the time(because of the early return). Which means when CalendarView is created, there is not way to change the time information of mSelectedDate from outside.

The second though is to change to the minimal and maximal date in the first initialization. So the solution is to make the maximal date be the end of the date and minimal date be the end of the date.


The source code of CalendarView.java (http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.0.1_r1/android/widget/CalendarView.java)

Comment 9

[Jing-wei Wu [:jwu]]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

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

Comment 10

[:Grisha Kruglov]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

https://reviewboard.mozilla.org/r/116564/#review124604

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:293
(Diff revision 2)
>              mCalendar.setVisibility(GONE);
>  
> +            // In Android 4.x, CalendarView can still crash if the selected date(mCalendar.getDate())
> +            // has same date as mMaxDate/mMinDate but the time is after mMaxDate or is before mMinDate.
> +            // Hence we modify the time of mMaxDate and mMinDate to the end of the date and the beginning of the date.
> +            // To make sure the selected time of mCalendar is between them.

Please add a reference to the bug number.

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:294
(Diff revision 2)
>  
> +            // In Android 4.x, CalendarView can still crash if the selected date(mCalendar.getDate())
> +            // has same date as mMaxDate/mMinDate but the time is after mMaxDate or is before mMinDate.
> +            // Hence we modify the time of mMaxDate and mMinDate to the end of the date and the beginning of the date.
> +            // To make sure the selected time of mCalendar is between them.
> +            final Calendar selectedDate = Calendar.getInstance();

OK, after reading CalendarView's implementation, I get what you're trying to do now.

Unless I'm missing something, if you're going to reset max/min boundaries when they fall onto the current date, you might as well do this in every case.

So I think you should just set mMaxDate to end of 
day, and mMinDate to beginning of day, without any conditionals and without `selectedDate` variable.

The whole `selectedDate` thing is confusing because you re-used an internal name from CalendarView, which makes very little sense in your current context. If you will keep the `isSameDate` checks (but I'd rather you didn't, without a good reason), please rename that variable to something descriptive like `calendarViewInternalSelectedDate`.

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:687
(Diff revision 2)
>          updateSpinners();
>          notifyDateChanged();
>      }
> +
> +    /**
> +     * @return True if the firstDate is the same as the secondDate.

You've copy/pasted this from CalendarView implementation. Please indicate as such.

But ideally, if you can avoid these checks, I'd rather you remove this function entirely.

Comment 11

[:Grisha Kruglov]

Apologies for a late review. I get what you're doing now, and I think you can do it in a simpler way. Please re-flag with either a new patch or at least with a patch that renames things, and I'll r+ promptly after.

Comment 12

[Jing-wei Wu [:jwu]]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

https://reviewboard.mozilla.org/r/116564/#review124750

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:294
(Diff revision 2)
>  
> +            // In Android 4.x, CalendarView can still crash if the selected date(mCalendar.getDate())
> +            // has same date as mMaxDate/mMinDate but the time is after mMaxDate or is before mMinDate.
> +            // Hence we modify the time of mMaxDate and mMinDate to the end of the date and the beginning of the date.
> +            // To make sure the selected time of mCalendar is between them.
> +            final Calendar selectedDate = Calendar.getInstance();

I'll send a new patch without the condition.

Comment 13

[Jing-wei Wu [:jwu]]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

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

Comment 14

[Jing-wei Wu [:jwu]]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116564/diff/3-4/

Comment 15

[:Grisha Kruglov]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

https://reviewboard.mozilla.org/r/116564/#review125006

LGTM, if that works for you!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c728250f70c
Adjust DateTimePicker's upper and lower bound. r=Grisha

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c728250f70c

Comment 18

[Jan Henning [:JanH]]

Tested on Android 4.1.2 and it no longer crashes - thanks for fixing. Can you request uplift for this?

Comment 19

[Jing-wei Wu [:jwu]]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

Approval Request Comment
[Feature/Bug causing the regression]:
Crash in java.lang.NullPointerException: at org.mozilla.gecko.prompts.PromptInput$DateTimeInput.getValue on Android 4.x

[User impact if declined]:
On Android 4.x, app crashes when browsing some webpages that have DateTimePicker with invalid upper/lower bound.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
1. Go to http://www.swr3.de/musik/playlisten/-/id=47424/cf=42/a0bmc9/index.html
2. Click on the date input field.
3. No crash occurs.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No. 

[Why is the change risky/not risky?]:
It just adjust the upper/lower bound in CalendarView.java to prevent crash. The modification is very limited and I can't see any risk.

[String changes made/needed]:
None.

Comment 20

[Gerry Chang [:gchang]]

 Hi Ioana, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

Sounds like Jan tested this. The change looks reasonable, let's take this up to beta.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/59706edc8cc0

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Oana Horvath [:ohorvath]]

Verified as fixed on Beta 53.0b9 (2017-04-04) with the STR described. 
Device: Lenovo A536 (Android 4.4.2)

Comment 25

[Jan Henning [:JanH]]

Seems like Android 5.x+ was affected as well, although probably not under the exact same circumstances and the crash signature ends up slightly different as well, which is why I diagnosed this as an Android 4.x issue.

According to
(Sergiu from bug 1356170 comment #0)
> 3. The issue is also reproducible in Nightly 55.0a1 (2017-04-12) (Mobile),
> but with a different Actual result: An incomplete date/time picker is
> displayed.

we no longer crash, so the fix from this bug at least prevents the crash on API21+ as well, although there are apparently some remaining issues.