382840 - Alarms in ics files don't fire when scheduled - but do fire after editing

Status: VERIFIED FIXED

(Calendar :: Provider: ICS/WebDAV, defect)

Description

[andrewz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: TB version 2.0.0.0 (20070326), Lightning 2007060103

I create a task with alarm for the future, but it doesn't fire.  About eighteen (18) hours after it should fire, I edit it, close the edit dialog, and then it fires.  This has happened at least twice.

Reproducible: Always

Steps to Reproduce:
1. Create task.
2. Set date/due date in a few minutes.
3. Set alarm for 1 minute before.
4. Wait until after alarm is due.
5. Edit task.
6. Close task editor.
Actual Results:  
Alarm /sometimes/ fires after step #6, but sometimes it never fires.

Expected Results:  
Alarm fires at step #4.

I tested with storage is WebDAV on passwordless LAN Apache (my usual) and local storage (which I just created).  The local storage seemed to work ok.

Comment 1

[Stefan Sitter [:ssitter]]

Confirmed using Mozilla/5.0 (Windows; U; Windows NT 5.0; de; rv:1.8.1.4pre) Gecko/20070530 Sunbird/0.5 and a .ics file on local disk.

The ics provider doesn't fire alarms for event or task. Neither on load, manually reload or application restart an alarm is fired. There are no errors in the Error Console. There are no dump messages in the console that the alarms are processed at all.

This definitely looks like a regression.

Comment 2

[Stefan Sitter [:ssitter]]

Regression range:

WORKS in Sunbird/0.5pre (2007-04-04-03)
FAILS in Sunbird/0.5pre (2007-04-05-06)

Checkins to directory mozilla/calendar during regression range:
http://tinyurl.com/2hn6pp

All checkins during regression range:
http://tinyurl.com/ytn7cq

Bug 373742 looks suspicious

Comment 3

[Stefan Sitter [:ssitter]]

(In reply to comment #2)
> Bug 373742 looks suspicious

Indeed, reverting the changes from Bug 373742 to calICSCalendar.js makes the alarms fire again.

Comment 4

[Simon Paquet [:sipaq]]

Mickey, can you please look at this ASAP as I agree with Stefan, that this may be something that we would like to respin for and your patch in bug 373742 probably regressed this.

Comment 5

[Simon Paquet [:sipaq]]

Matt, since you're the release driver. This might be a regression that we may want to provide a fix for *before* we release 0.5.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

This obviously blocks 0.5

The patch in bug 373742 is indeed wrong. It moved the call to addObserver, resulting in not notifying observers on the initial load or about any changes that were made to the ics file by some third party.
A solution might be to explicitly remove all items from the memory calendar instead of throwing the calendar away. Then the view know they have to refresh the event boxes.
Another, imo somewhat ugly, solution would be to make the alarm observer also observe onLoad calls. I'm not sure if that will fix all the cases.

Comment 7

[Daniel Boelzle [:dbo]]

Attachment: using read-lock for getting items

@mvl: I still think the mentioned patch does it right, it's semantically initialization, not item creation (->addItem).
The problem with process startup is that there is no read-lock mimic separating concurrent refresh() from getItem[s]() calls. Setting the initial uri triggers an async refresh filling up the calendar. The alarm service's initial getItems() just doesn't get any items at that stage, because the file hasn't been read yet. This patch fixes that, queuing into the existing lock queue.
Additionally, I have removed "this.loading = true" which seems to be a leftover.

Comment 9

[Stefan Sitter [:ssitter]]

I did a quick test with the patch and the following is working:
* missed alarms in .ics file are fired during Sunbird startup
* future alarms in .ics file are scheduled during Sunbird startup
* alarms on new created items fire on the scheduled time

The following is not working:
While Sunbird is running an item with alarm was added to .ics calendar using external application. Upon reload (automatically or manually via toolbar) the item is correctly displayed in the calendar but no alarm is scheduled/fired. Sunbird restart is required to get the alarm scheduled/fired.

Comment 10

[Daniel Boelzle [:dbo]]

Attachment: using onLoad reloading alarms

This patch addresses ssitter's comments. Minding calICalendar::refresh's comment, onLoad is sent out by the provider if details are unknown. Onload will now trigger a reget of the initial alarm range +/- 1 month from now, like at startup. Additionally, it fixes the missing get of alarms when a calendar is registered the first time.
IMO the current alarm system is very hard to predict and I think we should generally rework it.

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 267135 [details] [diff] [review]
using read-lock for getting items

>     refresh: function() {
>-        // Lock other changes to the item list.
>-        this.lock();

This makes me think that we should put refresh() in the queue too. Otherwise, you can call refresh twice, and things will get confused.

Comment 12

[Daniel Boelzle [:dbo]]

Attachment: using read-lock for getting items

(In reply to comment #11)
> This makes me think that we should put refresh() in the queue too. Otherwise,
> you can call refresh twice, and things will get confused.
Right, IMO a sensible improvement, although I cannot foresee whether this currently causes any harm.
Once we look at that code, IMO we should fix that, too. I've revised the patch (which now contains the onLoad change of calIcsCalendar from patch 267415, too).

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 267704 [details] [diff] [review]
using read-lock for getting items

>+                // reschedule a refresh for next round, after the file has been written;
>+                // strictly we may not need to refresh once the file has been successfully
>+                // written, but we don't know if that write will succeed.

I disagree with that change. Callers might put the refresh call before a change for good reason. They likely want to be sure to operate on the most recent file. I think that the refresh should be executed in the order it was in the queue. It should stop processing the queue though.

Comment 14

[Daniel Boelzle [:dbo]]

(In reply to comment #13)
> I disagree with that change. Callers might put the refresh call before a change
> for good reason. They likely want to be sure to operate on the most recent
> file. I think that the refresh should be executed in the order it was in the
> queue. It should stop processing the queue though.

Yes, makes sense. Although at least for *modifying* requests, programming the way you mentioned (refresh, then modify) is wrong, because a refresh() call runs asynchronously. Callers must wait for the calendar being refreshed (what they currently can't) before issuing modifying requests, else they apply changes on potentially old data.
However, IMO we should continue with this bug. The inclusion of refresh into the queue is just an additional fix for a potential problem you mentioned; the current code does not mind guarding of concurrent refresh/get/modify calls.

Comment 15

[Daniel Boelzle [:dbo]]

Attachment: using read-lock for getting items

Updated patch reflecting recent comments.

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 267415 [details] [diff] [review]
using onLoad reloading alarms

>+        onLoad: function co_onLoad(calendar) {
please use aCalendar instead of calendar, here and in a lot of other places in the patch.

>+                    // This is our first search for alarms.  We're going to look for
>+                    // alarms +/- 1 month from now.  If someone sets an alarm more than
>+                    // a month ahead of an event, or doesn't start Sunbird/Lightning
>+                    // for a month, they'll miss some, but that's a slim chance

>+        // Total refresh similar to startup.  We're going to look for
>+        // alarms +/- 1 month from now.  If someone sets an alarm more than
>+        // a month ahead of an event, or doesn't start Sunbird/Lightning
>+        // for a month, they'll miss some, but that's a slim chance

I really don't like the idea of having this logix in here twice. IS there really no way you can unify this?

r=mvl with at least the first comment fixed, and if you don't fix the second, please give a good reason.

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 267862 [details] [diff] [review]
using read-lock for getting items

r=mvl

Comment 18

[Daniel Boelzle [:dbo]]

(In reply to comment #16)
The timer notification code keeps track on shifting mRangeEnd while the refresh code doesn't care about mRangeEnd. It's pretty straight-forward code, a unification would IMO only complicate reading it.

Comment 19

[Daniel Boelzle [:dbo]]

Checked in on HEAD, MOZILLA_1_8_BRANCH, SUNBIRD_0_5_BRANCH and moved LIGHTNING_0_5_RELEASE and SUNBIRD_0_5_RELEASE tags.

Comment 20

[Michiel van Leeuwen (email: mvl+moz@)]

My concern isn't so much the duplicated code, as well as the duplicated logic. The code makes assumptions about one month being enough. If we ever change that, we now need to aware of the fact that you need to change it twice.
How about adding a comment to both places pointing to the other place? That way, when you change one piece of code, you know you should also change the other.

Comment 21

[Shinya Kuribayashi]

Verified.

0.5     | Sunbird    | Lightning
Linux   | 2007061404 | 2007061303
Windows | 2007061404 | 2007061304

Comment 22

[Martin Schröder [:mschroeder]]

Marking VERIFIED per comment #21.