Bug 167085 - Web Inspector: capture async stack trace when a Promise is created, resolved, rejected, or chained
Summary: Web Inspector: capture async stack trace when a Promise is created, resolved,...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-15 14:55 PST by Brian Burg
Modified: 2017-08-16 16:09 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.70 KB, patch)
2017-08-07 12:23 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2017-01-15 14:55:02 PST
This will be quite tricky. When a promise resolves, we want to see what caused it to become resolved in the previous tick. We may also want to see the backtrace for when the promise was created. So there may be some UI work to show both branches, or we may decide just one of these (likely, the resolver backtrace) is most important to show.
Comment 1 Radar WebKit Bug Importer 2017-01-15 14:55:14 PST
<rdar://problem/30033678>
Comment 2 Matt Baker 2017-08-07 12:23:18 PDT
Created attachment 317450 [details]
Patch
Comment 3 Matt Baker 2017-08-07 12:25:18 PDT
This is an early WIP. Two major issues:

1) InspectorInstrumentationObject its talking to the InspectorDebuggerAgent for the global object's environment.
2) The builtin code calls out to C++ land regardless of Inspector enabled state.
Comment 4 Joseph Pecoraro 2017-08-16 13:56:00 PDT
Comment on attachment 317450 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317450&action=review

I'd suggest writing in a comment what you expect the Debugger Sidebar's CallFrames to be for particular Promise scenarios.

For example this test case:

function doWork() {
    var promise = new Promise((resolve, reject) => {
        setTimeout(function createThen() { promise.then((value) => { debugger; }); });
        setTimeout(function resolvePromise() { resolve(999); });
    });
}

doWork();

> Source/JavaScriptCore/builtins/PromiseOperations.js:234
> +    @InspectorInstrumentation.didInitializePromise(this, executor);

I don't think you want to include `executor` here. It is also unused in the C++. `executor` is just the `new Promise(X)` X function required to access the resolve/reject resolving functions.

> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:110
> +    JSValue promise = exec->argument(0);

At this point, given you know it should be a

> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:114
> +    Inspector::JSGlobalObjectInspectorController& controller = exec->lexicalGlobalObject()->inspectorController();

I don't think you want to require REMOTE_INSPECTOR and go through the GlobalObject's InspectorController (which is the JSContext Inspector's Debugger Agent and not WebCore's Debugger Agent).

I think you should go through JSGlobalObject's JSC::Debugger like so:

    auto globalObject = exec->lexicalGlobalObject();
    auto debugger = globalObject->debugger();
    if (!debugger || !debugger->isInteractivelyDebugging())
        return;

    debugger->initializedPromise(promise);

The JSC::Debugger can then send messages out to DebuggerAgents through the ScriptDebugListener interface, and the right DebuggerAgent will be able to handle it.

This also early returns if breakpoints are not enabled (which I think is reasonable).
Comment 5 Joseph Pecoraro 2017-08-16 16:09:22 PDT
Comment on attachment 317450 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317450&action=review

>> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:110
>> +    JSValue promise = exec->argument(0);
> 
> At this point, given you know it should be a

I meant to say since you know it should be a JSPromise I think you should jsCast<JSPromise> at this point and pass on the nicely typed JSPromise and not an opaque JSValue.