Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Break on dom events

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete})

unspecified
Firefox 27
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 4 obsolete attachments)

4.08 KB, patch
Details | Diff | Splinter Review
283.92 KB, image/png
Details
77.06 KB, patch
Details | Diff | Splinter Review
40.38 KB, patch
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
(Assignee)

Description

5 years ago
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
(Assignee)

Comment 2

5 years ago
(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?
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
(not resume, but step).
If you look at bug 779306, addListenerForAllEvents is called before any other content listeners.
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Updated

5 years ago
Priority: -- → P3
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 10

5 years ago
Yuup. This and also bug 800857.
(Assignee)

Comment 11

5 years ago
Filed bug 832982.
(Assignee)

Comment 12

5 years ago
(In reply to Victor Porof [:vp] from comment #10)
> Yuup. This and also bug 800857.

D'oh. I mean 821610.
(Assignee)

Updated

5 years ago
Depends on: 832982
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.
(Assignee)

Comment 14

4 years ago
(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!
(Assignee)

Comment 15

4 years ago
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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(past)
(Assignee)

Comment 16

4 years ago
(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)
(Assignee)

Comment 18

4 years ago
(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.
(Assignee)

Comment 19

4 years ago
Created attachment 804988 [details] [diff] [review]
dbg-event-listeners.patch

This works. Tests will follow tomorrow.
Attachment #750963 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
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
(Assignee)

Updated

4 years ago
See Also: → bug 916439
(Assignee)

Comment 22

4 years ago
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.
Attachment #804988 - Attachment is obsolete: true
Attachment #807270 - Flags: review?(rcampbell)
(Assignee)

Comment 23

4 years ago
Created attachment 807271 [details] [diff] [review]
Part 2: Implement the frontend bits
Attachment #807271 - Flags: review?(rcampbell)
(Assignee)

Comment 24

4 years ago
Created attachment 807272 [details] [diff] [review]
Part 3: Tests
Attachment #807272 - Flags: review?(rcampbell)
(Assignee)

Comment 25

4 years ago
Created attachment 807274 [details]
This is how it looks
(Assignee)

Updated

4 years ago
Blocks: 918416
(Assignee)

Updated

4 years ago
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.
(Assignee)

Comment 27

4 years ago
Sounds like a backend bug. Will look into it.
(Assignee)

Comment 28

4 years ago
Created attachment 809693 [details] [diff] [review]
Part 2: Implement the frontend bits, rebased, fixed some typos
Attachment #807271 - Attachment is obsolete: true
Attachment #807271 - Flags: review?(rcampbell)
Attachment #809693 - Flags: review?(rcampbell)
(Assignee)

Comment 29

4 years ago
Created attachment 809694 [details] [diff] [review]
Part 3: Tests, rebased, fixed oranges
Attachment #807272 - Attachment is obsolete: true
Attachment #807272 - Flags: review?(rcampbell)
Attachment #809694 - Flags: review?(rcampbell)
(Assignee)

Comment 30

4 years ago
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.
Attachment #809697 - Flags: review?(past)
(Assignee)

Comment 31

4 years ago
Created attachment 809731 [details]
listener objects with no associated url - screenshot
(Assignee)

Comment 32

4 years ago
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?
(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.
(Assignee)

Comment 34

4 years ago
Created attachment 809792 [details] [diff] [review]
Part 5: Handle event listener objects added by plugins
Attachment #809792 - Flags: review?(past)
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+
(Assignee)

Comment 36

4 years ago
(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+
(Assignee)

Comment 38

4 years ago
(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.
(Assignee)

Comment 39

4 years ago
Try was green: https://tbpl.mozilla.org/?tree=Try&rev=88b14648c12c
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+
(Assignee)

Comment 43

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/faa22d372e88
https://hg.mozilla.org/integration/fx-team/rev/aef229098113
https://hg.mozilla.org/integration/fx-team/rev/1d5b4632d745
https://hg.mozilla.org/integration/fx-team/rev/8626082a3c7c
https://hg.mozilla.org/integration/fx-team/rev/3ceac6e4aaed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/faa22d372e88
https://hg.mozilla.org/mozilla-central/rev/aef229098113
https://hg.mozilla.org/mozilla-central/rev/1d5b4632d745
https://hg.mozilla.org/mozilla-central/rev/8626082a3c7c
https://hg.mozilla.org/mozilla-central/rev/3ceac6e4aaed
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 46

4 years ago
++
Flags: needinfo?(vporof)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.