fix(ios): avoid min>max date picker crash

[sterlingwes]

Summary

closes #1006
closes #1008

This change fixes a crash on iOS when the underlying UIDatePicker receives minimumDate > maximumDate by:

• adding validation in JS to assert that when both are set, min must be less than max
• updating the native fabric prop update logic to:
  • explicitly handles unsetting prior values on the underlying picker during any change before setting new ones
  • specifically handles the unset case which was handled implicitly before

This is the error that shows after this change in dev mode when this happens (before it just crashes):

Original Crash

*** Terminating app due to uncaught exception 'NSGenericException', reason: 'Start date cannot be later in time than end date!'
*** First throw call stack:
(
	0   CoreFoundation                      0x00000001804c9690 __exceptionPreprocess + 172
	1   libobjc.A.dylib                     0x00000001800937cc objc_exception_throw + 72
	2   Foundation                          0x0000000180f55184 -[_NSConcreteDateInterval initWithStartDate:endDate:] + 396
	3   UIKitCore                           0x00000001850dfee0 -[_UIDatePickerCalendarView _reloadCalendarView] + 352
	4   UIKitCore                           0x00000001850dffa4 -[_UIDatePickerCalendarView _reload] + 48
	5   UIKitCore                           0x0000000185bfd72c -[UIDatePicker _installPickerView:updatingSize:] + 152
	6   UIKitCore                           0x0000000185bfd650 -[UIDatePicker _updatePickerViewIfNecessary] + 120
	7   ReproducerApp.debug.dylib           0x0000000107242670 -[RNDateTimePickerComponentView updatePropsForPicker:props:oldProps:

Test Plan

Reproducer used in screenshots below: https://github.com/sterlingwes/date-picker-min-max-repro

The reproducer tests the following scenarios:

Behaviour before fixes

first	second	third	fourth	fifth
💥

Behaviour after fixes

first	second	third	fourth	fifth

What's required for testing (prerequisites)?

You can either:

• Use the example app here, which has a simple reproduction case for toggling-on min > max constraint
• Use the reproducer

What are the steps to reproduce (after prerequisites)?

To reproduce the crash in the example app, tap the new button "set min > max (error)" then tap the picker date and it should crash. If you test on this branch that includes the changes in ios/fabric/RNDateTimePickerComponentView.mm and in src/utils.js, you should instead see the logbox error shown first above.

Compatibility

The native crash was only evident on iOS, but the JS validation approach taken here ensures the expectation is aligned across both.

Checklist

• I have tested this on a device and a simulator
• I added the documentation in README.md
• I updated the typed files (TS and Flow)
• I added a sample use of the API in the example project (example/App.js)
• I have added automated tests, either in JS or e2e tests, as applicable

• i did not see a reason to add docs for this as the error more clearly documents the expectation now
• i only added js unit specs as the e2e spec tests similar things (js logbox behaviour)

[sterlingwes]

while my reproducer is a naive example, i believe the behaviour leading to the crash was less obvious b/c of this UIDatePicker instance re-update logic:

if a datepicker is rendered (inline on a screen or sheet for example, as is the case in the reproducer), and the component is re-rendered, the crash would occur when either minimumDate or maximumDate is updated in a way that breaks the expected constraint order (min < max) but the prior picker value for one of the props is not unset first

since these conditionals were evaluated independently this seems like the culprit, hence why I've grouped them together to validate them together

[sterlingwes]

Looks like this also might fix #1008

[yonitou]

Thanks for this PR. This bug is really problematic for us since our production users are experiencing a lot of crashes in our datepickers modals/screens. Looking forward to see it merged !
Meanwhile @sterlingwes , do you think it's safe to target your PR in our package.json to use your fix asap ?

[sterlingwes]

@yonitou i don't know enough about your setup or specific issue but i don't see a problem with referencing the PR. we've applied the patch but are still waiting on rolling it out to prod. personally i'd apply it as a patch to whatever version you're on since the NPM package content won't be the same as whatever you pull in from a github reference

[vonovak]

thanks for the PR and the context, really helps with evaluating the code 🙏

[dzbrozek]

tested it, but it still doesn't work correctly when the date is changed, probably because when the date is changed, the old minimumDate and maximumDate are still used

I guess the logic should be more like that:

if (minDateChanged || maxDateChanged || dateChanged) {
    ...
    picker.date = nil;
    picker.minimumDate = nil;
    picker.maximumDate = nil;
   ...
}

[vonovak]

@dzbrozek thanks for the comment, please feel free to open a PR for a fix, explaining how to get the issue you're seeing. Thank you! 🙏

[Junveloper]

tested it, but it still doesn't work correctly when the date is changed, probably because when the date is changed, the old minimumDate and maximumDate are still used

I guess the logic should be more like that:

if (minDateChanged || maxDateChanged || dateChanged) {
    ...
    picker.date = nil;
    picker.minimumDate = nil;
    picker.maximumDate = nil;
   ...
}

Hey mate, I also am experiencing the same issue where minimumDate is retained causing the error. Have you been able to find a workaround by any chance?

[sterlingwes]

If min or max date were unchanged but the date was changed I think this is expected behaviour, no? Can you guys elaborate on the problem scenario for me maybe with some pseudo react code so I can understand the issue? The logic as-is should handle constraint changes fine. Sounds to me like you may be relying on the broken behaviour from before?

[sterlingwes]

I think the only way this could happen is across picker uses since we retain the same underlying UIDatePicker reference. Like maybe one picker has constraints set and another is shown later without constraints and the old picker's constraints are applied to the new one?

I think we're probably overdue for refactoring this to not rely on a shared instance

[dzbrozek]

@sterlingwes It happens when you have multiple pickers with different date/minimumDate/maximumDate. The changes aren't atomic, so the picker is using the new date with old minimumDate/maximumDate, and it's crashing.

const [startDate, setStartDate] = useState<Date | undefined>();
const [birthDate, setBirthDate] =  useState<Date>(subYears(new Date(), 30)));

// first screen
<DateTimePickerModal
        date={startDate}
        mode="date"
        display="inline"
        minimumDate={new Date()}
/>

// second screen
<DateTimePickerModal
        date={birthDate}
        mode="date"
        display="inline"
        minimumDate={subYears(new Date(), 100)}
        maximumDate={subYears(new Date(), 18))}
      />

flow: first screen -> second screen (crash)

[Junveloper]

@dzbrozek @sterlingwes - interestingly, I am also using DateTimePicker inside of a modal. And, like @dzbrozek it crashes on the second screen (i.e. the screen that contains the form has no issues > form is submitted successfully > open the form again > click on the date picker > crashes).

@dzbrozek Do you have an example repository you can provide? That would be really helpful.

[sterlingwes]

Awesome thanks guys, yea looks like the reuse theory. I'll take a look, we probably just have to dealloc something

[Junveloper]

@sterlingwes @vonovak Hey mate, I've created a repository with a bug. Is there something else I can help with?

https://github.com/Junveloper/react-native-datetimepicker-bug

bug.mov

[sterlingwes]

Thanks @Junveloper that was helpful! PR up