356207 - Freeze (hang) after import of .ics file which has BYMONTHDAY and BYDAY in an RRULE

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Comment 1

[Joey Minta]

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.

Comment 2

[Keith G]

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

[Stefan Sitter [:ssitter]]

(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, ...)

Comment 4

[Keith G]

(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.

Comment 5

[Keith G]

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 6

[Keith G]

Is anything going to be done about this critical bug?

Comment 7

[Stefan Sitter [:ssitter]]

The only way to get rid of the problem is by removing entire storage.sdb file -> dataloss. Blocking 0.5?

Comment 8

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

Attachment: patch libical

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.

Comment 9

[Daniel Boelzle [:dbo]]

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

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

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

[Daniel Boelzle [:dbo]]

(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

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

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.

Comment 13

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

patch checked in

Comment 14

[Omar Bajraszewski]

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

Comment 15

[Stefan Sitter [:ssitter]]

If possible there should be a regression test case in Litmus for this issue.

Comment 16

[Stefan Sitter [:ssitter]]

Attachment: xpcshell testcase

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 374585 [details] [diff]
xpcshell testcase

r=philipp. Thanks for all the nice tests :-)

Comment 18

[Stefan Sitter [:ssitter]]

Comment on attachment 374585 [details] [diff]
xpcshell testcase

Pushed to comm-central: <https://hg.mozilla.org/comm-central/rev/de95459e4ad3>

Comment 19

[Keith G]

Please can you sanitize the VEVENT in the first post by removing the person's name from the DESCRIPTION field.  Thank you.

Comment 20

[Keith G]

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

[Daniel Boelzle [:dbo]]

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

[Philipp Kewisch [:Fallen][📱📆]]

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

[Keith G]

Thank you, much appreciated.