350334 - Core XBL widgets used in web pages can't listen to their own events

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[[e-mail]]

If an XBL widget is defined in content then its <handler> elements only listen to trusted events, even when the widget is bound to a content element, where it can't actually dispatch a trusted event. Bug 350257 was caused because the binding was only tested in content so nobody noticed that it was trying to listen to the same event twice. IMHO for XBL1 handlers bound to content should listen to untrusted events by default. For XBL2 I guess that core widgets could dispatch trusted events so that the problem does not apply.

Comment 1

[Neil Deakin]

Are 350332 and 350335 also caused by this? Bug 350335 is definitely a regression from something.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: something like this

Didn't test this yet

Comment 3

[[e-mail]]

Attachment: simple number box demo

Until bug 350257 is fixed, fixing this bug will make this fail in content too.

Comment 4

[Neil Deakin]

Nickolay, the editable tree bugs you've filed recently are all caused by some event handling regression, which may be this one. I know for sure that the issues in the bugs I saw were working when I checked in the tree patch.

Comment 5

[Nickolay_Ponomarev]

No, they don't appear to be caused by this bug. All that applying this patch does is break editable trees in content in the same way they are broken in chrome right now (because text selection event initiated in selectText in startEditing can now be - wrongly! - caught by http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tree.xml&rev=1.37&mark=408#400). It appears that phase="target" doesn't filter well enough or something.

Comment 6

[Neil Deakin]

smaug, did you want someone to review this?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 235593 [details] [diff] [review]
something like this

The patch seems to apply cleanly to trunk (just some fuzz)

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 235593 [details] [diff] [review]
something like this

- In content/xbl/src/nsXBLPrototypeHandler.h

-#define NS_HANDLER_TYPE_XBL_JS          (1 << 0)
-#define NS_HANDLER_TYPE_XBL_COMMAND     (1 << 1)
-#define NS_HANDLER_TYPE_XUL             (1 << 2)
-#define NS_HANDLER_ALLOW_UNTRUSTED      (1 << 5)
-#define NS_HANDLER_TYPE_SYSTEM          (1 << 6)
-#define NS_HANDLER_TYPE_PREVENTDEFAULT  (1 << 7)
+#define NS_HANDLER_TYPE_XBL_JS              (1 << 0)
+#define NS_HANDLER_TYPE_XBL_COMMAND         (1 << 1)
+#define NS_HANDLER_TYPE_XUL                 (1 << 2)
+#define NS_HANDLER_HAS_ALLOW_UNTRUSTED_ATTR (1 << 4)
+#define NS_HANDLER_ALLOW_UNTRUSTED          (1 << 5)
+#define NS_HANDLER_TYPE_SYSTEM              (1 << 6)
+#define NS_HANDLER_TYPE_PREVENTDEFAULT      (1 << 7)

This looks suspicious, aren't these values all flags? If so, there's some bit overlap there, i.e. with those valuyes flags & NS_HANDLER_HAS_ALLOW_UNTRUSTED_ATTR will test true for the last 4 of those values etc.

r- until that's clarified.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 235593 [details] [diff] [review]
something like this

Sorry, but can you clarify what actually overlaps?
1 << X shouldn't
overlap with 1 << Y
when X != Y

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 235593 [details] [diff] [review]
something like this

Duh, I got blinded by the 1, 2, 4, ..., the lack of the 3 there, and thought it was a direct filed value, w/o seeing the << right there in my face. Any idea why the 3 is missing there?

@@ -922,16 +918,17 @@ nsXBLPrototypeHandler::ConstructPrototyp
   if (aGroup && nsDependentString(aGroup).EqualsLiteral("system"))
     mType |= NS_HANDLER_TYPE_SYSTEM;
 
   nsAutoString preventDefault(aPreventDefault);
   if (preventDefault.EqualsLiteral("true"))

Want to replace that nsAutoString with a nsDependentString while you're at it? :)

r=jst

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 235593 [details] [diff] [review]
something like this

Will change that nsAutoString to nsDependentString.

Comment 12

[[e-mail]]

(In reply to comment #10)
>(From update of attachment 235593 [details] [diff] [review])
>Any idea why the 3 is missing there?
Two sets of flag bits working from each end of the byte.

>>nsAutoString preventDefault(aPreventDefault);
>>if (preventDefault.EqualsLiteral("true"))
>Want to replace that nsAutoString with a nsDependentString while you're at it?
Note that it's possible for aPreventDefault to be null.

Comment 13

[Johnny Stenback (:jst)]

(In reply to comment #12)
> >>nsAutoString preventDefault(aPreventDefault);
> >>if (preventDefault.EqualsLiteral("true"))
> >Want to replace that nsAutoString with a nsDependentString while you're at it?
> Note that it's possible for aPreventDefault to be null.

In that case we'll need to use a Substring here, not a nsDependentString.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a testcase

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fix also keyevents

Sorry, I didn't notice that key events should be fixed too.
The change to the previous patch is in nsXBLKeyEventHandler::HandleEvent
and  having SetIsBoundToChrome().

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 261341 [details] [diff] [review]
fix also keyevents

- In nsXBLKeyEventHandler::HandleEvent():

+  nsCOMPtr<nsIContent> content = do_QueryInterface(target);
+  NS_ENSURE_STATE(content);

The point of this QI is just to make sure that target is an nsIContent right? If so, please add a comment to explain that, as "content" is not used anywhere in this function AFAICT.

r+sr=jst with that.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #16)
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(target);
> +  NS_ENSURE_STATE(content);
> 

Oh, that is a leftover from a previous version of the patch.
I'll just remove it.

Comment 18

[Robert Sayre]

needs automation