Closed Bug 800857 Opened 7 years ago Closed 6 years ago

Break on dom events

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: vporof, Assigned: vporof)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(8 files, 4 obsolete files)

4.08 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
283.92 KB, image/png
Details
77.06 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
40.38 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
1.53 KB, patch
past
: review+
Details | Diff | Splinter Review
142.66 KB, image/png
Details
222.94 KB, application/json
Details
2.36 KB, patch
past
: review+
Details | Diff | Splinter Review
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).
I think we are going to need nsIEventListenerService.addListenerForAllEvents for this.
Depends on: 779306
(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.
(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?
(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.
(not resume, but step).
If you look at bug 779306, addListenerForAllEvents is called before any other content listeners.
(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.
Priority: -- → P3
FILTER ON PUMPKINS.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
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.
Yuup. This and also bug 800857.
Filed bug 832982.
(In reply to Victor Porof [:vp] from comment #10)
> Yuup. This and also bug 800857.

D'oh. I mean 821610.
Depends on: 832982
Attached patch WIP (obsolete) — Splinter Review
Attaching the hacky UI parts that I extracted from bug 832982. This should serve as a basis and inspiration for implementing this bug.
(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!
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.
Flags: needinfo?(past)
(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)
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?
Flags: needinfo?(past)
(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.
Attached patch dbg-event-listeners.patch (obsolete) — Splinter Review
This works. Tests will follow tomorrow.
Attachment #750963 - Attachment is obsolete: true
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.
(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
See Also: → 916439
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.
Attachment #804988 - Attachment is obsolete: true
Attachment #807270 - Flags: review?(rcampbell)
Attachment #807271 - Flags: review?(rcampbell)
Attached patch Part 3: Tests (obsolete) — Splinter Review
Attachment #807272 - Flags: review?(rcampbell)
Attached image This is how it looks
Blocks: 918416
Blocks: 918715
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.
Sounds like a backend bug. Will look into it.
Attachment #807271 - Attachment is obsolete: true
Attachment #807271 - Flags: review?(rcampbell)
Attachment #809693 - Flags: review?(rcampbell)
Attachment #807272 - Attachment is obsolete: true
Attachment #807272 - Flags: review?(rcampbell)
Attachment #809694 - Flags: review?(rcampbell)
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.
Attachment #809697 - Flags: review?(past)
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?
(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 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?
Attachment #809792 - Flags: review?(past) → review+
(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 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
  }
},
Attachment #809697 - Flags: review?(past) → review+
(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 on attachment 807270 [details] [diff] [review]
Part 1: Simplify the ensureElementIsVisible method in the SideMenuWidget

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

nice.
Attachment #807270 - Flags: review?(rcampbell) → review+
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.
Attachment #809693 - Flags: review?(rcampbell) → review+
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!
Attachment #809694 - Flags: review?(rcampbell) → review+
Keywords: dev-doc-needed
Depends on: 923802, 923779
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.
Flags: needinfo?(vporof)
++
Flags: needinfo?(vporof)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.