Last Comment Bug 663461 - Kill AddEventListenerByIID/RemoveEventListenerByIID from editor and embedding
: Kill AddEventListenerByIID/RemoveEventListenerByIID from editor and embedding
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on:
Blocks: 661297
  Show dependency treegraph
 
Reported: 2011-06-10 10:56 PDT by Jonas Sicking (:sicking) PTO Until July 5th
Modified: 2011-06-28 18:55 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove from editor (24.82 KB, patch)
2011-06-10 10:56 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Splinter Review
Remove from embedding (12.35 KB, patch)
2011-06-10 10:57 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Splinter Review
Remove from editor (26.10 KB, patch)
2011-06-13 10:17 PDT, Jonas Sicking (:sicking) PTO Until July 5th
bugs: review+
Details | Diff | Splinter Review
Remove from embedding (12.39 KB, patch)
2011-06-13 10:17 PDT, Jonas Sicking (:sicking) PTO Until July 5th
bugs: review+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) PTO Until July 5th 2011-06-10 10:56:52 PDT
Created attachment 538554 [details] [diff] [review]
Remove from editor

First couple of patches for bug 661297
Comment 1 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-10 10:57:36 PDT
Created attachment 538555 [details] [diff] [review]
Remove from embedding
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-10 10:59:51 PDT
The surprising part to me was that this actually *reduces* the amount of code that we have. I would have thought that having to register for each individual event would add to it, but it turns out not be less than the amount of code removed.
Comment 3 Olli Pettay [:smaug] 2011-06-13 03:15:02 PDT
This is tricky to review. Need to make sure that the event is always
QIed to right interface before handling it.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-13 03:39:05 PDT
How do you mean? All the functions on nsIDOMMouseListener etc take a nsIDOMEvent, as does nsIDOMEventListener::HandleEvent.
Comment 5 Olli Pettay [:smaug] 2011-06-13 03:49:01 PDT
It just happens to be a side-effect of nsIDOM***Listener interfaces that the
type of the event is checked before the listener is called.
(That check has been there forever)
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-13 03:52:26 PDT
Oh, so we need to make sure that the event is QI*able* to the correct interface?

Doesn't the fact that we only listen to trusted events make sure that that is the case?
Comment 7 Olli Pettay [:smaug] 2011-06-13 04:06:45 PDT
Trusted events don't guarantee anything about interfaces, but sure,
it would be chrome script which would be firing strange events.
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-13 09:52:03 PDT
Oh, one thing I should fix in both these patches is to explicitly specify the aWantsUntrusted parameter to PR_FALSE.

Probably by adding another inline AddEventListener to nsIDOMEventTarget which takes both aCapture and aWantsUntrusted and forwards to the existing AddEventListener with aOptionalCount set to 2.

Let me know if you want new patches for that.
Comment 9 Olli Pettay [:smaug] 2011-06-13 09:59:29 PDT
(In reply to comment #8)
> Oh, one thing I should fix in both these patches is to explicitly specify
> the aWantsUntrusted parameter to PR_FALSE.
I don't understand this?

Currently some of the listeners handle also untrusted events.
That is a bug, but should be fixed in a different bug.
Comment 10 Olli Pettay [:smaug] 2011-06-13 10:00:56 PDT
Er, I was wrong.
Comment 11 Olli Pettay [:smaug] 2011-06-13 10:01:50 PDT
The IID listeners don't listen for trusted events.

So yes, I'd like to see a new patch.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-13 10:17:13 PDT
Created attachment 538943 [details] [diff] [review]
Remove from editor
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-13 10:17:51 PDT
Created attachment 538944 [details] [diff] [review]
Remove from embedding
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-23 20:44:46 PDT
I just verified that all HandleEvent functions can handle events of the wrong type.

The only one that was a bit doubtful (the event got spread further than I was comfortable with) was ChromeContextMenuListener::HandleEvent, so I added

  nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aMouseEvent);
  NS_ENSURE_TRUE(mouseEvent, NS_ERROR_UNEXPECTED);

to the top of that function locally. Let me know if you want a new patch.
Comment 15 Olli Pettay [:smaug] 2011-06-27 11:35:24 PDT
Comment on attachment 538943 [details] [diff] [review]
Remove from editor

> nsEditor::CreateEventListeners()
> {
>   // Don't create the handler twice
>-  if (mEventListener)
>-    return NS_OK;
>-  mEventListener = do_QueryInterface(
>-    static_cast<nsIDOMKeyListener*>(new nsEditorEventListener()));
>-  NS_ENSURE_TRUE(mEventListener, NS_ERROR_OUT_OF_MEMORY);
>+  if (!mEventListener)
>+    mEventListener = new nsEditorEventListener();
if (expr) {
  stmt;
}

> nsHTMLEditor::CreateEventListeners()
> {
>   // Don't create the handler twice
>-  if (mEventListener)
>-    return NS_OK;
>-  mEventListener = do_QueryInterface(
>-    static_cast<nsIDOMKeyListener*>(new nsHTMLEditorEventListener()));
>-  NS_ENSURE_TRUE(mEventListener, NS_ERROR_OUT_OF_MEMORY);
>+  if (!mEventListener)
>+    mEventListener = new nsHTMLEditorEventListener();
Ditto
Comment 16 Olli Pettay [:smaug] 2011-06-27 11:41:40 PDT
Comment on attachment 538944 [details] [diff] [review]
Remove from embedding


>+NS_IMETHODIMP
>+ChromeTooltipListener::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  nsAutoString eventType;
>+  aEvent->GetType(eventType);
You should check that eEvent is key or mouse event
(depending on the event type)
Comment 17 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-27 12:44:38 PDT
(In reply to comment #16)
> >+NS_IMETHODIMP
> >+ChromeTooltipListener::HandleEvent(nsIDOMEvent* aEvent)
> >+{
> >+  nsAutoString eventType;
> >+  aEvent->GetType(eventType);
> You should check that eEvent is key or mouse event
> (depending on the event type)

The code that doesn't check the type doesn't use the event object at all. Doesn't seem worth it to check that it's of the right type then.

The MouseMove function does use the event object and does check its type.

Let me know if you still want something changed.
Comment 18 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-27 17:30:22 PDT
Landed the editor part:

http://hg.mozilla.org/mozilla-central/rev/347d715650f6
Comment 19 Olli Pettay [:smaug] 2011-06-28 01:57:07 PDT
(In reply to comment #17)
> The code that doesn't check the type doesn't use the event object at all.
> Doesn't seem worth it to check that it's of the right type then.
Technically you're changing the behavior then, since non-key events
can trigger the code.

Though, in practice it shouldn't matter. r=me
Comment 20 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-28 08:16:57 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/ec503528d2ef

Checked in to mozilla-inbound. Thanks for review!
Comment 21 Joe Drew (not getting mail) 2011-06-28 18:55:56 PDT
http://hg.mozilla.org/mozilla-central/rev/ec503528d2ef

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