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.

->hyatt

Added patch keyword

Marking NEW so someone will look at the patch.

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

With no patch forthcoming, futuring.

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.

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=brendan@mozilla.org. /be

Pulling in and reassigning to dr for checkin.

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

fixed. thanks mark!