61187 - nsIController ignores COM identity rules

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Mark Hammond [:markh] [:mhammond]]

This is possibly 2 bugs, but they are closely related.

The first is clearly a bug:  The functions AppendController() and 
InsertControllerAt() insert the nsIController directly into the nsISupports 
array.  The element should be explicitly QId for nsISupports before insertion 
into the array, so that pointer identity can be used correctly.  Note that 
RemoveController() etc do the correct QI.

Note that the current code will generally work for C++ or JS implemented 
controllers, as the QI for nsISupports will return the same pointer.  However, 
this is not true for all objects, and XPCOM states the rules.

The second problem is only possibly a bug:  This component hides failures.  
Attempting to remove an object not in the array currently returns NS_OK.  Thus, 
the bug above was masked - even though the pointer equality failed and no 
controller was removed, no error code indicated that the command failed.

I have witnessed no code breakage with this patch.  However, if any _does_ 
break, it is almost certain we are finding another bug in their code - it is 
very unlikely that someone will ask to remove a controller they know doesnt 
exist!  And if they _expect_ it to work but silently fails, things are bad - 
eg, bugs similar to #56073 could arise, as controllers they expect removed (as 
the remove request succeeded) are indeed still being referenced (and possibly 
participating in a cycle, for example)

Suggested patch attached.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Suggested patch

Comment 2

[Peter Trudelle]

->hyatt

Comment 3

[Keyser Sose]

Added patch keyword

Comment 4

[Keyser Sose]

Marking NEW so someone will look at the patch.

Comment 5

[Peter Trudelle]

->moz1.0. file has moved, could take sooner if patch updated.

Comment 6

[David Hyatt]

With no patch forthcoming, futuring.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated patch against the trunk

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Sorry - I forgot this patch rotted!

I just uploaded a new patch.  This one is quite a bit simpler as many of the 
other fixes have already been made.

The only problem left was that |removeController| still did not obey xpcom 
identity rules.  This patch ensures that nsISupports pointers are always 
compared.

I wrote a decent XUL test for some of the controller semantics, and did indeed 
verify that depending on the component implementation this does indeed fail, 
and the patch makes things correctly.

Comment 9

[Brendan Eich [:brendan]]

I like it, but are those .get() calls really necessary.  True, when comparing
comptr to rawptr, some gcc versions barf, requiring .get().  But doesn't == on
two nsCOMPtrs find the right operator?

Otherwise, r/sr=[e-mail].

/be

Comment 10

[David Hyatt]

Pulling in and reassigning to dr for checkin.

Comment 11

[Dan Rosen]

accepting for 0.9. i'll remove the extraneous .get()s and checkin. (r=dr).

Comment 12

[Dan Rosen]

Attachment: final patch, spiffied-up

Comment 13

[Dan Rosen]

fixed. thanks mark!