Bug 146886 - Web Inspector: Source links in Event Listeners panel are missing for bound functions
Summary: Web Inspector: Source links in Event Listeners panel are missing for bound fu...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 139380 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-07-11 22:27 PDT by Nikita Vasilyev
Modified: 2016-12-13 15:35 PST (History)
4 users (show)

See Also:


Attachments
[Image] No source links (137.95 KB, image/png)
2015-07-11 22:27 PDT, Nikita Vasilyev
no flags Details
Partial Patch (901 bytes, patch)
2015-07-13 16:10 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2015-07-11 22:27:28 PDT
Created attachment 256677 [details]
[Image] No source links

We use Function.prototype.bind for event listener handlers universally in Web Inspector. Event Listeners panel without the source links is pretty useless.
Comment 1 Radar WebKit Bug Importer 2015-07-11 22:27:52 PDT
<rdar://problem/21784293>
Comment 2 Devin Rousso 2015-07-13 16:10:33 PDT
Created attachment 256743 [details]
Partial Patch

These code changes are part of a fix for this issue, but this will drill down to the original function and ignore all steps in between (like the source code location where the function is bound) and that may not actually be what we want.
Comment 3 Joseph Pecoraro 2015-07-13 16:30:55 PDT
After discussion with Devin, I think what we really want is for getFunctionDetails to provide the complete chain of bound functions. For instance a bound function's target function may itself be a bound function.

You could imagine a chain like this:

    1  function addArguments() {
    2    var sum = 0;
    3    for (var x of arguments) { sum += x; }
    4    return sum;
    5  }
    6  var foo = addArguments.bind(null, 1);
    7  var bar = foo.bind(null, 2);

Where logging `bar` has a chain like this:

    JSBoundFunction (name=bound bound addArguments, location=script.js:7, boundThis=null, boundArguments=[1], scope=...)
      JSBoundFunction (name=bound addArguments, location=script.js:6, boundThis=null, boundArguments=[2], scope=...)
        JSFunction (name=addArguments, location=script.js:1, scope=...)

The frontend should probably get a list of function details about each of these functions in the chain and decide how to display that information. You could think of this as a call stack for the creation of this function and each of its pieces.

Note this chain is always one directional, it cannot be cyclic.

-----

The same data can be useful for backtraces which include bound functions.

For example this test:

    <script>
    function foo() { bar.bind(null)(); }
    function bar() { ({}).x.x; }
    foo();
    </script>

Currently Produces:

    [Error] TypeError: undefined is not an object (evaluating '({}).x.x')
    	bar (test.html:2)
    	bar
    	foo (test.html:1)
    	(anonymous function) (test.html:3)

There are a couple things wrong with this. It should produce:

    [Error] TypeError: undefined is not an object (evaluating '({}).x.x')
    	bar (test.html:2)
    	bound bar (test.html:1)
    	foo (test.html:1)
    	(global code) (test.html:3)

Note the "bound bar" being the location of the .bind() invocation. It seems JavaScriptCore is not stashing the location this function was created.

-----

So I think in fixing this we should consider consistent displaying of Functions given that any function may be bound and have a chain of origin.
Comment 4 Nikita Vasilyev 2015-07-14 09:06:16 PDT
Having bound functions in backtraces would be great!

As for the UI for the Event Listener panel, we should incorporate stacktrace popovers (https://bugs.webkit.org/show_bug.cgi?id=143862) once we have them.

I think, having a link to the original function is a desired behavior. Hovering over that link should bring a stacktrace with bound functions along with regular call frames.
Comment 5 Nikita Vasilyev 2015-07-22 22:30:56 PDT
*** Bug 139380 has been marked as a duplicate of this bug. ***
Comment 6 Nikita Vasilyev 2015-08-06 20:52:17 PDT
(In reply to comment #2)
> Created attachment 256743 [details]
> Partial Patch
> 
> These code changes are part of a fix for this issue, but this will drill
> down to the original function and ignore all steps in between (like the
> source code location where the function is bound) and that may not actually
> be what we want.

Can we land Devin's patch for now and implement everything you described a bit later? Seeing "(anonymous function)" for every bound function is really not helpful.
Comment 7 Joseph Pecoraro 2015-08-07 11:20:00 PDT
(In reply to comment #6)
> (In reply to comment #2)
> > Created attachment 256743 [details]
> > Partial Patch
> > 
> > These code changes are part of a fix for this issue, but this will drill
> > down to the original function and ignore all steps in between (like the
> > source code location where the function is bound) and that may not actually
> > be what we want.
> 
> Can we land Devin's patch for now and implement everything you described a
> bit later? Seeing "(anonymous function)" for every bound function is really
> not helpful.

I think fixing this correctly is not that hard, someone just has to do it. The existing patch is an excellent starting point. I think the source location where the function is bound is equally as important as knowing which function it was bound to.