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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

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)

Comment 1

[Andrzej Hunt :ahunt]

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

Comment 2

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 3

[Andrzej Hunt :ahunt]

(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

Comment 4

[:Margaret Leibovic]

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.

Comment 5

[Andrzej Hunt :ahunt]

(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.

Comment 6

[:Margaret Leibovic]

(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.

Comment 7

[Andrzej Hunt :ahunt]

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/

Comment 8

[Andrzej Hunt :ahunt]

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/

Comment 9

[Andrzej Hunt :ahunt]

Attachment: 1239823_nullcheck_beta.patch

Patch for beta (patch changes).

Comment 10

[:Margaret Leibovic]

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.

Comment 11

[Andrzej Hunt :ahunt]

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

Comment 12

[Andrzej Hunt :ahunt]

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.

Comment 13

[Andrzej Hunt :ahunt]

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.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-release/rev/fe7c25034192

Comment 17

[Kevin Brosnan [Ex-Mozilla]]

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

Comment 18

[u421692]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f58d8d338d4f

Comment 20

[:Margaret Leibovic]

(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.

Comment 21

[Kevin Brosnan [Ex-Mozilla]]

One option would be to back out the HTML feature and work on a fix for 45.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d13f19fcfd65

Comment 23

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 24

[Kevin Brosnan [Ex-Mozilla]]

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.

Comment 25

[Carsten Book [:Tomcat]]

(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 ?

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Yes, it seems.

Comment 27

[Andrzej Hunt :ahunt]

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

Comment 28

[Andrzej Hunt :ahunt]

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/

Comment 29

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 30

[Andrzej Hunt :ahunt]

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.)

Comment 31

[:Margaret Leibovic]

(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.

Comment 32

[:Margaret Leibovic]

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.

Comment 33

[:Margaret Leibovic]

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

Comment 34

[Andrzej Hunt :ahunt]

(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

Comment 35

[Andrzej Hunt :ahunt]

(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.

Comment 36

[Andrzej Hunt :ahunt]

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/

Comment 37

[:Margaret Leibovic]

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.

Comment 38

[Andrzej Hunt :ahunt]

(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 )

Comment 39

[Andrzej Hunt :ahunt]

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/

Comment 40

[Andrzej Hunt :ahunt]

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/

Comment 41

[:Margaret Leibovic]

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

Comment 42

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/f4bc4d44956b91d8e86ae7caf887a681c1885c2f
Bug 1239823 - Part 1: Parse min/max date before use r=margaret

https://hg.mozilla.org/integration/fx-team/rev/d0e1410dd1d386fc41f2ce1c0b5954af2d2fa66f
Bug 1239823 - Part 2: sanitise input dates for DatePicker r=margaret

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4bc4d44956b
https://hg.mozilla.org/mozilla-central/rev/d0e1410dd1d3

Comment 44

[Andrzej Hunt :ahunt]

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.)

Comment 45

[Andrzej Hunt :ahunt]

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.

Comment 46

[Andrzej Hunt :ahunt]

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

Comment 47

[u421692]

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

Comment 48

[Sylvestre Ledru [:Sylvestre]]

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?

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/36861b484b48
https://hg.mozilla.org/releases/mozilla-aurora/rev/19f7989b29da

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b32162b0ad7f
https://hg.mozilla.org/releases/mozilla-beta/rev/b75080708417

Comment 51

[Kevin Brosnan [Ex-Mozilla]]

I think this depends on specific hardware Sylvestre.

Comment 52

[u421692]

Verified as fixed on Firefox 45 Beta 9, on Lenovo Yoga Tablet 2 10.1 (Android 4.4.2) following STR from comment 17.