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

VERIFIED FIXED

Status

Calendar
Internal Components
--
critical
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: sipaq, Unassigned)

Tracking

({dataloss, hang})

Trunk
dataloss, hang
Bug Flags:
in-testsuite +

Details

Attachments

(1 attachment)

Comment 1

11 years ago
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

11 years ago
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

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

Comment 4

11 years ago
(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

11 years ago
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

11 years ago
Is anything going to be done about this critical bug?

Comment 7

11 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

11 years ago
Flags: blocking-calendar0.5? → blocking-calendar0.5+
Created attachment 248432 [details] [diff] [review]
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.
Attachment #248432 - Flags: first-review?(daniel.boelzle)

Comment 9

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

11 years ago
Attachment #248432 - Flags: first-review?(daniel.boelzle) → first-review+
patch checked in
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 14

11 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

11 years ago
If possible there should be a regression test case in Litmus for this issue.
Flags: in-litmus?

Updated

10 years ago
Flags: in-litmus? → in-testsuite?
(Reporter)

Updated

10 years ago
Flags: blocking-calendar0.5+

Comment 16

9 years ago
Created attachment 374585 [details] [diff]
xpcshell testcase
Comment on attachment 374585 [details] [diff]
xpcshell testcase

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

Comment 18

9 years ago
Comment on attachment 374585 [details] [diff]
xpcshell testcase

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

Updated

9 years ago
Flags: in-testsuite? → in-testsuite+

Comment 19

9 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

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

9 years ago
Thank you, much appreciated.
You need to log in before you can comment on or make changes to this bug.