Closed Bug 328011 Opened 14 years ago Closed 14 years ago

audit calendar code for item identity comparison by id only

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dmose, Assigned: jminta)

References

Details

(Whiteboard: [swag:.5d][patch in hand])

Attachments

(2 files, 1 obsolete file)

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.
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).
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)));
}
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.
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #213601 - Flags: first-review?(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.
Attachment #213601 - Flags: first-review?(dmose) → first-review+
Another possible name that just occurred to me is hasSameIds().  Take your pick.
@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
(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)
@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).
(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
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)
(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.
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.
Assignee: daniel.boelzle → jminta
Attachment #213836 - Flags: first-review?(dmose)
Oops, didn't mean to take it.
Assignee: jminta → daniel.boelzle
Status: ASSIGNED → NEW
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!
Attachment #213836 - Flags: first-review?(dmose) → first-review+
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!
Attachment #213601 - Attachment description: adding calIItemBase::sameId(), fixing two occurrences in month view → adding calIItemBase::sameId(), fixing two occurrences in month view (checked in)
Attachment #213601 - Attachment is obsolete: true
"use nsISupportsInterfacePointer" patch checked in.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
The other issues still need a fix one day, but can wait until after 0.1. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: daniel.boelzle → nobody
Status: REOPENED → NEW
This can cause internal dataloss and other ugly failures.  Blocking.
Flags: blocking0.3+
These are the remaining spots that I listed in comment #4.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attachment #233614 - Flags: second-review?(dmose)
Attachment #233614 - Flags: first-review?(mattwillis)
Whiteboard: [swag:.5d][patch in hand]
Comment on attachment 233614 [details] [diff] [review]
remaining listed points

r1=lilmatt
Attachment #233614 - Flags: first-review?(mattwillis) → first-review+
Comment on attachment 233614 [details] [diff] [review]
remaining listed points

r=dmose
Attachment #233614 - Flags: second-review?(dmose) → second-review+
patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.