1268538 - Event listener popup can be empty if the listeners are native code

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

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.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: MozReview Request: Bug 1268538 - Event listener popup can be empty if the listeners are from native code r?pbrosset

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/

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d96451cac245

Comment 3

[Patrick Brosset <:pbro>]

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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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

[Patrick Brosset <:pbro>]

Bug triage (filter on CLIMBING SHOES).

Comment 6

[Patrick Brosset <:pbro>]

(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);

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

[Tom Tromey :tromey]

(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.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Fixed by bug 1315639