Bug 146886 – Web Inspector: Source links in Event Listeners panel are missing for bound functions

• Status: NEW
• Component: Web Inspector

[Nikita Vasilyev]

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.

[Radar WebKit Bug Importer]

<rdar://problem/21784293>

[Devin Rousso]

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.

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

*** Bug 139380 has been marked as a duplicate of this bug. ***

[Nikita Vasilyev]

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

[Joseph Pecoraro]

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