The default bug view has changed. See this FAQ.

static_cast to the wrong event listener base class

RESOLVED FIXED in mozilla7

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gal, Assigned: Ehsan)

Tracking

unspecified
mozilla7
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
So what's this bug about?  Context?
(Reporter)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Created attachment 539640 [details] [diff] [review]
Patch (v1)

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.
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 9

6 years ago
Created attachment 540349 [details] [diff] [review]
Patch (v2)
Attachment #539640 - Attachment is obsolete: true
Attachment #539640 - Flags: review?(roc)
Attachment #540349 - Flags: review?(roc)
(Assignee)

Comment 10

6 years ago
(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+
(Assignee)

Comment 12

6 years ago
http://hg.mozilla.org/mozilla-central/rev/ca6646fc8ba4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Reporter)

Comment 13

6 years ago
Thanks guys
(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.
(Assignee)

Comment 15

6 years ago
(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 :(
(Assignee)

Comment 17

6 years ago
(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.