Last Comment Bug 448602 - Have a way to enumerate event listeners
: Have a way to enumerate event listeners
Status: RESOLVED FIXED
[firebug-p1]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal with 20 votes (vote)
: mozilla1.9.3a1
Assigned To: Olli Pettay [:smaug]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 372964 462910
Blocks: 454717 506961 522713
  Show dependency treegraph
 
Reported: 2008-07-30 16:16 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2013-08-21 12:06 PDT (History)
35 users (show)
jst: wanted‑next+
jst: blocking1.9.2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta2-fixed


Attachments
WIP (21.78 KB, patch)
2008-08-10 08:30 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
example (1.09 KB, text/html)
2008-08-10 08:31 PDT, Olli Pettay [:smaug]
no flags Details
example for WIP2 (2.85 KB, text/html)
2008-11-03 13:17 PST, Olli Pettay [:smaug]
no flags Details
WIP2 (38.30 KB, patch)
2008-11-03 13:44 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
WIP3 (38.12 KB, patch)
2008-11-10 11:50 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
WIP3.1 (38.16 KB, patch)
2009-01-21 10:49 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
up-to-date (38.42 KB, patch)
2009-08-20 06:07 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
up-to-date (40.61 KB, patch)
2009-08-21 04:07 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
up-to-date (40.88 KB, patch)
2009-08-21 04:16 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
Screen Shot of Firebug Prototype Event Panel (11.98 KB, image/png)
2009-08-21 16:44 PDT, John J. Barton
no flags Details
refresh for 10SEP09 (31.51 KB, text/plain)
2009-09-10 14:53 PDT, John J. Barton
no flags Details
v12 (44.56 KB, patch)
2009-10-08 05:54 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v12.1 (44.54 KB, patch)
2009-10-09 03:46 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v12.2 (44.54 KB, patch)
2009-10-09 08:50 PDT, Olli Pettay [:smaug]
bzbarsky: review-
Details | Diff | Splinter Review
with [array] (45.21 KB, patch)
2009-10-14 13:49 PDT, Olli Pettay [:smaug]
bzbarsky: review+
Details | Diff | Splinter Review
v14 (45.25 KB, patch)
2009-10-16 02:01 PDT, Olli Pettay [:smaug]
mbeltzner: approval1.9.2+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2008-07-30 16:16:42 PDT
Firebug could use a way to enumerate event listeners for a node from chrome.  Ideally, for each listener they can get the following information:

1) The nsIDOMEventListener (or ideally the JS Function if that's what it really
   is).
2) The name of the event the listener is registered for.
3) Bubble or capture.
4) Whether the listener receives untrusted events.

#2 might be fun with the AddEventListenerByIID stuff, I guess....  #2 might also be fun because the same string can map to different internal events (e.g. the various _LOAD events), right?
Comment 1 Olli Pettay [:smaug] 2008-07-30 16:51:51 PDT
Right, for #2 the API should return the same listener for many event names.
EventTypeData and EventDispatchData can be used to check which events those
listeners are registered for.

Do we want this be a [scriptable] interface or is C++ enough?
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2008-07-30 17:21:07 PDT
I think we want chrome-scriptable.
Comment 3 Olli Pettay [:smaug] 2008-08-08 01:40:13 PDT
I'll do this probably after bug 372964.

One option is to add .eventListenerInfoList, which is 
there only when script is privileged. That list would be a non-live list of
nsIEventListenerInfo objects.

So I assume the script to iterate over listeners would look something like this:

var eventListenerInfos = element.eventListenerInfoList;
for (var i = 0; i < eventListenerInfos.length; ++i) {
  var listenerInfo =  eventListenerInfos[i];
  var type = listenerInfo.type;
  var listener = listenerInfo.listener;
  var capturePhase = listenerInfo.capturePhase;
  var wantsUntrusted = listenerInfo.wantsUntrusted;
  // and then do something useful...
}

In C++ the list of listeners could be get via nsIEventListenerManager

Other option is to create a service, which has a method to query all the
listener registered to a nsIDOMEventTarget.
So instead of element.eventListenerInfoList, one would have to do something like
var eventListeners = Components.classes["@mozilla.org/eventlistener-service;1"]
  .getService(Components.interfaces.nsIEventListenerService)
  .getEventListenerInfoListFor(element);

Because the first option may slow down JS+DOM a bit, the second is perhaps
better.
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-08-08 03:03:49 PDT
There are not a whole lot of callers of AddEventListenerByIID, can we get rid of it?
Comment 5 Olli Pettay [:smaug] 2008-08-08 03:21:06 PDT
AddEventListenerByIID isn't the problem, but the special event listener interfaces
http://mxr.mozilla.org/mozilla-central/source/dom/public/coreEvents/
https://wiki.mozilla.org/Gecko:Obsolete_API
Comment 6 Olli Pettay [:smaug] 2008-08-10 08:30:54 PDT
Created attachment 333157 [details] [diff] [review]
WIP

The attribute names in nsIEventListenerInfo could be perhaps better
(suggestions welcome) and I'm not yet sure how JS event listeners should be 
exposed. Currently nsIEventListenerInfo::listener returns object, which is, in case of JS, actually nsIXPConnectWrappedJS. Would be perhaps better to be able to
get the actual JS object, but have to be careful to not cause any security 
problems. Perhaps JS callers should get XPCSafeJSObjectWrapper objects.
Comment 7 Olli Pettay [:smaug] 2008-08-10 08:31:33 PDT
Created attachment 333158 [details]
example
Comment 8 Olli Pettay [:smaug] 2008-08-10 12:27:40 PDT
jst, blake, any suggestions what kind of object would be safe, but still useful 
for JS and how it should be returned? I guess nsIXPCScriptable/nsDOMClassInfo
could check if .listener implements nsIXPConnectWrappedJS and return SJOW, but
do we have some automatic way to do that?

John, does Firebug have any specific requirements?
Comment 9 Blake Kaplan (:mrbkap) 2008-08-19 16:32:18 PDT
I like your strategy. We don't have such a helper right now, but I think it would be generally useful.
Comment 10 John J. Barton 2008-11-02 18:09:41 PST
(In reply to comment #8)
> John, does Firebug have any specific requirements?

Sorry I guess I missed this in the run up to Firebug 1.2 final. 

Having this kind of feature would be really neat! 

 var infos = els.getListenerInfoFor(document);


For interrogation this seems fine. I don't know what allowsUntrusted or inSystemEventGroup  mean.  The security problem comes from the temptation to call the event handler in extension context, but that seems unlikely since once you have the 'info' dispatching the event would be easy and more correct.  

For analysis I'm worried about scaling. For example, wouldn't be cool to have Firebug's inspect expanded to report the events that the indicated element could respond to and what elements it could send events to? Some graphical representation of the capturing/bubbling set up would be awesome.  But this would require traversing the DOM to build up all of the handlers, we'd have maybe 1 sec budget.

How about event slo-mo? Flash each element one color during capturing and another color during bubbling, maybe breakpoint into any handlers.

Would an event-handler add/remove listener on nsIEventListenerService be feasible? That is an event when an event listener is added/removed. That would allow incremental updates of state.

Anyway, sorry I missed this before.
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-11-03 01:18:56 PST
(In reply to comment #10)
> (In reply to comment #8)
> > John, does Firebug have any specific requirements?
> 
> Sorry I guess I missed this in the run up to Firebug 1.2 final. 
> 
> Having this kind of feature would be really neat! 
> 
>  var infos = els.getListenerInfoFor(document);

This looks expensive (both in lines of code and in cycles to execute) since we'd have to create some type of new object that contains information about each listener, where they are attached and so on.

Would something like

listener = someUtility.getListenerFor(object, index);

Where 'object' is for example a node, and index is iterated until null is returned. We'd also need

isCapturing = someUtility.getListenerCapturingFor(object, index);

etc. Alternatively we could have something like

info = someUtility.getListenerInfoFor(object, index);

where 'info' will return info on capturing etc, as well as the actual listener.

> For analysis I'm worried about scaling. For example, wouldn't be cool to have
> Firebug's inspect expanded to report the events that the indicated element
> could respond to and what elements it could send events to? Some graphical
> representation of the capturing/bubbling set up would be awesome.  But this
> would require traversing the DOM to build up all of the handlers, we'd have
> maybe 1 sec budget.

Information about which events *can* fire where we currently don't have. Basically what is needed is documentation (which as you know we are bad at) in some machine-readable format.

Information about what *does* fire could be accomplished using something like a wildcarded event listener that receives all events fired. Firebug could listen to that tell which events fired where and when. You can probably build a lot of cool functionality on top of that.

> How about event slo-mo? Flash each element one color during capturing and
> another color during bubbling, maybe breakpoint into any handlers.

This does sound cool indeed. If this is something that is highly requested we should definitely do it. Would need to add a hook to nsEventDispatcher.cpp that is called before each object is targetted. We could also provide information once we have built the target chain etc.

> Would an event-handler add/remove listener on nsIEventListenerService be
> feasible? That is an event when an event listener is added/removed. That would
> allow incremental updates of state.

This indeed sounds like something we could have as well.

Lots of this can be added. It's mostly a question of prioritization (and to some extent performance, but that's probably solvable generally). How requested are these features? Where should we start?
Comment 12 Olli Pettay [:smaug] 2008-11-03 01:28:19 PST
I never managed to expose event listener in any reasonable way.
What I'd like to see is some object which can be serialized to string.
Perhaps I could try to replace .listener with .toString().
That would prevent anyone calling the listener but should give enough 
information about it.

(In reply to comment #10)
> Some graphical
> representation of the capturing/bubbling set up would be awesome.
For this we could expose event target chain. Perhaps in two different forms;
(1) from event target to the |window|, (2) from event target to the chrome
handler's |window| and even up to the window root.

>  But this
> would require traversing the DOM to build up all of the handlers, we'd have
> maybe 1 sec budget.
Usually elements don't have listeners, so getting listener infos should be
fast.

> How about event slo-mo? Flash each element one color during capturing and
> another color during bubbling, maybe breakpoint into any handlers.
This could be at least partially done with event target chain.

> Would an event-handler add/remove listener on nsIEventListenerService be
> feasible?
Should be easy to do.
Comment 13 John J. Barton 2008-11-03 08:05:10 PST
(In reply to comment #11)
> >  var infos = els.getListenerInfoFor(document);
> 
> This looks expensive (both in lines of code and in cycles to execute) since
> we'd have to create some type of new object that contains information about
> each listener, where they are attached and so on.
> 
> Would something like
> 
> listener = someUtility.getListenerFor(object, index);
> 
> Where 'object' is for example a node, and index is iterated until null is
> returned. We'd also need
> 
> isCapturing = someUtility.getListenerCapturingFor(object, index);
> 
> etc. Alternatively we could have something like
> 
> info = someUtility.getListenerInfoFor(object, index);
> 
> where 'info' will return info on capturing etc, as well as the actual listener.

A third alternative would be enumerators:
    listenerEnum = someUtility.getListenersFor(object);
where null means "no listeners" (an hence no allocation for the null result). This makes the common case (no listeners) fast. The interface returned by the getNext() could return primitives so their is no object creation other than the enumerator, for which I guess there exists implementation.
Comment 14 John J. Barton 2008-11-03 08:17:17 PST
(In reply to comment #11)
> 
> > For analysis I'm worried about scaling. For example, wouldn't be cool to have
> > Firebug's inspect expanded to report the events that the indicated element
> > could respond to and what elements it could send events to? Some graphical
> > representation of the capturing/bubbling set up would be awesome.  But this
> > would require traversing the DOM to build up all of the handlers, we'd have
> > maybe 1 sec budget.
> 
> Information about which events *can* fire where we currently don't have.
> Basically what is needed is documentation (which as you know we are bad at) in
> some machine-readable format.
> 
> Information about what *does* fire could be accomplished using something like a
> wildcarded event listener that receives all events fired. Firebug could listen
> to that tell which events fired where and when. You can probably build a lot of
> cool functionality on top of that.

Actually Firebug has some event support like this. Its just not well documented ;-).


> 
> > How about event slo-mo? Flash each element one color during capturing and
> > another color during bubbling, maybe breakpoint into any handlers.
> 
> This does sound cool indeed. If this is something that is highly requested we
> should definitely do it. Would need to add a hook to nsEventDispatcher.cpp that
> is called before each object is targetted. We could also provide information
> once we have built the target chain etc.
> 
> > Would an event-handler add/remove listener on nsIEventListenerService be
> > feasible? That is an event when an event listener is added/removed. That would
> > allow incremental updates of state.
> 
> This indeed sounds like something we could have as well.
> 
> Lots of this can be added. It's mostly a question of prioritization (and to
> some extent performance, but that's probably solvable generally). How requested
> are these features? Where should we start?

The most requested feature is to know the handlers on elements. But some of these other ideas can't be requested since users don't now about the potential. 

Maybe we should consider a 'demo' implementation for some of these? I guess creating the unreviewed patch is not so much of time as the push to go from there to the end? That way we can judge if the result is cool enough to justify the push.
Comment 15 John J. Barton 2008-11-03 08:23:29 PST
(In reply to comment #12)
> I never managed to expose event listener in any reasonable way.
> What I'd like to see is some object which can be serialized to string.
> Perhaps I could try to replace .listener with .toString().
> That would prevent anyone calling the listener but should give enough 
> information about it.

Firebug would want to be able to find the 'listener' in source. I guess the listener is a string ('foo' or 'alert("hi");') until the event fires, then its compiled into a function?

...
> 
> > How about event slo-mo? Flash each element one color during capturing and
> > another color during bubbling, maybe breakpoint into any handlers.
> This could be at least partially done with event target chain.
...

That sounds very interesting. Can you say a bit more about the kinds of information in the chain and when that information is valid/available? That would be helpful in thinking about what we could do with an API in Firebug if we had it.
Comment 16 Olli Pettay [:smaug] 2008-11-03 09:03:02 PST
I have extended the patch so that serializing listener works at least in
some cases, calling .handleEvent is possible in a safe way (though need to fix still few things), and getting event target chain is possible too.
Event target chain is an array of event targets through which events flow.
It is computed before event dispatch. So event target chain + possibility to
call .handleEvent is pretty good combination.

Now I still need to figure out how to stringify compiled JS listeners
(onfoo="somecode" or target.onfoo = function() {} )
Comment 17 John J. Barton 2008-11-03 10:00:52 PST
(In reply to comment #16)
> Now I still need to figure out how to stringify compiled JS listeners
> (onfoo="somecode" or target.onfoo = function() {} )

Do you have a JSScript ? I guess you must have one eventually? For onfoo='somecode' I see a browser generated script when the event first triggers (compiled on demand). That buffer is what I need to locate from the event API.
Comment 18 Olli Pettay [:smaug] 2008-11-03 10:11:27 PST
(In reply to comment #17)
> That buffer is what I need to locate from the event API.
What you mean with "buffer".
The current API returns list of nsIEventListenerInfo objects, and each object
contain .listener which is a wrapper over the real listener.
The wrapper extends nsIDOMEventListener with a .toString() method, which
tries to serialize the listener to a string whenever possible.
(Not possible with C++ listeners etc.)
Comments welcome.

interface nsIWrappedEventListener : nsIDOMEventListener
{
  /**
   * Tries to serialize event listener to a string.
   * Returns null if serialization isn't possible
   * (for example with C++ listeners).
   */
  AString toString();
};

interface nsIEventListenerInfo : nsISupports
{
  readonly attribute AString                 type;
  readonly attribute nsIWrappedEventListener listener;
  readonly attribute boolean                 capturing;
  readonly attribute boolean                 allowsUntrusted;
  readonly attribute boolean                 inSystemEventGroup;
};

interface nsIEventListenerService : nsISupports
{
  /**
   * Returns an array of nsIEventListenerInfo objects.
   */
  nsIVariant getListenerInfoFor(in nsIDOMEventTarget aEventTarget);

  /**
   * Returns an array of event targets.
   * aEventTarget will be at the index 0.
   * The objects are the same, which would be used as DOMEvent.currentTarget
   * when dispatching an event.
   * @note Some events, especially 'load', may actually have shorter
   *       event target chain than what this methods returns.
  */
  nsIVariant getEventTargetChainFor(in nsIDOMEventTarget aEventTarget);
};
Comment 19 Olli Pettay [:smaug] 2008-11-03 10:26:39 PST
(In reply to comment #10)
> I don't know what allowsUntrusted or inSystemEventGroup  mean.
Event listeners can be added to listen either all events or only trusted events.
User initiated events are trusted and so are events dispatched by chrome.
By default content event listeners listen all events, chrome listeners only
trusted. http://mxr.mozilla.org/mozilla-central/source/dom/public/idl/events/nsIDOMNSEventTarget.idl#51

Gecko has 2 "event groups". Default and system. When event is dispatched, 
event target chain is looped and default listeners are called, then 
there is a new loop and listeners in the system event group are called.
Native default handling happens in the bubble phase of system event group (==PostHandleEvent).
Comment 20 John J. Barton 2008-11-03 11:52:17 PST
(In reply to comment #18)
> (In reply to comment #17)
> > That buffer is what I need to locate from the event API.
> What you mean with "buffer".

Here is what the event handlers look like in Firebug:

http://getfirebug.com/doc/js/BrowserGenereratedFunctions.gif

The source text comes from the JSScript object via jsdIScript which wraps JSScripts for javascript debugging. The source is decompiled from jsdIScript.functionSource. In principle if I have the JS 'Function' object I can find the buffer; I suppose I could also do string compares if your output was also decompile.

If you have a JS handler, what kind of object is it? My guess would be "string until the first call then Function"?
Comment 21 Olli Pettay [:smaug] 2008-11-03 12:00:59 PST
(In reply to comment #20)
> If you have a JS handler, what kind of object is it? My guess would be "string
> until the first call then Function"?
It can be "string until first call then Function", 
or it can be function() {...} or { handleEvent: function() {...}} if added using .addEventListener.
I'm not going to use jsd API for this, but hopefully just normal JS API.
Comment 22 John J. Barton 2008-11-03 13:06:10 PST
(In reply to comment #21)
> (In reply to comment #20)
> > If you have a JS handler, what kind of object is it? My guess would be "string
> > until the first call then Function"?
> It can be "string until first call then Function", 
> or it can be function() {...} or { handleEvent: function() {...}} if added
> using .addEventListener.

In that case how about an Object that is String, Function, or has property handleEvent. ?  I'll have to verify that I can match the result to the stuff from jsd.

> I'm not going to use jsd API for this, but hopefully just normal JS API.

Right, we don't want jsd in your API, but we don't want to correlate the results from them.
Comment 23 Olli Pettay [:smaug] 2008-11-03 13:17:07 PST
Created attachment 346112 [details]
example for WIP2
Comment 24 Olli Pettay [:smaug] 2008-11-03 13:44:53 PST
Created attachment 346122 [details] [diff] [review]
WIP2

This version allows creating event target chain and iterating over the event listeners. Serializing isn't perfect because of bug 462910, but works in common 
cases.
Doesn't contain the .handleEvent thing, because that API didn't really make
sense.
Comment 25 John J. Barton 2008-11-03 20:22:53 PST
If I understand WIP2 correctly,
nsJSEventListener::ToString
has a JSTYPE_FUNCTION then converts it to a string. Can you give the function and let callers do the toString if they want? To match the string I have to walk all of the functions and figure out how to convert them to strings to compare to the return.  Given the function I can walk all of the jsdIScript and compare their function to the return.
Comment 26 Olli Pettay [:smaug] 2008-11-04 01:57:14 PST
(In reply to comment #25)
> If I understand WIP2 correctly,
> nsJSEventListener::ToString
> has a JSTYPE_FUNCTION then converts it to a string. Can you give the function
> and let callers do the toString if they want? 
What would be the type of the return value? jsval? jsdIScript uses its own types, it seems. Perhaps the implementation of nsIEventListenerInfo
could implement also some other interface which is available only
when jsdIXXX is available and used?

nsIEventListenerInfo should still have .stringValue, IMO, to ease
simple debugging when Firebug isn't available etc.

Currently toString returns stringified function, but I'd like to use JS_ValueToSource once it is available..
Comment 27 Olli Pettay [:smaug] 2008-11-04 08:29:14 PST
I looked at JSD and I think nsIEventListenerInfo could perhaps 
have readonly attribute jsdIValue listener, which returns null if
jsd isn't active or if the listener isn't implemented in JS.
Comment 28 John J. Barton 2008-11-04 08:47:37 PST
I guess that you could also return a nsISupports backed by a wrapped JS object. If the listener is implemented in JS, then rv.wrappedJSObject is the object, else it is null.
Comment 29 Olli Pettay [:smaug] 2008-11-04 09:11:08 PST
So I'd need to return XPCNativeWrapper? It isn't too easy to use that
in C++. And XCPNativeWrapper doesn't implement nsISupports.
Ofc via classinfo JS could get different object than C++.

This really depends on the use cases. I hope no one ever needs to
call event listener explicitly. Knowing that there is one and seeing the
possible source code is hopefully enough, and maybe add breakpoints (but that happens via JSD, I guess).

If calling listener is needed, it might be safer to happen via some wrapper
which setups JS context the same way as what event handling code does currently.
Comment 30 John J. Barton 2008-11-04 09:28:52 PST
(In reply to comment #29)
> This really depends on the use cases. I hope no one ever needs to
> call event listener explicitly. Knowing that there is one and seeing the
> possible source code is hopefully enough, and maybe add breakpoints (but that
> happens via JSD, I guess).

To set breakpoints I need to find the jsdIScript object corresponding to the source. Normally the correspondence is determined by URL: when the user sets a breakpoint in foo.js then Firebug looks through the jsdIScript object until it finds filename foo.js and a line in range. The event handlers won't have URLs. If the source from the event info is *exactly* the same as the decompiled source Firebug gets from jsd, then we're good. Otherwise we need a way to match them.

jsdIValue will work, I was just fishing for away to avoid jsd in your life.
Comment 31 Olli Pettay [:smaug] 2008-11-10 11:50:18 PST
Created attachment 347341 [details] [diff] [review]
WIP3

Now stringValue gives back reasonable value even with {handleEvent: function() {}}, because JS_ValueToSource() is used for serializing.
Didn't yet find any way to return jsdIValue or reasonable way to return
jsval to script.
John, would this be already useful for Firebug - as a first step at least.
I know this is already useful for my own event handling debugging. I could
perhaps hook this to DOMi.
Comment 32 John J. Barton 2008-11-10 13:51:14 PST
I'll try to apply the patch and build a prototype or two.
Comment 33 John J. Barton 2008-11-14 15:55:01 PST
I finally think I'm past 379410, but I was not successful with the patch WIP3:
g:/mozilla/mozilla-central/src/dom/src/events/nsJSEventListener.cpp(151) : error
 C2027: use of undefined type 'nsIThreadJSContextStack'
and some more compile errors. So I'm missing something...
Comment 34 Olli Pettay [:smaug] 2008-11-15 05:12:30 PST
Hmm, I guess you just need to #include "nsIJSContextStack.h" in that file.
Not sure why it compiles here.
Comment 35 John J. Barton 2008-11-20 11:06:26 PST
I marked this dependent upon 465998 just so you know that I want to make progress but cannot at this time.
Comment 36 John J. Barton 2008-11-25 16:54:42 PST
ok i'm making progress again.

When I do this:

var info = eventListenerService.getListenerInfoFor(elt);

I get this back: [object XPCNativeWrapper [object EventListenerInfo]] 
but I don't know what to do with it. 
(info instanceof Components.interfaces.nsIVariant) is false and invo.wrappedJSObject does not exist.
Comment 37 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-11-25 21:20:08 PST
(In reply to comment #36)
> I get this back: [object XPCNativeWrapper [object EventListenerInfo]] 
> but I don't know what to do with it. 

Can you not access the various properties on it as in the example in comment 7? From the patch:

+interface nsIEventListenerInfo : nsISupports
+{
+  readonly attribute AString type;
+  readonly attribute boolean capturing;
+  readonly attribute boolean allowsUntrusted;
+  readonly attribute boolean inSystemEventGroup;
+  readonly attribute AString stringValue;
Comment 38 John J. Barton 2008-11-25 21:26:38 PST
Ok I'll try that. FWIW, I think usually see interfaces in the wrapper toString, so I can just instanceof on these and go.
Comment 39 Olli Pettay [:smaug] 2008-11-26 02:41:08 PST
.getListenerInfoFor returns an array of listener infos.
See the 2nd example.
Comment 40 John J. Barton 2008-12-11 18:18:54 PST
Making some progress. For the test case http://getfirebug.com/tests/script/Event.html I get for the button an eventInfo with stringValue:
function onclick(event) {fire();}

From jsd I get:
 function onclick(event) {\n fire();\n }
So if this holds up then some heuristic 'non-semantic white space remover' will be needed to do the matching.  

I'm hoping that the  getEventTargetChainFor function will return objects with more structure than nsIDOMEventTarget?
Comment 41 Olli Pettay [:smaug] 2008-12-12 02:48:37 PST
(In reply to comment #40)
> I'm hoping that the  getEventTargetChainFor function will return objects with
> more structure than nsIDOMEventTarget?
What kind of structure? Event target chain is a list of event targets through
which the event propagates.
Comment 42 John J. Barton 2008-12-12 09:27:18 PST
I meant: these objects can QI to other interfaces besides nsIDOMEventTarget? I guess these are DOM objects, windows, maybe a few others?

Even then I don't understand "event target chain". What relates the objects on this chain?
Comment 43 Olli Pettay [:smaug] 2008-12-12 09:33:59 PST
(In reply to comment #42)
> I meant: these objects can QI to other interfaces besides nsIDOMEventTarget? I
> guess these are DOM objects, windows, maybe a few others?
Sure you should be able to QI to whatever the object implements.
Usually those objects are nsIDOMNodes, but also nsIDOMWindow or
nsIXMLHttpRequest are possible.

> Even then I don't understand "event target chain".
Event target chain is the list of objects through which event propagates. So
in practice for example element-element-element-document-window.
The list is created before the actual event dispatching happens -
so event target chain isn't necessarily the same as the ancestor chain from
event.target to its parent nodes.
Comment 44 John J. Barton 2008-12-12 09:40:35 PST
(In reply to comment #43)

> > Even then I don't understand "event target chain".
> Event target chain is the list of objects through which event propagates. 

Ah, so this method only makes sense once an event has been raised and then only for objects involved in the event? That makes a lot more sense.

This is the capturing/bubbling path? So a UI could be the chain annotated by marks showing handlers and whether they are active on capture or bubbling directions?
Comment 45 Olli Pettay [:smaug] 2008-12-12 09:44:08 PST
(In reply to comment #44)
> This is the capturing/bubbling path? So a UI could be the chain annotated by
> marks showing handlers and whether they are active on capture or bubbling
> directions?
Right.
Comment 46 jongund 2008-12-18 11:07:13 PST
We really need a way from Firebug and other FF extensions to be able to learn which nodes have event listeners for helping to test web pages for accessibility to people with disabilities.
Comment 47 jongund 2008-12-18 11:09:29 PST
For accessibility I would like to know when a dom node has an event listener attached to it so we can do some accessibility testing.  For example a node has an "onmouseover" event, it should probably also have an "onfocus" event if it is a node that can take focus.  But these events are often added though javascript functions and not through the source markup.
Comment 48 John J. Barton 2008-12-20 09:06:15 PST
(In reply to comment #47)
> For accessibility I would like to know when a dom node has an event listener

Jon, yes that capability will be provide by this feature addition.
Comment 49 John J. Barton 2008-12-20 09:14:07 PST
I am requesting blocking on 1.9.2 since this feature fantastic for developers and is close enough that we should push it through. I will commit the Firebug team to implement the UI for FF3.2b1 (or as soon as we can run Firebug on FF3.2).

The main question mark for me is the 'stringValue' field:
  1) Why can't this field be a Javascript Function object?
  2) If it is a string, what is the specification? I can reverse engineer it but that does not seem wise.
  3) Can we call it something more descriptive (depending on #2)?
Comment 50 Olli Pettay [:smaug] 2008-12-20 09:33:30 PST
(In reply to comment #49)
> The main question mark for me is the 'stringValue' field:
>   1) Why can't this field be a Javascript Function object?
How would one use such field easily in C++?

Perhaps there should be another field, jsdI*** jsValue.
Unfortunately jsd doesn't seem to provide the features needed to create such
field. (Or I haven't found how to get jsdI*** from jsval)

>   2) If it is a string, what is the specification? I can reverse engineer it
> but that does not seem wise.
It is the serialization of the jsval (whatever JS_ValueToSource returns).
Comment 51 John J. Barton 2008-12-20 09:40:17 PST
(In reply to comment #50)
> (In reply to comment #49)
> > The main question mark for me is the 'stringValue' field:
> >   1) Why can't this field be a Javascript Function object?
> How would one use such field easily in C++?

Wouldn't you just cast the obj ref to its implementation type and use that?
But anyway how can the current stringValue be used easily in C++? 

Also we can have both as far as I am concerned...

> 
> Perhaps there should be another field, jsdI*** jsValue.
> Unfortunately jsd doesn't seem to provide the features needed to create such
> field. (Or I haven't found how to get jsdI*** from jsval)
> 
> >   2) If it is a string, what is the specification? I can reverse engineer it
> > but that does not seem wise.
> It is the serialization of the jsval (whatever JS_ValueToSource returns).

Ok, but that doesn't help me unless I have a way of getting the JS_ValueToSource from every live function in the browser to compare to the stringValue returned here so I can locate the object.
Comment 52 Olli Pettay [:smaug] 2008-12-20 09:56:50 PST
(In reply to comment #51)
> But anyway how can the current stringValue be used easily in C++? 
Just get the stringValue and check the value.
Note, for me it is enough to just see the source of the event listener.
I guess Firebug wants more.

jsdValue::FromPtr(JSDContext *aCx, JSDValue *aValue) is probably what I could
use to provide jsdIValue field. I wonder if that method is available outside
jsd.
Comment 53 John J. Barton 2008-12-20 11:29:40 PST
I agree that the jsd calls are not a good direction. I think we should look for either a way to get JS_ValueToSource for functions or find a different way to get the Function object from your handler.
Comment 54 Rob Campbell [:rc] (:robcee) 2009-01-17 09:04:26 PST
I ran a try build based on 3.1b2 code if anyone wants to play with this patch.

builds are here: 
https://build.mozilla.org/tryserver-builds/2009-01-16_07:16-rcampbell@mozilla.com-nsIEventListener/

source is here: 
http://hg.mozilla.org/users/rcampbell_mozilla.com/moz-191/rev/4df5406a4263

This patch will need to be updated as it doesn't apply cleanly to trunk.
Comment 55 Olli Pettay [:smaug] 2009-01-18 08:53:59 PST
(In reply to comment #54)
> This patch will need to be updated as it doesn't apply cleanly to trunk.
It does apply if some --fuzz is used.
Comment 56 Rob Campbell [:rc] (:robcee) 2009-01-19 13:22:27 PST
(In reply to comment #55)
> (In reply to comment #54)
> > This patch will need to be updated as it doesn't apply cleanly to trunk.
> It does apply if some --fuzz is used.

how much --fuzz? ;)

Disregard my previous comment. It looks like my hg repo was only partially updated and I was missing the all-important stuff. I'll work on this again a bit tomorrow.
Comment 57 Rob Campbell [:rc] (:robcee) 2009-01-21 08:40:03 PST
this really doesn't build. Tried WIP3 again and it burned the try servers after applying cleanly to moz-191 tree. For contents see:

http://hg.mozilla.org/users/rcampbell_mozilla.com/moz-central-191/shortlog

You can see the entire build log here:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1232548523.1232550624.23868.gz&fulltext=1#err0

from a local test build:

/Volumes/Data/Projects/MOZ-EVENTLISTENER/mozilla/dom/src/events/nsJSEventListener.cpp:147:   instantiated from here
../../../dist/include/xpcom/nsCOMPtr.h:550: error: no matching function for call to ‘nsCOMPtr_base::nsCOMPtr_base(nsIThreadJSContextStack*&)’
../../../dist/include/xpcom/nsCOMPtr.h:419: note: candidates are: nsCOMPtr_base::nsCOMPtr_base(nsISupports*)
../../../dist/include/xpcom/nsCOMPtr.h:416: note:                 nsCOMPtr_base::nsCOMPtr_base(const nsCOMPtr_base&)
make[6]: *** [nsJSEventListener.o] Error 1
make[5]: *** [libs] Error 2
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_gecko] Error 2
make[2]: *** [tier_gecko] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

I did an hg diff comparing the output to your patch and they're functionally equivalent (+/- some context lines and new file ordering).

Any ideas what's wrong here?
Comment 58 Olli Pettay [:smaug] 2009-01-21 09:20:35 PST
Something changed so that #include "nsIJSContextStack.h" is missing?
Comment 59 Rob Campbell [:rc] (:robcee) 2009-01-21 09:54:53 PST
(In reply to comment #58)
> Something changed so that #include "nsIJSContextStack.h" is missing?

where is that supposed to live? A search on MXR against Mozilla-1.9.1 doesn't turn anything up.
Comment 60 Rob Campbell [:rc] (:robcee) 2009-01-21 09:58:25 PST
sorry, mxr didn't like the #include part of that search. Searching for nsIJContextStack.h brings a bunch of results back, including dom/src/nsJSEventListener.cpp.

If it's missing elsewhere let me know.
Comment 61 Olli Pettay [:smaug] 2009-01-21 10:18:30 PST
I'll update a new patch.
Btw, seems like the nsIJSContextStack.h problem is opt-build-only
problem.
Comment 62 Olli Pettay [:smaug] 2009-01-21 10:49:31 PST
Created attachment 357987 [details] [diff] [review]
WIP3.1
Comment 63 Olli Pettay [:smaug] 2009-01-21 10:54:56 PST
I uploaded the patch to tryserver. 
Builds will be eventually here https://build.mozilla.org/tryserver-builds/?C=M;O=D something about eventlistenerservice
Comment 64 Rob Campbell [:rc] (:robcee) 2009-01-22 06:49:39 PST
are you still basing this off of release-1.9.1 branch or are these being built against trunk?

Thanks for the update!
Comment 65 Olli Pettay [:smaug] 2009-01-22 07:12:20 PST
WIP3.1 was done using trunk, but with some --fuzz it should apply to 1.9.1 too.

The tryserver builds are now here:
https://build.mozilla.org/tryserver-builds/2009-01-21_10:53-opettay@mozilla.com-eventlistenerservice/
Comment 66 Rob Campbell [:rc] (:robcee) 2009-01-22 07:14:32 PST
we'll take a look at builds. thanks smaug.
Comment 67 Olli Pettay [:smaug] 2009-02-09 09:43:18 PST
Any comments? I'd like to get the patch to trunk even without jsd/js stuff.
Hopefully those can be added later.
Comment 68 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-02-09 10:39:51 PST
Is it really a good idea to expose the event listener as a string rather than as a real nsIDOMEventListener? These days we have pretty good wrappers which should ensure that calling a content nsIDOMEventListener from chrome is safe.

Or is the plan to add access to the raw JS function in the future so that it can be displayed and identified appropriately from firebug?
Comment 69 Olli Pettay [:smaug] 2009-02-09 10:44:19 PST
For debugging getting the string is useful. That is all I usually need.
And no, we're not that good at automatically using the right context for
listeners.

I'd like to provide some jsdIXXX value for firebug, but I think that
needs some changes to jsd.
Comment 70 John J. Barton 2009-02-09 11:35:57 PST
Basically we don't know how to return an object which is Function. And the function strings that JSD has are not the same as this current patch returns. I tried to figure out how to add/subtract whitespace to find matches but got distracted. Its a stupid hack anyway.
Comment 71 jongund 2009-02-10 06:39:27 PST
(In reply to comment #70)
> Basically we don't know how to return an object which is Function. And the
> function strings that JSD has are not the same as this current patch returns. I
> tried to figure out how to add/subtract whitespace to find matches but got
> distracted. Its a stupid hack anyway.

So the information that this feature will enumerate is the type of event listener (onMouseMove, onKeyPress, etc...) and the text string of the associated function?
Comment 72 John J. Barton 2009-02-28 11:08:05 PST
(In reply to comment #69)
> I'd like to provide some jsdIXXX value for firebug, but I think that
> needs some changes to jsd.

(I don't think any changes are needed to jsd)

Here's another idea: provide a function which takes a JS function object and returns the same kind of string you do now. Then Firebug can apply that function to all of the Function objects it knows about and look for the match.

Yes, its a hack, but since we can't return a Function we need a hack. This one would be much more certain to work than one based on me reverse engineering whitespace.
Comment 73 Olli Pettay [:smaug] 2009-03-01 04:56:02 PST
(In reply to comment #72)
> (I don't think any changes are needed to jsd)
Well, could you tell how I could easily get a jsd object from jsval in C++ code?


> Here's another idea: provide a function which takes a JS function object and
> returns the same kind of string you do now. Then Firebug can apply that
> function to all of the Function objects it knows about and look for the match.
>
How does this approach work in C++?
Comment 74 John J. Barton 2009-03-01 09:26:34 PST
(In reply to comment #73)
> (In reply to comment #72)
> > (I don't think any changes are needed to jsd)
> Well, could you tell how I could easily get a jsd object from jsval in C++
> code?

I meant "I don't think jsd is the right tool for this job. If we could use it out of the box it would be ugly at best. Changing it for this purpose falls in the putting lipstick on a pig category."

> 
> 
> > Here's another idea: provide a function which takes a JS function object and
> > returns the same kind of string you do now. Then Firebug can apply that
> > function to all of the Function objects it knows about and look for the match.
> >
> How does this approach work in C++?

Is your question: how could this be implemented? Or: how would C++ function call this API given that it takes a JS Function object?

Actually either way my first order answer will be to point to addEventListener() which takes a JS function and your API here which gives the string value for the function. Instead of storing it for the DOM event handling and later giving it out in an EventListenerInfo,  use the same code and skip a lot of steps ;-).
Comment 75 Olli Pettay [:smaug] 2009-03-01 09:37:19 PST
(In reply to comment #74)

> Is your question: how could this be implemented? Or: how would C++ function
> call this API given that it takes a JS Function object?
My question is how would I use the API in C++?


> Actually either way my first order answer will be to point to
> addEventListener() which takes a JS function and your API here which gives the
> string value for the function
addEventListener takes an nsIDOMEventListener object as the first parameter.
XPConnect just "converts" JS function to such object.

> . Instead of storing it for the DOM event handling
> and later giving it out in an EventListenerInfo,  use the same code and skip a
> lot of steps ;-).
I don't want to give the same object back, because that would easily lead to
security problems if the method was called with some random js context.
Comment 76 John J. Barton 2009-03-01 10:08:27 PST
(In reply to comment #75)
> (In reply to comment #74)
> > Actually either way my first order answer will be to point to
> > addEventListener() which takes a JS function and your API here which gives the
> > string value for the function
> addEventListener takes an nsIDOMEventListener object as the first parameter.
> XPConnect just "converts" JS function to such object.

Ok, so the propose method would do the same:
   Astring getStringForEventListener(nsIDOMEventListener);

> 
> > . Instead of storing it for the DOM event handling
> > and later giving it out in an EventListenerInfo,  use the same code and skip a
> > lot of steps ;-).
> I don't want to give the same object back, because that would easily lead to
> security problems if the method was called with some random js context.

? What random context and same object? We're just talking about contexts with access to nsIEventListenerInfo (extensions) and string objects.

But another idea: Firebug can add an element 'foo' to the page, then take every js Function in the web page and attach it as an event listener to 'foo', then walk the nsIEventListenerInfo objects for 'foo' to find whose string matches a real element's stringValue. Not the kind of programming you'd like your kids to know you did ;-)
Comment 77 John J. Barton 2009-03-18 14:49:37 PDT
Trying to make progress again, I downloaded wip3.1, but the build fails:
g:/mozilla/mozilla-central/src/dom/src/events/nsJSEventListener.cpp(158) : error
 C2664: 'OBJECT_TO_JSVAL' : cannot convert parameter 1 from 'void *' to 'JSObjec
t *'
        Conversion from 'void*' to pointer to non-'void' requires an explicit ca
st

The patch does not seem to touch these lines. I did have to manually add #include "nsIEventListenerService.h" to nsDOMCLassInfo.cpp

Any hints?
Comment 78 Johnny Stenback (:jst, jst@mozilla.com) 2009-03-18 15:02:29 PDT
Seems to me that exposing event listeners as strings is *not* what we'd want here. What's wrong with simply exposing the actual event listener objects? Smaug, you say doing that could lead to security problem, can you please elaborate on that a bit more? And also, is this something we really want to expose to untrusted code in the first place?

FWIW, I think we should simply expose the nsIDOMEventListener objects here. If they're JS objects (or JS functions), then they'll be exposed a XPCWrappedJS objects, which have a property called "wrappedJSObject" that referes to the actual JSObject originally passed in to addEventListener(), if they're C++ listeners, there's not much we can do no matter what the API is.
Comment 79 Olli Pettay [:smaug] 2009-03-18 15:10:10 PDT
(In reply to comment #78)
> Seems to me that exposing event listeners as strings is *not* what we'd want
> here.
I agree. I just don't know what is the best way to expose the function/listener object. I mean in the .idl or so. Note, some of the listeners are C++
objects implementing nsIDOMEventListener.

> Smaug, you say doing that could lead to security problem, can you please
> elaborate on that a bit more?
Who ever gets access to nsIDOMEventListener object must not call it without
pushing the right JS context to stack.

> And also, is this something we really want to
> expose to untrusted code in the first place?
True
 
> FWIW, I think we should simply expose the nsIDOMEventListener objects here.
Not only listeners, but also their serialization if possible, IMO.
That makes it easier to debug things in C++.

> If
> they're JS objects (or JS functions), then they'll be exposed a XPCWrappedJS
> objects, which have a property called "wrappedJSObject"
I wonder if that is really true. Maybe. I'll test.
Comment 80 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-03-18 16:14:35 PDT
(In reply to comment #79)
> (In reply to comment #78)
> > Seems to me that exposing event listeners as strings is *not* what we'd want
> > here.
> I agree. I just don't know what is the best way to expose the function/listener
> object. I mean in the .idl or so. Note, some of the listeners are C++
> objects implementing nsIDOMEventListener.

That would just mean that there is no .wrappedJSObject property, no? And firebug could either ignore those or show "[Native listener]" or some such.

> > Smaug, you say doing that could lead to security problem, can you please
> > elaborate on that a bit more?
> Who ever gets access to nsIDOMEventListener object must not call it without
> pushing the right JS context to stack.

Blake says that it's fine even if the caller doesn't. Apparently we are ensured through various means that even if we use a chrome context, there will be no escalation of privileges.

Though it does mean that the event listener could clobber regexp results maybe. Wonder if that's something we need to fix. Blake?

> > FWIW, I think we should simply expose the nsIDOMEventListener objects here.
> Not only listeners, but also their serialization if possible, IMO.
> That makes it easier to debug things in C++.

I don't care much, but an alternative solution could be to create a ifdef-DEBUG-only function that takes an nsIDOMEventListener and dumps the serialization to the console. XPConnect has similar functions to dump the current JS stack.
Comment 81 Olli Pettay [:smaug] 2009-03-18 16:18:03 PDT
(In reply to comment #80)
> Blake says that it's fine even if the caller doesn't. Apparently we are ensured
> through various means that even if we use a chrome context, there will be no
> escalation of privileges.
See all the security bugs we have had with XHR event handling.
Comment 82 Olli Pettay [:smaug] 2009-03-18 16:20:38 PDT
And note, it is IMO a problem with xpconnect, not with event handling.
Comment 83 Olli Pettay [:smaug] 2009-03-18 16:24:14 PDT
(In reply to comment #81)
> (In reply to comment #80)
> > Blake says that it's fine even if the caller doesn't. Apparently we are ensured
> > through various means that even if we use a chrome context, there will be no
> > escalation of privileges.
> See all the security bugs we have had with XHR event handling.
For example bug 393762 and bug 393761
Comment 84 Sandy Foltz 2009-04-30 08:47:01 PDT
I was trying to test the eventlistenerservice with the latest Minefield

Error: Components.classes['@mozilla.org/eventlistenerservice;1'] is undefined
Source File: chrome://emptysidebar/content/headingscript.js

Where can I go to test this? (or is it not ready for testing yet)
Comment 85 Olli Pettay [:smaug] 2009-04-30 08:57:42 PDT
This bug isn't fixed yet and the patch hasn't landed, so unfortunately there
is nothing to test.

(I guess I need to discuss with blake how to expose JS listener to chrome.)
Comment 86 jongund 2009-06-04 08:02:17 PDT
The only thing we really want at this point is the ability to know if an event handler of a specific type is present or not.  So if it would be easier to implement and more secure the function could just return Boolean values on whether an event handler of a specific type is present or not on a node in the DOM.  We really do NOT care about the event object at this point.

Or even better being able to query a node and its descendants and return a node list of the nodes that have a particular event handler attached to it.  If the list is empty then no nodes have that event handler.

We want to use this information in identifying the presence or absence of user interface event handlers for testing for accessibility features for people with disabilities.  Nodes with mouse event handlers should have some keyboard event handlers and ARIA roles and properties and states associated with them.
Comment 87 Olli Pettay [:smaug] 2009-06-04 08:57:00 PDT
(In reply to comment #86)
> The only thing we really want at this point is the ability to know if an event
> handler of a specific type is present or not.
You can check that already using nsIEventListenerManager.
See bug 379763.
Comment 88 jongund 2009-07-08 08:26:07 PDT
(In reply to comment #87)
> (In reply to comment #86)
> > The only thing we really want at this point is the ability to know if an event
> > handler of a specific type is present or not.
> You can check that already using nsIEventListenerManager.
> See bug 379763.

Can you send a sample Javascript code that uses "nsIEventListenerManager" to enumerate onKeyXXX or onmouseXXX event handlers.

We are trying to build an accessibility extension for Firsfox and the rules will be written in Javascript. We basically want to know about the presence of event handlers at this point, we do not plan to try to analyze what the event handlers do or don't do.

Thanks,
Jon
Comment 89 Olli Pettay [:smaug] 2009-07-08 09:01:41 PDT
nsIEventListenerManager is C++ only, so it is available to binary extensions.

Note, event listener manager's API doesn't tell whether the listener was added
using onfoo attribute, or onfoo property or .addEvenListener("foo", ...)

I should get back to this bug and land at least something usable (but whatever
lands must be safe in security-wise).
Comment 90 jongund 2009-07-09 07:13:18 PDT
(In reply to comment #89)
We are not concerned with how the listener was added, right now our primary interest is knowing whether a listener of a certain type (onKeyXXX, onClick, onMouseXXX) is present.  We need this information to identify elements that may be interactive and need ARIA markup to describe the type widget and the widget properties to assistive technologies or need to add keyboard event handlers.

We need a javascript interface to at least some of the functionaliity of "nsIEventListenerManager" to make testing rules interoperable and easy for developers to create and modify rules related to accessibility.

At this point just having a Boolean function that can test a DOM node for specific event handler or a list of event handlers would be a great first step for us.  

Please contact me if you have any additional questions.

Thanks
Jon

> nsIEventListenerManager is C++ only, so it is available to binary extensions.
> 
> Note, event listener manager's API doesn't tell whether the listener was added
> using onfoo attribute, or onfoo property or .addEvenListener("foo", ...)
> 
> I should get back to this bug and land at least something usable (but whatever
> lands must be safe in security-wise).
Comment 91 Johnathan Nightingale [:johnath] 2009-07-09 07:53:57 PDT
The intelligent-ARIA-markup aspect of this might spur davidb into giving smaug something to review, if smaug doesn't get to it first.
Comment 92 David Bolter [:davidb] 2009-07-09 13:42:01 PDT
Johnathan thanks for the ping, but it looks like Olli has a patch and a plan. Would love to see this bug fixed for many reasons.  (Note I'm mostly on holiday)
Comment 93 Johnny Stenback (:jst, jst@mozilla.com) 2009-07-22 17:53:07 PDT
I'd love to see this fixed too, but I'm not going to block 1.9.2 on it.
Comment 94 John J. Barton 2009-07-22 20:16:38 PDT
I'll put some time into the last issue here.
Comment 95 John J. Barton 2009-07-27 21:27:13 PDT
(In reply to comment #79)
... 
> > Smaug, you say doing that could lead to security problem, can you please
> > elaborate on that a bit more?
> Who ever gets access to nsIDOMEventListener object must not call it without
> pushing the right JS context to stack.

I'm just trying to understand the issues here. I guess the idea here is that a bad web code could access an nsIDOMEventListener attached by chrome code, then use that function to gain access. Then one solution according to blake would be that the bad web code will only get a wrapped object ever from this API (because of the caller's lowly status), and thus cannot exploit; Bug 480205 would protect us from these callers.  (Or this API is not available to web pages?)

I guess all of the security problems of the actually event-handling are not made worse here. The only additional problems are via access to functions passed into the event system and out through this new API.

> 
> > And also, is this something we really want to
> > expose to untrusted code in the first place?
> True

I guess this means: "we don't really need to let web pages at this API". 
I think that is a good place to start. Strictly from the "web-api" point of view, exposing this needs a discussion. It could cause a shift in development paradigm, from a "be very careful how you add/remove handlers" to a "check at lot to decide when to add/remove handlers". I suggest we just take this option off the table for now.

> 
> > FWIW, I think we should simply expose the nsIDOMEventListener objects here.
> Not only listeners, but also their serialization if possible, IMO.
> That makes it easier to debug things in C++.

I don't understand this, because the API says:
+   * Tries to serialize event listener to a string.
+   * Returns null if serialization isn't possible
+   * (for example with C++ listeners).
+   */
+  readonly attribute AString stringValue;

> 
> > If
> > they're JS objects (or JS functions), then they'll be exposed a XPCWrappedJS
> > objects, which have a property called "wrappedJSObject"
> I wonder if that is really true. Maybe. I'll test.


In the interest of making some progress, how about adding the nsIDOMEventListener object access to the API but leaving the stringValue. Allow the stringValue to null, even allow it to be null for some objects in a release build that may be non-null in a debug build. The nsIDOMEventListener could also be null, meaning "not javascript" or "not for you, toady", though I think we will happier if null can't happen or for exact one reason.

What can I do to help make this happen?
Comment 96 Boris Zbarsky [:bz] (still a bit busy) 2009-07-27 23:06:16 PDT
> I guess the idea here is that a bad web code could access an
> nsIDOMEventListener attached by chrome code, then use that function to gain
> access.

No the idea there is that chrome code could get access to an nsIDOMEventListener attached by web code, accidentally call handleEvent on it, and end up running part of the web code with chrome privileges as a result, which can be bad if the web code is malicious.

The problem here is that one wouldn't expect consumers of this API (who would certainly be chrome only) to be aware of the above caveat.

> The only additional problems are via access to functions passed into the event
> system and out through this new API.

Right.
Comment 97 John J. Barton 2009-07-28 11:07:31 PDT
Bug 506961 is an effort to solve the open problem of access to the JS function underlying the nsIDOMEventListener.

If idea is to add the following to nsIEventListenerInfo (this bug):
    readonly attribute nsIDOMEventListener listener
then in 506961 we create a converter to allow the debugger to get the function. That means that we don't entangle event code and jsd, the debugger does a tiny bit of extra work.

From the architecture point of view this would be great; from the API point of view, not so much. Can anything useful be done with the listener? EG can it be called?

I did not mark dependency between these bugs; some good can come from each without the other.
Comment 98 Boris Zbarsky [:bz] (still a bit busy) 2009-07-28 11:11:00 PDT
> Can anything useful be done with the listener? 

Good question.

> EG can it be called?

Not safely.  Nor can the raw function be called safely.  Don't know whether it can be touched safely (e.g. calling its toString).
Comment 99 jongund 2009-08-12 13:26:42 PDT
Any idea when there will be something available for testing?
Comment 100 John J. Barton 2009-08-13 14:40:03 PDT
Can someone please create a tryserver build for Jon with the current patch? His work on accessibility tools is blocked by this bug and he can make a lot of progress with the current almost-complete api. I have used the parts of the API he needs and its fine to get going.
Comment 101 Olli Pettay [:smaug] 2009-08-13 14:57:04 PDT
I can do that tomorrow
Comment 102 Olli Pettay [:smaug] 2009-08-20 06:07:41 PDT
Created attachment 395572 [details] [diff] [review]
up-to-date

Sorry for the delay!

In a few hours there should be tryserver (eventlistenerservice) builds in https://build.mozilla.org/tryserver-builds/?C=M;O=D
Comment 103 John J. Barton 2009-08-20 15:26:01 PDT
Thanks. I tried this build but ran in to problems. First is mine, all of the extensions have maxVersion problems because this is a FF 3.7 based build. Second I was not able to get the interface:
var iface = Components.interfaces.nsIEventListenerService;

if (iface)
    window.dump("***************** eventbug.js got nsIEventListenerService ***********************\n");
else
    window.dump("***************** eventbug.js NO nsIEventListenerService ***********************\n");

Gives:
***************** eventbug.js NO nsIEventListenerService ***********************

and consequently 

var eventListenerService = eventListenerClass.getService(Components.interfaces.nsIEventListenerService);

gives 
NS_ERROR_XPC_BAD_IID.

Any ideas?
Comment 104 Olli Pettay [:smaug] 2009-08-20 15:37:17 PDT
I just tested the tryserver build (on OSX) and
a local copy of https://bugzilla.mozilla.org/attachment.cgi?id=346112 worked just fine.
Comment 105 John J. Barton 2009-08-20 16:27:25 PDT
Ok, just a hint to others, "a local copy" means you have to download the linked file and run it via File > Open File or similar.

But on my machine the result is the same as my extension:

uncaught exception: [Exception... "Component returned failure code: 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.getService]"  nsresult: "0x80570018 (NS_ERROR_XPC_BAD_IID)"  location: "JS frame :: file:///C:/Documents%20and%20Settings/bartonjj/My%20Documents/Downloads/eventlistenerservice.html :: test :: line 17"  data: no]

Line 0

I can see the "@mozilla.org/eventlistenerservice;1" entry in Components.classes and see that has the right GUID. So I believe that I am running the correct build.

But I don't see any nsIEventListenerInfo property in Components.interfaces.

What could be the difference between our environments? WinXP? How does Ci get built?
Comment 106 John J. Barton 2009-08-20 17:44:47 PDT
Ok, I got some info from Ben. The nsIEventListenerService should be in an .xpt file. The only .xpt file in the try-server build is components/browser.xpt. In that (binary) file, I don't see nsIEventListenerService.

So Olli, if you look into your profile under xpti.dat file you should see some entry  under [Files] that differs from mine:

[Files,1]
0,browser.xpt,0,363735,1250777866000

Just to be certain, delete this xpti.dat file, re-run your try-server test, then check the xpti.dat file (to insure it was generated from the try-server build and not a previous run with a different build.
Comment 107 Olli Pettay [:smaug] 2009-08-21 03:58:09 PDT
Ok, I can reproduce the problem on linux but not on OSX.
Comment 108 Olli Pettay [:smaug] 2009-08-21 04:07:36 PDT
Created attachment 395808 [details] [diff] [review]
up-to-date

Uploaded this to tryserver. Hopefully it fixes windows/unix packaging problems.
Comment 109 Olli Pettay [:smaug] 2009-08-21 04:16:09 PDT
Created attachment 395809 [details] [diff] [review]
up-to-date
Comment 110 Olli Pettay [:smaug] 2009-08-21 05:22:40 PDT
This works on linux
https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-eventlistenerservice10b/
Comment 111 John J. Barton 2009-08-21 16:41:03 PDT
Ok thanks! Your test case and my extension work now.

jon, you can look at the test code attached to this bug (save as file first) or my extension at 

http://code.google.com/p/fbug/source/browse/#svn/eventbug/branches/eventbug0.1/content
(the test code from this bug is also in that directory).

The 1.5a22 version of Firebug should be out first thing next week; it will have the version setting changed to FF 3.7 so it can load directly into the try-server build.
Comment 112 John J. Barton 2009-08-21 16:44:58 PDT
Created attachment 395987 [details]
Screen Shot of Firebug Prototype Event Panel

This is what the eventbug extension adds to firebug. The elements highlight in the page and link to the HTML panel. The function links don't work, because I can't get to the functions with the nsIEventListnerInfo as it stands.
Comment 113 jongund 2009-08-25 08:42:37 PDT
John,

Sandy has built the test extension and it gives us what we need.  

Sandy is already adding rules into the A11y Firebug Accessibility Extension.

Thank you to you and everyone who helped make these changes possible!

Jon
Comment 114 Jan Honza Odvarko [:Honza] 2009-09-01 03:24:54 PDT
It would be great if nsIEventListenerService supports also registering callbacks that are called when a new event listener is added or an existing removed. This would allow to keep the event panel (as depicted in comment #112) up to date without necessity to entirely refresh the view (which can be time consuming).

Should I report a new bug for this?

Honza
Comment 115 Olli Pettay [:smaug] 2009-09-01 03:29:47 PDT
(In reply to comment #114)
> It would be great if nsIEventListenerService supports also registering
> callbacks that are called when a new event listener is added or an existing
> removed.
Good idea. I'll add that. Should be easy to implement.
Comment 116 Olli Pettay [:smaug] 2009-09-01 03:34:00 PDT
I guess it might be useful to have notification for "event target deleted".
Just don't know what the notification should get as parameters. Maybe
a list of DOMEventListener objects which were registered to that event target.
Comment 117 Olli Pettay [:smaug] 2009-09-01 03:39:58 PDT
(In reply to comment #116)
>Maybe
> a list of DOMEventListener objects which were registered to that event target.
No, that wouldn't work. The same listener maybe used with many event target.
And event target itself can't be passed, because it is being deleted.

Maybe there should be a wrapper which has a weak ref to the event target.
Comment 118 Jan Honza Odvarko [:Honza] 2009-09-01 09:41:37 PDT
(In reply to comment #116)
> I guess it might be useful to have notification for "event target deleted".
Yes

(In reply to comment #117)
> Maybe there should be a wrapper which has a weak ref to the event target.
Yes, having the target passed to callbacks is important.

Honza
Comment 119 John J. Barton 2009-09-09 11:19:04 PDT
(In reply to comment #97)
> Bug 506961 is an effort to solve the open problem of access to the JS function
> underlying the nsIDOMEventListener.

I now believe that bug 5506961 will be INVALID because jsdIValue already has the function we need. The test case crashes but we can probably fix it.
 
> If idea is to add the following to nsIEventListenerInfo (this bug):
>     readonly attribute nsIDOMEventListener listener

So can we add this to the patch or just suggest where it might go and I can try it on my build.
Comment 120 John J. Barton 2009-09-10 11:32:20 PDT
I could not get the patch to apply on trunk from 10SEP2009
Comment 121 John J. Barton 2009-09-10 14:53:52 PDT
Created attachment 399839 [details]
refresh for 10SEP09 

The previous patch has two kinds of problems on the 3.7 source now. One I don't understand having to do with the install. The other one was patch rejections that I fixed. In all three spots that rejected I re-added the source lines above the ifdef MOZ_ENABLE_CANVAS3D. It looks like that block of code is changing and causing the rejection and it seems more logical to group the EventListenerInfo changes with the text above the ifdef anyway. 

The result builds and works for me.
Comment 122 John J. Barton 2009-09-11 17:00:59 PDT
Ok I've run out of ideas on how to extract a Function from an nsIDOMEventListener, so adding a 'listener' property to the interface here won't help me.

I propose we give up on connecting the event listener function to the source object in Firebug and just land what is here now. I'd rather have 1/2 a great feature than no feature at all.
Comment 123 John J. Barton 2009-09-18 15:10:59 PDT
Ok I took one more run at the function and finally succeeded.  Our prototype Event panel in Firebug now links the event listener to the Script panel source when you click on the function summary.

I read through the source to uncover what "stringValue" meant. In fact the listener stringValue is the result of calling function.toSource(). I verified this by comparing the listener.stringValue to function.toSource() for all the functions in the web page and finding the correct match (this is how the Event panel link is implemented).

Therefore I suggest that the name of this property be changed to 
   readonly attribute AString toSource;
and the documentation
   Tries to serialize event listener to a string
to
   Tries to serialize event listener using toSource() 

And we need to land this, please!
Comment 124 Olli Pettay [:smaug] 2009-09-19 03:46:53 PDT
(In reply to comment #123)
> And we need to land this, please!
I agree.
So would it be ok to you to take the current patch (well, maybe change the attribute name to "toSource", or "source", or change it to be method toSource())?
Comment 125 John J. Barton 2009-09-28 17:13:34 PDT
I did some more work with eventbug and tried it in Chromebug. The basic function was very cool, but unfortunately I discovered a problem. Firebug, like other JS frameworks, uses a closure to bind "this" and other arguments in eventlisteners. As a result, all of the listeners appear to have the same source code, and the toSource comparisons does not locate the correct one.
Comment 126 John J. Barton 2009-09-28 17:25:08 PDT
(In reply to comment #124)
> (In reply to comment #123)
> > And we need to land this, please!
> I agree.
> So would it be ok to you to take the current patch (well, maybe change the
> attribute name to "toSource", or "source", or change it to be method
> toSource())?

Peregrino suggested that this question was addressed to me but I don't understand it.
Comment 127 Olli Pettay [:smaug] 2009-09-29 01:01:44 PDT
So the question is, would it be ok to take the current patch as it is?
Or should I add something what was suggested in #114?
Comment 128 John J. Barton 2009-09-29 08:32:51 PDT
Changing the attribute name from stringValue to toSource would be enough.

Our second priority would be to add an attribute 'listener', see comment #80. I guess the addition is easy but you are still concerned about security despite blake's assurances (see comment #81). And I don't know enough (yet?) to use the resulting property anyway, so this would be future-proofing effort, they are usually dubious. 

The events suggested in #114 would be great, but based on the discussion on bug 518524 many new issues could be raised that would delay this bug. 

So I suggest we just go with the name change and get the rest next time.
Comment 129 Olli Pettay [:smaug] 2009-10-08 05:54:20 PDT
Created attachment 405249 [details] [diff] [review]
v12

Would be great to get this in even in its current form.
New features can be added when needed.

Jst, just say if you're too busy to review this.
Comment 130 Olli Pettay [:smaug] 2009-10-08 07:08:13 PDT
(In reply to comment #114)
> Should I report a new bug for this?
> 
> Honza
So, after all, please file a new bug.
Comment 131 Olli Pettay [:smaug] 2009-10-09 03:46:29 PDT
Created attachment 405440 [details] [diff] [review]
v12.1
Comment 132 Olli Pettay [:smaug] 2009-10-09 08:50:34 PDT
Created attachment 405501 [details] [diff] [review]
v12.2

Or perhaps bz can review this before bug 506961 ;)
Comment 133 Boris Zbarsky [:bz] (still a bit busy) 2009-10-09 21:51:53 PDT
Comment on attachment 405501 [details] [diff] [review]
v12.2

>+++ b/content/events/public/nsEventDispatcher.h
>+   * If aTargets is non-null, event target chain will be created, but
>+   * event won't be handled.

Document that aEvent->message must be null in this case?

>+++ b/content/events/public/nsIEventListenerManager.h

>+   * Sets nsIEventListenerInfo objects about each listener to aList.

"Sets aList to the list of nsIEventListenerInfo objects representing the listeners managed by this listener manager"?

>+++ b/content/events/src/nsEventListenerManager.cpp
>+nsEventListenerManager::GetListenerInfo(nsCOMArray<nsIEventListenerInfo>* aList)
>+    nsListenerStruct* ls = &mListeners.ElementAt(i);

Why not:

  const nsListenerStruct& ls = mListeners.ElementAt(i);

I'd prefer that (esp. the const part).

>+        CompileEventHandlerInternal(jslistener->GetEventContext(),
>+                                    jslistener->GetEventScope(),
>+                                    jslistener->GetEventTarget(),
>+                                    ls->mTypeAtom, ls, mTarget);

Is this why it can't be const?  I'd rather explicitly const-cast for that or something...

>+          nsAutoString eventType;
>+          eventType.AssignASCII(eventName);

  NS_ConvertASCIItoUTF16 eventType(eventName)

?

>+        nsAutoString eventType;
>+        eventType = Substring(atomName, 2, atomName.Length() - 2); // Remove "on"

const nsDependentSubstring& eventType =
  Substring(atomName, 2, atomName.Length() - 2)

>+        nsAutoString eventType;
>+        eventType.AssignASCII(eventName);

NS_ConvertASCIItoUTF16

>+++ mozilla/content/events/public/nsIEventListenerService.idl	2009-10-08 13:46:55.000000000 +0300
>+interface nsIEventListenerInfo : nsISupports

Document?

>+  readonly attribute AString type;

Document?  Esp., does this include "on"?

>+   * Returns an array of nsIEventListenerInfo objects.
>+   * If aEventTarget doesn't have any listeners, this returns null.
>+   */
>+  nsIVariant getListenerInfoFor(in nsIDOMEventTarget aEventTarget);

Why is this returning an nsIVariant instead of an actual array?

>+   * Returns an array of event targets.
>+   * aEventTarget will be at the index 0.

s/the index/index/

>+   * The objects are the same, which would be used as DOMEvent.currentTarget
>+   * when dispatching an event.

"The objects are the ones that would be used as DOMEvent.currentTarget while dispatching an event to aEventTarget"

>+   * @note Some events, especially 'load', may actually have shorter

"a shorter"

>+%{ C++
>+#define NS_EVENTLISTENERSERVICE_CID                 \
>+ { /* baa34652-f1f1-4185-b224-244ee82a413a */       \
>+   0xbaa34652, 0xf1f1, 0x4185,                      \
>+  {0xb2, 0x24, 0x24, 0x4e, 0xe8, 0x2a, 0x41, 0x3a } }
>+#define NS_EVENTLISTENERSERVICE_CONTRACTID \
>+  "@mozilla.org/eventlistenerservice;1"
>+%}

I'd really prefer we didn't dump this in the IDL.  Perhaps nsContentCID.h instead?

>+++ mozilla/content/events/src/nsEventListenerService.h	2009-10-08 12:31:05.000000000 +0300
>+class nsEventListenerInfo : public nsIEventListenerInfo
>+  nsRefPtr<nsIDOMEventListener> mListener;

nsCOMPtr?  And perhaps cycle collection for mListener?
Comment 134 Olli Pettay [:smaug] 2009-10-10 02:24:45 PDT
> >+  readonly attribute AString type;
> 
> Document?  Esp., does this include "on"?
Event types don't include "on", but sure, I'll document.

> Why is this returning an nsIVariant instead of an actual array?
What kind of array would it return, nsIArray? Using nsIArray in JS
isn't quite as nice as using nsIVariant.
Though, nsIArray would give me cycle-collectability.


> >+%{ C++
> >+#define NS_EVENTLISTENERSERVICE_CID                 \
> >+ { /* baa34652-f1f1-4185-b224-244ee82a413a */       \
> >+   0xbaa34652, 0xf1f1, 0x4185,                      \
> >+  {0xb2, 0x24, 0x24, 0x4e, 0xe8, 0x2a, 0x41, 0x3a } }
> >+#define NS_EVENTLISTENERSERVICE_CONTRACTID \
> >+  "@mozilla.org/eventlistenerservice;1"
> >+%}
> 
> I'd really prefer we didn't dump this in the IDL.  Perhaps nsContentCID.h
> instead?
Huh. I hate when the default CID and CONTRACTID aren't in the interface,
especially in the cases where there is really just one implementation.


> >+++ mozilla/content/events/src/nsEventListenerService.h	2009-10-08 12:31:05.000000000 +0300
> >+class nsEventListenerInfo : public nsIEventListenerInfo
> >+  nsRefPtr<nsIDOMEventListener> mListener;
> 
> nsCOMPtr?  And perhaps cycle collection for mListener?
nsIDOMEventListeners are a bit strange. They require nsRefPtr here, like
in nsListenerStruct.
Cycle collecting, not sure if it is really worth.
Comment 135 Olli Pettay [:smaug] 2009-10-10 02:28:42 PDT
(In reply to comment #134)
> > Why is this returning an nsIVariant instead of an actual array?
> What kind of array would it return, nsIArray? Using nsIArray in JS
> isn't quite as nice as using nsIVariant.

Or maybe I'm wrong here. Does nsIArray provide [] indexing? At least could
find any documentation about it.
Comment 136 Ingo.Krueger 2009-10-10 09:26:37 PDT
I'm happy to see that the eventlistener-service is on the way (it's also needed for one of my addons).
The attribute stringValue is useful for me in order to remove special event-listeners. However, I see no way to handle anonymous event-listener functions since the stringValue cannot be converted to a function handler.
Wouldn't it be a good idea to always include the function handlers in the enumeration, so that anonymous event-listener functions can be removed too?
Sorry for disturbing the discussion, I just wonder why nobody requested that feature. Regards
Comment 137 Olli Pettay [:smaug] 2009-10-10 09:31:35 PDT
Bug 506961 adds a way to get the debug object of the listener.
Perhaps that can be used to remove a listener.
Comment 138 Olli Pettay [:smaug] 2009-10-13 08:56:58 PDT
Boris, do you really want nsIArray or js array
or is the "easier for JS, and possible to use in C++" nsIVariant ok?
And what about CID and CONTRACTID? Should I move them to somewhere else (where it is harder to find them, IMO)?

I'll update the patch soon.
Comment 139 Boris Zbarsky [:bz] (still a bit busy) 2009-10-13 09:06:52 PDT
> What kind of array would it return, nsIArray?

Can it just return a native JS array (using [array])?

It doesn't look like nsIArray supports classinfo, so no [] indexing.  Does nsIVariant end up doing so in the VTYPE_ARRAY case?  If so, that seems like a good argument for doing it if we can't do [array], but we should document that the variant will always be of a given type.  And file a followup bug to make it possible to sanely address this use case?

> Huh. I hate when the default CID and CONTRACTID aren't in the interface,

I think we've generally decided we shouldn't be doing that.  Please put them in the CID file.

> They require nsRefPtr here, like in nsListenerStruct.

I don't see why.  If there's a good reason, it should be clearly documented (both places).  Certainly lots of code holds nsCOMPtr<nsIDOMEventListener> without seeming ill effects, right?

> Cycle collecting, not sure if it is really worth.

We don't want to leak if someone happens to stick the listener info on their listener or some such.  So yes, we need cycle colection here.

I don't think we should be relying on the js debugger thing for listener removals.  But we can  add a removeEventListener or equivalent that just takes nsIEventListenetInfo, in a followup bug.  Which someone should file.
Comment 140 John J. Barton 2009-10-13 09:19:27 PDT
(In reply to comment #139)
...
> I don't think we should be relying on the js debugger thing for listener
> removals.  But we can  add a removeEventListener or equivalent that just takes
> nsIEventListenetInfo, in a followup bug.  Which someone should file.

"the js debugger thing" is just a hack around the lack of platform support for getting the function back from this api. Adding a removeEventListener is another hack cause by the same problem. I know there are lots of reasons why the "right" thing is hard to do, but in effect these hacks are just as expensive only incrementally so.
Comment 141 Olli Pettay [:smaug] 2009-10-13 09:29:47 PDT
(In reply to comment #139)
> Can it just return a native JS array (using [array])?
And how would C++ callers use the API?

>  And file a followup bug to make it
> possible to sanely address this use case?
I'm not sure what you mean here. nsIVariant is pretty "sane" to me.

> I think we've generally decided we shouldn't be doing that.
I haven't heard or read about such decision.

> I don't see why.
IIRC it is because of 
nsIDOMXXXListener interfaces, which ELM handles in a special way.

> We don't want to leak if someone happens to stick the listener info on their
> listener or some such.  So yes, we need cycle colection here.
Well, callers are in chrome only, so we could just expect callers to do
sane things.
Comment 142 Boris Zbarsky [:bz] (still a bit busy) 2009-10-13 11:14:15 PDT
> And how would C++ callers use the API?

Why would they need to, honestly?  But in general, they'd use it like they do any [array] IDL api, no?  The get a C++ array.

> nsIVariant is pretty "sane" to me.

It's not sane from either C++ or JS, really.

> I haven't heard or read about such decision.

I can hunt down the references if you really want, I guess...

> IIRC it is because of  nsIDOMXXXListener interfaces

"special way" meaning you can't call QI on the pointer?   I mean, that's the main thin you get out of not using nsCOMPtr, right?

> so we could just expect callers to do sane things.

Experience with extensions shows that we can't.

John, it would be trivial to pass out the "function" (which is in fact not at all necessarily a function, but whatever).  The hard part is not doing that, but then having the caller (your code, say) not shoot itself in the foot trying to work with that object.
Comment 143 Olli Pettay [:smaug] 2009-10-13 11:44:40 PDT
> Why would they need to, honestly?
I need this for debugging. I don't use Firebug for anything, but do want to
check what the listeners are and also the event target chain.
So I'll probably write small C++ snippets when needed. 

>  But in general, they'd use it like they do
> any [array] IDL api, no?
Ok. I add the [array]. Ugly in .idl, ugly in C++, but sure, it should work

> It's not sane from either C++ or JS, really.
I've never understood what people have against nsIVariant.

> I can hunt down the references if you really want, I guess...
No need. Now I know :) (though, don't know the reasoning)

> "special way" meaning you can't call QI on the pointer?   I mean, that's the
> main thin you get out of not using nsCOMPtr, right?
Right. nsIDOMXXXListeners use DispatchToInterface in ELM to call right
method in the interface.
Comment 144 Boris Zbarsky [:bz] (still a bit busy) 2009-10-13 12:24:36 PDT
But does that mean the objects don't actually QI to nsIDOMEventListener or something?
Comment 145 Olli Pettay [:smaug] 2009-10-14 13:44:00 PDT
(In reply to comment #144)
> But does that mean the objects don't actually QI to nsIDOMEventListener or
> something?
Don't remember right now. Not about this bug anyway.
Comment 146 Olli Pettay [:smaug] 2009-10-14 13:49:31 PDT
Created attachment 406286 [details] [diff] [review]
with [array]

IMO, this is uglier to use even in JS. One needs to pass {} as a parameter.
But this was requested.
Comment 147 Boris Zbarsky [:bz] (still a bit busy) 2009-10-15 11:38:00 PDT
Comment on attachment 406286 [details] [diff] [review]
with [array]

>  NS_ConvertASCIItoUTF16 eventType(eventName)

Still need to address this (you changed the second instance of it, but not the first).

> "a shorter"

Still need to make this change.

Also still need to document the nsRefPtr thing so someone doesn't go and change it to nsCOMPtr.

With that, looks fine.
Comment 148 Olli Pettay [:smaug] 2009-10-16 02:01:35 PDT
Created attachment 406655 [details] [diff] [review]
v14
Comment 149 Olli Pettay [:smaug] 2009-10-16 02:45:01 PDT
http://hg.mozilla.org/mozilla-central/rev/382c73f2650f
Comment 150 John J. Barton 2009-10-22 15:09:20 PDT
I am trying to get the Firebug code to work with the FF 3.7 version of this and I got stuck. 

Before I had this call:
    var infos = els.getListenerInfoFor(elt); 
but now I get "not enough arguments" error.  The new IDL is completely opaque to me:

73   /**
74    * Returns an array of nsIEventListenerInfo objects.
75    * If aEventTarget doesn't have any listeners, this returns null.
76    */
77   void getListenerInfoFor(in nsIDOMEventTarget aEventTarget,
78                           out unsigned long aCount,
79                           [retval, array, size_is(aCount)] out
80                             nsIEventListenerInfo aOutArray);
81
Comment 151 Boris Zbarsky [:bz] (still a bit busy) 2009-10-22 15:15:45 PDT
You need getListenerInfoFor(elt, {}) until I land that patch I put up this morning to make the count out param optional.
Comment 152 John J. Barton 2009-10-23 15:33:54 PDT
I updated our UI code to run off the FF 3.7 nightly and as far as I can tell everything is working. 
http://code.google.com/p/fbug/source/browse/eventbug
This includes using the getDebugObject() and .script property:

         var fn = listenerInfo.getDebugObject();
         if (fn && fn instanceof Ci.jsdIValue)
         {
             var script = fn.script;
             return script;
         }

Honza will be extending our UI to cover the rest of the api and we'll try to get the extension and install instructions out late next week. As soon as we have some experience we need to ask for 1.9.2.
Comment 153 Jan Honza Odvarko [:Honza] 2009-10-26 12:38:51 PDT
I have extended Eventbug using the new getEventTargetChainFor method. All is working fine!

The last thing is the support for dynamic addition and removal of event handlers, see comment 114.

Should I report a new bug for this?

Honza
Comment 154 Olli Pettay [:smaug] 2009-10-26 13:00:45 PDT
(In reply to comment #153)
> Should I report a new bug for this?
Yeah, if you could file a new bug.
Comment 155 Olli Pettay [:smaug] 2009-10-26 13:20:09 PDT
And I think we need support for XBL event handlers (for Chromebug).
Another followup bug.
Comment 156 Jan Honza Odvarko [:Honza] 2009-10-27 08:03:11 PDT
I have reported a new bug related to the support for dynamic addition and removal of event handlers.

https://bugzilla.mozilla.org/show_bug.cgi?id=524674

Honza
Comment 157 Rob Campbell [:rc] (:robcee) 2009-10-29 14:41:55 PDT
sorry to keep beating on this, but I maintain that this would be an awesome addition to 3.6. *IF* it's sufficiently low-risk. 

olli and jst: do you think this poses any risks to landing on 1.9.2? Is this something we could approve in good conscience? Talking with Johnath, we agree this shouldn't hold the release, but could still land.
Comment 158 Rob Campbell [:rc] (:robcee) 2009-10-29 14:43:01 PDT
adding wanted-1.9.2 just-in-case.
Comment 159 Olli Pettay [:smaug] 2009-10-29 16:04:34 PDT
Just tested on 1.9.2, and the patches for this bug and for Bug 506961 applies
cleanly and the testcase works ok.
So if you want these to 1.9.2, feel free to ask approval for the patches.
Comment 160 Rob Campbell [:rc] (:robcee) 2009-10-29 16:51:54 PDT
Also confirmed (and using that build to enter this note)! Talking with Smaug earlier, used --fuzz=5 and both patches applied cleanly. Requesting approval on individual patches.
Comment 161 Rob Campbell [:rc] (:robcee) 2009-10-29 16:52:52 PDT
Comment on attachment 406655 [details] [diff] [review]
v14

test, working. Applied with --fuzz=5
Comment 162 John J. Barton 2009-10-29 20:21:41 PDT
If this lands in FF3.6 Firebug team will work to get users to try it and report back.
Comment 163 Rob Campbell [:rc] (:robcee) 2009-10-29 21:02:15 PDT
I am cloning a tree and will run this and patch in bug 506961 through try servers tomorrow to verify that there are no problems. Once that done, I'm recommending approval for checkin. Will post results here tomorrow.
Comment 164 Rob Campbell [:rc] (:robcee) 2009-10-30 07:15:03 PDT
Local mochitest results were:
108676 INFO Passed: 102912
108677 INFO Failed: 8
108678 INFO Todo:   1267
108679 INFO SimpleTest FINISHED

All failures were in /tests/layout/generic/test/test_backspace_delete.xul.

Not sure how serious that is given that my build environment and operating system are non-standard. E.g., OS X 10.6.1.

Still waiting on results from try builds.
Comment 165 Rob Campbell [:rc] (:robcee) 2009-10-30 08:13:47 PDT
big 3 platforms have completed. Try builds available here:

https://build.mozilla.org/tryserver-builds/rcampbell@mozilla.com-192-events/

still waiting on unittest completion, but that could be awhile based on the number of builds running on each of the machines right now. Initial talos run on Linux looked good (green).

Maemo failed, WinMO and WinCE both passed. Not sure if we're watching Maemo for 1.9.2.

So, I'm going to recommend we try getting this in and running it through the checkin gauntlet.
Comment 166 Rob Campbell [:rc] (:robcee) 2009-10-30 08:34:27 PDT
linux and mac try unittests have passed.
Comment 167 John J. Barton 2009-10-30 10:00:08 PDT
Rob, does your try build include bug 521010? It is required for eventbug implementation.
Comment 168 Rob Campbell [:rc] (:robcee) 2009-10-30 11:21:16 PDT
(In reply to comment #167)
> Rob, does your try build include bug 521010? It is required for eventbug
> implementation.

ah, no. I missed that one. I'll add it to my tree and try again.
Comment 169 Rob Campbell [:rc] (:robcee) 2009-10-30 13:03:28 PDT
builds are showing up here with 521010 applied to the others:

http://build.mozilla.org/tryserver-builds/rcampbell@mozilla.com-192-events-2/

so far linux and windows have completed.
Comment 170 Rob Campbell [:rc] (:robcee) 2009-10-30 15:07:17 PDT
warnings on Win32 (t-fail) and mac (leak). Not sure how reliable these are. All builds completed though.
Comment 171 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-03 12:12:27 PST
Comment on attachment 406655 [details] [diff] [review]
v14

a192=beltzner
Comment 172 Rob Campbell [:rc] (:robcee) 2009-11-04 05:22:11 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5ec9beeb58bd
Comment 173 Eric Shepherd [:sheppy] 2010-11-18 13:41:33 PST
Initial documentation. Pinging robcee for review and to answer some questions:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIEventListenerService
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIEventListenerInfo

Since the comments are scarce-ish in the IDL, I have some concerns about the accuracy of the docs. They need a once-over by someone "in the know".
Comment 174 Rob Campbell [:rc] (:robcee) 2010-11-19 09:17:54 PST
solid, sheppy. thanks!

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