I'm told there's currently no way to examine the function that a bound function will actually call. That is, given this: var f = (function g() { "use strict"; m.call(arguments); }).bind(null, 1, 2, 3); f(); there's no way to know that the second line will invoke |g|, with |this === null|, or that it'll invoke it with the leading arguments |1, 2, 3|. We should be able to expose this; it apparently can come in handy when debugging, sometimes (see the URL, although the use case is admittedly esoteric).

It would be good to expose these as getters on Debugger.Object. The spec calls for similarly exposing a "proxyTarget" getter for similar purposes (which isn't yet implemented, however).

We have a use case for this in Firebug: in https://code.google.com/p/fbug/issues/detail?id=5440 we'll start displaying what event listeners are bound to a node selected in the HTML panel. The user can then click the function to jump to where it's defined in the source code (according to its Debugger.Script), see its name (fn.name, will likely change to Debugger.Script.displayName eventually) + parameter names (from fn.toString()), and sometimes hover it to see its source code. This breaks completely when users do: > node.addEventListener('something', function() { > // ... > }.bind(this)); :( Just getting access to the [[TargetFunction]], or having the Debugger.Object#script of fn.bind() be the same as that of fn, would be good enough for us. Arguments are useful but not critical. Any thoughts on how this can work after bug 1000780?

I don't think that adding this would be all that hard, really. With or without a self-hosted .bind function. We obviously have the information around, so all that's needed is exposing it to the debugger. Benvie's proposal for that sounds good to me, but I know too little about the debugging infrastructure to really usefully comment on that part.

This is also blocking us from displaying events in Firefox DevTools (bug 736078). Bound handlers produce the following listenerDebugObject: listenerDebugObject.callable: false listenerDebugObject.class: Object listenerDebugObject.name: undefined listenerDebugObject.displayName: undefined listenerDebugObject.parameterNames: undefined listenerDebugObject.script: undefined listenerDebugObject.environment: undefined listenerDebugObject.proxyHandler: undefined listenerDebugObject.global: [object Object] listenerDebugObject.class: Object Because listenerDebugObject.script is undefined we can't get any useful information. For handleEvent functions we use this: let desc = listenerDebugObject.proto.getOwnPropertyDescriptor("handleEvent"); if (desc) { script = desc.value.script; scriptSource = handler.listenerObject.handleEvent.toString(); } For bound handleEvent functions this results in script.text === "function() { [native code] }", which is again not useful.

STR: 1. Open browser_markupview_events.html (attached) 2. Open scratchpad. 3. Change the scratchpad environment to Browser. 4. Open scratchpad2.js (attached) 5. Click Run. In the dumped output you will see that: 1. listenerDebugObject.script === undefined for "Results for bound function" 2. Script source is function () { [native code] } for "Results for bound handleEvent"

I really don't have time to dedicate to this, but here is a quick patch. I'm going to have to leave it to jimb/ejpbruel to test and land. :(

It's possible that hooking this up may not totally solve the problem - there might be other kinds of callable objects (that is, function wrappers) that we need to know how to unwrap. Offhand, I don't know how to expose this general concept to the debugger programmatically in a way that'll be convenient to use. :-\

I can look into this.

With this patch on top of Jason's patch the debugger can list and break on bound event listeners. Bound (or not) event handlers are still an issue though, possibly because the platform API treats WebIDL and XPCOM callbacks differently. I'm still investigating.

Added some docs.

Here is a simplified test case with only 3 events that registers an event listener, an event handler and a bound event handler. With these 2 fixes applied, opening the events pane in the debugger (https://developer.mozilla.org/docs/Tools/Debugger#Events_pane) will show 2 nodes with [native code] listener, which is actually the debugger not getting any script information, and 1 node with identified script location information. While getting the event listeners, EventListenerInfo::GetJSVal takes a different path in the first case vs. the other two cases. What is interesting is that the JSObject we get from wrappedJS->GetJSObject() in the first case is [object Function], while the JSObject we get from jsHandler->GetTypedEventHandler().Ptr()->Callable() in the other two cases is [object Object]. DebuggerObject_makeDebuggeeValue seems to handle them in the same way further on. So my guess is that JSEventHandler callbacks come as wrappers of some sort that either the event listener service or the debugger would have to unwrap at some point. Paging smaug who might know what is going on.

I don't see any event handler in that test. There is an event listener which is passed as a function, then there is an object which has handleEvent, and then 3rd one is similar - handleEvent just happens to be in the object, and not in the prototype. An EventHandler is an onfoo thing.

Sorry for the bogus terminology, that's exactly what I meant. Any idea why the objects with handleEvent functions appear differently? Does jsHandler->GetTypedEventHandler().Ptr()->Callable() return the entire object and not handleEvent?

jsHandler->GetTypedEventHandler().Ptr()->Callable() shouldn't even get called for the test case.

D'oh! I must have been confused at some point, thanks for clearing that up. It looks like the listener we get in that case is the object with the handleEvent method, not the function itself. I'll attach an updated patch shortly.

This patch makes all the cases in both Mike's and my test pages work as expected. All events are listed with the right script information and the debugger can break on the event. I'll add some tests and submit for review.

Comment on attachment 8439839 [details] [diff] [review] bug-746622-debugger-bound-function-v2.patch The platform part seems ready for review. I'll take care of any updates requested.

Now with tests. I took the liberty of renaming a couple of other test files to reduce confusion, so apologies in advance if Splinter makes this harder than it has to be.

Comment on attachment 8440773 [details] [diff] [review] Properly handle bound functions in event listeners and objects with 'handleEvent' methods v2 Review of attachment 8440773 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/script.js @@ +1260,5 @@ > l.type = handler.type; > let listener = handler.listenerObject; > + let listenerDO = this.globalDebugObject.makeDebuggeeValue(listener); > + // If the listener is an object with a 'handleEvent' method, use that. > + if (listenerDO.class == "Object") { This would miss XBL handleEvent methods. Use this instead: if (listenerDO.class === "Object" || listenerDO.class === "XULElement") { @@ +1851,5 @@ > listenerForm.type = handler.type; > listenerForm.capturing = handler.capturing; > listenerForm.allowsUntrusted = handler.allowsUntrusted; > listenerForm.inSystemEventGroup = handler.inSystemEventGroup; > listenerForm.isEventHandler = !!node["on" + listenerForm.type]; This fails for XUL elements containing oncommand="yadayada()" you will need to use: listenerForm.isEventHandler = !!node.hasAttribute("on" + listenerForm.type);

Debugging events in chrome code is not supported yet (bug 925784), but we should definitely fix any problematic cases we can spot. Asking Mike for an additional formal review particularly on those parts, since he has been looking at the patch already.

Given: function f() {} bf = f.bind(1, 2); bbf = bf.bind(3, 4); , does bbf's boundTargetFunction refer to f or to bf? In the former case it's worth documenting/testing, in the latter the 'if' in if (listenerDO.isBoundFunction) { listenerDO = listenerDO.boundTargetFunction; } ought to be a 'while'.

Good catch! Tweaked the test to verify the correct behavior in this case.

Comment on attachment 8442695 [details] [diff] [review] Properly handle bound functions in event listeners and objects with 'handleEvent' methods v4 Review of attachment 8442695 [details] [diff] [review]: ----------------------------------------------------------------- Not sure if related, but I'm now consistently able to reproduce bug 942899 with this patch applied. STR: 1. Open fresh instance of firefox 2. Go to cnn.com and wait for it to load 3. Open the debugger 4. Open the events listeners pane DBG-SERVER: Received packet 169: { "from": "conn0.pausedobj150", "error": "noScript", "message": "conn0.pausedobj150 has no Debugger.Script" } The messages appear in the webconsole as "Error getting function definition site: conn0.pausedobj150 has no Debugger.Script". Holding r+ for now until the above issue is addressed. Without the patch applied, bug 942899 is a bit hard to reproduce. Now it's consistent. I wouldn't necessarily hold off this landing because of bug 942899, but I'd be happy if it got investigated a bit. ::: browser/devtools/debugger/test/browser_dbg_event-listeners-02.js @@ +23,5 @@ > + "Root actor should identify itself as a browser."); > + > + addTab(TAB_URL) > + .then(() => attachThreadActorForUrl(gClient, TAB_URL)) > + .then(pauseDebuggee) Nit: slight indentation inconsistency here.

It's unrelated, you are getting this error now because without this patch it used to fail earlier. It appears that my patch from bug 942899 got accidentally undone in some refactoring. I can fix it either here or in that bug.

...and since bzexport is still eating my comments, I'll repeat it here: This fixes the regression from bug 991797, by making the failure to fetch a definition site non-fatal again. I discovered at least another case of a similar regression from that bug that I will file a followup for. Writing a test for catching that sort of bug is still pending on the investigation in bug 942899.

Comment on attachment 8450073 [details] [diff] [review] Properly handle bound functions in event listeners and objects with 'handleEvent' methods v5 Review of attachment 8450073 [details] [diff] [review]: ----------------------------------------------------------------- I distinctly remember this same error happening long (!) before bug 991797 landed, with the exact same effect (Events tab empty, debugger staying paused), so I'm not entirely convinced it's a regression from there. However, this patch does seem to fix things, so I'm happy. ::: browser/devtools/debugger/debugger-controller.js @@ +1746,5 @@ > if (aResponse.error) { > + // Don't make this error fatal, because it would break the entire events > + // pane. > + const msg = "Error getting function definition site: " + > + aResponse.message; Uber nit: this is just *my* coding style: Since none of this file strictly adheres to the 80 char column rule, I'd say the code would be a bit more readable without the line breaks before 'pane' in the comment and 'aResponse.message' in the assignment. But that's just me :)

Thanks, I've made the suggested changes locally.

Jim - ping for review, this is blocking visual events so if review is possible before uplift it would allos us to ship that.

Comment on attachment 8439839 [details] [diff] [review] bug-746622-debugger-bound-function-v2.patch Review of attachment 8439839 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/doc/Debugger/Debugger.Object.md @@ +161,5 @@ > + function that was bound to a particular `this` object. If the referent > + is not a bound function, this is `undefined`. > + > +`boundThis` > +: If the referent is a bound function, this is the `this` object it was Note that `this` can be any value, not just objects, in non-strict-mode. Even in strict-mode, it could be `null`. @@ +165,5 @@ > +: If the referent is a bound function, this is the `this` object it was > + bound to. If the referent is not a bound function, this is `undefined`. > + > +`boundArguments` > +: If the referent is a bound function, this is the `arguments` object it Code looks like it creates an array, not the actual `arguments` object. Are the values in the array D.Objects?

(In reply to Nick Fitzgerald [:fitzgen] from comment #35) > Comment on attachment 8439839 [details] [diff] [review] > bug-746622-debugger-bound-function-v2.patch > > Review of attachment 8439839 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/doc/Debugger/Debugger.Object.md > @@ +161,5 @@ > > + function that was bound to a particular `this` object. If the referent > > + is not a bound function, this is `undefined`. > > + > > +`boundThis` > > +: If the referent is a bound function, this is the `this` object it was > > Note that `this` can be any value, not just objects, in non-strict-mode. > Even in strict-mode, it could be `null`. Nick's point that `this` may not be an object is correct, but for what it's worth the comments about strict mode aren't. The docs should probably just refer to "the 'this' value" Non-strict functions call ToObject on the given this value; strict functions take whatever 'this' they're passed. js> function f() { return this; } typeof f.call(42); "object" js> "use strict"; function f() { return this; } typeof f.call(42); "number" js> This is ES5.1 10.4.3. > @@ +165,5 @@ > > +: If the referent is a bound function, this is the `this` object it was > > + bound to. If the referent is not a bound function, this is `undefined`. > > + > > +`boundArguments` > > +: If the referent is a bound function, this is the `arguments` object it > > Code looks like it creates an array, not the actual `arguments` object. Are > the values in the array D.Objects? Yeah, it's an array of debuggee values (which are D.Os or primitives).

Comment on attachment 8439839 [details] [diff] [review] bug-746622-debugger-bound-function-v2.patch Review of attachment 8439839 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, with some doc and test fixes. We need tests that apply D.O.p.isBoundFunction and all the other accessors to non-bound functions: - an ordinary non-bound function; - a native function; - and an object that isn't a function at all. We should test and document the behavior when inspecting a bound function that was bound again: js> function f() { return this; } (f.bind(42).bind(43)()) (new Number(42)) js> The behavior of the running code is well-specified by ES, but what does boundTargetFunction return? Are the 'this' and argument list just updated, with the target remaining as the non-bound function? Or is the target of the second bound function the first bound function, with their 'this' and argument arrays left as given to the 'bind' calls? ::: js/src/doc/Debugger/Debugger.Object.md @@ +167,5 @@ > + > +`boundArguments` > +: If the referent is a bound function, this is the `arguments` object it > + was bound to. If the referent is not a bound function, this is > + `undefined`. As Nick pointed out: this is an array (in the Debugger object's compartment) of debuggee values, not an arguments object. ::: js/src/jit-test/tests/debug/Object-boundTargetFunction.js @@ +9,5 @@ > +assertEq(pushw.boundThis, arrw); > +assertEq(pushw.boundArguments.length, 0); > + > +var arr2 = gw.evalInGlobal("var arr2 = []; arr2").return; > +pushw.call(arr2, "tuesday"); Let's assert that this completed successfully. Unlike the g.eval calls, errors won't propagate out of a D.O.p.call call.

Addressed review comments.

Comment on attachment 8459184 [details] [diff] [review] bug-746622-debugger-bound-function-v3.patch Carrying over r+.

https://hg.mozilla.org/integration/fx-team/rev/0a952f186c98 https://hg.mozilla.org/integration/fx-team/rev/489816b74790

https://hg.mozilla.org/mozilla-central/rev/0a952f186c98 https://hg.mozilla.org/mozilla-central/rev/489816b74790