506961 - Add a method to get jsdIValue from JS implemented event listeners

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Nochum Sossonko [:Natch]]

Were you trying to say something here :)

Comment 2

[John J. Barton]

Summary from newsgroup topic Returning function objects, reply by Boris Zbarsky
> Could we have a jsd "converter" function? EG
>   var theFunction = jsdConverter.toIValue(info.listener);

Absolutely.  I'd just put it on the jsd service, probably.

> The converter would cast the C++ object to a base type it can understand, look up the JS object, wrap it in jsd goop and return it.

Yep.

Comment 3

[John J. Barton]

The motivating use case for this enhancement is Bug 448602.
In Bug 449464 I have started jsdIDebuggerService2, we might merge these efforts depending on whether one effort to extend jsd is better than two.

Comment 4

[timeless]

so, a newsgroup link would be appreciated.

Comment 5

[John J. Barton]

(In reply to comment #4)
> so, a newsgroup link would be appreciated.

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/152d7d0f590efb15/d553b23423a769a4?show_docid=d553b23423a769a4

Comment 6

[John J. Barton]

I need a test case to help me work on this, something similar to what nsIEventListenerInfo.listener would be if it existed. I guess that this could be pretty much any thing that goes into the platform API as JS and then comes back out, but I'd rather not guess and get off track. So: what is the closest thing in the current API to what 'listener' might be?

Comment 7

[John J. Barton]

I wonder if jsd already provides this feature:
354 
355     /**
356      * When called from JavaScript, this method returns the jsdIValue wrapper
357      * for the given value.  If a wrapper does not exist one will be created.
358      * When called from another language this method returns an xpconnect
359      * defined error code.
360      */
361     jsdIValue wrapValue (/*in jsvalue value*/);

I did a quick hack, modifying spy.js XMLHttpRequestSpy.attach to add:

if (this.xhrRequest.onreadystatechange instanceof Ci.nsIDOMEventListener)
        {
            FBTrace.sysout("jsd explorer hack, onreadystatechange is instanceof nsIDOMEventListener: ", this.xhrRequest.onreadystatechange);
            var handler = Firebug.Debugger.getFunctionByProperty(this.xhrRequest.onreadystatechange);
            if (handler instanceof Ci.jsdIValue)
            {
                FBTrace.sysout("jsd explorer hack,got jsdIValue from wrapValue: "+handler, handler);
                var value = handler.getWrappedValue();
                FBTrace.sysout("jsd explorer hack,  wrapValue.getWrappedValue: "+value, value);
            }
        }

whhere getFunctionByProperty just returns jsd.wrapValue(prop).

What I get back is 
jsd explorer hack,got jsdIValue from wrapValue: [xpconnect wrapped jsdIValue]
jsd explorer hack, wrapValue.getWrappedValue: [xpconnect wrapped nsIDOMEventListener]

Two problems: first the request.onreadystatechange is directly set in javascript, not quite like addEventListener(), and two I crashed as soon as I tried to look into the object returned by getWrappedValue.

http://crash-stats.mozilla.com/report/pending/5d4e899f-0eb6-47f4-8ec4-560c82090831

Comment 8

[timeless]

i've posted a patch for the incident from comment 7 in bug 514073

Comment 9

[John J. Barton]

I'm stuck.

Comment 10

[John J. Barton]

I dug around some more and now I guess that wrapValue() does nothing for us.

The 
this.xhrRequest.onreadystatechange
object is [xpconnect wrapped nsIDOMEventListener] so it agrees to instanceof nisDOMEventListener, but when we put it into wrapValue() we are getting a jsdIValue wrapper around the wrapper. So when we pull it out again we've just gone in a circle.

Also the [xpconnect wrapped nsIDOMEventListener] has no wrappedJSObject (of course I guess).

Comment 11

[John J. Barton]

I thought I would try the wrappedJSObject trick:

    var request = new XMLHttpRequest();
    request.onreadystatechange = function aFunction() {
        window.dump("From aFunction");
    };
    request.onreadystatechange.wrappedJSObject = request.onreadystatechange;
    //window.dump(request.onreadystatechange);

But I got a weird error message:
errors.observe nsIScriptError: uncaught exception: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  

Then I tried it the other way:
   function aFunction() {
        window.dump("From aFunction");
    };
    aFunction.wrappedJSObject = aFunction;
    request.onreadystatechange = aFunction;
Now when I look the [xpconnect wrapped nsIDOMEventListener] object I see a wrappedJSObject property that points to aFunction.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> But I got a weird error message:

That error message says exactly what it means.  For most XPCWrappedNatives (i.e. most C++ objects reflected into JS) you can't just add random "expando" properties.

Comment 13

[John J. Barton]

But I can add a (not so random? wrappedJSObject) property to a JS object, then set the XPCWrappedNative's (non-expando) property to the JS object. And later when I look that the XPCWrappedNative property, it has the JS object's new property. 

I guess this entirely by coincidence, because XPConnect was not really invoked here? So this does not simulate the event listener case.

Comment 14

[James Ross]

request.onreadystatechange = foo; passes the JS object into the native code for setting onreadystatechange, which is typed as expecting nsIDOMEventListener. XPConnect thus creates a nsIDOMEventListener wrapper (tearoff?) for the native code.

request.onreadystatechange.wrappedJSObject = ...; first gets the onreadystatechange property, which is typed as nsIDOMEventListener. The native code returns the XPConnect-created wrapper, and XPConnect creates a JS native wrapper for that (XPCNativeWrapper).

So you have the original object wrapped twice when it returns to JS. This is how things are meant to work - the code (both native and JS) using the nsIDOMEventListener implementation shouldn't be able to tell what implemented it when using it normally.

Accessing properties of XPCWrappedNative wrappers look for the property on any interfaces they know the native object implements - you can only get and set properties that exist on interfaces the native object implements (even if that wraps a JS object). wrappedJSObject is the exception; it will look for the property (of the same name) on the JS object underneath both wrappers. Note that you can set wrappedJSObject to any JS Object and you'll get that out of XPCWrappedNative.wrappedJSObject - it doesn't have any requirement to be object getting wrapped.

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/idl/nsIXPConnect.idl#248 has some more comments on this.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> And later when I look that the XPCWrappedNative property, it has the JS
> object's new property. 

That's correct, because in that case the wrappedJSObject is being used the way it's supposed to be (to gain access to the underlying JSObject when dealing with a doubly-wrapped JSObject in JS).

> I guess this entirely by coincidence

Not at all.  It's working as designed.  The above ONLY works for the "wrappedJSObject" property.  If you try a different property name it won't work.

Comment 14 is right on the money.

Comment 16

[John J. Barton]

I found away around the need for this feature, plus I now thing that jsd.wrapValue is buggy: it needs to be able to give a jsdIValue from an xpconnect wrapped value. But that is over my head.

Comment 17

[John J. Barton]

I reset the firebug priority since my experience with the source-string comparison was unsatisfactory.

(In reply to comment #15)
> > And later when I look that the XPCWrappedNative property, it has the JS
> > object's new property. 
> 
> That's correct, because in that case the wrappedJSObject is being used the way
> it's supposed to be (to gain access to the underlying JSObject when dealing
> with a doubly-wrapped JSObject in JS).
> 
> > I guess this entirely by coincidence
> 
> Not at all.  It's working as designed.  The above ONLY works for the
> "wrappedJSObject" property.  If you try a different property name it won't
> work.
> 
> Comment 14 is right on the money.

So I guess the only way forward here is to intercept the addEventListener in the window with a function like
  function hack_addEventListener(type, handler, capture) 
  {
     handler.wrappedJSObject = handler;
     _real_addEventListener(type, handler, capture)
  }

Comment 18

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

So if there was something like wrapValue for C++ callers, then
nsIEventListenerInfo objects could find their jsval and use that
method to return jsdIValue.

I wouldn't expect that to be hard for anyone familiar with jsdI*
(which I'm not).

Comment 19

[John J. Barton]

(In reply to comment #18)
> So if there was something like wrapValue for C++ callers, then
> nsIEventListenerInfo objects could find their jsval and use that
> method to return jsdIValue.

But if wrapValue actually worked for xpconnect wrapped objects, then you, and all others that need this functionality, would not need to make such a call nor would we need to have your code include jsd headers.

Comment 20

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

Well, because of security reasons I don't want to expose nsIDOMListener in
nsIEventListenerInfo. Otherwise someone might actually call its handleEvent method.
Returning jsdIValue would emphasize that the value is for debugging.

Comment 21

[John J. Barton]

(In reply to comment #20)
> Well, because of security reasons I don't want to expose nsIDOMListener in
> nsIEventListenerInfo. Otherwise someone might actually call its handleEvent
> method. 

How can this be a security exposure? Are you saying that a web page from a.com could use nsIEventListenerInfo to get the handleEvent methods from page b.com? Why do we even allow a.com to read event listeners from b.com?

Comment 22

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

Calling the listener without right JSContext in stack might give web page
chrome privileges.
See links in https://bugzilla.mozilla.org/show_bug.cgi?id=448602#c83

Comment 23

[Boris Zbarsky [:bzbarsky]]

So at this point I'm really confused about what the goal is here.  Is it:

1)  To be able to reliably get the underlying JSObject of an XPCWrappedJS in the
    debugger?  That would help you if you have the nsIDOMEventListener in the
    debugger, but per comment 20 you don't.
2)  To be able to reliably get the underlying JS function of an event listener
    given its nsIEventListenerInfo?
3)  Something else?

This is why bugs are supposed to have sane summaries and initial comments, by the way.  This bug is becoming more and more useless with each passing day, with discussion going off in multiple unrelated directions.

Comment 24

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

If 2), would it be enough to get the JS function as jsdIValue?

Comment 25

[John J. Barton]

(In reply to comment #23)
> So at this point I'm really confused about what the goal is here.  

My goal is to obtain the jsdIScript object for the event listener reported by nsIEventListenerInfo.  It seems to me that this ought to live in jsd, not event listener code. It seems to me that the function ought to take any js interface reference and return either a reference that acts like a js object or null.

Is it:
> 
> 1)  To be able to reliably get the underlying JSObject of an XPCWrappedJS in
> the
>     debugger?  That would help you if you have the nsIDOMEventListener in the
>     debugger, but per comment 20 you don't.

I don't know what these are so its not part of my goal.

> 2)  To be able to reliably get the underlying JS function of an event listener
>     given its nsIEventListenerInfo?

Last week I would have said yes. Now I don't know what this means. So I'll stick to the goal above.

> 3)  Something else?

If my goal above is something else, then yes.

> 
> This is why bugs are supposed to have sane summaries and initial comments, by
> the way.  This bug is becoming more and more useless with each passing day,
> with discussion going off in multiple unrelated directions.

I can't write such a summary; I don't know which directions are related and which are not.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> My goal is to obtain the jsdIScript object for the event listener reported by
> nsIEventListenerInfo.

For which of the various JSObjects associated with that event listener?  There are at least 2.

> I don't know what these are so its not part of my goal.

An XPCWrappedJS is what makes some random JSObject look like an nsIDOMEventListener to C++ code.  If you then pass that XPCWrappedJS back out to Javascript, _another_ JSObject is created to reflect it.

Maybe I should ask another question.  What are you actually trying to do, on a high level, not on the level of jsdIValues and such?  That is, from a user perspective, what's supposed to be happening?

Comment 27

[John J. Barton]

(In reply to comment #26)
> > My goal is to obtain the jsdIScript object for the event listener reported by
> > nsIEventListenerInfo.
> 
> For which of the various JSObjects associated with that event listener?  There
> are at least 2.

As I said in the newsgroup posting cited above, if the web page issues 
    elt.addEventListener('click', foo, false);
I want 'foo'.

> 
> > I don't know what these are so its not part of my goal.
> 
> An XPCWrappedJS is what makes some random JSObject look like an
> nsIDOMEventListener to C++ code.  If you then pass that XPCWrappedJS back out
> to Javascript, _another_ JSObject is created to reflect it.
> 
> Maybe I should ask another question.  What are you actually trying to do, on a
> high level, not on the level of jsdIValues and such?  That is, from a user
> perspective, what's supposed to be happening?

If the web page issues 
    elt.addEventListener('click', foo, false);
and Firebug's Event panel shows an event handler bound to "elt", I want to be able to show the user "foo", allow them to set breakpoints, find all of the elements the handler is bound to, monitor and profile calls to foo, build a graph of how foo can be triggered, analyze why foo did not get called. (To be honest I don't understand why getting the handler is so hard; now I think we should have just hacked addEventListener and not bothered with the nsIEventListenerInfo).

Comment 28

[Boris Zbarsky [:bzbarsky]]

OK.  So you want a jsdIValue for the underlying JSObject of the XPCWrappedJS.

smaug, I think we can expose that on the nsIEventListenerInfo without too much trouble.  QI our nsIDOMEventListener to nsIXPConnectWrappedJS, get the underlying JSObject from it, get the debugger service, call wrapValue on it (adding a C++ API for it that takes a JSObject* directly instead of getting it from XPConnect).  Sound reasonable?

Comment 29

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

(In reply to comment #28)
> OK.  So you want a jsdIValue for the underlying JSObject of the XPCWrappedJS.
I'm just writing a patch for that.


> 
> smaug, I think we can expose that on the nsIEventListenerInfo without too much
> trouble. 
Yeah. I'm going to return nsISupports in the interface so that the code
compiles ok also without jsd. The implementation will return jsdIValue if
jsd is available. 

> QI our nsIDOMEventListener to nsIXPConnectWrappedJS, get the
> underlying JSObject from it, get the debugger service, call wrapValue on it
> (adding a C++ API for it that takes a JSObject* directly instead of getting it
> from XPConnect).  Sound reasonable?
Right, already writing this.

Comment 30

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

Attachment: patch

So something like this. This is on top of the
event listener enumeration patch.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Testing |if (jsval)| is nonsensical.  Same thing for the |v| check in the js event listener case.  Same thing for the !funval test.

Why do you even need these tests?

The js event listener case looks to me like it should probably root the jsval, to be safe...

Comment 32

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

(In reply to comment #31)
> Testing |if (jsval)| is nonsensical.  Same thing for the |v| check in the js
> event listener case.  Same thing for the !funval test.
Ah. I guess I need to read JS API.

> The js event listener case looks to me like it should probably root the jsval,
> to be safe...
Could be. Root it where, I mean in which method?
I'm not at all familiar with the somewhat awkward JS API.

Comment 33

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

Ok, shaver explained why if (jsval) doesn't make sense. Will remove.
About rooting, what could call GC?

Comment 34

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

Though, perhaps I need to add a JSAutoRequest

Comment 35

[Boris Zbarsky [:bzbarsky]]

> About rooting, what could call GC?

I have no idea as things stand, but do you want to depend on implementation details of getService and the debugger service?

I think you'd want to root in GetDebugObject right after declaring |jsval v|, and nsAutoGCRoot (the version that takes jsval*) should do the trick.  You probably want something like:

  jsval v = JSVAL_NULL;
  nsAutoGCRoot root(&v, &rv);
  NS_ENSURE_SUCCESS(rv);
  // do stuff with v here.

The nsScriptObjectHolder roots the object, and you'll assign to the jsval before that leaves scope, so the object will be rooted throughout.

I don't understand the request model well enough to tell you anything useful on that front.

Comment 36

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

Attachment: v2

Comment 37

[John J. Barton]

I was able to use your try-server build to extract a jsdIScript for the listener


/**
+   * If jsdIDebuggerService is active and the listener is implemented in JS,
+   * this returns an object which implements jsdIValue.
+   */
+  nsISupports getDebugObject();

May I quibble about the API? How about:
/**
   * If jsdIDebuggerService is active and the listener is implemented in JS,
   * return the listener as a jsdIValue. Otherwise null.
   */
  nsISupports getListenerAsIValue();

Comment 38

[Boris Zbarsky [:bzbarsky]]

You don't need the root in the nsIXPConnectWrappedJS case, since the nsIXPConnectWrappedJS guarantees it'll keep the object alive.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Also, why null-check the object in that case?  I _think_ OBJECT_TO_JSVAL is ok on a null JSObject pointer (but check on this), and the jsd code will happily wrap a null in a jsdIValue, right?

Then you can outdent a bunch of stuff.

Comment 40

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

I prefer getDebugObject() since it makes it more clear that the returned object
is for debugging. (which to me implies that the caller should be careful when
handling the return value, and also implies that it shouldn't be used in
"production" code.)
But will improve the comment.

(In reply to comment #39)
> I _think_ OBJECT_TO_JSVAL is ok
> on a null JSObject pointer (but check on this),
Yeah, seems like it is ok to use null there.

Comment 41

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

Attachment: v3

Comment 42

[Boris Zbarsky [:bzbarsky]]

Can you nix the else-after-return?  And why is the NS_SUCCEEDED(rv) check needed after GetJSVal?

Comment 43

[John J. Barton]

I still think that we would be better off with the original goal of this bug, a method of jsd that takes things with hidden JS objects and returns them in the variant type jsdIValue. This is a fundamental operation, not special to event listeners. Or is this the only place in the platform where a JS object is stored?

Comment 44

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

JS event listeners can be stored in 2 different ways. The normal
nsIXPConnectWrappedJS (which is used with .addEventListener) or
nsIJSEventListener (using with onfoo listeners). So whatever method
creates jsdIValue needs to handle both cases, not only the common
nsIXPConnectWrappedJS.

If there was a generic method to get jsdIValue from those, how would
the event listeners be exposed in nsIEventListenerInfo?

Comment 45

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

Attachment: removed 'else'

NS_SUCCEEDED is there for a reason; if we can't get the JS value, return null.

Comment 46

[Boris Zbarsky [:bzbarsky]]

But rv is known to be success on that line.  Did you mean to assign the return value of GetJSVal into rv?

John, the debugger service could interrogate the xpconnect stuff, possibly, but it can't depend on DOM implementation details, or any other module's, and hence can't do a generic "find the hidden jsobject" operation if someone other than xpconnect is hiding it.

Comment 47

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

Ugh, yes, that assignment is missing. I should just stop hacking this.

Comment 48

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

Attachment: + rv =

Won't update this until I get the enumeration patch reviewed and landed.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Feel free to ask me for r= on this patch anytime, btw.  ;)

Comment 50

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

Attachment: Generic GetJSVal which can be used in GetDebugObject and ToSource

Comment 51

[Boris Zbarsky [:bzbarsky]]

That generally looks fine, but why the void* argument?  Why not jsval*?

Comment 52

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

Because I don't know (well, I do know if I look at some header) what jsval is, and
I don't want to include JS headers in other headers.

class jsval; would probably work, but is kind of wrong.

Comment 53

[Boris Zbarsky [:bzbarsky]]

> I don't want to include JS headers in other headers.

We're talking about nsIJSEventListener.h, yes?  That already includes jsapi.h, which includes jspubtd.h, which is what declares jsval.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Er, sorry.  I was looking at nsJSEventListener.h.  One sec.

Comment 55

[Boris Zbarsky [:bzbarsky]]

I think you should just include jsapi.h in nsIJSEventListener.h, personally.  If not, document that this is supposed to be a jsval*, and up front in nsEventListenerInfo::GetJSVal do:

  jsval* vp = static_cast<jsval*>(aJSVal);

then use vp throughout.  I'd prefer the include, though.

Comment 56

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

Attachment: patch, include jsapi.h

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 406723 [details] [diff] [review]
patch, include jsapi.h

Looks great.  Ship it!

Comment 58

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

http://hg.mozilla.org/mozilla-central/rev/06cecc61ffeb

Comment 59

[Rob Campbell [:rc] (:robcee)]

any plans to check this into 1.9.2?

Comment 60

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

Is there any Firebug code available which uses this?
I'd guess without that it is pretty hard to convince drivers to take this to
1.9.2.

Comment 61

[John J. Barton]

(In reply to comment #60)
> Is there any Firebug code available which uses this?

eventbug uses it.

> I'd guess without that it is pretty hard to convince drivers to take this to
> 1.9.2.

Yes,I know. eventbug will be out on Monday Nov 3 at the latest and we will do what we can to get folks to use it.  I'll see if Honza can make a version sooner.

Comment 62

[Jan Honza Odvarko [:Honza] (always need-info? me)]

eventbug-0.1a2.xpi 
http://getfirebug.com/releases/eventbug/1.5/

Source:
http://code.google.com/p/fbug/source/browse/#svn/eventbug/branches/eventbug0.1

You need Firebug 1.5b1
http://getfirebug.com/releases/firebug/1.5X/

Honza

Comment 63

[Rob Campbell [:rc] (:robcee)]

so yeah, this is pretty-much ready to go. It'd be great to let 3.6 users have this.

Comment 64

[Rob Campbell [:rc] (:robcee)]

smaug, any idea how difficult this would be to land on 1.9.2? Any worries associated with it?

Comment 65

[Mike Beltzner [:beltzner, not reading bugmail]]

Probably want to get it on 1.9.2 ASAP, then.

Comment 66

[Rob Campbell [:rc] (:robcee)]

clarification, this patch requires the patch from bug 448602 to work.

I am writing this in a build that includes those two patches with the EventBug extension installed. Seems to be working, as I'm able to list the event listeners registered on the page and attached to DOM nodes.

Comment 67

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 406723 [details] [diff] [review]
patch, include jsapi.h

tested, working. applies with --fuzz=5.

Comment 68

[Boris Zbarsky [:bzbarsky]]

It's blocking1.9.2+.  You don't need approval to check it in on 1.9.2.

Comment 69

[Rob Campbell [:rc] (:robcee)]

yeah, I know. This is dependent on the patch in 448602 though, so we need approval there. I'm going to run this through the try servers and report back how that goes. Shouldn't be any problems, but it'll be good to get the extra verification on all platforms.

Comment 70

[Mike Beltzner [:beltzner, not reading bugmail]]

And that bug carries some risk, so punting this up to Shaver for a decision.

Comment 71

[John J. Barton]

For Firebug to use the feature implemented here we need bug 521010 to land on 1.9.2 for FF 3.6.

Comment 72

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 406723 [details] [diff] [review]
patch, include jsapi.h

a192=beltzner

Comment 73

[Rob Campbell [:rc] (:robcee)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/67dae79e2400