Closed
Bug 1268538
Opened 9 years ago
Closed 8 years ago
Event listener popup can be empty if the listeners are native code
Categories
(DevTools :: Inspector, defect, P1)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: miker)
References
Details
(Whiteboard: [btpp-fix-now])
Attachments
(2 files)
STEPS TO REPRODUCE:
1) Load attached testcase.
2) Open the devtools and select inspector.
3) Click the "ev" thing next to the <img> in the inspector.
EXPECTED RESULTS: Show something about the listener. Or not have the "ev" button at all, but that's less desirable.
ACTUAL RESULTS: Empty popup shown.
ADDITIONAL INFORMATION:
Note that I ran into this pattern on an actual page.
I experimented a bit and it looks like bound page-provided functions work correctly, but "native functions", whether bound or not, do not. For example, you get similar fail if you add Array.prototype.sort, whether bound or not, as the listener.
mikeratcliffe, I was told on IRC this is something I should ask you about.
Flags: needinfo?(mratcliffe)
Assignee | ||
Comment 1•9 years ago
|
||
When native functions were used as handlers we were showing an empty event popup. We now display the following:
function() {
[native code]
}
Review commit: https://reviewboard.mozilla.org/r/49563/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49563/
Assignee | ||
Updated•9 years ago
|
Attachment #8746779 -
Flags: review?(pbrosset)
Assignee | ||
Comment 2•9 years ago
|
||
Flags: needinfo?(mratcliffe)
Comment 3•9 years ago
|
||
Comment on attachment 8746779 [details]
MozReview Request: Bug 1268538 - Event listener popup can be empty if the listeners are from native code r?pbrosset
https://reviewboard.mozilla.org/r/49563/#review46397
::: devtools/client/inspector/markup/test/browser_markup_events3.js:162
(Diff revision 1)
> + {
> + selector: "#promise",
> + expected: [
> + {
> + type: "click",
> + filename: ":0",
Why :0 here?
I have tested this locally, and indeed, this is what appears in the popup, isntead of the URL to the HTML page.
If I add other event listeners in the HTML page, then I see the URL with the correct line number.
If we just can't retrieve it, I would advise hiding this information altogether. Just showing :0 is confusing I think.
I'd be way better if we could show the right url:lineNumber though, can this be done?
I found out that this causes another problem: clicking on the debugger button doesn't lead to the right script and line number in this case. If you try this on a test page that has other external resources, you will see, when the debugger opens after you've clicked the debugger button in the event popup, that the right script isn't being selected.
::: devtools/server/actors/inspector.js:602
(Diff revision 1)
> - let line = script.startLine;
> - let url = script.url;
> + let line = script ? script.startLine : 0;
> + let url = script ? script.url : "";
So if we don't have a line number and script for a listener, I think we should send null (or something) to the UI so it knows not to display this information at all.
I'd rather the UI not try to link to the debugger at all than link to an incorrect location.
But perhaps the listener API we use could be improved to return the right information in this case?
Attachment #8746779 -
Flags: review?(pbrosset)
Reporter | ||
Comment 4•9 years ago
|
||
> I'd be way better if we could show the right url:lineNumber though, can this be done?
You can certainly show the URL of the page the function comes from, assuming it comes from a page and not from chrome.
Builtin functions don't have a line number, obviously. But they _do_ sometimes have a useful .name...
> But perhaps the listener API we use could be improved to return the right information in this case?
Can you define "right information"? What is the right information when the listener is Array.prototype.sort?
Comment 5•9 years ago
|
||
Bug triage (filter on CLIMBING SHOES).
Priority: -- → P1
Whiteboard: [btpp-fix-now]
Updated•9 years ago
|
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Comment 6•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #4)
> > I'd be way better if we could show the right url:lineNumber though, can this be done?
>
> You can certainly show the URL of the page the function comes from, assuming
> it comes from a page and not from chrome.
>
> Builtin functions don't have a line number, obviously. But they _do_
> sometimes have a useful .name...
>
> > But perhaps the listener API we use could be improved to return the right information in this case?
>
> Can you define "right information"? What is the right information when the
> listener is Array.prototype.sort?
Hmm, you're right, there's no "right" information here I guess.
I think for built-in functions, the next best thing devtools could link to is the location where the event listener is added.
In the testcase, I'd probably expect the debugger to open at this location:
img.addEventListener("load", resolve);
Reporter | ||
Comment 7•9 years ago
|
||
> the location where the event listener is added
Hmm. I was going to say that we don't store that, but I guess we sort of do when async stacks are enabled. That's not the default release configuration, though...
Assignee | ||
Updated•8 years ago
|
Summary: Event listener popup can be empty if the listeners are not page-provided functions (e.g. Promise callbacks), which is a bit confusing → Event listener popup can be empty if the listeners are native code
Assignee | ||
Comment 8•8 years ago
|
||
I have taken some time looking into this.
- We do not store the location that listeners are added although this would be very useful.
- We should hide the debugger icon when code is not reachable.
- We should always avoid having line number 0. We need to bare in mind that this applies to native code event listeners and DOM0 event listeners (added inside an html page).
- There are numerous bugs involving issues where we get the wrong / empty source. We shouldn't use the script itself to get the source. We can use listenerDO.isArrowFunction() and it's friends to display appropriate source... not sure if parameter names are available though.
Comment 9•8 years ago
|
||
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #8)
> I have taken some time looking into this.
>
> - We do not store the location that listeners are added although this would
> be very useful.
If you mean the location at which addEventListener is called -- we do store it,
but it isn't accessible.
It is stored as the async stack parent here:
https://dxr.mozilla.org/mozilla-central/source/dom/bindings/CallbackObject.h#260
Maybe it would be possible to add some devtools-only function to extract this.
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 10•8 years ago
|
||
Fixed by bug 1315639
Assignee | ||
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•