Last Comment Bug 800857 - Break on dom events
: Break on dom events
Status: RESOLVED FIXED
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: Firefox 27
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 923802 779306 832982 923779
Blocks: 918416 918715
  Show dependency treegraph
 
Reported: 2012-10-12 03:28 PDT by Victor Porof [:vporof][:vp]
Modified: 2013-11-03 08:37 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (5.62 KB, patch)
2013-05-17 03:26 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
dbg-event-listeners.patch (67.97 KB, patch)
2013-09-15 02:39 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
Part 1: Simplify the ensureElementIsVisible method in the SideMenuWidget (4.08 KB, patch)
2013-09-19 09:59 PDT, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review
Part 2: Implement the frontend bits (73.39 KB, patch)
2013-09-19 10:00 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
Part 3: Tests (35.32 KB, patch)
2013-09-19 10:00 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
This is how it looks (283.92 KB, image/png)
2013-09-19 10:02 PDT, Victor Porof [:vporof][:vp]
no flags Details
Part 2: Implement the frontend bits, rebased, fixed some typos (77.06 KB, patch)
2013-09-25 00:25 PDT, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review
Part 3: Tests, rebased, fixed oranges (40.38 KB, patch)
2013-09-25 00:25 PDT, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review
Part 4: Handle getOwnPropertyDescriptor throwing when accessing "displayName" (1.53 KB, patch)
2013-09-25 00:29 PDT, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Splinter Review
listener objects with no associated url - screenshot (142.66 KB, image/png)
2013-09-25 02:07 PDT, Victor Porof [:vporof][:vp]
no flags Details
listener objects with no associated url - protocol response (222.94 KB, application/json)
2013-09-25 02:08 PDT, Victor Porof [:vporof][:vp]
no flags Details
Part 5: Handle event listener objects added by plugins (2.36 KB, patch)
2013-09-25 03:57 PDT, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2012-10-12 03:28:09 PDT
Digging through the source is hard, especially if we don't know anything about it.

If there's a listener attached to a node for a particular event, there should be an easier way of setting a breakpoint for when entering it. I'm imagining adding a "break on [that]" on the infobar dropdown, and/or even displaying information about nodes with events attached in the markup panel (<event type> icon on the node line).

The tricky part is finding where exactly to add the breakpoint (source+line). We probably can't rely on the parser API for this (it's trivial to find the places where an event listener is registered, it's pretty hard to determine which is the node in question).
Comment 1 Panos Astithas [:past] 2012-10-12 05:21:00 PDT
I think we are going to need nsIEventListenerService.addListenerForAllEvents for this.
Comment 2 Victor Porof [:vporof][:vp] 2012-10-12 07:27:54 PDT
(In reply to Panos Astithas [:past] from comment #1)
> I think we are going to need nsIEventListenerService.addListenerForAllEvents
> for this.

So we'd basically rely on a (similar to) break-on-next functionality here? Or am I understanding this incorrectly? This raises some interesting complications.
Comment 3 Panos Astithas [:past] 2012-10-12 07:41:33 PDT
(In reply to Victor Porof [:vp] from comment #2)
> (In reply to Panos Astithas [:past] from comment #1)
> > I think we are going to need nsIEventListenerService.addListenerForAllEvents
> > for this.
> 
> So we'd basically rely on a (similar to) break-on-next functionality here?
> Or am I understanding this incorrectly? This raises some interesting
> complications.

Right, that's the idea. What are the complications you are thinking of?
Comment 4 Victor Porof [:vporof][:vp] 2012-10-12 08:08:59 PDT
(In reply to Panos Astithas [:past] from comment #3)
> 
> Right, that's the idea. What are the complications you are thinking of?

We'd have to automatically resume in the initial handler. And make sure that's the actual initial listener called.
Comment 5 Victor Porof [:vporof][:vp] 2012-10-12 08:09:21 PDT
(not resume, but step).
Comment 6 Panos Astithas [:past] 2012-10-12 08:59:39 PDT
If you look at bug 779306, addListenerForAllEvents is called before any other content listeners.
Comment 7 Victor Porof [:vporof][:vp] 2012-10-12 09:35:28 PDT
(In reply to Panos Astithas [:past] from comment #6)
> If you look at bug 779306, addListenerForAllEvents is called before any
> other content listeners.

Ah, that's excellent.
Comment 8 Victor Porof [:vporof][:vp] 2012-11-01 09:59:30 PDT
FILTER ON PUMPKINS.
Comment 9 Rob Campbell [:rc] (:robcee) 2013-01-21 07:16:09 PST
This'll probably need to be broken into at least one more bug. There's a server component and a front-end/controller piece to make it work.
Comment 10 Victor Porof [:vporof][:vp] 2013-01-21 07:55:24 PST
Yuup. This and also bug 800857.
Comment 11 Victor Porof [:vporof][:vp] 2013-01-21 07:57:11 PST
Filed bug 832982.
Comment 12 Victor Porof [:vporof][:vp] 2013-01-21 08:03:30 PST
(In reply to Victor Porof [:vp] from comment #10)
> Yuup. This and also bug 800857.

D'oh. I mean 821610.
Comment 13 Panos Astithas [:past] 2013-05-17 03:26:52 PDT
Created attachment 750963 [details] [diff] [review]
WIP

Attaching the hacky UI parts that I extracted from bug 832982. This should serve as a basis and inspiration for implementing this bug.
Comment 14 Victor Porof [:vporof][:vp] 2013-05-17 08:04:21 PDT
(In reply to Panos Astithas [:past] from comment #13)
> Created attachment 750963 [details] [diff] [review]
> WIP
> 
> Attaching the hacky UI parts that I extracted from bug 832982. This should
> serve as a basis and inspiration for implementing this bug.

Thanks!
Comment 15 Victor Porof [:vporof][:vp] 2013-09-14 04:55:00 PDT
Panos, how would you say it's best to handle dynamically added event listeners? Just asking for them when the sources are added is not enough, because they might be added at any time in unforeseeable ways.

I was thinking of updating the list
1. After the sources are fetched,
2. Whenever the debuggee pauses and/or
3. Providing a refresh mechanism in the events pane

I'm leaning towards all three. Obviously having a notification for such things would be nicer, but afaik nsIEventListenerService doesn't emit such events.
Comment 16 Victor Porof [:vporof][:vp] 2013-09-14 04:58:05 PDT
(suggestion 2 from the comment above could obviously make things too chatty, but there are easy ways to handle rapid stepping and not flood the connection pipe, e.g. with the named timeouts mechanism from bug 891439)
Comment 17 Panos Astithas [:past] 2013-09-14 09:22:40 PDT
You are right, the current design doesn't support dynamic updates in a sane way, but that wasn't in the plan laid out in comment 0. If we want to display a separate list of event listener sources in the debugger, we could either cache in the client and update on events 1 and 2 from comment 15, or keep a cache in the server and send notifications to the client. The low-level API lets us get notified when event listeners are triggered, not when they are added. To be fair, the only case not properly detected with our current hooks is adding an existing function as a listener, which was defined far away from the location of the registration (otherwise updating on the "newSource" packet will take care of that). Perhaps that's not too common to worry about in v1?
Comment 18 Victor Porof [:vporof][:vp] 2013-09-14 09:45:46 PDT
(In reply to Panos Astithas [:past] from comment #17)

Gotcha. I think we should just worry about having something functional in a v1, edge cases can be taken care of later. It's easier and less verbose to keep the cache on the client, just like I did with breakpoints in bug 901271.
Comment 19 Victor Porof [:vporof][:vp] 2013-09-15 02:39:26 PDT
Created attachment 804988 [details] [diff] [review]
dbg-event-listeners.patch

This works. Tests will follow tomorrow.
Comment 20 Victor Porof [:vporof][:vp] 2013-09-15 02:59:00 PDT
Comment on attachment 804988 [details] [diff] [review]
dbg-event-listeners.patch

Review of attachment 804988 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/debugger/debugger-controller.js
@@ +1288,5 @@
> +    // Make sure we're not sending a batch of closely repeated requests.
> +    // This can easily happen whenever new sources are fetched, thus the same.
> +    setNamedTimeout("event-listeners-fetch", FETCH_EVENT_LISTENERS_DELAY, () => {
> +      if (gThreadClient.state != "paused") {
> +        gThreadClient.interrupt(() => getListeners() + gThreadClient.resume());

I think I shouldn't be required to do this. Just like with the pauseOnDOMEvents call, it'd be nice if this worked regardless of the thread client state.
Comment 21 Panos Astithas [:past] 2013-09-16 01:04:16 PDT
(In reply to Victor Porof [:vp] from comment #20)
> I think I shouldn't be required to do this. Just like with the
> pauseOnDOMEvents call, it'd be nice if this worked regardless of the thread
> client state.

It should work, the "eventListeners" request handler doesn't test for a specific thread state:

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#1594
Comment 22 Victor Porof [:vporof][:vp] 2013-09-19 09:59:45 PDT
Created attachment 807270 [details] [diff] [review]
Part 1: Simplify the ensureElementIsVisible method in the SideMenuWidget

This implementation relied on the false premise that "repeated calls to ensureElementIsVisible would interfere with each other", just like in the case of an arrowscrollbox. While this is certainly true for the BreadcrumbsWidget, the SideMenuWidget doesn't use a arrowscrollbox, so the premise was false.

This became apparent with the different ways in which the widget is used for displaying event listeners. The items would sometimes shift away because of the timeouts. This patch removes them.
Comment 23 Victor Porof [:vporof][:vp] 2013-09-19 10:00:19 PDT
Created attachment 807271 [details] [diff] [review]
Part 2: Implement the frontend bits
Comment 24 Victor Porof [:vporof][:vp] 2013-09-19 10:00:49 PDT
Created attachment 807272 [details] [diff] [review]
Part 3: Tests
Comment 25 Victor Porof [:vporof][:vp] 2013-09-19 10:02:17 PDT
Created attachment 807274 [details]
This is how it looks
Comment 26 Rob Campbell [:rc] (:robcee) 2013-09-24 13:19:23 PDT
preliminary testing with these patches applied, I ran into this error when opening the Events panel on:

http://www.mozilla.org/en-US/firefox/central/

16:18:42.506 error occurred while processing 'eventListeners: Error: Permission denied to access property 'displayName' main.js:923
16:18:42.507 Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'eventListeners: Error: Permission denied to access property 'displayName'"

Will continue review.
Comment 27 Victor Porof [:vporof][:vp] 2013-09-24 14:32:20 PDT
Sounds like a backend bug. Will look into it.
Comment 28 Victor Porof [:vporof][:vp] 2013-09-25 00:25:10 PDT
Created attachment 809693 [details] [diff] [review]
Part 2: Implement the frontend bits, rebased, fixed some typos
Comment 29 Victor Porof [:vporof][:vp] 2013-09-25 00:25:55 PDT
Created attachment 809694 [details] [diff] [review]
Part 3: Tests, rebased, fixed oranges
Comment 30 Victor Porof [:vporof][:vp] 2013-09-25 00:29:25 PDT
Created attachment 809697 [details] [diff] [review]
Part 4: Handle getOwnPropertyDescriptor throwing when accessing "displayName"

This fixes the error found by Rob in comment 26, but I can't understand it. I haven't found anything anywhere about why accessing displayName would fail with some function listener objects.

I think this is a good enough fix for now, but we should file a followup about figuring out what exactly the cause is.
Comment 31 Victor Porof [:vporof][:vp] 2013-09-25 02:07:22 PDT
Created attachment 809731 [details]
listener objects with no associated url - screenshot
Comment 32 Victor Porof [:vporof][:vp] 2013-09-25 02:08:50 PDT
Created attachment 809733 [details]
listener objects with no associated url - protocol response

There are some cases in which the listener functions have no url. Any hints on this Panos? I assume they might be from gc'd scripts? Should we skip displaying them in the Events tab?
Comment 33 Panos Astithas [:past] 2013-09-25 03:04:52 PDT
(In reply to Victor Porof [:vp] from comment #32)
> There are some cases in which the listener functions have no url. Any hints
> on this Panos? I assume they might be from gc'd scripts? Should we skip
> displaying them in the Events tab?

All of them are on one of the video elements in the page, which makes me think they are added from ActionScript code, which gets translated to native code by the plugin. See also how some of them have function.class == "Object" and not "Function". They may be of interest to some developers (like bsmedberg), so I'd say let's just display [native code] in those cases, like toSource() does.
Comment 34 Victor Porof [:vporof][:vp] 2013-09-25 03:57:01 PDT
Created attachment 809792 [details] [diff] [review]
Part 5: Handle event listener objects added by plugins
Comment 35 Panos Astithas [:past] 2013-09-25 04:54:52 PDT
Comment on attachment 809792 [details] [diff] [review]
Part 5: Handle event listener objects added by plugins

Review of attachment 809792 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't looked at the rest of the patch, but I assume that these listeners will not be clickable, right?
Comment 36 Victor Porof [:vporof][:vp] 2013-09-25 05:09:20 PDT
(In reply to Panos Astithas [:past] from comment #35)
> Comment on attachment 809792 [details] [diff] [review]

None are yet, I filed bug 918416 and bug 918715 as followups.
Comment 37 Panos Astithas [:past] 2013-09-26 06:16:55 PDT
Comment on attachment 809697 [details] [diff] [review]
Part 4: Handle getOwnPropertyDescriptor throwing when accessing "displayName"

Review of attachment 809697 [details] [diff] [review]:
-----------------------------------------------------------------

OK, but please add an comment with the bug number.

These 2 listeners seem to be the interesting ones:

{
  "node": {
    "selector": "#htmlPlayer",
    "object": {
      "type": "object",
      "class": "HTMLVideoElement",
      "actor": "conn0.obj363",
      "extensible": true,
      "frozen": false,
      "sealed": false
    }
  },
  "type": "media-showStatistics",
  "capturing": false,
  "allowsUntrusted": true,
  "inSystemEventGroup": false,
  "isEventHandler": false,
  "function": {
    "type": "object",
    "class": "Function",
    "actor": "conn0.obj364",
    "extensible": true,
    "frozen": false,
    "sealed": false
  }
},
{
  "node": {
    "selector": "#htmlPlayer",
    "object": {
      "type": "object",
      "class": "HTMLVideoElement",
      "actor": "conn0.obj363",
      "extensible": true,
      "frozen": false,
      "sealed": false
    }
  },
  "type": "keypress",
  "capturing": false,
  "allowsUntrusted": true,
  "inSystemEventGroup": false,
  "isEventHandler": false,
  "function": {
    "type": "object",
    "class": "Function",
    "actor": "conn0.obj366",
    "extensible": true,
    "frozen": false,
    "sealed": false
  }
},
Comment 38 Victor Porof [:vporof][:vp] 2013-09-26 07:55:33 PDT
(In reply to Panos Astithas [:past] from comment #37)
> Comment on attachment 809697 [details] [diff] [review]
> Part 4: Handle getOwnPropertyDescriptor throwing when accessing "displayName"
> 
> Review of attachment 809697 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, but please add an comment with the bug number.

Will do! Thanks.
Comment 39 Victor Porof [:vporof][:vp] 2013-09-26 07:55:47 PDT
Try was green: https://tbpl.mozilla.org/?tree=Try&rev=88b14648c12c
Comment 40 Rob Campbell [:rc] (:robcee) 2013-09-26 12:03:50 PDT
Comment on attachment 807270 [details] [diff] [review]
Part 1: Simplify the ensureElementIsVisible method in the SideMenuWidget

Review of attachment 807270 [details] [diff] [review]:
-----------------------------------------------------------------

nice.
Comment 41 Rob Campbell [:rc] (:robcee) 2013-10-01 09:04:49 PDT
Comment on attachment 809693 [details] [diff] [review]
Part 2: Implement the frontend bits, rebased, fixed some typos

Review of attachment 809693 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/debugger/debugger-controller.js
@@ +1126,5 @@
>  
> +    // Make sure the events listeners are up to date.
> +    if (DebuggerView.instrumentsPaneTab == "events-tab") {
> +      DebuggerController.Breakpoints.DOM.scheduleEventListenersFetch();
> +    }

two of these in a row. Almost new function time!

::: browser/devtools/debugger/debugger-panes.js
@@ +1512,5 @@
> +  initialize: function() {
> +    dumpn("Initializing the EventListenersView");
> +
> +    this.widget = new SideMenuWidget(document.getElementById("event-listeners"), {
> +      theme: "light",

should this depend on the user selected theme?

@@ +1547,5 @@
> +   *        Additional options for adding the source. Supported options:
> +   *        - staged: true to stage the item to be appended later
> +   */
> +  addListener: function(aListener, aOptions = {}) {
> +    let { node: { selector }, function: { url }, type } = aListener;

these are getting a little lengthy.

@@ +1621,5 @@
> +    } else if (starts("touch")) {
> +      group = L10N.getStr("touchEvents");
> +    } else {
> +      group = L10N.getStr("otherEvents");
> +    }

halp.

Is this really the best way to do this?

@@ +1662,5 @@
> +   * @return array
> +   *         List of event types, for example ["load", "click"...]
> +   */
> +  getCheckedEvents: function() {
> +    return this.attachments.filter(e => e.checkboxState).map(e => e.type);

you're getting pretty cascadey with your sends.

::: browser/devtools/shared/widgets/SideMenuWidget.jsm
@@ +654,5 @@
>      if (!this._checkbox) {
>        throw new Error("Cannot check items that do not have checkboxes.");
>      }
> +    // Don't set or remove the "checked" attribute, assign the property instead.
> +    // Otherwise, the "CheckboxStateChange" event will not be fired. XUL!!

need more exclamation points here. And maybe some extra UUUs and LLs.
Comment 42 Rob Campbell [:rc] (:robcee) 2013-10-01 09:08:30 PDT
Comment on attachment 809694 [details] [diff] [review]
Part 3: Tests, rebased, fixed oranges

Review of attachment 809694 [details] [diff] [review]:
-----------------------------------------------------------------

you bet!

::: browser/devtools/debugger/test/browser_dbg_break-on-dom-03.js
@@ +12,5 @@
> +    let gDebugger = aPanel.panelWin;
> +    let gView = gDebugger.DebuggerView;
> +    let gEvents = gView.EventListeners;
> +
> +    Task.spawn(function() {

I love these!

::: browser/devtools/debugger/test/browser_dbg_break-on-dom-event.js
@@ +211,5 @@
>    gInput.focus();
>  }
>  
>  function triggerButtonClick() {
> +  EventUtils.sendMouseEvent({ type: "click" }, gButton);

thank heavens you found that!
Comment 45 Will Bamberg [:wbamberg] 2013-11-01 17:30:56 PDT
I've added a couple of sections to the debugger doc for this:

* one for the general "tour of the UI": https://developer.mozilla.org/en-US/docs/Tools/Debugger#Events_pane
* one for the task oriented "how do I" section: https://developer.mozilla.org/en-US/docs/Tools/Debugger#Break_on_a_DOM_event

Let me know if these work for you.
Comment 46 Victor Porof [:vporof][:vp] 2013-11-02 04:15:49 PDT
++

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