354194 - Disabling alarm of a snoozed task/events causes an error, menu items are disabled

Status: VERIFIED FIXED

(Calendar :: Alarms, defect)

Description

[Omar Bajraszewski]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060923 BonEcho/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060923 BonEcho/2.0

When you snooze alarm of a task and then change the option to 'none' it causes an java error:

Error: alarmTime has no properties
Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/js/calAlarmService.js
Line: 378
Error: alarmTime has no properties
Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/js/calAlarmService.js
Line: 378
Error: alarmTime has no properties
Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/js/calAlarmService.js
Line: 378
Error: uncaught exception: [Exception... "'[JavaScript Error: "alarmTime has no properties" {file: "file:///C:/Program%20Files/Mozilla%20Sunbird/js/calAlarmService.js" line: 378}]' when calling method: [calIOperationListener::onGetResult]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Sunbird/components/calStorageCalendar.js :: queueItems :: line 621"  data: yes]

Reproducible: Always

Steps to Reproduce:
1.Set up a task with alarm and wait until it is fired
2.Snooze alarm for 5 minutes
3.Edit the task and change alarm option to 'none'
4.Restart Sunbird
Actual Results:  
1.Error in console
2.'Edit selected event' and 'Delete Selected Event' in context menu in calendar view is disabled


Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060925 Calendar/0.3a2+

Comment 1

[Omar Bajraszewski]

I can still reproduce it using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20061227 Calendar/0.4a1 but I can see different errors in console:
Error: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [calIDateTime.addDuration]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: file:///C:/Documents%20and%20Settings/Omar/Pulpit/sunbird/js/calAlarmService.js :: anonymous :: line 395"  data: no]
Source File: file:///C:/Documents%20and%20Settings/Omar/Pulpit/sunbird/js/calAlarmService.js
Line: 395

And I can also reproduce it for events. Menu items are still disabled

Comment 2

[Stefan Sitter [:ssitter]]

Issue still exists using Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.7pre) Gecko/20070824 Calendar/0.7pre

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [calIDateTime.addDuration]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: file:///D:/Programme/sunbird/js/calAlarmService.js :: anonymous :: line 428"  data: no]

Comment 3

[Stefan Sitter [:ssitter]]

Attachment: rev0 - add error check, remove snooze time if setting alarm to none

Added an error check in addAlarm() to not fail on existing event/tasks created with previous releases. If the alarm is reset to None in the dialog also remove the obsolete snooze property. I also took opportunity to remove the anonymous functions.

Comment 4

[Stefan Sitter [:ssitter]]

Attachment: ics testcase from sunbird 0.5

Comment 5

[Daniel Boelzle [:dbo]]

Comment on attachment 278183 [details] [diff] [review]
rev0 - add error check, remove snooze time if setting alarm to none

>Index: mozilla/calendar/base/src/calAlarmService.js
>         var offset = aItem.alarmOffset || aItem.parentItem.alarmOffset;
>-
>-        alarmTime.addDuration(offset);
>+        if (offset) {
>+            alarmTime.addDuration(offset);
>+        }
Although this check makes the code more robust, it is strictly not needed if hasAlarm() works as expected, returning true iff the item actually has an alarm specified. Thus IMO the core problem is that hasAlarm (from line 255) may return true if only a snooze X-props is present, which I doubt is sensible. I'd suggest to use hasAlarm as per line 88:

            function hasAlarm(a) {
                return a.alarmOffset || a.parentItem.alarmOffset;
            }

Maybe the snooze part from the second definition is a leftover; someone knows why it's in?

>Index: mozilla/calendar/prototypes/wcap/sun-calendar-event-dialog.js
> function saveReminder(item) {
>     var reminderPopup = document.getElementById("item-alarm");
>     if (reminderPopup.value == 'none') {
>         item.alarmOffset = null;
>         item.alarmLastAck = null;
>         item.alarmRelated = null;
>+        item.deleteProperty("X-MOZ-SNOOZE-TIME");
This won't cover snoozing of occurrences, which are "X-MOZ-SNOOZE-TIME-" + aItem.recurrenceId.nativeTime, and even then I'd prefer that resetting the alarm offset would implicitly also reset alarmLastAck and any snoozes which would be an issue of calItemBase.js then.

Comment 6

[Stefan Sitter [:ssitter]]

(In reply to comment #5)
> Thus IMO the core problem is that hasAlarm (from line 255) may
> return true if only a snooze X-props is present, which I doubt is sensible.

The check for X-MOZ-SNOOZE-TIME in hasAlarm was added in Bug 298358. 
Jminta, do you remember the reasons for this?

Comment 7

[Joey Minta]

(In reply to comment #6)
I don't recall the exact reasoning behind the inclusion of the snooze check.  I think it may have been in order to address this case, where an alarm is deleted while a snooze is pending.  In that instance, the check would be designed to still fire the alarm.

However, I'm not convinced that firing the snooze alarm is the most intuitive response to that set of user-actions.  If someone decides that it would be better to cancel the snooze as well, I *think* it would be ok to remove the line.  Still, the alarm code has started getting swapped out my brain, so I'm not as familiar with it as I once was.

Comment 8

[Stefan Sitter [:ssitter]]

Attachment: rev1 - change hasAlarm() as suggested

This patch changes hasAlarm() as suggested in Comment #5 to prevent the error. During testing I saw no issues with snoozed alarms. In addition onAddItem() uses the same logic: <http://lxr.mozilla.org/mozilla1.8/source/calendar/base/src/calAlarmService.js#87>.

This patch doesn't take care of removing the "X-MOZ-SNOOZE-TIME" property upon removing the alarm from the item / parent item. This needs to be done in a followup patch or bug.

Comment 9

[Daniel Boelzle [:dbo]]

Comment on attachment 279951 [details] [diff] [review]
rev1 - change hasAlarm() as suggested

r=dbo, thanks for the patch.

Comment 10

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 11

[Omar Bajraszewski]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070908 Calendar/0.7pre