Bug 167084 - Web Inspector: capture async stack trace when workers/main context posts a message
Summary: Web Inspector: capture async stack trace when workers/main context posts a me...
Status: RESOLVED FIXED
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: 174738
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-15 14:52 PST by BJ Burg
Modified: 2017-08-16 16:15 PDT (History)
11 users (show)

See Also:


Attachments
Patch (18.11 KB, patch)
2017-07-23 22:46 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Image] postMessage stack trace (314.16 KB, image/png)
2017-07-23 22:51 PDT, Matt Baker
no flags Details
Patch (21.19 KB, patch)
2017-07-31 17:25 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (20.99 KB, patch)
2017-08-04 12:10 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (21.55 KB, patch)
2017-08-16 12:37 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 BJ Burg 2017-01-15 14:52:37 PST
This will require some UI work, since separate halves of the stack trace will be in two (or more, if posting a message while handling a dispatched message) contexts.
Comment 1 Radar WebKit Bug Importer 2017-01-15 14:52:56 PST
<rdar://problem/30033673>
Comment 2 Matt Baker 2017-07-23 22:46:16 PDT
Created attachment 316266 [details]
Patch
Comment 3 Matt Baker 2017-07-23 22:51:21 PDT
Created attachment 316268 [details]
[Image] postMessage stack trace
Comment 4 BJ Burg 2017-07-24 10:58:29 PDT
Comment on attachment 316266 [details]
Patch

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

I'm going to mark this as r- for now since the error cases are not tested. There may be some churn to address comments in dependent patches. Other than those two things, this change looks good.

> Source/JavaScriptCore/ChangeLog:8
> +

Nit: created

> Source/WebCore/inspector/InspectorInstrumentation.h:129
> +    static void didPostMessage(Frame*, TimerBase&, JSC::ExecState&);

Please make this name match the other method in the pair: didDispatchPostMessage

> Source/WebCore/inspector/PageDebuggerAgent.cpp:207
> +

I don't know much about this didFailPostMessage case. Does this only happen because of a security violation? What if you detach the iframe synchronously after posting a message?

> LayoutTests/inspector/debugger/async-stack-trace.html:51
> +    });

You need to test the case where the postMessage fails.
Comment 5 Matt Baker 2017-07-31 17:25:26 PDT
Created attachment 316820 [details]
Patch
Comment 6 BJ Burg 2017-08-04 11:31:21 PDT
Comment on attachment 316820 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 2017-08-04 11:32:36 PDT
Comment on attachment 316820 [details]
Patch

Rejecting attachment 316820 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 316820, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
debugger/async-stack-trace.html
Hunk #2 FAILED at 96.
1 out of 4 hunks FAILED -- saving rejects to file LayoutTests/inspector/debugger/async-stack-trace.html.rej
patching file LayoutTests/inspector/debugger/resources/postMessage-echo.html
patching file LayoutTests/inspector/dom-debugger/dom-breakpoints.html
Hunk #1 succeeded at 36 with fuzz 1.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Brian Burg']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4254271
Comment 8 Matt Baker 2017-08-04 12:10:46 PDT
Created attachment 317264 [details]
Patch
Comment 9 WebKit Commit Bot 2017-08-04 15:12:44 PDT
Comment on attachment 317264 [details]
Patch

Clearing flags on attachment: 317264

Committed r220299: <http://trac.webkit.org/changeset/220299>
Comment 10 WebKit Commit Bot 2017-08-04 15:12:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Ryan Haddad 2017-08-06 20:37:39 PDT
(In reply to WebKit Commit Bot from comment #9)
> Comment on attachment 317264 [details]
> Patch
> 
> Clearing flags on attachment: 317264
> 
> Committed r220299: <http://trac.webkit.org/changeset/220299>

This change caused inspector/dom-debugger/dom-breakpoints.html to fail:

+!! EXCEPTION: Can't find variable: awaitEvaluateInPage
+Stack Trace: #0: (anonymous) (unknown)
+#1: promiseReactionJob [native code]

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r220299%20(2825)/results.html
Comment 12 Ryan Haddad 2017-08-07 09:44:16 PDT
Reverted r220299 for reason:

This change caused LayoutTest inspector/dom-debugger/dom-breakpoints.html to fail.

Committed r220342: <http://trac.webkit.org/changeset/220342>
Comment 13 Matt Baker 2017-08-16 12:37:40 PDT
Created attachment 318282 [details]
Patch
Comment 14 WebKit Commit Bot 2017-08-16 16:15:36 PDT
Comment on attachment 318282 [details]
Patch

Clearing flags on attachment: 318282

Committed r220815: <http://trac.webkit.org/changeset/220815>
Comment 15 WebKit Commit Bot 2017-08-16 16:15:38 PDT
All reviewed patches have been landed.  Closing bug.