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

VERIFIED FIXED in Firefox 53

Status

()

defect
P1
critical
VERIFIED FIXED
2 years ago
7 months ago

People

(Reporter: JanH, Assigned: jwu)

Tracking

({crash})

Trunk
Firefox 55
All
Android
Points:
---

Firefox Tracking Flags

(fennec+, firefox51 wontfix, firefox52 wontfix, firefox-esr52 unaffected, firefox53 verified, firefox54 fixed, firefox55 verified)

Details

(crash signature, )

Attachments

(1 attachment)

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.
tracking-fennec: ? → +
Priority: -- → P1
Assignee

Comment 1

2 years ago
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.
Assignee: nobody → topwu.tw
Assignee

Comment 2

2 years ago
The mSelectedDate is created in android.widget.CalendarView and initialized to System.currentTimeMillis().
Comment hidden (mozreview-request)
Reporter

Comment 4

2 years ago
mozreview-review
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.
Assignee

Comment 5

2 years ago
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 on attachment 8842778 [details]
Bug 1339884 - Adjust DateTimePicker's upper and lower bound.

@grisha: Can you take this review from me?
Attachment #8842778 - Flags: review?(s.kaspari) → review?(gkruglov)

Comment 7

2 years ago
mozreview-review
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?
Attachment #8842778 - Flags: review?(gkruglov) → review-
Assignee

Comment 8

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
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.
Attachment #8842778 - Flags: review?(gkruglov) → review-

Comment 11

2 years ago
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.
Assignee

Comment 12

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review
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!
Attachment #8842778 - Flags: review?(gkruglov) → review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 16

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c728250f70c
Adjust DateTimePicker's upper and lower bound. r=Grisha
Keywords: checkin-needed

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c728250f70c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Tested on Android 4.1.2 and it no longer crashes - thanks for fixing. Can you request uplift for this?
Flags: needinfo?(topwu.tw)
Assignee

Comment 19

2 years ago
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.
Flags: needinfo?(topwu.tw)
Attachment #8842778 - Flags: approval-mozilla-aurora?
 Hi Ioana, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(ioana.chiorean)
Attachment #8842778 - Flags: approval-mozilla-beta?
Flags: qe-verify+
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.
Attachment #8842778 - Flags: approval-mozilla-beta?
Attachment #8842778 - Flags: approval-mozilla-beta+
Attachment #8842778 - Flags: approval-mozilla-aurora?
Attachment #8842778 - Flags: approval-mozilla-aurora+

Comment 24

2 years ago
Verified as fixed on Beta 53.0b9 (2017-04-04) with the STR described. 
Device: Lenovo A536 (Android 4.4.2)
Status: RESOLVED → VERIFIED
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.
Crash Signature: [@ java.lang.NullPointerException: at org.mozilla.gecko.prompts.PromptInput$DateTimeInput.getValue(PromptInput.java)] → [@ java.lang.NullPointerException: at org.mozilla.gecko.prompts.PromptInput$DateTimeInput.getValue(PromptInput.java)] [@ java.lang.NullPointerException: Attempt to invoke virtual method ''long org.mozilla.gecko.widget.DateTimePicker.getTimeInMillis()'' o…
See Also: → 1356170

Updated

2 years ago
Flags: needinfo?(ioana.chiorean)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.