Last Comment Bug 353887 - Calendar Internal Code has errors when calculating recurrences with all-day and non-allday items
: Calendar Internal Code has errors when calculating recurrences with all-day a...
Status: VERIFIED FIXED
: dataloss
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: unspecified
: All All
: -- normal (vote)
: 0.8
Assigned To: Daniel Boelzle [:dbo]
:
Mentors:
http://lxr.mozilla.org/seamonkey/sour...
Depends on: 333363
Blocks: 349788 356764
  Show dependency treegraph
 
Reported: 2006-09-22 15:58 PDT by cmtalbert
Modified: 2008-01-15 01:52 PST (History)
6 users (show)
dbo.moz: wanted‑calendar0.8+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Test patch that shows some of the changes that we will have to make for this change (8.15 KB, patch)
2006-09-22 16:52 PDT, cmtalbert
no flags Details | Diff | Splinter Review
test calendar (927 bytes, text/plain)
2007-12-21 09:38 PST, Sebastian Schwieger
no flags Details
fix (3.47 KB, patch)
2008-01-02 04:17 PST, Daniel Boelzle [:dbo]
sebo.moz: review+
michael.buettner: review+
Details | Diff | Splinter Review

Description cmtalbert 2006-09-22 15:58:31 PDT
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.
Comment 1 cmtalbert 2006-09-22 16:52:19 PDT
Created attachment 239738 [details] [diff] [review]
Test patch that shows some of the changes that we will have to make for this change

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.
Comment 2 cmtalbert 2007-02-26 06:54:48 PST
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.  
Comment 3 cmtalbert 2007-05-09 22:18:07 PDT
(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.
Comment 4 Kenneth Tanzer 2007-07-06 12:00:08 PDT
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.
Comment 5 Daniel Boelzle [:dbo] 2007-11-08 08:50:07 PST
Should be resolved for 0.8.
Comment 6 Daniel Boelzle [:dbo] 2007-11-26 03:03:40 PST
All-day handling (with RRULE) has been fixed with bug 333363; could this one resolved, too?
Comment 7 Daniel Boelzle [:dbo] 2007-12-13 09:07:17 PST
Clint, could you please comment if this one has been fixed with bug 333363?
Comment 8 Sebastian Schwieger 2007-12-21 09:38:50 PST
Created attachment 294234 [details]
test calendar

Unfortunately, this bug still exists. I used the attached ics calendar and switched system timezone and Lightning timezone to America/Chicago.
Comment 9 Sebastian Schwieger 2007-12-21 09:59:37 PST
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).
Comment 10 Daniel Boelzle [:dbo] 2008-01-02 04:17:58 PST
Created attachment 295080 [details] [diff] [review]
fix

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)
Comment 11 Daniel Boelzle [:dbo] 2008-01-03 02:00:32 PST
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).
Comment 12 Sebastian Schwieger 2008-01-04 08:34:40 PST
Comment on attachment 295080 [details] [diff] [review]
fix

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

Shouldn't it be "floating" here?
Comment 13 Sebastian Schwieger 2008-01-04 09:31:24 PST
(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

Comment 14 Michael Büttner (no reviews TFN) 2008-01-10 02:08:02 PST
Comment on attachment 295080 [details] [diff] [review]
fix

r=mickey.
Comment 15 Daniel Boelzle [:dbo] 2008-01-10 02:20:28 PST
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Comment 16 Andreas Treumann 2008-01-15 01:52:08 PST
Verified in latest nightly build 20080114 -> task is fixed.

Note You need to log in before you can comment on or make changes to this bug.