For bug 800857.

Adding this on my list, as discussed with Rob.

(In reply to Panos Astithas [:past] from comment #1) > Adding this on my list, as discussed with Rob. Your hero list is getting pretty long.

This is the prototype that I've been working on to implement a new "eventListeners" protocol request to retrieve the list of event listeners on the page and display them as sources in a separate group. The backend part is almost ready, modulo the unique selector helper copy & paste job. I probably need to create a shared jsm in toolkit/devtools/shared and make the style inspector code use that as well. The frontend bits are quite hacky, just to see how it works. Adding the listeners in the same container as sources doesn't work, unless we allow duplicates, which comes with its own issues. I think it will need its own container below the sources. This should probably be done in bug 800857 anyway. A problem with this approach is what do we select when a listener breakpoint is hit: the listener source or the script source? I think it's better to always focus the script source, since it contains more context that the user may find useful (f.e. closures). Another issue is that we are currently presenting sources as they were sent by the web server, and this new widget would want to present parts of those sources instead. We might be able to overcome this by adding startLine & lineCount (and soon column info) back to the listener's source form, and using them to highlight that region in the editor. Might require some source editor work though.

It's not a separate widget in this prototype, but hopefully you get the idea. Clicking on the listeners displays the source script they came from, but does not focus the right line, since source forms no longer have the necessary info.

Another approach followed by Chrome Devtools is to present a list of all the available event types and let the user select the ones to break on. This has the benefit of not having both a source and a listener source to show when hitting that breakpoint. The way this is implemented in the second patch is by looking for any installed event listeners for the events that the user has selected. Note that a pause does not happen each time gecko dispatches an event, only when there is a registered listener, otherwise the stack would have been empty, with no useful information to inspect. Apply on top of the first patch. I didn't implement the UI for this one, but I have hardwired the "click" and "keyup" events in the client. A good page to test against is: http://htmlpad.org/debugger/

I have written a few words about the new packets that these two patches add. Not a formal documentation, but should hopefully clarify some implementation details.

Rebased.

Rebased.

neat. Things I want: 1) ability to collapse headings in that sidebar. (probably should have a bug on it anyway) 2) popup menu on mouse events to create a break point. Looks like a decent first-attempt at showing these. Nice!

(In reply to Rob Campbell [:rc] (:robcee) from comment #9) > neat. > > Things I want: > > 1) ability to collapse headings in that sidebar. (probably should have a bug > on it anyway) > 2) popup menu on mouse events to create a break point. > I'm gonna work on those in bug 800857.

Combined the two approaches and removed the UI stuff. Still needs work.

Lots of cleanups and fixes, along with a thorough test for the new eventListeners request. Still need to finish the test for the pauseOnDOMEvents feature.

This is now ready for review. I've left a workaround in the code for a gecko crash that I'll file a followup for, but this should be OK for a first cut. All devtools tests pass locally. I'll ask Jim's review on the protocol spec changes.

Comment on attachment 742422 [details] Protocol doc I'm working on the real protocol documentation, but does this look OK Jim?

I've sent a pull request on GitHub, but this is probably easier for review.

Do you think the 'eventListeners' request would work better on an inspector actor if one existed?

I think it would look equally weird on an inspector actor. _getAllEventListeners looks like it would fit better there, but onEventListeners needs to deal with script sources, so that part would look out of place. Moreover _getAllEventListeners is required by _allEventsListener, which is more intertwined with the pause/resume/breakpoint parts of the thread actor.

*changes relationship status between eventListeners and thread actor to "it's complicated"*

Comment on attachment 742422 [details] Protocol doc should we be using jsbin for protocol examples?

Comment on attachment 752780 [details] [diff] [review] Patch v5 Review of attachment 752780 [details] [diff] [review]: ----------------------------------------------------------------- this looks great to me. Awaiting protocol feedback before continuing review. Please re-request when ready. ::: toolkit/devtools/server/actors/script.js @@ +134,3 @@ > */ > _addDebuggees: function TA__addDebuggees(aWindow) { > + let globalDO = this.addDebuggee(aWindow); global Debug Object, I presume?

Comment on attachment 752816 [details] [diff] [review] Protocol spec changes Review of attachment 752816 [details] [diff] [review]: ----------------------------------------------------------------- Thank you so much for documenting this! I think it's really valuable to have the docs reflect reality. A few comments: ::: protocol @@ +1114,5 @@ > This closes communications with <i>breakpointActor</i>. > > += Event Listeners = > + > +A client can request a list of all the event listeners and event handlers (see [https://developer.mozilla.org/docs/Web/Guide/DOM/Events/Event_handlers#Definitions DOM Event Handlers] for definitions of the two terms) attached to the page. This is done with a request of the form: I think it's clearer to write: "To request a list of ..., the client sends a request of the form:" Can one send such a request to a non-paused thread? @@ +1118,5 @@ > +A client can request a list of all the event listeners and event handlers (see [https://developer.mozilla.org/docs/Web/Guide/DOM/Events/Event_handlers#Definitions DOM Event Handlers] for definitions of the two terms) attached to the page. This is done with a request of the form: > + > + { "to":<i>thread</i>, "type":"eventListeners" } > + > +The thread will reply with a response of the form: "The thread replies with..." @@ +1130,5 @@ > + "capturing":<i>capturing</i>, > + "allowsUntrusted":<i>allowsUntrusted</i>, > + "inSystemEventGroup":<i>inSystemEventGroup</i>, > + "isEventHandler":<i>isEventHandler</i>, > + "sources":[<i>sourceForm1</i>, ..., <i>sourceFormN</i>] } Could we include object actors for the node and the handler function? Why an array of sources, not just one? (It sure would be nice if we could automatically generate CSS selectors for any arbitrary D.O, assuming they refer to DOM notes that are in a document; we could include the selector in the printed form of the object. I don't know if that's possible.) @@ +1132,5 @@ > + "inSystemEventGroup":<i>inSystemEventGroup</i>, > + "isEventHandler":<i>isEventHandler</i>, > + "sources":[<i>sourceForm1</i>, ..., <i>sourceFormN</i>] } > + > +Each <i>node-identifier</i> is a unique CSS selector of the DOM element on which the event handler is attached, or "window". <i>event-type</i> is the type of the DOM event as specified in the DOM specification. <i>capturing</i> is a boolean flag indicating whether the event listener is in capture mode. <i>allowsUntrusted</i> is a boolean flag that indicates whether the listener allows untrusted events. <i>inSystemEventGroup</i> is a boolean flag that indicates whether or not the event listener is in the system event group. <i>isEventHandler</i> is a boolean flag indicating whether it is an event handler or an event listener (see [https://developer.mozilla.org/docs/Web/Guide/DOM/Events/Event_handlers#Definitions DOM Event Handlers] for definitions of the two terms). <i>sourceForms</i> are described in [[#Loading_Script_Sources|Loading Script Sources]]. This is a long list; could we use a <ul> or <dl>? What is an "untrusted event"? What is a "system event group"? Could we include links to explanations of those terms? This doesn't say what the sourceForms actually mean. Are they the location at which the handler was established (the write to the attribute)? The code of the handler? How do HTML attribute handlers appear? What about assignments to WebIDL properties? @@ +1134,5 @@ > + "sources":[<i>sourceForm1</i>, ..., <i>sourceFormN</i>] } > + > +Each <i>node-identifier</i> is a unique CSS selector of the DOM element on which the event handler is attached, or "window". <i>event-type</i> is the type of the DOM event as specified in the DOM specification. <i>capturing</i> is a boolean flag indicating whether the event listener is in capture mode. <i>allowsUntrusted</i> is a boolean flag that indicates whether the listener allows untrusted events. <i>inSystemEventGroup</i> is a boolean flag that indicates whether or not the event listener is in the system event group. <i>isEventHandler</i> is a boolean flag indicating whether it is an event handler or an event listener (see [https://developer.mozilla.org/docs/Web/Guide/DOM/Events/Event_handlers#Definitions DOM Event Handlers] for definitions of the two terms). <i>sourceForms</i> are described in [[#Loading_Script_Sources|Loading Script Sources]]. > + > +The client can request that execution pauses on a DOM event, by sending a request of the form: Similarly: "To request that execution pause..., the client sends [or "may send"?] a request of the form:" @@ +1136,5 @@ > +Each <i>node-identifier</i> is a unique CSS selector of the DOM element on which the event handler is attached, or "window". <i>event-type</i> is the type of the DOM event as specified in the DOM specification. <i>capturing</i> is a boolean flag indicating whether the event listener is in capture mode. <i>allowsUntrusted</i> is a boolean flag that indicates whether the listener allows untrusted events. <i>inSystemEventGroup</i> is a boolean flag that indicates whether or not the event listener is in the system event group. <i>isEventHandler</i> is a boolean flag indicating whether it is an event handler or an event listener (see [https://developer.mozilla.org/docs/Web/Guide/DOM/Events/Event_handlers#Definitions DOM Event Handlers] for definitions of the two terms). <i>sourceForms</i> are described in [[#Loading_Script_Sources|Loading Script Sources]]. > + > +The client can request that execution pauses on a DOM event, by sending a request of the form: > + > + { "to":<i>thread</i>, "type":"resume", "pauseOnDOMEvents": [<i>event-type</i>, ... ] } This can only be sent while <i>thread</i> is paused, right? Can't this be combined with resumeLimit and pauseOnExceptions? If so, this needs to be documented in the == Resuming a Thread == section, in a way that makes it clear what it can be combined with. @@ +1138,5 @@ > +The client can request that execution pauses on a DOM event, by sending a request of the form: > + > + { "to":<i>thread</i>, "type":"resume", "pauseOnDOMEvents": [<i>event-type</i>, ... ] } > + > +The <tt>pauseOnDOMEvents</tt> property contains an array of the types of DOM events that should pause execution. Execution will pause whenever an event of the specified type is fired and a listener has been defined for it, otherwise the stack on pause would have been empty, which wouldn't provide a useful inspection ability to the user. Unless "fired" has a specific technical meaning, this doesn't seem to actually say exactly when the pause occurs. Would this be any better? "Execution pauses immediately after the frame for the call to the listener has been pushed, and before the listener has begun execution." I think it reads better to use the present tense: "Execution pauses" instead of the future tense: "Execution will pause" in almost all descriptions of this sort. @@ +1142,5 @@ > +The <tt>pauseOnDOMEvents</tt> property contains an array of the types of DOM events that should pause execution. Execution will pause whenever an event of the specified type is fired and a listener has been defined for it, otherwise the stack on pause would have been empty, which wouldn't provide a useful inspection ability to the user. > + > +A request for pause on all kinds of events can be made using the "*" wildcard event type: > + > + { "to":<i>thread</i>, "type":"resume", "pauseOnDOMEvents": [ "*" ] } Since "*" doesn't meaningfully combine with other events (right?), could the "*" appear instead of the array? "pauseOnDOMEvents": "*" @@ +1144,5 @@ > +A request for pause on all kinds of events can be made using the "*" wildcard event type: > + > + { "to":<i>thread</i>, "type":"resume", "pauseOnDOMEvents": [ "*" ] } > + > +In order to change the types of events that should pause execution, a new array of event types should be sent to the server. Any events not present in the new list will no longer trigger pauses. Consequently, sending an empty array or simply omitting the <tt>pauseOnDOMEvents</tt> property disables pausing on DOM events. Does this mean that a "pauseOnDOMEvents" applies only until the next pause? It would be nice to spell that out. @@ +1146,5 @@ > + { "to":<i>thread</i>, "type":"resume", "pauseOnDOMEvents": [ "*" ] } > + > +In order to change the types of events that should pause execution, a new array of event types should be sent to the server. Any events not present in the new list will no longer trigger pauses. Consequently, sending an empty array or simply omitting the <tt>pauseOnDOMEvents</tt> property disables pausing on DOM events. > + > +Execution pauses appear as regular breakpoint pauses, with the only difference being that the client is not required to keep track of these breakpoint actors, or deal with them in any way. They can be considered as <i>hidden breakpoints</i> from the point of view of a debugger frontend. I like the statelessness. I hope we don't end up wasting time setting and re-setting the same event watchpoints every time we resume. What <i>reason</i> value appears in the "paused" packet?

Comment on attachment 742422 [details] Protocol doc I apologize for having taken so long to get back to you on this. I did end up suggesting some functional changes, which is kind of rude having not been prompt with feedback when the patch was being written... :(

(In reply to Rob Campbell [:rc] (:robcee) from comment #20) > ::: toolkit/devtools/server/actors/script.js > @@ +134,3 @@ > > */ > > _addDebuggees: function TA__addDebuggees(aWindow) { > > + let globalDO = this.addDebuggee(aWindow); > > global Debug Object, I presume? That's right. (In reply to Jim Blandy :jimb from comment #21) > ::: protocol > @@ +1114,5 @@ > > This closes communications with <i>breakpointActor</i>. > > > > += Event Listeners = > > + > > +A client can request a list of all the event listeners and event handlers (see [https://developer.mozilla.org/docs/Web/Guide/DOM/Events/Event_handlers#Definitions DOM Event Handlers] for definitions of the two terms) attached to the page. This is done with a request of the form: > > I think it's clearer to write: > > "To request a list of ..., the client sends a request of the form:" > > Can one send such a request to a non-paused thread? I think we need to allow that in order to let the inspector display the same information. > @@ +1130,5 @@ > > + "capturing":<i>capturing</i>, > > + "allowsUntrusted":<i>allowsUntrusted</i>, > > + "inSystemEventGroup":<i>inSystemEventGroup</i>, > > + "isEventHandler":<i>isEventHandler</i>, > > + "sources":[<i>sourceForm1</i>, ..., <i>sourceFormN</i>] } > > Could we include object actors for the node and the handler function? > > Why an array of sources, not just one? Source maps can associate a source in one language with more than one in another language, right? > (It sure would be nice if we could automatically generate CSS selectors for > any arbitrary D.O, assuming they refer to DOM notes that are in a document; > we could include the selector in the printed form of the object. I don't > know if that's possible.) I think it's doable and it would probably be a good idea to use that form in the "summary" property or request I suggested in bug 753332. > @@ +1142,5 @@ > > +The <tt>pauseOnDOMEvents</tt> property contains an array of the types of DOM events that should pause execution. Execution will pause whenever an event of the specified type is fired and a listener has been defined for it, otherwise the stack on pause would have been empty, which wouldn't provide a useful inspection ability to the user. > > + > > +A request for pause on all kinds of events can be made using the "*" wildcard event type: > > + > > + { "to":<i>thread</i>, "type":"resume", "pauseOnDOMEvents": [ "*" ] } > > Since "*" doesn't meaningfully combine with other events (right?), could the > "*" appear instead of the array? > > "pauseOnDOMEvents": "*" In the same line of thinking, should pausing on a single event use a string instead of an array? Are you OK with the type variance in pauseOnDOMEvents? > @@ +1146,5 @@ > > + { "to":<i>thread</i>, "type":"resume", "pauseOnDOMEvents": [ "*" ] } > > + > > +In order to change the types of events that should pause execution, a new array of event types should be sent to the server. Any events not present in the new list will no longer trigger pauses. Consequently, sending an empty array or simply omitting the <tt>pauseOnDOMEvents</tt> property disables pausing on DOM events. > > + > > +Execution pauses appear as regular breakpoint pauses, with the only difference being that the client is not required to keep track of these breakpoint actors, or deal with them in any way. They can be considered as <i>hidden breakpoints</i> from the point of view of a debugger frontend. > > I like the statelessness. I hope we don't end up wasting time setting and > re-setting the same event watchpoints every time we resume. > > What <i>reason</i> value appears in the "paused" packet? That's what I'm doing right now: clearing everything on pause and setting it again on resume. How can we have statelessness otherwise? The reason is { type: "breakpoint", ... } for simplicity. Do you think it would be better to hide that from the client using a different type and concealing the breakpoint actors?

(In reply to Panos Astithas [:past] from comment #23) > (In reply to Jim Blandy :jimb from comment #21) > > Can one send such a request to a non-paused thread? > > I think we need to allow that in order to let the inspector display the same > information. Okay. It would be good to explicitly, if briefly, mention that. > > @@ +1130,5 @@ > > > + "capturing":<i>capturing</i>, > > > + "allowsUntrusted":<i>allowsUntrusted</i>, > > > + "inSystemEventGroup":<i>inSystemEventGroup</i>, > > > + "isEventHandler":<i>isEventHandler</i>, > > > + "sources":[<i>sourceForm1</i>, ..., <i>sourceFormN</i>] } > > > > Could we include object actors for the node and the handler function? > > > > Why an array of sources, not just one? > > Source maps can associate a source in one language with more than one in > another language, right? That's true, but handlers are defined at a specific place; they don't occupy entire files. It seems to me that instead of 'sources' we should have a "source location" form, as described in == Source Locations ==. Is that possible? Perhaps the best thing would be for the protocol to send a function grip, and expand function grips to include the source location of the function's definition. > > (It sure would be nice if we could automatically generate CSS selectors for > > any arbitrary D.O, assuming they refer to DOM notes that are in a document; > > we could include the selector in the printed form of the object. I don't > > know if that's possible.) > > I think it's doable and it would probably be a good idea to use that form in > the "summary" property or request I suggested in bug 753332. Sure! > > > +A request for pause on all kinds of events can be made using the "*" wildcard event type: > > > + > > > + { "to":<i>thread</i>, "type":"resume", "pauseOnDOMEvents": [ "*" ] } > > > > Since "*" doesn't meaningfully combine with other events (right?), could the > > "*" appear instead of the array? > > > > "pauseOnDOMEvents": "*" > > In the same line of thinking, should pausing on a single event use a string > instead of an array? Are you OK with the type variance in pauseOnDOMEvents? This suggestion isn't critical at all; what you described originally is perfectly okay. My thought was just that, if I were describing the functionality here to someone, I would say that there are two alternatives: - Break on all DOM events. - Break on any one of a specific set of DOM events. And then the type variance in the JSON form should match the type variance in the explanation: "all DOM events" is "*"; a set of specific DOM events is an array of type names. But of course, there are many effectively equivalent ways to describe the functionality; and a protocol matching each of them. It's hard to say which is best. The protocol you specified would match a description like: 'You can specify a set of DOM event types to break on, as an array of type names. The pseudo-type name "*" denotes any type of DOM event.' That's perfectly okay; it does mean that you can have meaningless combinations like ["*", "MouseClick"], but that's just redundant, not ambiguous. In my suggestion, one could say ["MouseClick", "MouseClick"], and that's just as redundant. > > I like the statelessness. I hope we don't end up wasting time setting and > > re-setting the same event watchpoints every time we resume. > > > > What <i>reason</i> value appears in the "paused" packet? > > That's what I'm doing right now: clearing everything on pause and setting it > again on resume. How can we have statelessness otherwise? I don't see any other way. > The reason is { type: "breakpoint", ... } for simplicity. Do you think it > would be better to hide that from the client using a different type and > concealing the breakpoint actors? Yeah, I think we should have { type: "pauseOnDOMEvents" }.

(In reply to Panos Astithas [:past] from comment #23) > (In reply to Jim Blandy :jimb from comment #21) > > (It sure would be nice if we could automatically generate CSS selectors for > > any arbitrary D.O, assuming they refer to DOM notes that are in a document; > > we could include the selector in the printed form of the object. I don't > > know if that's possible.) > > I think it's doable and it would probably be a good idea to use that form in > the "summary" property or request I suggested in bug 753332. Actually it's easy to do it in the eventListener request since we're certain that we are dealing with DOM nodes, but we lack the introspection capabilities for a random JS object. bz had suggested that adding a nsIWindowUtils.isDOMObject helper method would be very easy in C++, so I'll file a followup to get that first, and then use it to augment the Debugger.Object actor form.

Updated the PR on GitHub according to your comments and I've also added a section on pauseOnExceptions. I'm also going to post an updated patch shortly.

Updated per the changed spec. I also stopped using _getAllEventListeners from both onEventListeners and _allEventsListener, in order to optimize the latter and avoid the crash workaround from the former. This way onEventListeners will now return event listeners set on the window object, but we can't break on them using the break-on-event-type functionality. Followup for that to come.

Comment on attachment 758670 [details] [diff] [review] Patch v6 Review of attachment 758670 [details] [diff] [review]: ----------------------------------------------------------------- canceling review pending discussion in #devtools. ::: toolkit/devtools/client/dbg-client.jsm @@ +904,5 @@ > get state() { return this._state; }, > get paused() { return this._state === "paused"; }, > > _pauseOnExceptions: false, > + _pauseOnDOMEvents: null, null or false? I guess null works since this is going to be an array. @@ +1053,5 @@ > + // If the debuggee is paused, the value of the array will be communicated in > + // the next resumption. Otherwise we have to force a pause in order to send > + // the array. > + if (this.paused) > + return void setTimeout(onResponse, 0); people scanning for setTimeout abuse in code may spot this and think we're contributing to race conditions. Could avoid this using thread.dispatch() but I leave that to you. ::: toolkit/devtools/server/actors/script.js @@ +93,3 @@ > */ > addDebuggee: function TA_addDebuggee(aGlobal) { > + let globalDO; nit: globalDebugObject. May want to use a slightly more descriptive variable name and set it explicitly to null here. @@ +134,3 @@ > */ > _addDebuggees: function TA__addDebuggees(aWindow) { > + let globalDO = this.addDebuggee(aWindow); nit: same as above re: name. @@ +148,5 @@ > * depending on the debugging context being required (chrome or content). > */ > globalManager: { > findGlobals: function TA_findGlobals() { > + this.globalDO = this._addDebuggees(this.global); I can see why you wanted to cutdown on typing! :) @@ +435,5 @@ > + this._breakOnEnter(listener.script); > + } > + } > + } > + }, sweet. @@ +448,5 @@ > + > + let nodes = this.global.document.querySelectorAll("*"); > + // XXX: using this.global instead of this.global.document causes > + // getListenerInfoFor() to crash when called from _allEventsListener. > + // With this workaround we can't pause on window.onerror (and others?) however. eek! Do we need a bug reference for this? @@ +449,5 @@ > + let nodes = this.global.document.querySelectorAll("*"); > + // XXX: using this.global instead of this.global.document causes > + // getListenerInfoFor() to crash when called from _allEventsListener. > + // With this workaround we can't pause on window.onerror (and others?) however. > + nodes = [this.global.document].concat([].slice.call(nodes)); that is quite a thing. querySelectorAll is going to be pretty heavy isn't it? We could be talking about a massive list for a very deep page. This looks like it could be expensive and slow. I don't know how else I'd like to see it done, but I want it noted that this is scary. :) (also, I wish NodeLists had an Array-like interface so you could just call things like concat(NodeList) directly, but that's wishful thinking). @@ +468,5 @@ > + l.type = handler.type; > + l.script = this.globalDO.makeDebuggeeValue(listener).script; > + listeners.push(l); > + } > + } yup. Heavy. nodes * listeners heavy. Potentially N^2. Scary. @@ +476,5 @@ > + /** > + * Set a breakpoint on the first bytecode offset in the provided script. > + */ > + _breakOnEnter: function(script) { > + let offsets = script.getAllOffsets(); does getAllOffsets return column offsets as well? Appears so. @@ +672,5 @@ > + scriptBreakpoints[aLocation.line] = { > + url: aLocation.url, > + line: aLocation.line, > + column: aLocation.column > + }; beauty.

(In reply to Rob Campbell [:rc] (:robcee) from comment #28) > ::: toolkit/devtools/client/dbg-client.jsm > @@ +904,5 @@ > > get state() { return this._state; }, > > get paused() { return this._state === "paused"; }, > > > > _pauseOnExceptions: false, > > + _pauseOnDOMEvents: null, > > null or false? I guess null works since this is going to be an array. Right, I thought that type variance would only confuse in this case. > @@ +1053,5 @@ > > + // If the debuggee is paused, the value of the array will be communicated in > > + // the next resumption. Otherwise we have to force a pause in order to send > > + // the array. > > + if (this.paused) > > + return void setTimeout(onResponse, 0); > > people scanning for setTimeout abuse in code may spot this and think we're > contributing to race conditions. > > Could avoid this using thread.dispatch() but I leave that to you. I actually deliberately used setTimeout due to its better familiarity for future developers. I thought it was agreed that we should gradually move away from using the thread manager directly to Timer.jsm: https://groups.google.com/d/msg/mozilla.dev.platform/IDjMJNczoKg/zZ009T1_5boJ What do you think? > @@ +448,5 @@ > > + > > + let nodes = this.global.document.querySelectorAll("*"); > > + // XXX: using this.global instead of this.global.document causes > > + // getListenerInfoFor() to crash when called from _allEventsListener. > > + // With this workaround we can't pause on window.onerror (and others?) however. > > eek! Do we need a bug reference for this? Yes, I filed bug 883096 and it already has a working patch, so I removed the workaround completely! > @@ +449,5 @@ > > + let nodes = this.global.document.querySelectorAll("*"); > > + // XXX: using this.global instead of this.global.document causes > > + // getListenerInfoFor() to crash when called from _allEventsListener. > > + // With this workaround we can't pause on window.onerror (and others?) however. > > + nodes = [this.global.document].concat([].slice.call(nodes)); > > that is quite a thing. > > querySelectorAll is going to be pretty heavy isn't it? We could be talking > about a massive list for a very deep page. This looks like it could be > expensive and slow. I don't know how else I'd like to see it done, but I > want it noted that this is scary. :) > > (also, I wish NodeLists had an Array-like interface so you could just call > things like concat(NodeList) directly, but that's wishful thinking). Indeed, I've changed it to use the much more performant getEventTargetChainFor() method suggested by dcamp and I can't see any trace of this code in my profiles. Using it on heavy sites like G+ and GitHub seems to work adequately.

Jim, do we still need another round of feedback on the protocol or are you ok with the current version of the documentation?

Comment on attachment 762714 [details] [diff] [review] Patch v7 Review of attachment 762714 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/client/dbg-client.jsm @@ +200,5 @@ > const UnsolicitedPauses = { > "resumeLimit": "resumeLimit", > "debuggerStatement": "debuggerStatement", > "breakpoint": "breakpoint", > + "DOMEvent": "DOMEvent", the ever-present unfortunate capitalization of DOM. ::: toolkit/devtools/server/actors/script.js @@ +450,5 @@ > /** > + * A listener that gets called for every event on the page when a list of > + * interesting events was provided with the pauseOnDOMEvents property. It is > + * used to set server-managed breakpoints on any existing event listeners for > + * those events. @param event @@ +465,5 @@ > + }, > + > + /** > + * Return an array containing all the event listeners added to every DOM > + * element on the page. Is this every DOM element on the page? getEventTargetChainFor(target) only returns the targets for this particular target (window, document or DOM node) according to the IDL. nit: @param target @returns Array this file isn't really strict about javadoc comments, is it? Still, not sure what "target" is looking at this function definition. @@ +471,5 @@ > + _getAllEventListeners: function(target) { > + let els = Cc["@mozilla.org/eventlistenerservice;1"] > + .getService(Ci.nsIEventListenerService); > + > + let nodes = els.getEventTargetChainFor(target); documentation claims this returns an array of "event targets". While these are most often going to be Nodes, they might be window or document objects, I guess. Maybe rename this to "targets" for consistency with the IDL. @@ +476,5 @@ > + let listeners = []; > + > + for (let node of nodes) { > + let handlers = els.getListenerInfoFor(node); > + for (let handler of handlers) { ok, double-nested loop aversion aside, this is probably reasonable. @@ +483,5 @@ > + // a JS debugger. > + if (!handler || !handler.listenerObject || !handler.type) > + continue; > + > + let l = Object.create(null); l is our listener copy? @@ +947,5 @@ > + > + let els = Cc["@mozilla.org/eventlistenerservice;1"] > + .getService(Ci.nsIEventListenerService); > + > + let nodes = this.global.document.querySelectorAll("*"); this is scary. Do we not have a better way to get a flat list of nodes from a document? @@ +954,5 @@ > + > + for (let node of nodes) { > + let handlers = els.getListenerInfoFor(node); > + > + for (let handler of handlers) { another nested for loop. Is this going to block the server while it's running. This could potentially be a pretty large set of nodes we're iterating. Feels ... dangerous. @@ +955,5 @@ > + for (let node of nodes) { > + let handlers = els.getListenerInfoFor(node); > + > + for (let handler of handlers) { > + let l = Object.create(null); again, l is a local listener copy? @@ +3137,5 @@ > dumpn(aError.message + ":\n" + aError.stack); > } > + > +// The following are copied here verbatim from css-logic.js, until we create a > +// server-friendly helper module. bleh. do we need a toolkit/devtools/utils module? Also, this method can generate some really nasty-looking selectors. @@ +3169,5 @@ > + > + // We might be able to find a unique class name > + var selector, index, matches; > + if (ele.classList.length > 0) { > + for (var i = 0; i < ele.classList.length; i++) { why is this using var declarations?

Olli, I tagged you for review. Could you take a look at the _getAllListeners method in toolkit/devtools/server/actors/script.js ? I'd just like a sanity check on whether we're using those methods correctly. Also, if you have any suggestions on improvements to using document.querySelectorAll("*") for gathering our nodelist and alternatives to those iteration schemes, I'd be happy to hear about them. Thanks!

The comment for the function says something else what it does. As written the method returns all the JS event listeners attached to target and its parents in the event target chain. And the chain contains chrome too. Events dispatched to an element propagate to parent nodes, including document. Then from document they propagate to window ('load' is an exception. It does not propagate). From window events propagate to message manager from which to <xul:browser> and its parent up to chrome document and its window and finally to WindowRoot object. So "added to every DOM element on the page." is wrong, or misleading.

Comment on attachment 762714 [details] [diff] [review] Patch v7 Would be also better to talk about event targets and not nodes, since window is an event target (and so it XHR or WebSocket etc.).

(In reply to Rob Campbell [:rc] (:robcee) from comment #31) > > @@ +947,5 @@ > > + > > + let els = Cc["@mozilla.org/eventlistenerservice;1"] > > + .getService(Ci.nsIEventListenerService); > > + > > + let nodes = this.global.document.querySelectorAll("*"); > > this is scary. Do we not have a better way to get a flat list of nodes from > a document? > From my brief previous attempts to resurrect Tilt, I did a little benchmarking on this, and querying for * is actually the fastest way possible of achieving this. Actually, much, much faster than even using the built-in document or tree walkers, or building our own recursive/declarative iterator.

I'll add the rest of the comments and make the cleanups requested, but let me respond to a few questions you had. (In reply to Rob Campbell [:rc] (:robcee) from comment #31) > @@ +947,5 @@ > > + > > + let els = Cc["@mozilla.org/eventlistenerservice;1"] > > + .getService(Ci.nsIEventListenerService); > > + > > + let nodes = this.global.document.querySelectorAll("*"); > > this is scary. Do we not have a better way to get a flat list of nodes from > a document? I don't know of a faster way to get all the document's nodes. Anecdotal testing in heavy apps like G+ and GitHub seems to confirm that this is pretty fast. > @@ +954,5 @@ > > + > > + for (let node of nodes) { > > + let handlers = els.getListenerInfoFor(node); > > + > > + for (let handler of handlers) { > > another nested for loop. > > Is this going to block the server while it's running. This could potentially > be a pretty large set of nodes we're iterating. Feels ... dangerous. In practice it's quite fast. My tests on the following huge GitHub page diddn't reveal any serious performance degradation using a debug build: https://github.com/mozilla/mozilla-central/blob/master/layout/base/nsPresShell.cpp#L3216 In normal pages it's basically instant. > @@ +3137,5 @@ > > dumpn(aError.message + ":\n" + aError.stack); > > } > > + > > +// The following are copied here verbatim from css-logic.js, until we create a > > +// server-friendly helper module. > > bleh. do we need a toolkit/devtools/utils module? Probably. IIRC dcamp and jwalker suggested to do something about it after the loader work is finished and all of our modules use the jetpack API. > Also, this method can generate some really nasty-looking selectors. True, but I need these for disambiguation. Perhaps the front-end would be better off constructing a label from the rest of the properties. In the inspector's case for example, I imagine just the event name would be sufficient, as the plan is to attach the list to the selected element. > @@ +3169,5 @@ > > + > > + // We might be able to find a unique class name > > + var selector, index, matches; > > + if (ele.classList.length > 0) { > > + for (var i = 0; i < ele.classList.length; i++) { > > why is this using var declarations? Presumably because it's from GCLI, which also runs as a web page? Dunno, really. Like I said, it's a verbatim copy from css-logic.js and I've purposefully stayed away from any modifications, since they will be undone when we start using a common module.

Fixed the method comment that referred to getting all the listeners on the page, that ceased to be accurate when I changed the code to use getEventTargetChainFor(). It may not be terribly obvious, but _getAllEventListeners skips chrome listeners because they can't be converted to a debuggee value, as their compartment hasn't been added as a debuggee. I've added a comment in the code to clarify this. I've also retested and I don't see any chrome listeners when using this patch.

Here are the changes between rebased v7 and v8 for easier review. It doesn't include the comment I just mentioned about chrome listeners because I had already folded the changes back in the main patch when I thought about it. Now if I was using git...

Comment on attachment 777106 [details] [diff] [review] Patch v8 Reviewed just ELS usage. >+ /** >+ * Return an array containing all the event listeners attached to the >+ * specified event target and its parent in the event target chain. s/its parent in the event target chain./its ancestors in the event target chain./ >+ * >+ * @param EventTarget target >+ * The target the event was dispatched on. >+ * @returns Array >+ */ >+ _getAllEventListeners: function(target) { >+ let els = Cc["@mozilla.org/eventlistenerservice;1"] >+ .getService(Ci.nsIEventListenerService); >+ >+ let targets = els.getEventTargetChainFor(target); >+ let listeners = []; >+ >+ for (let node of targets) { perhaps s/node/target/ >+ l.script = this.globalDebugObject.makeDebuggeeValue(listener).script; >+ // Chrome listeners won't be converted to debuggee values, since their >+ // compartment is not added as a debuggee. >+ if (!l.script) >+ continue; Someone else needs to review this part. I know nothing about makeDebuggeeValue. But assuming it is right, then this is ok.

(In reply to Panos Astithas [:past] from comment #36) > I'll add the rest of the comments and make the cleanups requested, but let > me respond to a few questions you had. > > (In reply to Rob Campbell [:rc] (:robcee) from comment #31) > > @@ +947,5 @@ > > > + > > > + let els = Cc["@mozilla.org/eventlistenerservice;1"] > > > + .getService(Ci.nsIEventListenerService); > > > + > > > + let nodes = this.global.document.querySelectorAll("*"); > > > > this is scary. Do we not have a better way to get a flat list of nodes from > > a document? > > I don't know of a faster way to get all the document's nodes. Anecdotal > testing in heavy apps like G+ and GitHub seems to confirm that this is > pretty fast. it's probably fine. I've been asking around and there is basically no low-level API to get a list of nodes from the DOM. The only other alternative I've seen is the DOM level 2 getElementsByTagName("*") and it's probably equivalent to qSA("*");. > > @@ +954,5 @@ > > > + > > > + for (let node of nodes) { > > > + let handlers = els.getListenerInfoFor(node); > > > + > > > + for (let handler of handlers) { > > > > another nested for loop. > > > > Is this going to block the server while it's running. This could potentially > > be a pretty large set of nodes we're iterating. Feels ... dangerous. > > In practice it's quite fast. My tests on the following huge GitHub page > didn't reveal any serious performance degradation using a debug build: > > https://github.com/mozilla/mozilla-central/blob/master/layout/base/ > nsPresShell.cpp#L3216 > > In normal pages it's basically instant. most pages don't have a lot of listeners on every node so it shouldn't be too bad. I'm happy with this summary. > > @@ +3137,5 @@ > > > dumpn(aError.message + ":\n" + aError.stack); > > > } > > > + > > > +// The following are copied here verbatim from css-logic.js, until we create a > > > +// server-friendly helper module. > > > > bleh. do we need a toolkit/devtools/utils module? > > Probably. IIRC dcamp and jwalker suggested to do something about it after > the loader work is finished and all of our modules use the jetpack API. ok. [...] > > why is this using var declarations? > > Presumably because it's from GCLI, which also runs as a web page? Dunno, > really. Like I said, it's a verbatim copy from css-logic.js and I've > purposefully stayed away from any modifications, since they will be undone > when we start using a common module. yeah, no worries. Just observing out loud. :)

Comment on attachment 777106 [details] [diff] [review] Patch v8 Review of attachment 777106 [details] [diff] [review]: ----------------------------------------------------------------- Good.

(In reply to Rob Campbell [:rc] (:robcee) from comment #40) > it's probably fine. I've been asking around and there is basically no > low-level API to get a list of nodes from the DOM. The only other > alternative I've seen is the DOM level 2 getElementsByTagName("*") and it's > probably equivalent to qSA("*");. This is a good idea, I thought the same, but after some microbenchmarks it appears that getElementsByTagName("*") is slightly faster, so I used that. It seems that qsA doesn't take a shortcut for "*" and uses the common-case selector parsing path. Made the latest changes mentioned by Olli, too.

woo! also, as a thought, this probably doesn't deal with iframes. We'll need to add another level of for frame of window.frames to really get everything here. Could we file a followup?

(In reply to Rob Campbell [:rc] (:robcee) from comment #44) > also, as a thought, this probably doesn't deal with iframes. We'll need to > add another level of for frame of window.frames to really get everything > here. > > Could we file a followup? Filed bug 895446 for this.

Comment on attachment 758618 [details] [diff] [review] Protocol spec changes v2 Review of attachment 758618 [details] [diff] [review]: ----------------------------------------------------------------- I like the text. I'm afraid that our documentatino of pause reasons is getting scattered across several locations in the document. ::: protocol @@ +764,5 @@ > +To request that execution pause when an exception is thrown, the client may send a request of the form: > + > + { "to":<i>thread</i>, "type":"resume", "pauseOnExceptions": true } > + > +If <tt>pauseOnExceptions</tt> has a falsy value or is omitted, execution will continue in the face of thrown exceptions. When a thread pauses because an exception was thrown, the "paused" packet's <i>reason</i> will have the following form: This is a JSON protocol, not JS; let's require it be 'false' or omitted. @@ +784,5 @@ > +A <tt>pauseOnDOMEvents</tt> applies only until the next pause. In order to change the types of events that should pause execution, a new array of event types should be sent to the server. Any events not present in the new list will no longer trigger pauses. Consequently, sending an empty array or simply omitting the <tt>pauseOnDOMEvents</tt> property disables pausing on DOM events. > + > +When a thread pauses because a DOM event was triggered, the "paused" packet's <i>reason</i> will have a type of <tt>"pauseOnDOMEvents"</tt>. > + > +If a <tt>"forceCompletion"</tt> property is present in a <tt>"resume"</tt> packet, along with <tt>"resumeLimit"</tt>, <tt>"pauseOnExceptions"</tt> or <tt>"pauseOnDOMEvents"</tt>, it will take precedence and cause them to be ignored. Perhaps this is just documenting our present behavior --- but I do think such a packet is just ill-conceived: any client that sends both of them is not keeping track of what it's trying to do very well. I think this should be an error.

OK, I updated the pull request with the suggested changes.