304027 - XBL handlers for custom events no longer working

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Mark Brocato]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

It appears to me that as of firefox 1.0.5, <handler> elements can no longer
handle custom events. The following code is provided as an example. In the example,
<handler event="click"> succesfully handles the click event, creating and
dispatching a custom "afterclick" event. In firefox 1.0.4, this "afterclick"
event is handled by <handler event="afterclick"> as well as the "afterclick"
event handler bound in the constructor using addEventListener. In 1.0.5 and
1.0.6 only the addEventListener bound handler works. Can anyone offer any
insight as to what has changed and/or how i can fix my code?

eventtest.xul
----------------
<window
orient="vertical"
xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
xmlns:html="http://www.w3.org/1999/xhtml"
>
<label value="Click in the box below"/>
<eventtest flex="1" style="background-color:grey;
-moz-binding:url(eventtest.xml#eventtest); border:1px solid black;"/>
</window>


eventtest.xml
----------------
<bindings xmlns="http://www.mozilla.org/xbl"
xmlns:xbl="http://www.mozilla.org/xbl"
xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
<binding id="eventtest">
<implementation>
<constructor>
this.addEventListener("afterclick", function(event) {
alert("afterclick bound in constructor.");
}, false);
</constructor>
<content>
<label value="click here"/>
</content>
</implementation>
<handlers>
<handler event="click">
alert("click bound in handlers.");
var event = document.createEvent("Events");
event.initEvent("afterclick",false,true);
this.dispatchEvent(event);
</handler>
<handler event="afterclick">
alert("afterclick bound in handlers");
</handler>
</handlers>
</binding>
</bindings>

Reproducible: Always

Steps to Reproduce:
1.Copy the above code into two files, eventtext.xml and eventtest.xul as labeled
2.run eventtest.xul in firefox 1.0.6
3.click in the large grey box as prompted.

Actual Results:  
2 js alerts popup, one from the <handler event="click"> and one from
this.addEventListener("afterclick"...) specified in the constructor. <handler
event="afterclick"> is never reached.

Expected Results:  
3 js alerts should popup, one from <handler event="click">, another from
<handler event="afterclick"> and a third from
this.addEventListener("afterclick"...).

removing this.addEventListener("afterclick"...) from the constructor does not
effect whether or not <handler event="afterclick"> is called - I bound the
additional listener in the constructor to show that the event is being
dispatched properly

Comment 1

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

Didn't test, but this may happen because XBL uses AddGroupedEventListener,
which doesn't ever set the NS_PRIV_EVENT_UNTRUSTED_PERMITTED flag.
Regression from Bug 289940.

Should we add a new attribute to XBL <handler>? It would tell whether
untrusted events are allowed.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase from reporter

This regressed between 2005-04-28 and 2005-04-29, that is the period where the
fix for bug 289940 got checked in.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

An attribute might be a good idea. But I think we should have better 'defaults'
so that it wouldn't be needed in most cases.

Maybe xbl-files from non-chrome urls should always recive untrusted events.

It should even be safe to let all xbl attached to non-chrome documents recive
such events, since such xbl should run with safe privileges. So a page should
not be able to trick a binding to do anything by throwing an event at it that it
couldn't just do itself.

But that seems a tad scary. And it wouldn't work if we ever give chorme-xbl
elevated privileges.

Actually, could we check the principal used to compile the binding? And enable
untrusted events whenever that principal doesn't have elevated privilages?

Comment 4

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

Right now we have two versions of addEventListener, one with wantsUntrusted
parameter and one without it. The existence of that parameter is tested in
nsEventReceiverSH::AddEventListenerHelper. 

I think we should have same kind of versions of addGroupedEventListener method.
But nsIDOM3EventTarget is not mapped to classinfos, only nsIDOMEventTarget is.
So should we expose also methods from nsIDOM3EventTarget and handle 
addGroupedEventListener exactly the same way as addEventListener or do something
else?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Adding support for 'unsafe' addGroupedEventListener sounds fine if there's a
real need for it (I've never really seen a lot of values in w3c-eventgroups).
But that wouldn't affect this bug, would it?

Comment 6

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

(In reply to comment #5)
> Adding support for 'unsafe' addGroupedEventListener sounds fine if there's a
> real need for it (I've never really seen a lot of values in w3c-eventgroups).
> But that wouldn't affect this bug, would it?

XBL uses addGroupedEventListener.
For example here
http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLBinding.cpp#685
So it might be easiest to modify addGroupedEventListener and then depending
on the context where binding is loaded setting the wantsUntrusted parameter.

Comment 7

[Boris Zbarsky [:bzbarsky]]

We need to fix this for beta so that people can develop against the platform
sanely...  Just deciding on trusted or not via the URI of the binding for now
makes sense to me.... Smaug, want to do it?

Comment 8

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

Attachment: proposed patch (for the branch)

Like this?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I won't be able to review in the foreseeable future (2+ weeks).  :(  Please find
someone else.... 

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 192879 [details] [diff] [review]
proposed patch (for the branch)

Just a couple of minor things

>--- public/nsIXBLDocumentInfo.h	3 Jun 2005 01:54:50 -0000	1.15
>+++ public/nsIXBLDocumentInfo.h	16 Aug 2005 23:42:00 -0000
>@@ -69,14 +69,17 @@ public:
> 
>   /* Never returns null */
>   NS_IMETHOD_(nsIURI*) DocumentURI()=0;
> 
>   NS_IMETHOD GetPrototypeBinding(const nsACString& aRef, nsXBLPrototypeBinding** aResult)=0;
>   NS_IMETHOD SetPrototypeBinding(const nsACString& aRef, nsXBLPrototypeBinding* aBinding)=0;
> 
>   NS_IMETHOD FlushSkinStylesheets()=0;
>+
>+  // Tells whether the scheme of the document URI is "chrome".
>+  NS_IMETHOD_(PRBool) IsChrome()=0;
> };
> 

You should rev the IID of nsIXBLDocumentInfo for this change.

>--- src/nsXBLPrototypeHandler.cpp	12 Aug 2005 04:10:51 -0000	1.94
>+++ src/nsXBLPrototypeHandler.cpp	16 Aug 2005 23:42:03 -0000
>@@ -896,16 +902,25 @@ nsXBLPrototypeHandler::ConstructPrototyp
>   }
> 
>   if (aGroup && nsDependentString(aGroup).EqualsLiteral("system"))
>     mType |= NS_HANDLER_TYPE_SYSTEM;
> 
>   nsAutoString preventDefault(aPreventDefault);
>   if (preventDefault.EqualsLiteral("true"))
>     mType |= NS_HANDLER_TYPE_PREVENTDEFAULT;
>+
>+  if (aAllowUntrusted) {
>+    nsAutoString allowUntrusted(aAllowUntrusted);

Use nsDependentString to avoid a copy.

>+    if (allowUntrusted.EqualsLiteral("true")) {


>+      mType |= NS_HANDLER_ALLOW_UNTRUSTED;
>+    } else if (allowUntrusted.EqualsLiteral("false")) {
>

Maybe just make this an "else" condition so that we're sure the untrusted bit
is cleared if I do allowuntrusted="foobar"?

If you do that, since we're only doing a single compare, just use nsCRT::strcmp
instead of the nsDependentString stuff.

>--- src/nsXBLPrototypeHandler.h	30 Mar 2005 12:53:52 -0000	1.25
>+++ src/nsXBLPrototypeHandler.h	16 Aug 2005 23:42:04 -0000
>@@ -132,30 +134,36 @@ public:
>     return mHandler;
>   }
> 
>   nsXBLEventHandler* GetCachedEventHandler()
>   {
>     return mHandler;
>   }
> 
>+  PRBool AllowUntrustedEvents()
>+  {
>+    return !!(mType & NS_HANDLER_ALLOW_UNTRUSTED);
>+  }
>+

I'd prefer:

  return (mType & NS_HANDLER_ALLOW_UNTRUSTED) != 0;

just seems a little easier to read.

Looks ok otherwise.

Comment 11

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

Attachment: +bryner's comment

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 192998 [details] [diff] [review]
+bryner's comment

- In nsXBLPrototypeHandler::ConstructPrototyp...

+  if (aAllowUntrusted) {
+    nsDependentString allowUntrusted(aAllowUntrusted);
+    if (allowUntrusted.EqualsLiteral("true")) {
+      mType |= NS_HANDLER_ALLOW_UNTRUSTED;
+    } else if (allowUntrusted.EqualsLiteral("false")) {
+      mType &= ~NS_HANDLER_ALLOW_UNTRUSTED;

As bryner said, loose the if after else.

sr=jst with that fixed.

Comment 13

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

Attachment: DependentString used, no else if, just else

Comment 14

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

Fixed, trunk and branch

Comment 15

[Peter van der Woude [:Peter6]]

(In reply to comment #14)
> Fixed, trunk and branch

as in RESOLVED/FIXED ?  (the status still shows NEW)