349228 - Wrong set observers iterated.

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

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

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: lxr source 18.08.2006 19:57 UTC

The current source iterates throught the passed aObserver, but should iterate through this.mObservers. I will attach a patch that uses indexOf similar to the wcap provider. 

Reproducible: Always

Comment 1

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

Attachment: addObserver using mObservers

Comment 2

[Joey Minta]

Comment on attachment 234480 [details] [diff] [review]
addObserver using mObservers

You should ask for review for patches you attach. :-)

Comment 3

[Bruno Browning]

Comment on attachment 234480 [details] [diff] [review]
addObserver using mObservers

Thanks for catching this, and for the patch!

The JS in this patch is just dandy; my only concern is the usual tiresome style nit. The patch leaves this part of the file formatted differently than the rest, and does not follow the general guidelines, see:
http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual
If I were the one who will eventually check this in, I'd just fix this myself, but as that's not the case I hope you will revise and resubmit. Thanks!

Comment 4

[Matthew (lilmatt) Willis]

Attachment: fixes formatting issue

Original patch with formatting fixed.

Comment 5

[Bruno Browning]

Comment on attachment 236613 [details] [diff] [review]
fixes formatting issue

looks & works great. Thanks!

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 236613 [details] [diff] [review]
fixes formatting issue

Unfortunately, this doesn't deal with the vagaries of XPConnect identity comparison.  In particular, we need something like the compareItems() function in calUtils.js.  I'd suggest adding a new, slightly more generic version of that function to calUtils.js called compareObjects() which takes a third argument, that of the IIDs to compare, and then using that here.

Comment 7

[Joey Minta]

(In reply to comment #6)
> (From update of attachment 236613 [details] [diff] [review] [edit])
> Unfortunately, this doesn't deal with the vagaries of XPConnect identity
> comparison. 
I think I disagree.  The problem of xpconnect identity comparison only flares up when the possibility of 'double-wrapping' appears.  As none of the observers here ever cross the language barrier (they're all and always js), I don't believe this should be an issue here.  (As I read dmose's statement of the problem, it would be much more wide-ranging than it appears to actually be.)  I still don't feel like I have my head totally wrapped around this problem though, so I could be completely off-base.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

I suspect that what jminta says is true given our in-tree observer implementations (my understanding of all the wrapping issues here is also incomplete).  That said, I don't think we want to require that anyone using our interfaces implement their observers in JS.  I could be talked into accepting this patch as-is, and splitting that issue out to another bug, though.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Despite the fact that this doesn't fix the issue 100%, as per my review comments, this is so obviously correct, and is likely to have pretty bad consequences, I think we should probably take this patch as-is today.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Joey helped me realize that I was misunderstanding the failure mode here; we can live without this for 0.3.

Comment 11

[Matthew (lilmatt) Willis]

Since we're out of the 0.3 release cycle, this can leave my queue.

-> nobody@.m.o

Comment 12

[Bruno Browning]

Attachment: create, use compareObjects()

I think this is the function dmose was looking for.

Comment 13

[Joey Minta]

Comment on attachment 245956 [details] [diff] [review]
create, use compareObjects()

Drive-by...

+/** generic object comparer
+ */
Bonus points if you add a comment explaining how/when this should be used, and more bonus points if you use @param to describe each argument.

+        for each (obs in this.mObservers) {
+            if (compareObjects(obs, aObserver, Ci.nsISupports)) {
How about Ci.calIObserver?

Comment 14

[Bruno Browning]

Attachment: shameless ploy for bonus points from jminta

thanks for driving by!

Comment 15

[Matthew (lilmatt) Willis]

Comment on attachment 245959 [details] [diff] [review]
shameless ploy for bonus points from jminta

>Index: calendar/base/src/calUtils.js
>===================================================================
>+ 
>+/** calIItemBase comparer
>+ */

>+/** generic object comparer - use to compare two objects which 
>+ *  are not of type calIItemBase, in order to avoid the js-wrapping
>+ *  issues mentioned above
>+ *
>+ * @param aObject        first object to be compared
>+ * @param aOtherObject   second object to be compared
>+ * @param aIID           IID to use in comparison
>+ */

Style nit (even though the comment above violates it):
Don't start your comments on the /** line. Start on the first * line after.
That will make your more verbose comment move left one more character.

r=lilmatt with that

Comment 16

[Bruno Browning]

Attachment: correction per reviewer

Comment 17

[Joey Minta]

Comment on attachment 246241 [details] [diff] [review]
correction per reviewer

r2=jminta Thanks for the patch! :-)

Comment 18

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED