Closed Bug 353887 Opened 18 years ago Closed 16 years ago

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

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: cmtalbert, Assigned: dbo)

References

()

Details

(Keywords: dataloss)

Attachments

(3 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
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.
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
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]
(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]
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.
Blocks: 356764
Should be resolved for 0.8.
Flags: wanted-calendar0.8+
Depends on: 333363
All-day handling (with RRULE) has been fixed with bug 333363; could this one resolved, too?
Clint, could you please comment if this one has been fixed with bug 333363?
Attached 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
Attached 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
Status: NEW → ASSIGNED
Attachment #295080 - Flags: review?(sebo.moz)
Comment on attachment 295080 [details] [diff] [review]
fix

>-            // 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]
fix

[snip]
>+            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.
r=sebo

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

r=mickey.
Attachment #295080 - Flags: review?(michael.buettner) → review+
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: relnote
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Verified in latest nightly build 20080114 -> task is fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.