Bug 167084

Summary: Web Inspector: capture async stack trace when workers/main context posts a message
Product: WebKit Reporter: Blaze Burg <bburg>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, buildbot, commit-queue, inspector-bugzilla-changes, keith_miller, mark.lam, mattbaker, msaboff, ryanhaddad, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 174738    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
[Image] postMessage stack trace
none
Patch
none
Patch
none
Patch none

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