328011 - audit calendar code for item identity comparison by id only

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Joey pointed out that in various places in the code (some of the provider impls of getItem and addItem, some of the selection code in the views, and perhaps elsewhere), there are tests for whether two items are equal which test only id, and not the (id, recurrenceId) pair.  Allowing editing on non-parent items is likely to expose these bugs, and if there's one in the wrong place, it will cause dataloss.  

The getItem impls don't worry me much since they don't currently have any callers.  The selection code in the views could lose data if somebody does a quick "click-to-select, then press the delete key" in the wrong place.  The addItem case is really for error-checking, so it doesn't seem horribly risky.  I'm not sure what else is lurking out there.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Another thing to keep an eye out for while doing this audit is attempts to test identity by comparing "interface pointers", which may lose because of xpconnect-wrapping (bug 325636).

Comment 2

[gekacheka]

Because of its name, it is easy to forget recurrenceId is a calIDateTime.
I suggest creating method (or a function) to do the identity comparison.

sameIds: function itemBase_sameIds(that) { 
  return (that && this.id == that.id && 
          (this.recurrenceId == that.recurrenceId || // both null
           (this.recurrenceId && that.recurrenceId &&
            this.recurrenceId.compare(that.recurrenceId) == 0)));
}

Comment 3

[Daniel Boelzle [:dbo]]

Attachment: adding calIItemBase::sameId(), fixing two occurrences in month view (checked in)

IMO there is currently no reported problem, so I would vote to only fix what is obviously wrong.  I grepped over calendar/, and had a look at
- the providers: seem to be correct
- month view: fix for "recurrence_id" => in patch
There is more stuff I can't judge on, e.g. selection/alarms/alerts.

Comment 4

[Joey Minta]

Doing a quick scan, I came up with the following suspicious instances.  They may not all be problematic, but they at least caught my eye.

http://lxr.mozilla.org/mozilla/source/calendar/base/src/calRecurrenceInfo.js#225
http://lxr.mozilla.org/mozilla/source/calendar/providers/composite/calCompositeCalendar.js#236
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-month-view.xml#228
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-month-view.xml#239
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-month-view.xml#1064
??http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-multiday-view.xml#429
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-multiday-view.xml#493
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-multiday-view.xml#1278
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-multiday-view.xml#1290

My biggest concern is with calMemoryCalendar.js, which stores items entirely by id.  Unfortunately, I'm too tired tonight to go digging into the architecture to make sure that it doesn't actually try to store occurrences.  As long as only parent-items are stored we should be ok.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 213601 [details] [diff] [review]
adding calIItemBase::sameId(), fixing two occurrences in month view (checked in)

In general, looks good.  Just a couple of nits:

>+  /**
>+   * Checks whether two item objects refer the same calendar item.
>+   */
>+  boolean sameId( in calIItemBase aItem );
>+  

How about calling this "equalsByIds"?  This strikes me as likely to be clearer to readers of calling code.  Also, if you could make the doxygen comment explain why a caller would want to use this (as opposed to just comparing .id fields manually), that would be helpful to people learning the code, I think.

>+    sameId: function(that) {
>+        return (that && this.id == that.id &&
>+                (this.recurrenceId == that.recurrenceId || // both null
>+                 (this.recurrenceId && that.recurrenceId &&
>+                  this.recurrenceId.compare(that.recurrenceId) == 0)));

While this is a concise way to write the function, it's difficult to read.  Can you split this up into multiple if/return statements, possibly with comments if that seems appropriate?

r=dmose with those nits fixed.  Thanks for the patch.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Another possible name that just occurred to me is hasSameIds().  Take your pick.

Comment 7

[Daniel Boelzle [:dbo]]

@jminta:
> http://lxr.mozilla.org/mozilla/source/calendar/base/src/calRecurrenceInfo.js#225
is ok (recurrence items don't have an id)

> http://lxr.mozilla.org/mozilla/source/calendar/providers/composite/calCompositeCalendar.js#236
ought to be ok, because no two calendar share the same uri

concerning the memory provider:
IMO there is no problem, because it always works on [parent] items, not on proxies, see

http://lxr.mozilla.org/mozilla/source/calendar/providers/memory/calMemoryCalendar.js#224

Comment 8

[Joey Minta]

(In reply to comment #7)
> @jminta:
> > http://lxr.mozilla.org/mozilla/source/calendar/base/src/calRecurrenceInfo.js#225
> is ok (recurrence items don't have an id)
> 
> > http://lxr.mozilla.org/mozilla/source/calendar/providers/composite/calCompositeCalendar.js#236
> ought to be ok, because no two calendar share the same uri
> 
Are you saying these are 'ok' in the current code or ok to update/fix without risk?  To me, they look like prime examples of comparing XPCOM objects, vs comparing unique identifying properties of those objects (uri or id/recurrenceId)

Comment 9

[Daniel Boelzle [:dbo]]

@jminta:
IIRC object identity is supported for wrapped js objects, i.e. nsISupport pointers are compared which have to stay constant over an object's lifetime (same as in COM).

Comment 10

[Joey Minta]

(In reply to comment #9)
> @jminta:
> IIRC object identity is supported for wrapped js objects, i.e. nsISupport
> pointers are compared which have to stay constant over an object's lifetime
> (same as in COM).
> 
Bug 325636

Comment 11

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

http://lxr.mozilla.org/seamonkey/source/calendar/providers/memory/calMemoryCalendar.js#222 is what saves the memory calendar. (My quick tests with the ics provider didn't show any problems. ics uses memory internally)

Comment 12

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

(In reply to comment #4)
> http://lxr.mozilla.org/mozilla/source/calendar/base/src/calRecurrenceInfo.js#225
> http://lxr.mozilla.org/mozilla/source/calendar/providers/composite/calCompositeCalendar.js#236

Those two need fixing. Thanks to bug 325636, just checking objects can lead to random behaviour, which can be hard to debug.

The multiweek problems have to do with deleting a box from the view, and it indeed seems to remove too much boxes. But then, thanks to some observer, they are added back. (I see the boxes getting redrawn.) One bug fixes the other. We can leave that for now, because fixing of the bugs but not the other might lead to strange results.

the month problems look like selection, wich already is semi-broken. But I coulnd't find out the result of the problems. I can just select one occurence (or exception) just like it should.

Comment 13

[Joey Minta]

Attachment: use nsISupportsPointerInterface

This patch fixes the problems in calRecurrenceInfo.js (yay timeless!) and calCompositeCalendar.js.  I'll go ahead and buy mvl's comments about the checks in the views when adding/modifying/deleting, since those all will call relayout();, which ought to reset everything anyway.  The selection bugs shouldn't be a problem in Lightning until I implement calISelectionManager and start actually using the external selectItem commands.

Comment 14

[Joey Minta]

Oops, didn't mean to take it.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 213836 [details] [diff] [review]
use nsISupportsPointerInterface

>@@ -216,18 +216,23 @@ calRecurrenceInfo.prototype = {
> 
>     deleteRecurrenceItem: function(aItem) {
>         if (!this.mBaseItem)
>             throw Components.results.NS_ERROR_NOT_INITIALIZED;
> 
>         if (this.mImmutable)
>             throw Components.results.NS_ERROR_OBJECT_IS_IMMUTABLE;
> 
>+        // Be careful about XPConnect wrapping

Could you make this comment a little specific?

r=dmose with that fixed.

Thanks for the patch!

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 213601 [details] [diff] [review]
adding calIItemBase::sameId(), fixing two occurrences in month view (checked in)

In the hopes of having tomorrow mornings builds have all the actual bugs for 0.1 fixed, I went ahead and changed the method name to hasSameIds(), left the code itself alone, and landed the patch.  Thanks for the fix, Daniel!

Comment 17

[Joey Minta]

"use nsISupportsInterfacePointer" patch checked in.

Comment 18

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

The other issues still need a fix one day, but can wait until after 0.1. Reopening.

Comment 19

[Joey Minta]

This can cause internal dataloss and other ugly failures.  Blocking.

Comment 20

[Joey Minta]

Attachment: remaining listed points

These are the remaining spots that I listed in comment #4.

Comment 21

[Matthew (lilmatt) Willis]

Comment on attachment 233614 [details] [diff] [review]
remaining listed points

r1=lilmatt

Comment 22

[Joey Minta]

Comment on attachment 233614 [details] [diff] [review]
remaining listed points

r=dmose

Comment 23

[Joey Minta]

patch checked in.