Last Comment Bug 660199 - static_cast to the wrong event listener base class
: static_cast to the wrong event listener base class
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla7
Assigned To: :Ehsan Akhgari (away Aug 1-5)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-27 03:44 PDT by Andreas Gal :gal
Modified: 2011-06-21 08:16 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (7.74 KB, patch)
2011-06-15 13:35 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Patch (v2) (7.05 KB, patch)
2011-06-19 14:24 PDT, :Ehsan Akhgari (away Aug 1-5)
roc: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2011-05-27 03:44:38 PDT

    
Comment 1 Boris Zbarsky [:bz] 2011-05-27 08:21:03 PDT
So what's this bug about?  Context?
Comment 2 Andreas Gal :gal 2011-05-27 08:26:58 PDT
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 :Ehsan Akhgari (away Aug 1-5) 2011-06-15 13:02:11 PDT
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 :Ehsan Akhgari (away Aug 1-5) 2011-06-15 13:35:47 PDT
Created attachment 539640 [details] [diff] [review]
Patch (v1)

Well, let's just fix this!
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-15 16:26:45 PDT
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) (Exited; email my personal email if necessary) 2011-06-15 16:38:00 PDT
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 :Ehsan Akhgari (away Aug 1-5) 2011-06-17 15:08:28 PDT
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) (Exited; email my personal email if necessary) 2011-06-17 16:19:16 PDT
nsCOMPtr<nsIDOMEventListener> listenerBase = do_QueryInterface(this)? That's fine.
Comment 9 :Ehsan Akhgari (away Aug 1-5) 2011-06-19 14:24:29 PDT
Created attachment 540349 [details] [diff] [review]
Patch (v2)
Comment 10 :Ehsan Akhgari (away Aug 1-5) 2011-06-19 14:25:48 PDT
(do_QueryInterface wouldn't work, since the conversion to nsISupports is ambiguous.)
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-19 14:37:33 PDT
Comment on attachment 540349 [details] [diff] [review]
Patch (v2)

Review of attachment 540349 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 12 :Ehsan Akhgari (away Aug 1-5) 2011-06-20 13:12:13 PDT
http://hg.mozilla.org/mozilla-central/rev/ca6646fc8ba4
Comment 13 Andreas Gal :gal 2011-06-20 13:21:31 PDT
Thanks guys
Comment 14 neil@parkwaycc.co.uk 2011-06-20 16:50:43 PDT
(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 :Ehsan Akhgari (away Aug 1-5) 2011-06-20 16:55:36 PDT
(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) PTO Until July 5th 2011-06-21 00:07:24 PDT
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 :Ehsan Akhgari (away Aug 1-5) 2011-06-21 05:39:43 PDT
(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) PTO Until July 5th 2011-06-21 08:16:27 PDT
I already merged to it, so lets leave things the way they are. Just don't do any more of these :)

Note You need to log in before you can comment on or make changes to this bug.