Closed Bug 1287067 Opened 8 years ago Closed 8 years ago

Cannot snooze alarms for more than 22 days

Categories

(Calendar :: Alarms, defect)

Lightning 5.2
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jorgk-bmo, Assigned: MakeMyDay)

Details

Attachments

(1 file, 2 obsolete files)

I have an event that I want to postpone by a year. Since this option doesn't exist, I tried to postpone by 365 days.

Sadly the event pops up the very next day.
Does that happen too, if you have ical.js disabled? This is with the current Daily, right?
Yes, current Daily. So you want me to postpone the event not using ical.js? I'll do this next time it pops up tomorrow.
OK, I postponed 365 days with ical.js switched off. The event hasn't sprung up again. Is there any way to check pending reminders to see when they will pop-up next?
The issue appears with both backends and is an integer overflow. calIDuration attributes are of type short which have a maximum size of 32767. The calendar-alarm-wigdet converts all the values to minutes before assigning it to the calIDuration object, so the current maximum snoozing length is limited to about 22 days. The Interface provides the ability to assign in different units up to weeks, which would extend the maximum snooze time to a probably sufficient ammount of time if used.
Component: General → Alarms
Summary: Cannot postpone event by 365 days (using ical.js) → Cannot postpone event by more than 22 days
Version: Trunk → Lightning 5.2
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Summary: Cannot postpone event by more than 22 days → Cannot snooze alarms for more than 22 days
Attached patch IntegerOverflowWhenSnoozing-V1.diff (obsolete) — — Splinter Review
This patch simply takes care to assign the weeks to the duration if any. This extends the maximum snoozing length up to about 68 years, which should be sufficient for this piece of software.
Attachment #8771663 - Flags: review?(matthew.mecca)
Comment on attachment 8771663 [details] [diff] [review]
IntegerOverflowWhenSnoozing-V1.diff

Unfortunately this won't fully work as expected, the alarm still won't fire if it's snoozed out beyond 1 month due to a hard coded limit in the alarm service. For performance reasons, when the service initializes it only retrieves items from the past month, then finds alarms only from that item set.

The simplest solution might be to limit the options in the UI to prevent setting or snoozing alarms more than a month out. A slightly more complicated alternative might be to pref off the alarm range rather than hard coding it, and limit the UI options accordingly, but I could see that causing major performance issues especially with un-cached calendars if a user sets it too high.

A real fix might be to cache all alarms in a local database as discussed in Bug 861594. If we store each alarm along with it's ack status and use it's next calculated fire date as an index to the table, that might resolve the performance issue and allow for an unrestricted date range.
Attachment #8771663 - Flags: review?(matthew.mecca) → review-
Attached patch IntegerOverflowWhenSnoozing-V2.diff (obsolete) — — Splinter Review
Ah yes, I didn't recall that. But we should avoid the integer overflow and the resulting odd user expirience of randomly reappearing alarms that the user expected to have snoozed until a further future even if we cannot accept unlimited snoozing.

As bug 861594 still seems to need some work and I agree that we don't want to degrade performance, I extended the patch (which would be needed anyway once we're able to respect snoozing for more than a month) to prompt the user with a warning when exceeding the current limit and effectively preventing snoozing beyond one month ahead.

This will at least enable to snooze reliably within the period we can deal with and lets the user know when his/her choise cannot be considered.
Attachment #8771663 - Attachment is obsolete: true
Attachment #8771777 - Flags: review?(matthew.mecca)
Comment on attachment 8771777 [details] [diff] [review]
IntegerOverflowWhenSnoozing-V2.diff

Review of attachment 8771777 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't seem to be working for me.

::: calendar/base/content/dialogs/calendar-alarm-dialog.js
@@ +207,5 @@
> +    const LIMIT = Components.interfaces.calIAlarmService.MAX_SNOOZE_MONTHS;
> +
> +    let currentTime = cal.now().getInTimezone(cal.UTC());
> +    let limitTime = currentTime.clone();
> +    limitTime += LIMIT;

probably want limitTime.month += LIMIT here

@@ +209,5 @@
> +    let currentTime = cal.now().getInTimezone(cal.UTC());
> +    let limitTime = currentTime.clone();
> +    limitTime += LIMIT;
> +
> +    let durationUntilLimit = limitTime.substractDate(currentTime);

typo in subtractDate

@@ +212,5 @@
> +
> +    let durationUntilLimit = limitTime.substractDate(currentTime);
> +    if (aDuration.compare(durationUntilLimit) > 0) {
> +        let msg = PluralForm.get(LIMIT, cal.calGetString("calendar", "alarmSnoozeLimitExceeded"));
> +        showError(msg.replace("#1", LIMIT));

With the previous comments fixed I'm still getting "ReferenceError: window is not defined" here

::: calendar/base/src/calAlarmService.js
@@ +569,5 @@
>          // for a month, they'll miss some, but that's a slim chance
>          let start = nowUTC();
>          let until = start.clone();
> +        start.month -= Components.interfaces.calIAlarmService.MAX_SNOOZE_MONTHS;
> +        until.month += Components.interfaces.calIAlarmService.MAX_SNOOZE_MONTHS;

There are a couple other places that should be updated:

https://dxr.mozilla.org/comm-central/source/calendar/base/src/calAlarmService.js#244
https://dxr.mozilla.org/comm-central/source/calendar/base/src/calAlarmService.js#251
Attachment #8771777 - Flags: review?(matthew.mecca) → review-
> > +    let durationUntilLimit = limitTime.substractDate(currentTime);
> > +    if (aDuration.compare(durationUntilLimit) > 0) {
> > +        let msg = PluralForm.get(LIMIT, cal.calGetString("calendar", "alarmSnoozeLimitExceeded"));
> > +        showError(msg.replace("#1", LIMIT));
> 
> With the previous comments fixed I'm still getting "ReferenceError: window
> is not defined" here

Hmm. I don't get this error here. Is your tree up to date? Not the same error, but maybe related to bug 1260768.

The attached patch fixes all the other issues from your last review. Can you give it a try?
Attachment #8771777 - Attachment is obsolete: true
Attachment #8776403 - Flags: review?(matthew.mecca)
Comment on attachment 8776403 [details] [diff] [review]
IntegerOverflowWhenSnoozing-V3.diff

Looks good, r=mmecca
Attachment #8776403 - Flags: review?(matthew.mecca) → review+
https://hg.mozilla.org/comm-central/rev/1e3364773abc64c63afd69cf9a7fcea4cec8cb20
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 5.3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: