Calendar Internal Code has errors when calculating recurrences with all-day and non-allday items



13 years ago
11 years ago


(Reporter: cmtalbert, Assigned: dbo)



Dependency tree / graph
Bug Flags:
wanted-calendar0.8 +




(3 attachments)



13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20060909 Firefox/
Build Identifier: Lightning BuildID 2006092220 Thunderbird version 3 alpha 1 20060919 (personal build)

There are two basic bugs here. One is detailed by the bug 349788. And the resulting fix made in bug 353797 should be rolled back once this bug is fixed.

The other bug that must be addressed here is that because the second icaltime_compare (line 477) does not take into account the fact that the dtend may actually be after the recur until date, you might get extra occurrences in some special cases. (Usually negative offset timezones). The steps for this are below.

Reproducible: Always

Steps to Reproduce:
1. Set your machine and your Lightning/Sunbird timeozne to America/Chicago. (any negative offset timezone should do).
2. Create a recurring all day event, set it to repeat weekly on Wednesday, Thursday, and Saturday.
3. Set the Repeat Until date to be a *FRIDAY* about three weeks away.
4. Allow the event to be created and check in the view

Actual Results:  
There will be an ocurrence of the event on the Saturday following the "Repeat Until" date you selected.

Expected Results:  
You would not have any occurrences of the event after the Repeat Until date.

Note that this fix should also address the failures of bug 349788 and bug 353797.

The internal components, specifically calRecurrenceRule::GetOccurrences does not truly handle the case of all-day events versus non-all day events when calculating occurrences for recurring events. The issue is the icaltime_compare done at line 472. If either one of the parameters into this comparison have the is_date attribute set, then the timezone offset will not be applied to that item during the comparison, and an incorrect comparison may result. 

The items being compared should either BOTH have is_date set to 1 or they should BOTH have is_date set to 0. Then, the comparison will work properly.

Comment 1

13 years ago
This patch shows some of the changes that dmose and I discussed making in the chat. I still don't have a good grasp on how we would set such an attribute. The places that calRecurrenceItem::isNegative is set are as follows:
(grep'd from lxr),
RecurrenceInfo.js 537
calrecurrenceDate.cpp 59
calRecurrenceRule.cpp 61
calStorage Calendar 1497 and 1819
WCAPCalendarItems 152

Also, there might be code that needs to be tweaked in calRecurrenceRule::GetNextOccurrence for these changes as well.
Keywords: relnote
Whiteboard: [cal relnote]
Blocks: 349788

Comment 2

12 years ago
It's not clear to me how bad this bug is.  I don't know how likely people are to be affected by it, and I am not clear on what all the failure modes are anymore.  I'm going to mark [qa discussion needed] to see if we can get some feedback on these two things before nominating it to block 0.5.  
Whiteboard: [cal relnote] → [cal relnote][qa discussion needed]

Comment 3

12 years ago
(In reply to comment #2)
Bug is only reproducible in the case where you have recurring, all-day events AND an RECURUNTIL date that is <= 24 hours from an occurrence of the event.

For example, an all day event that occurs on Mon, Wed, Sat from May 1 2007 to Friday May 25 2007 will show an invalid instance on Saturday May 26 2007.  However, if you change the Recur Until date to be Thursday May 31, 2007, you will not see any extra events.  This is because Thursday May 31, 2007 is more than 24 hours away from the next instance of this occurrence (which would be Saturday June 2, 2007 if the RECUR UNTIL date was not set).

Should be rel-noted.  The probability is pretty high that people will see this if they use all-day weekly occurring events.  If they only have all-day monthly events, then the probability that they will see this is very low.
Whiteboard: [cal relnote][qa discussion needed] → [cal relnote]
Whiteboard: [cal relnote]

Comment 4

12 years ago
FWIW, this shows up for me for daily repeating events.  I use all day events as a lazy shortcut to track when people are on vacation.  I enter an all-day event for the first day someone is out, and then repeat for the rest of the days.  Except now everyone shows as being out one extra day!  (This is my long winded way of saying that, at least for me, this bug is pretty bad!).  Thanks for listening.


12 years ago
Blocks: 356764

Comment 5

12 years ago
Should be resolved for 0.8.
Flags: wanted-calendar0.8+


12 years ago
Depends on: 333363

Comment 6

12 years ago
All-day handling (with RRULE) has been fixed with bug 333363; could this one resolved, too?

Comment 7

12 years ago
Clint, could you please comment if this one has been fixed with bug 333363?
Posted file test calendar
Unfortunately, this bug still exists. I used the attached ics calendar and switched system timezone and Lightning timezone to America/Chicago.
The UNTIL value is actually 20070526T045959Z. That was created using Lightning with these timezones set. If I change that to 20070525T235959Z everything works fine (works also in Lightning v0.7).
Attachment #294234 - Attachment mime type: text/calendar → text/plain

Comment 10

12 years ago
Posted patch fixSplinter Review
This patch
- feeds UNTIL datepicker control with floating jsDates
- enforces same value type as DTSTART
- sets DTSTART's hour/minute/second (unless DTSTART is a date)
Assignee: nobody → daniel.boelzle
Attachment #295080 - Flags: review?(sebo.moz)

Comment 11

11 years ago
Comment on attachment 295080 [details] [diff] [review]

>-            // Convert the datetime from UTC to localtime.
>-            endDate = endDate.getInTimezone(calendarDefaultTimezone());
>+            endDate = endDate.getInTimezone(gStartTime.timezone); // calIRecurrenceRule::endDate is always UTC or floating
>+            var endDate = jsDateToDateTime(getElementValue("repeat-until-date"), gStartTime.timezone);

Further comment: The datepicker for UNTIL follows DTSTART in the very same timezone, not using calendarDefaultTimezone(). IMO this is ok since UNTIL is typically specified with regards to DTSTART (and it's timezone).
Attachment #295080 - Flags: review?(michael.buettner)
Comment on attachment 295080 [details] [diff] [review]

>+            var endDate = jsDateToDateTime(getElementValue("repeat-until-date"), gStartTime.timezone);

Shouldn't it be "floating" here?
(In reply to comment #12)
> Shouldn't it be "floating" here?
No, it shouldn't. endDate will be converted to UTC in calRecurrenceRule::SetEndDate.


11 years ago
Attachment #295080 - Flags: review?(sebo.moz) → review+
Comment on attachment 295080 [details] [diff] [review]

Attachment #295080 - Flags: review?(michael.buettner) → review+

Comment 15

11 years ago
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Last Resolved: 11 years ago
Keywords: relnote
Resolution: --- → FIXED
Target Milestone: --- → 0.8

Comment 16

11 years ago
Verified in latest nightly build 20080114 -> task is fixed.
You need to log in before you can comment on or make changes to this bug.