660199 - static_cast to the wrong event listener base class

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

So what's this bug about?  Context?

Comment 2

[Andreas Gal :gal]

nsListenerStruct has a nsRefPtr mListener member. We tried to make it an nsCOMPtr but it turns out people frequently pass the wrong base class when registering an event handler. Editor, for example, specifies that the Key event is supposed to be the canonical base class of its listener, but then when installing the event handler for a mouse event it does a static cast to the mouse event. This blows up if you use an NsCOMPtr in nsListenerStruct. We decided this is not a bug in opt builds because the event handler setup does another qi and arrives at the right base class after all.

Comment 3

[(no longer active)]

Are you talking about the horrible casts going on in nsEditorEventListener::InstallToEditor?  I can fix them up for you if it's blocking you on something else.

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Well, let's just fix this!

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 539640 [details] [diff] [review]
Patch (v1)

Review of attachment 539640 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/nsEditorEventListener.cpp
@@ +136,5 @@
>    nsIEventListenerManager* elmP = piTarget->GetListenerManager(PR_TRUE);
>    NS_ENSURE_STATE(elmP);
>  
> +  nsCOMPtr<nsISupports> base;
> +  CallQueryInterface(this, getter_AddRefs(base));

Why do you need this?

@@ +167,4 @@
>                                      NS_LITERAL_STRING("dragexit"),
>                                      NS_EVENT_FLAG_BUBBLE, sysGroup);
>    NS_ENSURE_SUCCESS(rv, rv);
> +  rv = elmP->AddEventListenerByType(keyListener,

Wouldn't it make more sense to use mouseListener for all the drag-and-drop events?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, I misunderstood the problem.

nsEditorEventListener.cpp has
  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIDOMEventListener, nsIDOMKeyListener)

So the issue in comment #2 is that when we statically cast to nsIDOMMouseEvent and pass that as the listener, which is implicitly cast to nsIDOMEventListener, the resulting pointer is different to the pointer for the canonical nsIDOMEventListener base class as obtained by do_QueryInterface, which is returning the base for nsIDOMKeyListener.

So the correct thing to do is to cast to the nsIDOMKeyListener base for *all* of the places we need to pass 'this' as an nsIDOMEventListener.

Probably the clearest, most foolproof and most efficient way to do that would be to write
  nsIDOMEventListener* listenerBase = static_cast<nsIDOMKeyListener*>(this);
and then use listenerBase everywhere.

Comment 7

[(no longer active)]

That would make things rely on the QI table details, and it will break again if we change it some day.  What's wrong with just QIing explicitly?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsCOMPtr<nsIDOMEventListener> listenerBase = do_QueryInterface(this)? That's fine.

Comment 9

[(no longer active)]

Attachment: Patch (v2)

Comment 10

[(no longer active)]

(do_QueryInterface wouldn't work, since the conversion to nsISupports is ambiguous.)

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 540349 [details] [diff] [review]
Patch (v2)

Review of attachment 540349 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/ca6646fc8ba4

Comment 13

[Andreas Gal :gal]

Thanks guys

Comment 14

[[e-mail]]

(In reply to comment #10)
> (do_QueryInterface wouldn't work, since the conversion to nsISupports is
> ambiguous.)
That's what do_QueryObject was invented for: when at least one of the source or destination isn't an interface.

Comment 15

[(no longer active)]

(In reply to comment #14)
> (In reply to comment #10)
> > (do_QueryInterface wouldn't work, since the conversion to nsISupports is
> > ambiguous.)
> That's what do_QueryObject was invented for: when at least one of the source or
> destination isn't an interface.

I didn't know about do_QueryObject.  Do you want to write up a quick patch to switch this over to that function?  :-)

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please lets not muck around with these. All of these casts are going away in bug 661297 and its dependencies. In fact, I kind'a wish that this hadn't landed since it's a pain to merge :(

Comment 17

[(no longer active)]

(In reply to comment #16)
> Please lets not muck around with these. All of these casts are going away in
> bug 661297 and its dependencies. In fact, I kind'a wish that this hadn't landed
> since it's a pain to merge :(

Oh, I didn't know about that.  If it's OK by Andreas, I'm fine if you want me to back out the patch...

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I already merged to it, so lets leave things the way they are. Just don't do any more of these :)