Closed Bug 1519597 Opened 11 months ago Closed 11 months ago

Bad event handlers on DOM nodes produce bogus popups when inspected

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox66 fixed)

RESOLVED FIXED
Firefox 66
Tracking Status
firefox66 --- fixed

People

(Reporter: jimb, Assigned: miker)

References

Details

(Whiteboard: [qa-66b-p2])

Attachments

(2 files)

On any web page, select a node in the inspector and type the following in the console:

$0.addEventHandler('click', {})

Then, in the inspector, click on the 'event' marker that appears to the right of the node's start tag display. The attached image shows what I see: a silly-looking popup with an interior height of zero.

This problem arises from the following code in NodeActor's processHandlerForEvent method in devtools/server/actors/inspector/node.js:

    // If the listener is an object with a 'handleEvent' method, use that.
    if (listenerDO.class === "Object" || /^XUL\w*Element$/.test(listenerDO.class)) {
      let desc;

      while (!desc && listenerDO) {
        desc = listenerDO.getOwnPropertyDescriptor("handleEvent");
        listenerDO = listenerDO.proto;
      }

      if (desc && desc.value) {
        listenerDO = desc.value;
      }
    }

    // If the listener is bound to a different context then we need to switch
    // to the bound function.
    if (listenerDO.isBoundFunction) {
      listenerDO = listenerDO.boundTargetFunction;
    }

If the while loop exits without having found a handleEvent property, listenerDO is left set to null, and the check of the isBoundFunction property throws an exception.

It seems like the following changeset was meant to address these issues:
https://hg.mozilla.org/mozilla-central/rev/72d19d0827de
Bug 1060933 - Prevent jQuery Live event bubbles from throwing on invalid selector r=bgrins

+      try {
+        let eventInfos = getListeners(node);
+
+        if (!eventInfos) {
+          continue;
         }
 
-        this.processHandlerForEvent(node, events, dbg, eventInfo);
+        for (let eventInfo of eventInfos) {
+          if (normalizeHandler) {
+            eventInfo.normalizeHandler = normalizeHandler;
+          }
+
+          this.processHandlerForEvent(node, events, dbg, eventInfo);
+        }
+      } catch(e) {
+        // An object attached to the node looked like a listener but wasn't...
+        // do nothing.
       }

but we should display something better than a horizontal sliver of a pop-up label.

(Using catch in that way can also conceal coding errors in the code of the try block and anything it calls. It takes a lot more time to track down a coding error when you don't have a backtrace; when I find the catch (e) {}, I really feel like I've wasted my time. We should avoid this approach.)

Priority: -- → P3

@jimb I agree that we should avoid using empty catch blocks and totally understand your frustration but we have had so many different situations come up where this blows up that this seemed the safest thing to do.

This is a difficult one to solve because using an object as a listener is completely fine e.g.

var obj = {
  handleEvent(e) {
    console.log(this === obj);
  }
};

document.body.addEventListener('click', obj);

I'll take a look... maybe we can do this better.

If I remember rightly, the melon that broke the monkey's back was when I discovered that some event listeners are not even JS but are somehow C++.

I will add something to log a warning so we can at least see what went wrong.

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #2)

@jimb I agree that we should avoid using empty catch blocks and totally understand your frustration but we have had so many different situations come up where this blows up that this seemed the safest thing to do.

This is a difficult one to solve because using an object as a listener is completely fine e.g.

var obj = {
  handleEvent(e) {
    console.log(this === obj);
  }
};

document.body.addEventListener('click', obj);

I'll take a look... maybe we can do this better.

If I remember rightly, the melon that broke the monkey's back was when I discovered that some event listeners are not even JS but are somehow C++.

I will also add something to log a warning so we can at least see what went wrong.

Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED

@jimb Sorry, I am blocked by bug 1520823.

Attachment #9038015 - Attachment description: Bug 1519597 - Bad event handlers on DOM nodes produce bogus popups when inspected → Bug 1519597 - Bad event handlers on DOM nodes produce bogus popups when inspected r?ochameau
Attachment #9038015 - Attachment description: Bug 1519597 - Bad event handlers on DOM nodes produce bogus popups when inspected r?ochameau → Bug 1519597 - Bad event handlers on DOM nodes produce bogus popups when inspected r?ochameau!
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4afc95f9c7dd
Bad event handlers on DOM nodes produce bogus popups when inspected r=pbro,ochameau
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Whiteboard: [qa-66b-p2]
You need to log in before you can comment on or make changes to this bug.