Closed
Bug 1450201
Opened 6 years ago
Closed 6 years ago
Fix TypeError chrome://devtools/content/inspector/inspector.js in _onContextMenu e.originalTarget.closest is not a function
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
Tracking
(firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: pbro, Assigned: bharatraghunthan9767, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [nightly-js-sentry:2828621])
Attachments
(1 file)
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.
Whiteboard: [nightly-js-sentry:2828621]
Whiteboard: [nightly-js-sentry:2828621] → [nightly-js-sentry:2828621][nightly-js-sentry:3193844]
Whiteboard: [nightly-js-sentry:2828621][nightly-js-sentry:3193844] → [nightly-js-sentry:2828621]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8966095 -
Flags: review?(pbrosset)
Reporter | ||
Comment 2•6 years ago
|
||
mozreview-review |
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; } ```
Attachment #8966095 -
Flags: review?(pbrosset)
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → bharatraghunthan9767
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8966095 -
Flags: review?(jdescottes)
Assignee | ||
Updated•6 years ago
|
Attachment #8966095 -
Flags: review?(jdescottes) → review?(gl)
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8966095 [details] Bug 1450201 - Fix TypeError by processing the function only if DOMNodes https://reviewboard.mozilla.org/r/234840/#review242478
Attachment #8966095 -
Flags: review?(gl) → review+
Pushed by gabriel.luong@gmail.com: 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•6 years ago
|
||
Thanks for the patch Bharat. I have landed this for you.
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1256e4bb644c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•