crash in java.lang.NullPointerException: at org.mozilla.gecko.prompts.PromptInput$DateTimeInput.getValue(PromptInput.java)

RESOLVED FIXED in Firefox 45, Firefox OS v2.5

Status

()

--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Margaret, Assigned: ahunt)

Tracking

({crash, regression})

43 Branch
Firefox 46
x86
Android
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 unaffected, firefox43 wontfix, firefox44+ wontfix, firefox45 verified, firefox46 fixed, firefox47 verified, b2g-v2.5 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-79cddad6-70ce-44f1-b1f5-bc3792160113.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.prompts.PromptInput$DateTimeInput.getValue(PromptInput.java:255)
	at org.mozilla.gecko.prompts.Prompt.addInputValues(Prompt.java:252)
	at org.mozilla.gecko.prompts.Prompt.cancelDialog(Prompt.java:471)
	at org.mozilla.gecko.prompts.Prompt.addInputs(Prompt.java:420)
	at org.mozilla.gecko.prompts.Prompt.show(Prompt.java:124)
	at org.mozilla.gecko.prompts.Prompt.show(Prompt.java:117)
	at org.mozilla.gecko.prompts.PromptService$2.run(PromptService.java:67)
	at android.os.Handler.handleCallback(Handler.java:605)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:4436)
	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:784)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:551)
	at dalvik.system.NativeStart.main(Native Method)
(Assignee)

Updated

3 years ago
Assignee: nobody → ahunt
(Assignee)

Comment 1

3 years ago
I've got a patch which should prevent the crash, however I'm not quite sure what the underlying isssue is yet - as far as I can tell this code is only used when the user selects a specific type of input field (with my patch, instead of crashing, we will just not show a Dialog when the user clicks on whatever input field previously caused the crash):

(This also seems to happen only on devices <= API 19 (4.4).)

I'm guessing we're unable to create one of the inputs that should be part of the dialog, i.e. getting no return value at:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java#91

(It's also possible that some of our internal code, or extension code, is passing invalid inputs into PromptService, but that seems less likely to me?)

I've tested quickly with a page containing all the input types supported in InputWidgetHelper.js [1], in addition to the colourpicker, but they all succesfully show a dialog - there doesn't seem to be a simple way of finding something that breaks here.

[1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/InputWidgetHelper.js#70
(Assignee)

Comment 2

3 years ago
Created attachment 8708586 [details]
MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret

We shouldn't be returning the data that the user may have entered into a dialog/prompt
when it is cancelled, since it will not (or should not) be used by the calling code.
Moreover, we sometimes cancel the dialog if we cannot construct the input fields,
in this case we cannot read this data (and trying to read it will cause crashes).

Review commit: https://reviewboard.mozilla.org/r/31111/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31111/
Attachment #8708586 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 3

3 years ago
(In reply to Andrzej Hunt :ahunt from comment #2)
> Created attachment 8708586 [details]
> MozReview Request: Bug 1239823 - Don't read (possibly inexistant) data when
> cancelling dialog r?margaret
> 
> We shouldn't be returning the data that the user may have entered into a
> dialog/prompt
> when it is cancelled, since it will not (or should not) be used by the
> calling code.
> Moreover, we sometimes cancel the dialog if we cannot construct the input
> fields,
> in this case we cannot read this data (and trying to read it will cause
> crashes).
> 
> Review commit: https://reviewboard.mozilla.org/r/31111/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/31111/

For aurora/beta it might be safer to add a null-check for mInputs[i] at [1], and only use this patch for trunk?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java#245
(Reporter)

Comment 4

3 years ago
Comment on attachment 8708586 [details]
MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret

https://reviewboard.mozilla.org/r/31111/#review27965

::: mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java
(Diff revision 1)
> -        addInputValues(ret);

What would cause this to be a problem in the cancel case, yet not in the close case below?

I would rather have use figure out what's going wrong in `addInputValues`.

I agree that it probably don't make sense to include the prompt values in the cancel case, but we should audit our code to make sure we're not expecting that anywhere.
Attachment #8708586 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 5

3 years ago
(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 8708586 [details]
> MozReview Request: Bug 1239823 - Don't read (possibly inexistant) data when
> cancelling dialog r?margaret
> 
> https://reviewboard.mozilla.org/r/31111/#review27965
> 
> ::: mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java
> (Diff revision 1)
> > -        addInputValues(ret);
> 
> What would cause this to be a problem in the cancel case, yet not in the
> close case below?

I think what's happening is:

1: We fail to create one of the input fields (this uses JSON passed in from elsewhere, which I assume PromptInput is unable to handle) - hence we have an mInput[x] == null:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java#91

2: We'll get an NPE accessing mInputs[x], catch that exception, and call cancelDialog during addInputs() at:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java#387

3: We then get the NPE causing this crash when mInputs[x] is null when trying to read the values at addInputValues:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java#245

> 
> I would rather have use figure out what's going wrong in `addInputValues`.
> 
> I agree that it probably don't make sense to include the prompt values in
> the cancel case, but we should audit our code to make sure we're not
> expecting that anywhere.

I'll try do to that - the few places I've checked ignore the input values when the dialog is cancelled (InputWidgetHelper.js, PromptService.js), however I've not done a comprehensive review of all the other users of this code yet.
(Reporter)

Comment 6

3 years ago
(In reply to Andrzej Hunt :ahunt from comment #5)
> (In reply to :Margaret Leibovic from comment #4)
> > Comment on attachment 8708586 [details]
> > MozReview Request: Bug 1239823 - Don't read (possibly inexistant) data when
> > cancelling dialog r?margaret
> > 
> > https://reviewboard.mozilla.org/r/31111/#review27965
> > 
> > ::: mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java
> > (Diff revision 1)
> > > -        addInputValues(ret);
> > 
> > What would cause this to be a problem in the cancel case, yet not in the
> > close case below?
> 
> I think what's happening is:
> 
> 1: We fail to create one of the input fields (this uses JSON passed in from
> elsewhere, which I assume PromptInput is unable to handle) - hence we have
> an mInput[x] == null:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/prompts/Prompt.java#91

I'd be interested to know how we end up with this null value.

> 2: We'll get an NPE accessing mInputs[x], catch that exception, and call
> cancelDialog during addInputs() at:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/prompts/Prompt.java#387

This seems like a bug... I feel like we shouldn't keep a null input in that array. Could we create a local variable with the getInput call and do a null check? Also, catching a generic Exception is pretty bad programming practice :/

Can we audit where mInputs is used and see if there's any reason we'd actually want this null value? Otherwise, I think we should avoid adding a null item, and that should hopefully avoid this NPE.

> 3: We then get the NPE causing this crash when mInputs[x] is null when
> trying to read the values at addInputValues:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/prompts/Prompt.java#245
> 
> > 
> > I would rather have use figure out what's going wrong in `addInputValues`.
> > 
> > I agree that it probably don't make sense to include the prompt values in
> > the cancel case, but we should audit our code to make sure we're not
> > expecting that anywhere.
> 
> I'll try do to that - the few places I've checked ignore the input values
> when the dialog is cancelled (InputWidgetHelper.js, PromptService.js),
> however I've not done a comprehensive review of all the other users of this
> code yet.

Let's file a follow-up bug for the more generic fix of removing the inputs from the cancel dialog, that seems like a band-aid for this more fundamental problem of null inputs.
(Assignee)

Comment 7

3 years ago
Comment on attachment 8708586 [details]
MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31111/diff/1-2/
Attachment #8708586 - Attachment description: MozReview Request: Bug 1239823 - Don't read (possibly inexistant) data when cancelling dialog r?margaret → MozReview Request: Bug 1239823 - don't crash when cancelling dialog due to malformed input r=margaret
Attachment #8708586 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8708586 [details]
MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31111/diff/2-3/
(Assignee)

Comment 9

3 years ago
Created attachment 8709573 [details] [diff] [review]
1239823_nullcheck_beta.patch

Patch for beta (patch changes).
Attachment #8709573 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 10

3 years ago
Comment on attachment 8708586 [details]
MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret

https://reviewboard.mozilla.org/r/31111/#review28163

Nice, this seems like the lowest risk fix.
Attachment #8708586 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 11

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/f58d8d338d4f526f62d9f983e1c5c497683f8169
Bug 1239823 - don't crash when cancelling dialog due to malformed input r=margaret
(Assignee)

Comment 12

3 years ago
Comment on attachment 8708586 [details]
MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: crashes, presumed cause is clicking on certain kinds of input fields on websites.
[Describe test coverage new/current, TreeHerder]: no testing - bug hasn't been reproduced locally.
[Risks and why]: low-risk - we add only one null check.
[String/UUID change made/needed]: none.
Attachment #8708586 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 13

3 years ago
Comment on attachment 8709573 [details] [diff] [review]
1239823_nullcheck_beta.patch

(Separate patch for beta due to path changes)

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: crashes, presumed cause is clicking on certain kinds of input fields on websites.
[Describe test coverage new/current, TreeHerder]: no testing - bug hasn't been reproduced locally.
[Risks and why]: low-risk - we add only one null check.
[String/UUID change made/needed]: none.
Attachment #8709573 - Flags: review?(margaret.leibovic) → approval-mozilla-beta?
Comment on attachment 8709573 [details] [diff] [review]
1239823_nullcheck_beta.patch

Margaret and team wants us to take this fix and do a Fennec 44.0b11 as this might fix a top crash. Let's uplift to both moz-beta and moz-release.
Attachment #8709573 - Flags: approval-mozilla-release+
Attachment #8709573 - Flags: approval-mozilla-beta?
Attachment #8709573 - Flags: approval-mozilla-beta+

Updated

3 years ago
status-firefox44: --- → affected
tracking-firefox44: --- → +

Comment 15

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d13f19fcfd65
status-firefox44: affected → fixed
This was caused by adding "HTML input element's max / min attribute does not work on Fennec" going to the Spanish train site http://www.renfe.com/ and tapping on the date input box (Salida/Regreso) will cause the crash on affected devices. 


57:43.95 LOG: MainThread Bisector INFO Last good revision: 4b8387fe954cff55d04bfb3a1c59077dd4ac7d55
57:43.95 LOG: MainThread Bisector INFO First bad revision: 52a402b0f7bce7cad6b494cc1696b2b8495fe3ce
57:43.95 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=4b8387fe954cff55d04bfb3a1c59077dd4ac7d55&tochange=52a402b0f7bce7cad6b494cc1696b2b8495fe3ce

57:44.19 LOG: MainThread main INFO Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1177510
Blocks: 1177510
status-firefox42: --- → unaffected
status-firefox43: --- → wontfix

Comment 18

3 years ago
I was still able to reproduce this issue following the STR from Kevin's previous comment on Lenovo Yoga Tablet 2 10.1 (Android 4.4.2) on Firefox 44 Beta 11
https://crash-stats.mozilla.com/report/index/5bbe6948-7db4-4e5a-b2b4-126172160120

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f58d8d338d4f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Updated

3 years ago
status-firefox44: fixed → affected
(Reporter)

Comment 20

3 years ago
(In reply to Mihai Pop from comment #18)
> I was still able to reproduce this issue following the STR from Kevin's
> previous comment on Lenovo Yoga Tablet 2 10.1 (Android 4.4.2) on Firefox 44
> Beta 11
> https://crash-stats.mozilla.com/report/index/5bbe6948-7db4-4e5a-b2b4-
> 126172160120

Reopening to investigate.

Thanks for the regression range and STR, Kevin! That should make this much easier to investigate.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
One option would be to back out the HTML feature and work on a fix for 45.
status-firefox44: affected → wontfix
status-firefox45: --- → affected
status-firefox46: fixed → affected
Comment on attachment 8708586 [details]
MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret

Looks like it wasn't the solution.
Attachment #8708586 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
We should back the fix that did not do anything out from m-c and m-a. Despite Sylvestre's decline to take the patch on aurora this was included as part of the uplift of 46 m-c.
Flags: needinfo?(ahunt)
(In reply to Kevin Brosnan [:kbrosnan] from comment #24)
> We should back the fix that did not do anything out from m-c and m-a.
> Despite Sylvestre's decline to take the patch on aurora this was included as
> part of the uplift of 46 m-c.

so we need to back this out from aurora ?
Flags: needinfo?(sledru)
Yes, it seems.
Flags: needinfo?(sledru)
(Assignee)

Comment 27

3 years ago
I've managed to find the underlying problem, the only reproducible device I have access to is the Asus TF300T.

We're receiving an IllegalArgumentException at:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java#291

mMinDate and mMaxDate haven't been set, hence are today's date, so we're actually passing in garbage to setMinDate and setMaxDate - this is quite a simple fix.

However I'm also adding a Part 2 which sanitises the max/min dates, since websites could pass in incorrect dates - I feel that this is better solution than just catching IllegalArgumentExceptions at setMinDate/setMaxDate/setDate (setDate throws too if it's date is inconsistent with min or max).

Interestingly setMinDate and setMaxDate aren't specified as throwing IllegalArgumentExceptions - this really seems to be a case of a few implementations being different:
http://developer.android.com/reference/android/widget/CalendarView.html#setMaxDate%28long%29
Flags: needinfo?(ahunt)
(Assignee)

Updated

3 years ago
Attachment #8708586 - Attachment description: MozReview Request: Bug 1239823 - don't crash when cancelling dialog due to malformed input r=margaret → MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret
(Assignee)

Comment 28

3 years ago
Comment on attachment 8708586 [details]
MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31111/diff/3-4/
(Assignee)

Comment 29

3 years ago
Created attachment 8715542 [details]
MozReview Request: Bug 1239823 - Part 2: sanitise input dates for DatePicker r=margaret

Review commit: https://reviewboard.mozilla.org/r/33499/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33499/
Attachment #8715542 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 30

3 years ago
Also worth noting: on those devices which don't crash, the date selector doesn't allow you to change the date (tested on a Nexus 9, only in landscape mode) - my patches fix that too.

(The calendar-view is quite hard to use on the N9, which I'll file a separate bug about.)
(Assignee)

Updated

3 years ago
Blocks: 1245692
(Reporter)

Comment 31

3 years ago
(In reply to Andrzej Hunt :ahunt from comment #27)
> I've managed to find the underlying problem, the only reproducible device I
> have access to is the Asus TF300T.
> 
> We're receiving an IllegalArgumentException at:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/widget/DateTimePicker.java#291
> 
> mMinDate and mMaxDate haven't been set, hence are today's date, so we're
> actually passing in garbage to setMinDate and setMaxDate - this is quite a
> simple fix.

I did some digging to see how we got into this state, and it looks like this regression was introduced here:
http://hg.mozilla.org/mozilla-central/rev/52a402b0f7bc

We used to set reasonable initial values for mMinDate and mMaxDate earlier in the constructor, but that patch removed that. Sigh. At least it only landed in Fx43.
Keywords: regression
(Reporter)

Comment 32

3 years ago
https://reviewboard.mozilla.org/r/31111/#review30321

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:279
(Diff revision 4)
> +        } catch (Exception ex) {

I don't like that we're catching a generic Excpetion here, but you're just copying over the existing code, so this is fine.
(Reporter)

Comment 33

3 years ago
Comment on attachment 8715542 [details]
MozReview Request: Bug 1239823 - Part 2: sanitise input dates for DatePicker r=margaret

https://reviewboard.mozilla.org/r/33499/#review30323

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:310
(Diff revision 1)
> +            mMinDate = actualMinDate;

Is this expected? Instead of reversing the values, I feel like we should just log an error and fall back to the default min/max. Or just set the min and fall back to a default max if the provided max is lower than the provided min.

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:316
(Diff revision 1)
> +            mMinDate.set(DEFAULT_START_YEAR, Calendar.JANUARY, 1);

Where does mTempDate come from? Briefly skimming the code it looks like that's just today's date, but I could imagine someone making a datetime picker to select a date from a range of values that don't include today. So maybe in that case the temp date should be the min? Or should we also just log an error here and say that you must specify a selected date within the min/max range if you're going to specify a min/max.

Looking at the docs here, it looks like web developers can specify a min/max without a default date:
https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Forms/The_native_form_widgets#Date_and_time_picker
Attachment #8715542 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 34

3 years ago
(In reply to :Margaret Leibovic from comment #33)
> Comment on attachment 8715542 [details]
> MozReview Request: Bug 1239823 - Part 2: sanitise input dates for DatePicker
> r=margaret
> 
> https://reviewboard.mozilla.org/r/33499/#review30323
> 
> ::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:310
> (Diff revision 1)
> > +            mMinDate = actualMinDate;
> 
> Is this expected? Instead of reversing the values, I feel like we should
> just log an error and fall back to the default min/max. Or just set the min
> and fall back to a default max if the provided max is lower than the
> provided min.

I didn't think this through too well... I now think using the default min/max is probably the best solution here to be safe in all cases - there's no way to know what the site developer really wanted if they mixed up their dates, so we should avoid restricting the user in either direction?

Instead if we decide to use the (probably garbage) min or max we could prevent the user from selecting dates that might have been inside the intended range (I suspect that swapping the min/max is the most likely cause of this happening, but there could be other errors too).
 
> ::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:316
> (Diff revision 1)
> > +            mMinDate.set(DEFAULT_START_YEAR, Calendar.JANUARY, 1);
> 
> Where does mTempDate come from? Briefly skimming the code it looks like
> that's just today's date, but I could imagine someone making a datetime
> picker to select a date from a range of values that don't include today. So
> maybe in that case the temp date should be the min? Or should we also just
> log an error here and say that you must specify a selected date within the
> min/max range if you're going to specify a min/max.
>
> Looking at the docs here, it looks like web developers can specify a min/max
> without a default date:
> https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Forms/
> The_native_form_widgets#Date_and_time_picker

mTempDate is just today's date - unless supplied by the website (and it seems perfectly valid to not supply it, especially if the developer wants today to be the default date).

So we'll still need to decide on a default date for those cases where today's date isn't in the time range. I think we should select the middle of the range in this case, as opposed to using the min or max - am I trying to be too clever here? (If we don't supply a date, or supply a date that's not in the expected range, the CalendarView crashes again, so we need to do something)

The reason I'd want this, is because the behaviour of the date-picker (but not the Calendar) can get quite annoying if you start with either the min- or max-date as the currently selected date. I personally tend to start by selecting the day, then the month, and finally the year - if you're at the min/max date already, you can only move the day and month pickers in one direction, which prevents that selection flow.


Just for the record (this doesn't seem like something we can fix): on the Asus device I'm using for testing, if you set a default date that's in the future but in the same month as today's date (i.e. if today is February 2016, set a default date of Febryary 2020), the CalendarView shows the current year as the year number (i.e. February 2016), this updates as soon as you switch months by scrolling (the days in the calendar match the future date, it's just the year number that's wrong). This doesn't happen on the N9.


I've created a simple page to allow testing various scenarios, we should probably use something like this for some tests in future:
http://people.mozilla.org/~ahunt/datetimepicker/test.html
(Assignee)

Comment 35

3 years ago
(In reply to Andrzej Hunt :ahunt from comment #34)
> So we'll still need to decide on a default date for those cases where
> today's date isn't in the time range. I think we should select the middle of
> the range in this case, as opposed to using the min or max - am I trying to
> be too clever here? (If we don't supply a date, or supply a date that's not
> in the expected range, the CalendarView crashes again, so we need to do
> something)

Actually, this is incorrect - on the Asus TF300T CalendarView selects the max-date if no default is supplied (i.e. we don't call setDate). However I don't feel confident in changing that as part of a crash-fix Bug (we reuse mTempDate in various other places, and disentangling that is well beyond the scope of what we're doing here) - we still need to calculate the default date, even if we just copy what Android would do if we didn't provide a default date.
(Assignee)

Comment 36

3 years ago
Comment on attachment 8715542 [details]
MozReview Request: Bug 1239823 - Part 2: sanitise input dates for DatePicker r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33499/diff/1-2/
Attachment #8715542 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 37

3 years ago
Comment on attachment 8715542 [details]
MozReview Request: Bug 1239823 - Part 2: sanitise input dates for DatePicker r=margaret

https://reviewboard.mozilla.org/r/33499/#review30497

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:324
(Diff revision 2)
> +            mTempDate.setTimeInMillis(midDate);

I'm still not sure if this is the right thing to do... what does Chrome do? I feel like picking a date in the middle seems arbitarty to the user, and starting at the start of the range might be more sensible.
Attachment #8715542 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 38

3 years ago
(In reply to :Margaret Leibovic from comment #37)
> Comment on attachment 8715542 [details]
> MozReview Request: Bug 1239823 - Part 2: sanitise input dates for DatePicker
> r=margaret
> 
> https://reviewboard.mozilla.org/r/33499/#review30497
> 
> ::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:324
> (Diff revision 2)
> > +            mTempDate.setTimeInMillis(midDate);
> 
> I'm still not sure if this is the right thing to do... what does Chrome do?
> I feel like picking a date in the middle seems arbitarty to the user, and
> starting at the start of the range might be more sensible.

They select the start of the range - so I'll just go for that.

:antlam is suggesting using the calendar-view everywhere, which would probably make date selection much easier in any case. (See https://bugzilla.mozilla.org/show_bug.cgi?id=1245692#c4 )
(Assignee)

Comment 39

3 years ago
Comment on attachment 8708586 [details]
MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31111/diff/4-5/
(Assignee)

Comment 40

3 years ago
Comment on attachment 8715542 [details]
MozReview Request: Bug 1239823 - Part 2: sanitise input dates for DatePicker r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33499/diff/2-3/
Attachment #8715542 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 41

3 years ago
Comment on attachment 8715542 [details]
MozReview Request: Bug 1239823 - Part 2: sanitise input dates for DatePicker r=margaret

https://reviewboard.mozilla.org/r/33499/#review31309
Attachment #8715542 - Flags: review?(margaret.leibovic) → review+

Comment 43

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4bc4d44956b
https://hg.mozilla.org/mozilla-central/rev/d0e1410dd1d3
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 44

3 years ago
Comment on attachment 8708586 [details]
MozReview Request: Bug 1239823 - Part 1: Parse min/max date before use r=margaret

Approval Request Comment
[Feature/regressing bug #]: Bug 1177510 seemingly caused this crash
[User impact if declined]: Crash when selecting date input fields on some devices (seemingly samsung and asus tablets with older Android versions)
[Describe test coverage new/current, TreeHerder]: manual testing
[Risks and why]: medium-risk: date parsing logic is done earlier, with some changes to our default date-selection.
[String/UUID change made/needed]: none.

(I think bugzilla is confused about approval-mozilla-aurora for this patch, possibly due to some interactions with mozreview and a previous patch being approval-mozilla-aurora being -. As far as I can tell this patch has never had approval- requested for any branches.)
Attachment #8708586 - Flags: approval-mozilla-beta?
Attachment #8708586 - Flags: approval-mozilla-aurora?
Attachment #8708586 - Flags: approval-mozilla-aurora-
(Assignee)

Comment 45

3 years ago
Comment on attachment 8715542 [details]
MozReview Request: Bug 1239823 - Part 2: sanitise input dates for DatePicker r=margaret

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Approval Request Comment
[Feature/regressing bug #]: Bug 1177510 seemingly caused this crash
[User impact if declined]: Crash when selecting date input fields on some devices (seemingly samsung and asus tablets with older Android versions)
[Describe test coverage new/current, TreeHerder]: manual testing
[Risks and why]: medium-risk: date parsing logic is done earlier, with some changes to our default date-selection.
[String/UUID change made/needed]: none.
Attachment #8715542 - Flags: approval-mozilla-beta?
Attachment #8715542 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 46

3 years ago
Ooops, sorry I messed up with the bugzilla templates there : / .

Comment 47

3 years ago
Verified as fixed on latest Nightly on Lenovo Yoga Tablet 2 10.1 (Android 4.4.2) following STR from comment 17. Also there are no crash with this signature on crash-reports
status-firefox47: fixed → verified
Comment on attachment 8715542 [details]
MozReview Request: Bug 1239823 - Part 2: sanitise input dates for DatePicker r=margaret

Taking it for both aurora & beta as it fixes a crash (with many occurrences).
Should be in 45 beta 9.

I think we could write a non regression test here, no?
Attachment #8715542 - Flags: approval-mozilla-beta?
Attachment #8715542 - Flags: approval-mozilla-beta+
Attachment #8715542 - Flags: approval-mozilla-aurora?
Attachment #8715542 - Flags: approval-mozilla-aurora+
Attachment #8708586 - Flags: approval-mozilla-beta?
Attachment #8708586 - Flags: approval-mozilla-beta+
Attachment #8708586 - Flags: approval-mozilla-aurora?
Attachment #8708586 - Flags: approval-mozilla-aurora+
I think this depends on specific hardware Sylvestre.

Comment 52

3 years ago
Verified as fixed on Firefox 45 Beta 9, on Lenovo Yoga Tablet 2 10.1 (Android 4.4.2) following STR from comment 17.
status-firefox45: fixed → verified
Version: unspecified → 43 Branch
You need to log in before you can comment on or make changes to this bug.