Closed
Bug 356207
Opened 18 years ago
Closed 18 years ago
Freeze (hang) after import of .ics file which has BYMONTHDAY and BYDAY in an RRULE
Categories
(Calendar :: Internal Components, defect)
Calendar
Internal Components
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: sipaq, Unassigned)
Details
(Keywords: dataloss, hang)
Attachments
(1 file)
845 bytes,
patch
|
dbo
:
first-review+
|
Details | Diff | Splinter Review |
The hang isn't in importing, but once we try to display the event by calling ev.recurrenceInfo.getOccurrences(). That will go into an infinite loop trying to return the proper occurrences here.
It has been suggested (by Stefan Sitter) that the problem lies with the unusual recurrance rule BYMONTHDAY.
I guess that BYMONTHDAY=1;BYDAY=WE is supposed to be equivilent to BYDAY=1WE. If
so, then Sunbird should handle them both in the same way.
Keith.
Comment 3•18 years ago
|
||
(In reply to comment #2)
No, BYMONTHDAY=1;BYDAY=WE and BYDAY=1WE have a different meaning.
I found this example in RFC 2445 [http://www.faqs.org/rfcs/rfc2445.html]:
Every Friday the 13th, forever:
RRULE:FREQ=MONTHLY;BYDAY=FR;BYMONTHDAY=13
==> (1998 9:00 AM EST)February 13;March 13;November 13
(1999 9:00 AM EDT)August 13
(2000 9:00 AM EDT)October 13
...
That means for monthly recurrence using:
BYMONTHDAY=1;BYDAY=WE ==> Every Wednesday the 1st
(e.g. 01-Feb-2006, 01-Mar-2006, 01-Nov-2006, 01-Aug-2007, ...)
BYDAY=1WE ==> Every 1st Wednesday in month
(e.g. 04-Jan-2006, 01-Feb-2006, 01-Mar-2006, 05-Apr-2006, ...)
(In reply to comment #3)
> That means for monthly recurrence using:
> BYMONTHDAY=1;BYDAY=WE ==> Every Wednesday the 1st
> (e.g. 01-Feb-2006, 01-Mar-2006, 01-Nov-2006, 01-Aug-2007, ...)
> BYDAY=1WE ==> Every 1st Wednesday in month
> (e.g. 04-Jan-2006, 01-Feb-2006, 01-Mar-2006, 05-Apr-2006, ...)
>
This is odd. The original event came from my iCalendar file exported from Outlook using Outlook2vCal. In Outlook the recurrance of this event was "The 1st Wednesday of every 1 months". Possibly Outlook2vCal is creating an incorrect RRULE?
Either way Sunbird ideally should handle this RRULE properly, and it certianly shouldn't hang when it encounters it.
Additional Information:
The recurrance pattern BYMONTHDAY=n;BYDAY= seems to work fine for all values of n between 2 and 31. For example the following event recurs every Wednesday 4th without problem:
BEGIN:VCALENDAR
PRODID:-//Randy L Pearson//NONSGML Outlook2vCal V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20040829T163323
UID:00000000EBFAC68C9B92BF119D643623FBD17E1424312000
SEQUENCE:1
LAST-MODIFIED:20060615T231158
DTSTAMP:20040829T163323
ORGANIZER:Unknown
DTSTART:20061004T141500
DESCRIPTION:None.
CLASS:PUBLIC
LOCATION:Church
CATEGORIES:Church Events
SUMMARY:RRULE TEST
PRIORITY:1
DTEND:20061004T141500
RRULE:FREQ=MONTHLY;INTERVAL=1;BYMONTHDAY=4;BYDAY=WE
END:VEVENT
END:VCALENDAR
But when BYDAY is set and BYMONTHDAY equals 1 or is greater than 31, Sunbird hangs as described. This isn't so serious when BYMONTHDAY > 31 as it's invalid, but BYMONTHDAY=1 is valid and should be handled properly.
It should also be noted that events that set both BYMONTHDAY and BYDAY can't be edited properly, and the UI doesn't allow for this.
Comment 7•18 years ago
|
||
The only way to get rid of the problem is by removing entire storage.sdb file -> dataloss. Blocking 0.5?
Component: Import and Export → Internal Components
Flags: blocking-calendar0.5?
Keywords: dataloss
QA Contact: import-export → base
Summary: Sunbird hangs when importing .ics files which have BYMONTHDAY and BYDAY in an RRULE → Freeze (hang) after import of .ics file which has BYMONTHDAY and BYDAY in an RRULE
Version: unspecified → Trunk
Updated•18 years ago
|
Flags: blocking-calendar0.5? → blocking-calendar0.5+
Comment 8•18 years ago
|
||
The problem is that libical first finds the end of the month, and sees that it is invalid. Then it increases the month, but forgets to re-set the day. So now it start searching at day 2, never finding the item that should recur on day 1.
This patch fixes that. I can now have a recurring event on every friday the 13th.
Attachment #248432 -
Flags: first-review?(daniel.boelzle)
Comment 9•18 years ago
|
||
Comment on attachment 248432 [details] [diff] [review]
patch libical
> impl->last.day = 1;
> increment_month(impl);
>+ impl->last.day--; /* Go back one day, so searches next month start at day 1 */
Having a look at increment_month()'s else-branch (no by-data), I don't see impl->last.day being modified. So in this case, impl->last.day will be decremented to 0. Is this correct?
Michiel, can you elaborate a bit more on your fix, please? If searches for next month ought to start at day 1, why not force it to 1?
Comment 10•18 years ago
|
||
That's right, day is now set to zero. But look at the for loop:
1506 for(day = impl->last.day+1; day <= days_in_month; day++){
The loop starts at the next day. And it must, because otherwise it will find the same day over and over again, ending up in a (different) never ending loop.
Comment 11•18 years ago
|
||
(In reply to comment #10)
> That's right, day is now set to zero. But look at the for loop:
> 1506 for(day = impl->last.day+1; day <= days_in_month; day++){
>
> The loop starts at the next day. And it must, because otherwise it will find
> the same day over and over again, ending up in a (different) never ending loop.
>
Ok. Is impl->last.day affected by increment_month() at all? If not, why can't we force impl->last.day = 0?
Comment 12•18 years ago
|
||
I used the subtraction, because I think that is a smaller change. I can't think of every rrule this that can tigger this code, so i don't know if the day may ever not be 1. So using -1 felt safer.
Updated•18 years ago
|
Attachment #248432 -
Flags: first-review?(daniel.boelzle) → first-review+
Comment 13•18 years ago
|
||
patch checked in
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 14•17 years ago
|
||
Verified on Sunbird 0.5 RC 1 Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.4pre) Gecko/20070524 Sunbird/0.5
Status: RESOLVED → VERIFIED
Comment 15•17 years ago
|
||
If possible there should be a regression test case in Litmus for this issue.
Flags: in-litmus?
Updated•17 years ago
|
Flags: in-litmus? → in-testsuite?
Reporter | ||
Updated•17 years ago
|
Flags: blocking-calendar0.5+
Comment 16•16 years ago
|
||
Comment 17•16 years ago
|
||
Comment on attachment 374585 [details] [diff]
xpcshell testcase
r=philipp. Thanks for all the nice tests :-)
Comment 18•16 years ago
|
||
Comment on attachment 374585 [details] [diff]
xpcshell testcase
Pushed to comm-central: <https://hg.mozilla.org/comm-central/rev/de95459e4ad3>
Updated•16 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 19•15 years ago
|
||
Please can you sanitize the VEVENT in the first post by removing the person's name from the DESCRIPTION field. Thank you.
Comment 20•15 years ago
|
||
I AM SERIOUS ABOUT THIS!!!
Please can you sanitize the VEVENT in the first post by removing the person's
name from the DESCRIPTION field. Thank you.
Comment 21•15 years ago
|
||
You talk about the attached unit test? We could change that in the source tree, but we cannot change attachments of this bug; we're not admins of this bugzilla setup.
Comment 22•15 years ago
|
||
Comment on attachment 374585 [details] [diff]
xpcshell testcase
Marking the testcase private as per comment #19. Sorry for the delay.
It will be hard to remove the name from the hg history, since we've already checked it in. I'll try to find an admin that might be able to take care.
Comment 23•15 years ago
|
||
Thank you, much appreciated.
You need to log in
before you can comment on or make changes to this bug.
Comment 1
•