Closed
Bug 506961
Opened 15 years ago
Closed 15 years ago
Add a method to get jsdIValue from JS implemented event listeners
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta2-fixed |
People
(Reporter: johnjbarton, Assigned: smaug)
References
Details
(Whiteboard: [firebug-p1])
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
beltzner
:
approval1.9.2+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 2•15 years ago
|
||
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.
OS: Linux → All
Hardware: x86 → All
Summary: Imp → Implement jsd.convertToIValue
Whiteboard: [firebug-p1]
Version: 1.9.1 Branch → Trunk
![]() |
||
Updated•15 years ago
|
Component: General → JavaScript Debugging APIs
QA Contact: general → jsd
Reporter | ||
Comment 3•15 years ago
|
||
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.
Blocks: 449452
Reporter | ||
Comment 5•15 years ago
|
||
(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
Reporter | ||
Comment 6•15 years ago
|
||
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?
Reporter | ||
Comment 7•15 years ago
|
||
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
Reporter | ||
Comment 10•15 years ago
|
||
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).
Reporter | ||
Comment 11•15 years ago
|
||
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•15 years ago
|
||
> 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.
Reporter | ||
Comment 13•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
> 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.
Reporter | ||
Comment 16•15 years ago
|
||
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.
Whiteboard: [firebug-p1] → [firebug-p3]
Reporter | ||
Comment 17•15 years ago
|
||
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) }
Whiteboard: [firebug-p3] → [firebug-p1]
Assignee | ||
Comment 18•15 years ago
|
||
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).
Reporter | ||
Comment 19•15 years ago
|
||
(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.
Assignee | ||
Comment 20•15 years ago
|
||
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.
Reporter | ||
Comment 21•15 years ago
|
||
(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?
Assignee | ||
Comment 22•15 years ago
|
||
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•15 years ago
|
||
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.
Reporter | ||
Comment 25•15 years ago
|
||
(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•15 years ago
|
||
> 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?
Reporter | ||
Comment 27•15 years ago
|
||
(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•15 years ago
|
||
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?
Assignee | ||
Comment 29•15 years ago
|
||
(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.
Assignee | ||
Comment 30•15 years ago
|
||
So something like this. This is on top of the event listener enumeration patch.
Assignee: nobody → Olli.Pettay
![]() |
||
Comment 31•15 years ago
|
||
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...
Assignee | ||
Comment 32•15 years ago
|
||
(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.
Assignee | ||
Comment 33•15 years ago
|
||
Ok, shaver explained why if (jsval) doesn't make sense. Will remove. About rooting, what could call GC?
![]() |
||
Comment 35•15 years ago
|
||
> 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.
Reporter | ||
Comment 37•15 years ago
|
||
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•15 years ago
|
||
You don't need the root in the nsIXPConnectWrappedJS case, since the nsIXPConnectWrappedJS guarantees it'll keep the object alive.
![]() |
||
Comment 39•15 years ago
|
||
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.
Assignee | ||
Comment 40•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Depends on: 448602
Summary: Implement jsd.convertToIValue → Add a method to get jsdIValue from JS implemented event listeners
![]() |
||
Comment 42•15 years ago
|
||
Can you nix the else-after-return? And why is the NS_SUCCEEDED(rv) check needed after GetJSVal?
Reporter | ||
Comment 43•15 years ago
|
||
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?
Assignee | ||
Comment 44•15 years ago
|
||
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?
Assignee | ||
Comment 45•15 years ago
|
||
NS_SUCCEEDED is there for a reason; if we can't get the JS value, return null.
Attachment #405439 -
Attachment is obsolete: true
![]() |
||
Comment 46•15 years ago
|
||
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.
Assignee | ||
Comment 47•15 years ago
|
||
Ugh, yes, that assignment is missing. I should just stop hacking this.
Assignee | ||
Comment 48•15 years ago
|
||
Won't update this until I get the enumeration patch reviewed and landed.
Attachment #405486 -
Attachment is obsolete: true
Assignee | ||
Comment 50•15 years ago
|
||
Attachment #405487 -
Attachment is obsolete: true
Attachment #406663 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 52•15 years ago
|
||
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•15 years ago
|
||
> 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 55•15 years ago
|
||
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.
Assignee | ||
Comment 56•15 years ago
|
||
Attachment #406663 -
Attachment is obsolete: true
Attachment #406723 -
Flags: review?(bzbarsky)
Attachment #406663 -
Flags: review?(bzbarsky)
![]() |
||
Comment 57•15 years ago
|
||
Comment on attachment 406723 [details] [diff] [review] patch, include jsapi.h Looks great. Ship it!
Attachment #406723 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 58•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/06cecc61ffeb
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 60•15 years ago
|
||
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.
Reporter | ||
Comment 61•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
so yeah, this is pretty-much ready to go. It'd be great to let 3.6 users have this.
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Comment 64•15 years ago
|
||
smaug, any idea how difficult this would be to land on 1.9.2? Any worries associated with it?
Whiteboard: [firebug-p1] → [firebug-p1][firebug-blocks]
Comment 65•15 years ago
|
||
Probably want to get it on 1.9.2 ASAP, then.
Flags: blocking1.9.2? → blocking1.9.2+
Comment 66•15 years ago
|
||
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•15 years ago
|
||
Comment on attachment 406723 [details] [diff] [review] patch, include jsapi.h tested, working. applies with --fuzz=5.
Attachment #406723 -
Flags: approval1.9.2?
Comment 69•15 years ago
|
||
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•15 years ago
|
||
And that bug carries some risk, so punting this up to Shaver for a decision.
Flags: blocking1.9.2+ → blocking1.9.2?
Reporter | ||
Comment 71•15 years ago
|
||
For Firebug to use the feature implemented here we need bug 521010 to land on 1.9.2 for FF 3.6.
Updated•15 years ago
|
Attachment #406723 -
Flags: approval1.9.2? → approval1.9.2+
Comment 72•15 years ago
|
||
Comment on attachment 406723 [details] [diff] [review] patch, include jsapi.h a192=beltzner
Comment 73•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/67dae79e2400
Whiteboard: [firebug-p1][firebug-blocks] → [firebug-p1]
Updated•15 years ago
|
status1.9.2:
--- → beta2-fixed
Updated•15 years ago
|
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2+
Updated•13 years ago
|
Component: JavaScript Debugging/Profiling APIs → JavaScript Engine
You need to log in
before you can comment on or make changes to this bug.
Description
•