333363 - Some providers return an allday event on the day of the event, and the day after

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

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

Follow-up from bug 306157.
In that bug, it was discovered that at least the memory and the storage calendar can return an all-day event on the day of the event, and on the day after (when querying for events in one day).

The problem seems to be in using calIDateTime.nativeTime, which isn't really defined for an all-day event. Also, there are problems with timezone conversion.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

I think this could lead to dataloss issues if one tried to manipulate the bogus occurrence.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Marking as major, since this involves incorrect display of calendar data.

Comment 3

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

Comment 4

[Sebastian Schwieger]

Bug 330496 seems to be related to this one.

Comment 5

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

I don't see how this can lead to dataloss.
bug 306157 makes the items that should not be returned from the providers not show up, so the user doesn't have access to the bogus occurences.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

As much as it pains me, I think we can live without this for 0.3.

Comment 7

[Omar Bajraszewski]

0.7 is released- removing blocking flag and proposing wanted0.8

Comment 8

[Sebastian Schwieger]

A partial fix is provided in bug 354198.

Comment 9

[Daniel Boelzle [:dbo]]

Attachment: fix - calRecurrenceInfo, calMemoryCalendar

- Due to the fact that the memory calendar's queries are hardly maintainable, I've rewritten that code using calUtils' checkIfInRange.
- It has shown up that the inherent problem of e.g. bug 354198 (recurring all-day events) is (once again) calRecurrenceInfo's expansion code, namely calculateDates.

Comment 10

[Daniel Boelzle [:dbo]]

Attachment: fix - calStorageCalendar

This changes the storage calendar's query to not incorporate the end date into the result set. Maybe I am wrong, then maybe mvl might tell me... :)

These two patches fix bug 354198 for me as well.

Comment 11

[Daniel Boelzle [:dbo]]

Since code in this area is very sensitive to changes, I'd want some additional testing of the attached patches; setting QAWANTED.

Comment 12

[Sebastian Schwieger]

(In reply to comment #10)
> Created an attachment (id=287692) [details]
> fix - calStorageCalendar
> 
Wouldn't this fail for zero-length items starting at rangeStart?

Comment 13

[Daniel Boelzle [:dbo]]

Attachment: fix - calStorageCalendar

@sebo: correct.
This patch should include zero-length items; we need a unit test for this.
Omitting review for now since this patch is only roughly tested, although comments/further testing is greatly appreciated, especially on tasks.

Comment 14

[Sebastian Schwieger]

(In reply to comment #13)
> comments/further testing is greatly appreciated, especially on tasks.
> 
If a due date or a start date is not set it gets assigned "null" (calStorageCalendar::setDateParamHelper). I think a check for that is needed as well, as we have assigned duedate = startdate in other places in these situations.

Would that work?
   WHERE  (event_end > :range_start AND event_start < :range_end) OR
          (event_end = :range_start AND event_start = :range_start) OR 
          (event_end = null AND event_start = :range_start) OR
          (event_end = :range_start AND event_start = null)

Comment 15

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

Those zero-length events make no real sense to me. The end-time is exclusive, so they start at a certain time, and end just a little bit before that same time. I fail to understand that. But I already wrote that in bug 333362.

> Wouldn't this fail for zero-length items starting at rangeStart?
Is that really a bug? The event ends just before the rangeStart, so I don't think it needs to be included in the result set.
Although that does lead to problems for zero-length events at midnight when using the dayview, because those events would not show. But that's what you get for having events that make no sense...

Comment 16

[Sebastian Schwieger]

Attachment: preliminary unit Tests

two unit tests for the memory and the storage calendar. Note that at the moment, the test.sdb database for the storageCalendar test needs to be removed between each run of the unit test.
The storageCalendar fails for the third todo entry (no DTSTART set), while memoryCal does not fail.

Comment 17

[Sebastian Schwieger]

Daniel, I cannot build at home atm due to bug 387239. I tested your patch during last week and it seems to work well. Final review will be during next week.

Comment 18

[Sebastian Schwieger]

Attachment: new version of unit test for memory calendar

This is a new version of the unit test for the memory calendar. It showed an error for daily recurring all-day events. If the range is set to the same times as the parent item, getItems() returns the parent item and the following occurrence. That means endRange seems not to be exclusive.

Comment 19

[Sebastian Schwieger]

The bug I mentioned in comment 18 seems to be addressed in bug 353887. I have verified, that it is not a regression of the proposed patch. 
As a workaround, one could check in calReccurrenceInfo::calculateDates if startDate isDate. Consequently, rangeStart and rangeEnd should be set to isDate as well.
I will attach your patch with this small modification.

Comment 20

[Sebastian Schwieger]

Attachment: same patch for memoryCal and recurrenceInfo with small workaround

Comment 21

[Daniel Boelzle [:dbo]]

(In reply to comment #18)
> Created an attachment (id=288584) [details]
> new version of unit test for memory calendar
> 
> This is a new version of the unit test for the memory calendar. It showed an
> error for daily recurring all-day events. If the range is set to the same times
> as the parent item, getItems() returns the parent item and the following
> occurrence. That means endRange seems not to be exclusive.
> 

I can't reproduce this. Removing the break statement from your (new) unit tests

>+    } else {
>+        dump("Problem\n");
>+        break;
>+    }

the tests pass successfully:

make[1]: Leaving directory `/d/moz18/sbird-debug/calendar/test'
../../dist/bin/test_all.sh ../../dist/bin/test_calendar
../../dist/bin/test_calendar/test_attendee.js: PASS
../../dist/bin/test_calendar/test_datetime.js: PASS
../../dist/bin/test_calendar/test_ics_roundtrip.js: PASS
../../dist/bin/test_calendar/test_memoryCal.js: PASS
../../dist/bin/test_calendar/test_recur.js: PASS

(In reply to comment #19)
> The bug I mentioned in comment 18 seems to be addressed in bug 353887. I have
> verified, that it is not a regression of the proposed patch. 
> As a workaround, one could check in calReccurrenceInfo::calculateDates if
> startDate isDate. Consequently, rangeStart and rangeEnd should be set to isDate
> as well.

I don't consider this being ok since enforcing a DATE effectively cuts the range.

Comment 22

[Daniel Boelzle [:dbo]]

(In reply to comment #15)
> Those zero-length events make no real sense to me. The end-time is exclusive,
> so they start at a certain time, and end just a little bit before that same
> time. I fail to understand that. But I already wrote that in bug 333362.

True. Moreover 2445bis states [1]

     "The value type
      of this property MUST be the same as the "DTSTART" property, and
      its value MUST be later in time than the value of the "DTSTART"
      property."

[1]
<http://tools.ietf.org/rfcmarkup?doc=draft-ietf-calsify-rfc2445bis-07#section-3.8.2.2>

But even though we are actually (once again) facing that wrong data and should reflect it in the UI, so users could correct it.
Admittedly, one could easily produce those items with Sunbird/Lightning; we should correct that (known bug?).

Comment 23

[Sebastian Schwieger]

Attachment: new version of unit tests (storage and memoryCal)

A new version of the unit tests, storage Calendar inclusive.

I'm also obsoleting patch 288831, Daniel pointed out on IRC, that this would cut a range if it is not on midnight.

Comment 24

[Sebastian Schwieger]

(In reply to comment #21)
>
> I can't reproduce this. Removing the break statement from your (new) unit tests
> 
> >+    } else {
> >+        dump("Problem\n");
> >+        break;
> >+    }
> 
> the tests pass successfully:
> 
Sorry, I submitted before reloading the bug. The events that fail (with and without the patch applied) are commented out!

> (In reply to comment #19)
> I don't consider this being ok since enforcing a DATE effectively cuts the
> range.
> 
I agree.

Comment 25

[Sebastian Schwieger]

Attachment: unit tests, failing in memory and storage provider.

Just to eliminate any confusion, I'm adding the tests with the events uncommented. They should now really fail.

Comment 26

[Sebastian Schwieger]

Comment on attachment 287690 [details] [diff] [review]
fix - calRecurrenceInfo, calMemoryCalendar

r- based on the failing unit tests.

Comment 27

[Daniel Boelzle [:dbo]]

Attachment: fix - calRecurrenceInfo, calMemoryCalendar, calRecurrenceRule

Passes all tests, but

  dump("Test 19\n");
  // Test a zero-length event with DUE set at the start of range and no  DTSTART
  testGetItems("BEGIN:VTODO\n" +
               "DUE:20020402T000000Z\n" +
               "END:VTODO\n", 1);

(storage only).

Comment 28

[Sebastian Schwieger]

Using this statament:

            "SELECT * FROM cal_todos " +
            "WHERE " +
            " (("+floatingTodoDue+" > :range_start + :start_offset) OR " +
            "  ("+nonFloatingTodoDue+" > :range_start) OR " +
            "  (todo_due IS NULL) OR " +
            "  ((("+floatingTodoDue+" = :range_start + :start_offset) OR " +
            "    ("+nonFloatingTodoDue+" = :range_start)) AND " +
            "   (("+floatingTodoEntry+" = :range_start + :start_offset) OR " +
            "    ("+nonFloatingTodoEntry+" = :range_start) OR " +
-->         "    (todo_entry IS NULL)))) " +
            " AND " +
            " (("+floatingTodoEntry+" < :range_end + :end_offset) OR " +
            "  ("+nonFloatingTodoEntry+" < :range_end) OR " +
            "  (todo_entry IS NULL)) " +
            " AND cal_id = :cal_id AND recurrence_id IS NULL"

all unit tests are passed. (--> this line added)

Comment 29

[Daniel Boelzle [:dbo]]

Attachment: fix - calRecurrenceInfo, calRecurrenceRule, calMemoryCalendar, calStorageCalendar

final patch

Comment 30

[Daniel Boelzle [:dbo]]

Comment on attachment 288989 [details] [diff] [review]
fix - calRecurrenceInfo, calRecurrenceRule, calMemoryCalendar, calStorageCalendar

Some more comments on the patch:
>Index: mozilla/calendar/base/src/calDatetime.cpp
>-        buffer, sizeof(buffer), "%04d/%02d/%02d %02d:%02d:%02d %s",
>-        mYear, mMonth + 1, mDay, mHour, mMinute, mSecond, mTimezone.get());
>+        buffer, sizeof(buffer), "%04hd/%02hd/%02hd %02hd:%02hd:%02hd %s isDate=%01hd",
>+        mYear, mMonth + 1, mDay, mHour, mMinute, mSecond,
>+        mTimezone.get(), static_cast<PRInt16>(mIsDate));
adds isDate to calIDateTime::toString() and corrects format specifiers.

>Index: mozilla/calendar/base/src/calRecurrenceInfo.js
...
fixes searchStart (libical) hack to not include items before rangeStart

>Index: mozilla/calendar/base/src/calRecurrenceRule.cpp
fixes to handle all-day items correctly.
...

>-#ifdef DEBUG_vlad
>+#ifdef DEBUG
will change to DEBUG_dbo as plain DEBUG is too verbose for everyday development

>Index: mozilla/calendar/base/src/calUtils.js
>     var newDate = aDate.clone();
>-    newDate.hour = 0;
>-    newDate.minute = 0;
>-    newDate.second = 0;
>     newDate.isDate = false;
// newDate.isDate = false sets hour/minute/second to 0

>Index: mozilla/calendar/providers/memory/calMemoryCalendar.js
rewrite due to lack of maintainability, using checkIfInRange now

>Index: mozilla/calendar/providers/storage/calStorageCalendar.js
fixes for all-day, zero-length and tasks without DUE.

Comment 31

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 288989 [details] [diff] [review]
fix - calRecurrenceInfo, calRecurrenceRule, calMemoryCalendar, calStorageCalendar

Sebo asked me to review the C++ part. Here we go:


>+// static int PR_CALLBACK
>+// calDateTimeComparator (calIDateTime *aElement1,
>+//                        calIDateTime *aElement2,
>+//                        void *aData)
>+// {
>+//     PRInt32 result;
>+//     aElement1->Compare(aElement2, &result);
>+//     return result;
>+// }
If we decide to take out the sorting, remove this blokc



>-#ifdef DEBUG_vlad
>+#ifdef DEBUG
>     {
>         char* ss = icalrecurrencetype_as_string(mIcalRecur);
>         nsCAutoString tst, tend;
>         aRangeStart->ToString(tst);
>         aRangeEnd->ToString(tend);
>-        fprintf (stderr, "RULE: [%s -> %s, %d]: %s\n", tst.get(), tend.get(), mIcalRecur->count, ss);
>+        printf("RULE: [%s -> %s, %d]: %s\n", tst.get(), tend.get(), mIcalRecur->count, ss);
Why not to stderr?

>+#ifdef DEBUG
>+        {
>+            nsCAutoString str;
>+            cdt->ToString(str);
>+            printf("  occ: %s\n", str.get());
>+        }
>+#endif
Should probably take this out, no valuable information.


>-    dates.Sort(calDateTimeComparator, nsnull);
>+    // xxx todo: discuss/review!
>+    // do we really need sorting here since datetimes have been added in increasing order?
>+//     dates.Sort(calDateTimeComparator, nsnull);
I'll think about it. Need sleep for now.

>Index: mozilla/calendar/base/src/calUtils.h
>+#define CAL_ENSURE_MEMORY(p) NS_ENSURE_TRUE(p, NS_ERROR_OUT_OF_MEMORY)
>+
Should this not be NS_ENSURE_FALSE ?

Comment 32

[Sebastian Schwieger]

>/** calRecurrenceInfo.js **/
>+
>+                var index = 0;
>+                const len = cur_dates.length;
>+
>+                // skip items before rangeStart due to searchStart libical >hack:
>+                if (rangeStart && baseDuration) {
>+                    for (; index < len; ++index) {

Is this an style issue? You could use while()

>+                        var date = cur_dates[index].clone();
>+                        date.addDuration(baseDuration);
>+                        if (rangeStart.compare(date) < 0)
>+                            break;

brackets, please.
Does compare() result in trouble if date is a DATE value? I think it doesn't, but..

>+                    }
>+                }
>+                for (; index < len; ++index) {

Like above, this could be changed to use while()


>/** calMemoryCalendar.js **/
>+            var isEvent_ = isEvent(item);
>+            if (isEvent_) {
>+                if (!wantEvents)
>                     continue;
>-
>-                itemStartTime = (item.entryDate
>-                                 ? item.entryDate.nativeTime
>-                                 : START_OF_TIME);
>-                itemEndTime = (item.dueDate
>-                               ? item.dueDate.nativeTime
>-                               : END_OF_TIME);
>-            } else {
>-                // XXX unknown item type, wth do we do?
>+            } else if (!wantTodos)
>                 continue;

Could this be changed to 

 if ((isEvent_ && !wantEvents) || (!isEvent && !wantTodos)) {
    continue;
 }


>+            if (itemReturnOccurrences && item.recurrenceInfo) {
>+                var occurrences = item.recurrenceInfo.getOccurrences(
>+                    aRangeStart, aRangeEnd, aCount ? aCount - >itemsFound.length : 0, {});
>+                if (!isEvent_)
>+                    occurrences = occurrences.filter(checkCompleted);

brackets


>Index: mozilla/calendar/providers/storage/calStorageCalendar.js
>===================================================================
<         // The more readable version of the next where-clause is:
>-        //   WHERE event_end >= :range_start AND event_start < :range_end
>+        //   WHERE  ((event_end > :range_start OR
>+        //           (event_end = :range_start AND
>+        //           event_start = :range_start))
>+        //          AND event_start < :range_end)
>+        //         

I think we need a similar summary for the todos, since the query is now different


The patch fixes the bug and a bit more. All unit tests are passed. r+ for the js part with the nits addressed.

Comment 33

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

> >Index: mozilla/calendar/base/src/calUtils.h
> >+#define CAL_ENSURE_MEMORY(p) NS_ENSURE_TRUE(p, NS_ERROR_OUT_OF_MEMORY)
> >+
> Should this not be NS_ENSURE_FALSE ?
I take that back, of course it should be NS_ENSURE_TRUE

Comment 34

[Daniel Boelzle [:dbo]]

(In reply to comment #31)
> Why not to stderr?
because it's DEBUG information, not error output

> >+#ifdef DEBUG
> >+        {
> >+            nsCAutoString str;
> >+            cdt->ToString(str);
> >+            printf("  occ: %s\n", str.get());
> >+        }
> >+#endif
> Should probably take this out, no valuable information.
I've used that for debugging purposes. Since I'll scope the code into DEBUG_dbo, nobody will suffer from it... ;)

> >-    dates.Sort(calDateTimeComparator, nsnull);
> >+    // xxx todo: discuss/review!
> >+    // do we really need sorting here since datetimes have been added in increasing order?
> >+//     dates.Sort(calDateTimeComparator, nsnull);
> I'll think about it. Need sleep for now.

After all IMO we should remove that code (even if it actually would resort the output), because no code should rely on sorted output.

(In reply to comment #32)
> >+                    for (; index < len; ++index) {
> 
> Is this an style issue? You could use while()
Then I need to decouple the index modification into the block. My experience has shown that for-loops are better maintainable: Once somebody adds a continue statement to the block in the future, one has to keep care of the index, too. After all, I doubt that the style is bad or unreadable.

> >+                        var date = cur_dates[index].clone();
> >+                        date.addDuration(baseDuration);
> >+                        if (rangeStart.compare(date) < 0)
> >+                            break;
> 
> brackets, please.
> Does compare() result in trouble if date is a DATE value? I think it doesn't,
> but..
Good point. I think it doesn't, but it's worth to think about it: If date is a DATE, comparison will be performed on a DATE basis, i.e. rangeStart will eventually be cut by some hours. This IMO isn't a problem w.r.t. to the range check, because the item behind date is all-day (or more), thus falling into the range.

> Could this be changed to 
> 
>  if ((isEvent_ && !wantEvents) || (!isEvent && !wantTodos)) {
>     continue;
>  }
I think that's ok, but IMO harder to read, and isEvent_ is eventually checked twice.

> <         // The more readable version of the next where-clause is:
> >-        //   WHERE event_end >= :range_start AND event_start < :range_end
> >+        //   WHERE  ((event_end > :range_start OR
> >+        //           (event_end = :range_start AND
> >+        //           event_start = :range_start))
> >+        //          AND event_start < :range_end)
> >+        //         
> 
> I think we need a similar summary for the todos, since the query is now
> different
yes.

Comment 35

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 288989 [details] [diff] [review]
fix - calRecurrenceInfo, calRecurrenceRule, calMemoryCalendar, calStorageCalendar

> > >-    dates.Sort(calDateTimeComparator, nsnull);
> > >+    // xxx todo: discuss/review!
> > >+    // do we really need sorting here since datetimes have been added in increasing order?
> > >+//     dates.Sort(calDateTimeComparator, nsnull);
> > I'll think about it. Need sleep for now.
> 
> After all IMO we should remove that code (even if it actually would resort the
> output), because no code should rely on sorted output.
I agree. Go ahead and take out the sorting and the Comparator.

Comment 36

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 37

[Stefan Sitter [:ssitter]]

This checkin regressed Bug 405459.

Comment 38

[Andreas Treumann]

Checked in latest nightly build -> task is fixed and verified.