1450201 - Fix TypeError chrome://devtools/content/inspector/inspector.js in _onContextMenu e.originalTarget.closest is not a function

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Patrick Brosset <:pbro>]

We started experimenting with Sentry to collect JS errors on nightly, and one such error that seems to have occurred in the past is:

TypeError chrome://devtools/content/inspector/inspector.js in _onContextMenu
e.originalTarget.closest is not a function

  _onContextMenu: function (e) {
    if (e.originalTarget.closest("input[type=text]") ||
        e.originalTarget.closest("input:not([type])") ||
        e.originalTarget.closest("textarea")) {
      return;
    }

It looks like it is possible for e.originalTarget not to be a DOMNode. Maybe it is a document node? Or a text node?

It would be good to avoid this error altogether and just guard against this case by bailing out.

Comment 1

[Bharat Raghunathan]

Attachment: Bug 1450201 - Fix TypeError by processing the function only if DOMNodes

Review commit: https://reviewboard.mozilla.org/r/234840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234840/

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8966095 [details]
Bug 1450201 - Fix TypeError by processing the function only if DOMNodes

https://reviewboard.mozilla.org/r/234840/#review240498

Looks great. Thank you for picking this up. Just a couple of minor comments that would be good to address before checking this in if you can.

::: devtools/client/inspector/inspector.js
(Diff revision 1)
>      if (content && content.trim().length > 0) {
>        return content;
>      }
>      return null;
>    },
> -

nit: looks like you removed an empty line between these 2 functions. Could you please add it back?

::: devtools/client/inspector/inspector.js:1408
(Diff revision 1)
>      }
>      return null;
>    },
> -
>    _onContextMenu: function(e) {
> +    if (e.originalTarget instanceof Element) {

In the DevTools code base, we usually prefer inverting conditions like this to avoid indenting the whole body of the function and make exit conditions more obvious (also in this case you can combine it with the other conditions):

```
// Bail out of the target isn't an element or isn't the right element.
if (!(e.originalTarget instanceof Element) ||
    e.originalTarget.closest("input[type=text]") ||
    e.originalTarget.closest("input:not([type])") ||
    e.originalTarget.closest("textarea")) {
  return;
}
```

Comment 3

[Bharat Raghunathan]

Comment on attachment 8966095 [details]
Bug 1450201 - Fix TypeError by processing the function only if DOMNodes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234840/diff/1-2/

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8966095 [details]
Bug 1450201 - Fix TypeError by processing the function only if DOMNodes

https://reviewboard.mozilla.org/r/234840/#review242478

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1256e4bb644c
Fix TypeError by processing the function only if it is a DOMNodes. r=gl

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Thanks for the patch Bharat. I have landed this for you.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1256e4bb644c