Closed Bug 660199 Opened 13 years ago Closed 13 years ago
_cast to the wrong event listener base class
No description provided.
So what's this bug about? Context?
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.
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.
Well, let's just fix this!
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #539640 - Flags: review?(roc)
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?
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.
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?
nsCOMPtr<nsIDOMEventListener> listenerBase = do_QueryInterface(this)? That's fine.
(do_QueryInterface wouldn't work, since the conversion to nsISupports is ambiguous.)
Comment on attachment 540349 [details] [diff] [review] Patch (v2) Review of attachment 540349 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #540349 - Flags: review?(roc) → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(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.
(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? :-)
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 :(
(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...
I already merged to it, so lets leave things the way they are. Just don't do any more of these :)
You need to log in before you can comment on or make changes to this bug.