Closed Bug 349228 Opened 14 years ago Closed 13 years ago

Wrong set observers iterated.

Categories

(Calendar :: Provider: CalDAV, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Sunbird 0.5

People

(Reporter: Fallen, Unassigned)

References

()

Details

Attachments

(2 files, 3 obsolete files)

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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #234480 - Flags: first-review?
Comment on attachment 234480 [details] [diff] [review]
addObserver using mObservers

You should ask for review for patches you attach. :-)
Attachment #234480 - Flags: second-review?(dmose)
Attachment #234480 - Flags: first-review?(browning)
Attachment #234480 - Flags: first-review?
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!
Attachment #234480 - Flags: first-review?(browning) → first-review-
Attached patch fixes formatting issue β€” β€” Splinter Review
Original patch with formatting fixed.
Assignee: nobody → mattwillis
Attachment #234480 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #236613 - Flags: second-review?(dmose)
Attachment #236613 - Flags: first-review?
Attachment #234480 - Flags: second-review?(dmose)
Attachment #236613 - Flags: first-review? → first-review?(browning)
Comment on attachment 236613 [details] [diff] [review]
fixes formatting issue

looks & works great. Thanks!
Attachment #236613 - Flags: first-review?(browning) → first-review+
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.
Attachment #236613 - Flags: second-review?(dmose) → second-review-
(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.

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.
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.
Target Milestone: --- → Sunbird 0.3
Joey helped me realize that I was misunderstanding the failure mode here; we can live without this for 0.3.
Target Milestone: Sunbird 0.3 → Sunbird 0.5
Since we're out of the 0.3 release cycle, this can leave my queue.

-> nobody@.m.o
Assignee: lilmatt → nobody
Status: ASSIGNED → NEW
Attached patch create, use compareObjects() (obsolete) β€” β€” Splinter Review
I think this is the function dmose was looking for.
Attachment #245956 - Flags: first-review?(lilmatt)
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?
thanks for driving by!
Attachment #245956 - Attachment is obsolete: true
Attachment #245959 - Flags: first-review?(lilmatt)
Attachment #245956 - Flags: first-review?(lilmatt)
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
Attachment #245959 - Flags: first-review?(lilmatt) → first-review+
Attached patch correction per reviewer β€” β€” Splinter Review
Attachment #245959 - Attachment is obsolete: true
Attachment #246241 - Flags: second-review?(jminta)
Comment on attachment 246241 [details] [diff] [review]
correction per reviewer

r2=jminta Thanks for the patch! :-)
Attachment #246241 - Flags: second-review?(jminta) → second-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.