Bug 156685

Summary:

Hook up ShadowChicken to the debugger to show tail deleted frames

Product:

WebKit

Reporter:

Saam Barati <saam>

Component:

JavaScriptCore

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

agomez, bburg, benjamin, buildbot, commit-queue, fpizlo, ggaren, gskachkov, joepeck, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, oliver, rniwa, ryanhaddad, sukolsak, timothy, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
WIP	none
WIP	none
WIP	buildbot: commit-queue-
Archive of layout-test-results from ews115 for mac-yosemite	none
Archive of layout-test-results from ews104 for mac-yosemite-wk2	none
WIP	buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-yosemite-wk2	none
Archive of layout-test-results from ews112 for mac-yosemite	none
Archive of layout-test-results from ews101 for mac-yosemite	none
WIP	none
WIP	none
WIP	none
WIP	buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite	none
Archive of layout-test-results from ews117 for mac-yosemite	none
Archive of layout-test-results from ews104 for mac-yosemite-wk2	none
WIP	none
patch	none
patch	none
patch	buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-yosemite-wk2	none
Archive of layout-test-results from ews114 for mac-yosemite	none
Archive of layout-test-results from ews101 for mac-yosemite	none
patch	fpizlo: review+
patch for landing	none
what it looks like in inspector	none
patch for landing	none

Saam Barati

Reported 2016-04-17 18:58:00 PDT

...

Attachments
WIP (17.26 KB, patch) 2016-05-09 17:55 PDT, Saam Barati	no flags	Details Formatted Diff Diff
WIP (75.95 KB, patch) 2016-05-12 18:16 PDT, Saam Barati	no flags	Details Formatted Diff Diff
WIP (75.11 KB, patch) 2016-05-12 18:41 PDT, Saam Barati	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews115 for mac-yosemite (1.76 MB, application/zip) 2016-05-12 19:49 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.01 MB, application/zip) 2016-05-12 19:53 PDT, Build Bot	no flags	Details
WIP (76.14 KB, patch) 2016-05-12 20:04 PDT, Saam Barati	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.04 MB, application/zip) 2016-05-12 21:17 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews112 for mac-yosemite (2.03 MB, application/zip) 2016-05-12 21:27 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews101 for mac-yosemite (1.56 MB, application/zip) 2016-05-12 22:31 PDT, Build Bot	no flags	Details
WIP (77.10 KB, patch) 2016-05-13 11:17 PDT, Saam Barati	no flags	Details Formatted Diff Diff
WIP (83.85 KB, patch) 2016-05-13 14:22 PDT, Saam Barati	no flags	Details Formatted Diff Diff
WIP (93.16 KB, patch) 2016-05-13 19:19 PDT, Saam Barati	no flags	Details Formatted Diff Diff
WIP (123.84 KB, patch) 2016-05-15 16:49 PDT, Saam Barati	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-yosemite (1.15 MB, application/zip) 2016-05-15 17:53 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews117 for mac-yosemite (2.16 MB, application/zip) 2016-05-15 18:24 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.46 MB, application/zip) 2016-05-15 18:46 PDT, Build Bot	no flags	Details
WIP (123.84 KB, patch) 2016-05-15 18:48 PDT, Saam Barati	no flags	Details Formatted Diff Diff
patch (144.22 KB, patch) 2016-05-16 00:49 PDT, Saam Barati	no flags	Details Formatted Diff Diff
patch (144.34 KB, patch) 2016-05-16 00:56 PDT, Saam Barati	no flags	Details Formatted Diff Diff
patch (144.34 KB, patch) 2016-05-16 01:00 PDT, Saam Barati	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (944.03 KB, application/zip) 2016-05-16 02:01 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews114 for mac-yosemite (1.45 MB, application/zip) 2016-05-16 02:06 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews101 for mac-yosemite (797.79 KB, application/zip) 2016-05-16 02:12 PDT, Build Bot	no flags	Details
patch (144.31 KB, patch) 2016-05-16 09:26 PDT, Saam Barati	fpizlo: review+	Details Formatted Diff Diff
patch for landing (146.77 KB, patch) 2016-05-16 14:44 PDT, Saam Barati	no flags	Details Formatted Diff Diff
what it looks like in inspector (134.89 KB, image/png) 2016-05-16 15:01 PDT, Saam Barati	no flags	Details
patch for landing (146.81 KB, patch) 2016-05-16 15:52 PDT, Saam Barati	no flags	Details Formatted Diff Diff
Show Obsolete (25) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2016-04-17 18:58:16 PDT

<rdar://problem/25770521>

Saam Barati

Comment 2 2016-05-09 17:55:19 PDT

Created attachment 278465 [details] WIP it shows tail deleted frames in the inspector, but it's hacky.

WebKit Commit Bot

Comment 3 2016-05-09 18:34:45 PDT

Attachment 278465 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.h:46: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.h:47: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:103: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:104: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:121: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:122: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:141: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:153: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:164: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:214: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 10 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Joseph Pecoraro

Comment 4 2016-05-10 01:52:54 PDT

Comment on attachment 278465 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=278465&action=review > Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:152 > + return makeString(func->name(), " - tail deleted"); Ideally the frontend can provide a proper (localized) string where possible. So probably func->name() and empty string if we couldn't get a function name. The frontend can then handle both. > Source/JavaScriptCore/inspector/JSJavaScriptCallFramePrototype.cpp:194 > + ASSERT_GC_OBJECT_INHERITS(castedThis, JSJavaScriptCallFrame::info()); We can drop this ASSERT (the dynamic cast above makes it unnecessary). I'm removing it from all the others in bug 157510.

Timothy Hatcher

Comment 5 2016-05-10 10:05:10 PDT

Comment on attachment 278465 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=278465&action=review > Source/WebInspectorUI/UserInterface/Views/CallFrameIcons.css:44 > +.tail-deleted .icon { > + content: url(../Images/HeapSnapshotDiff.svg); > +} This isn't a good icon to use. That icon is designed for navigation bars. I would suggest just using the normal function icon, can giving it opacity: 0.7 or so. > Source/WebInspectorUI/UserInterface/Views/CallFrameTreeElement.js:48 > this._callFrame = callFrame; > + if (callFrame.isTailDeleted) { Newline between lines. > Source/WebInspectorUI/UserInterface/Views/CallFrameView.css:72 > + color:gray; Space after :. hsla(0, 0%, 0%, 0.5) would be better, so it works on any background effectively.

Timothy Hatcher

Comment 6 2016-05-10 10:06:53 PDT

Comment on attachment 278465 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=278465&action=review >> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:152 >> + return makeString(func->name(), " - tail deleted"); > > Ideally the frontend can provide a proper (localized) string where possible. So probably func->name() and empty string if we couldn't get a function name. The frontend can then handle both. Yeah, I would prefer the front-end to use the isTailDeleted flag to provide these names. > Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:154 > + return "<tail-deleted>"; Ditto.

Saam Barati

Comment 7 2016-05-12 18:12:40 PDT

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

Saam Barati

Comment 8 2016-05-12 18:16:13 PDT

Created attachment 278793 [details] WIP

Saam Barati

Comment 9 2016-05-12 18:25:18 PDT

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

Saam Barati

Comment 10 2016-05-12 18:41:33 PDT

Created attachment 278797 [details] WIP rebased

WebKit Commit Bot

Comment 11 2016-05-12 18:42:32 PDT

Attachment 278797 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.h:51: The parameter name "callFrame" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/CCallHelpers.cpp:37: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/jit/CCallHelpers.cpp:52: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:119: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:200: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:349: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:373: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:396: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:422: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:428: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:431: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:435: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:438: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:444: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:470: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:471: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:477: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:81: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:175: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:299: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:616: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:704: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/Debugger.cpp:80: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 23 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 12 2016-05-12 19:49:42 PDT

Comment on attachment 278797 [details] WIP Attachment 278797 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1312459 Number of test failures exceeded the failure limit.

Build Bot

Comment 13 2016-05-12 19:49:47 PDT

Created attachment 278804 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 14 2016-05-12 19:52:59 PDT

Comment on attachment 278797 [details] WIP Attachment 278797 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1312475 New failing tests: inspector/debugger/hit-breakpoint-from-console.html inspector/debugger/setBreakpoint-options-exception.html inspector/debugger/breakpoint-condition-with-exception.html inspector/debugger/setBreakpoint-actions.html inspector/debugger/breakpoint-condition-with-bad-script.html inspector/debugger/setBreakpoint-condition.html inspector/debugger/breakpoint-inside-conditons-and-actions.html inspector/debugger/breakpoint-condition-detach.html

Build Bot

Comment 15 2016-05-12 19:53:04 PDT

Created attachment 278805 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Saam Barati

Comment 16 2016-05-12 20:04:50 PDT

Created attachment 278806 [details] WIP

WebKit Commit Bot

Comment 17 2016-05-12 20:06:50 PDT

Attachment 278806 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.h:51: The parameter name "callFrame" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/CCallHelpers.cpp:37: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/jit/CCallHelpers.cpp:52: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:119: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:200: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:341: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:347: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:358: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:382: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:392: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:401: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:410: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:436: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:442: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:445: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:449: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:452: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:458: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:484: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:485: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:491: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:81: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:175: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:299: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:616: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:704: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/Debugger.cpp:80: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 27 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 18 2016-05-12 21:17:47 PDT

Comment on attachment 278806 [details] WIP Attachment 278806 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1312775 New failing tests: inspector/debugger/hit-breakpoint-from-console.html inspector/debugger/setBreakpoint-options-exception.html inspector/debugger/breakpoint-condition-with-exception.html inspector/debugger/setBreakpoint-actions.html inspector/debugger/breakpoint-condition-with-bad-script.html inspector/debugger/setBreakpoint-condition.html inspector/debugger/breakpoint-inside-conditons-and-actions.html inspector/debugger/breakpoint-condition-detach.html

Build Bot

Comment 19 2016-05-12 21:17:52 PDT

Created attachment 278810 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Build Bot

Comment 20 2016-05-12 21:27:10 PDT

Comment on attachment 278806 [details] WIP Attachment 278806 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1312780 New failing tests: inspector/debugger/hit-breakpoint-from-console.html inspector/debugger/setBreakpoint-options-exception.html inspector/debugger/breakpoint-condition-with-exception.html inspector/debugger/setBreakpoint-actions.html inspector/debugger/breakpoint-condition-with-bad-script.html inspector/debugger/setBreakpoint-condition.html inspector/debugger/breakpoint-inside-conditons-and-actions.html inspector/debugger/breakpoint-condition-detach.html

Build Bot

Comment 21 2016-05-12 21:27:14 PDT

Created attachment 278811 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 22 2016-05-12 22:31:01 PDT

Comment on attachment 278806 [details] WIP Attachment 278806 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1313051 New failing tests: inspector/debugger/hit-breakpoint-from-console.html inspector/debugger/setBreakpoint-options-exception.html inspector/debugger/breakpoint-condition-with-exception.html inspector/debugger/setBreakpoint-actions.html inspector/debugger/breakpoint-condition-with-bad-script.html inspector/debugger/setBreakpoint-condition.html inspector/debugger/breakpoint-inside-conditons-and-actions.html inspector/debugger/breakpoint-condition-detach.html

Build Bot

Comment 23 2016-05-12 22:31:05 PDT

Created attachment 278819 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Saam Barati

Comment 24 2016-05-13 11:17:08 PDT

Created attachment 278853 [details] WIP

WebKit Commit Bot

Comment 25 2016-05-13 11:20:33 PDT

Attachment 278853 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.h:51: The parameter name "callFrame" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/CCallHelpers.cpp:37: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/jit/CCallHelpers.cpp:52: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:119: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:200: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:341: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:347: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:358: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:382: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:392: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:401: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:410: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:436: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:442: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:445: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:449: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:452: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:458: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:490: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:491: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:497: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:81: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:175: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:299: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:626: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:714: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/debugger/Debugger.cpp:80: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 27 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 26 2016-05-13 14:22:33 PDT

Created attachment 278867 [details] WIP some 32-bit code

WebKit Commit Bot

Comment 27 2016-05-13 14:24:48 PDT

Attachment 278867 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.h:51: The parameter name "callFrame" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:119: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:200: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:341: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:347: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:358: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:382: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:392: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:401: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:410: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:436: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:442: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:445: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:449: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:452: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:458: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:490: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:491: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:497: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 19 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 28 2016-05-13 19:19:29 PDT

Created attachment 278910 [details] WIP

Saam Barati

Comment 29 2016-05-15 16:49:39 PDT

Created attachment 278984 [details] WIP Just about done now. Just need to write a changelog, rebase a shadow chicken test, run debug build tests, run 32-bit tests.

WebKit Commit Bot

Comment 30 2016-05-15 16:52:11 PDT

Attachment 278984 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.h:50: The parameter name "callFrame" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:75: 'currentParent' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 2 in 54 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 31 2016-05-15 17:53:37 PDT

Comment on attachment 278984 [details] WIP Attachment 278984 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1327683 New failing tests: inspector/codemirror/prettyprinting-css.html inspector/console/console-table.html inspector/model/scope-chain-node.html

Build Bot

Comment 32 2016-05-15 17:53:42 PDT

Created attachment 278985 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 33 2016-05-15 18:24:19 PDT

Comment on attachment 278984 [details] WIP Attachment 278984 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1327744 New failing tests: inspector/debugger/setBreakpoint.html inspector/debugger/break-in-constructor-before-super.html inspector/debugger/setBreakpoint-autoContinue.html inspector/debugger/breakpoint-eval-with-exception.html inspector/console/console-table.html inspector/debugger/removeBreakpoint.html inspector/debugger/call-frame-this-host.html inspector/debugger/tail-recursion.html inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html inspector/debugger/breakpoint-scope.html inspector/debugger/debugger-statement.html inspector/debugger/call-frame-this-nonstrict.html

Build Bot

Comment 34 2016-05-15 18:24:23 PDT

Created attachment 278988 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 35 2016-05-15 18:46:04 PDT

Comment on attachment 278984 [details] WIP Attachment 278984 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1327894 New failing tests: inspector/debugger/tail-recursion.html inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html

Build Bot

Comment 36 2016-05-15 18:46:09 PDT

Created attachment 278991 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Saam Barati

Comment 37 2016-05-15 18:48:16 PDT

Created attachment 278992 [details] WIP

WebKit Commit Bot

Comment 38 2016-05-15 18:51:11 PDT

Attachment 278992 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.h:50: The parameter name "callFrame" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:75: 'currentParent' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 2 in 54 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 39 2016-05-16 00:49:00 PDT

Created attachment 279004 [details] patch

WebKit Commit Bot

Comment 40 2016-05-16 00:51:57 PDT

Attachment 279004 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:75: 'currentParent' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 1 in 62 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 41 2016-05-16 00:56:58 PDT

Created attachment 279006 [details] patch

WebKit Commit Bot

Comment 42 2016-05-16 00:59:50 PDT

Attachment 279006 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:75: 'currentParent' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 1 in 62 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 43 2016-05-16 01:00:26 PDT

Created attachment 279007 [details] patch

WebKit Commit Bot

Comment 44 2016-05-16 01:02:51 PDT

Attachment 279007 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:75: 'currentParent' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 1 in 62 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 45 2016-05-16 02:01:42 PDT

Comment on attachment 279007 [details] patch Attachment 279007 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1329447 New failing tests: inspector/debugger/tail-deleted-frames-this-value.html

Build Bot

Comment 46 2016-05-16 02:01:48 PDT

Created attachment 279011 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Build Bot

Comment 47 2016-05-16 02:06:47 PDT

Comment on attachment 279007 [details] patch Attachment 279007 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1329446 New failing tests: inspector/debugger/tail-deleted-frames-this-value.html

Build Bot

Comment 48 2016-05-16 02:06:52 PDT

Created attachment 279012 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 49 2016-05-16 02:12:24 PDT

Comment on attachment 279007 [details] patch Attachment 279007 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1329480 New failing tests: inspector/debugger/tail-deleted-frames-this-value.html

Build Bot

Comment 50 2016-05-16 02:12:29 PDT

Created attachment 279014 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Saam Barati

Comment 51 2016-05-16 09:26:22 PDT

Created attachment 279021 [details] patch

WebKit Commit Bot

Comment 52 2016-05-16 09:28:30 PDT

Attachment 279021 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:75: 'currentParent' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 1 in 62 files If any of these errors are false positives, please file a bug against check-webkit-style.

Timothy Hatcher

Comment 53 2016-05-16 10:32:28 PDT

Comment on attachment 279021 [details] patch The inspector parts look fine to me.

Filip Pizlo

Comment 54 2016-05-16 10:35:43 PDT

I think this looks good, but I want to read this some more before r+ing.

Mark Lam

Comment 55 2016-05-16 10:36:22 PDT

I'm reading it too.

Mark Lam

Comment 56 2016-05-16 11:16:42 PDT

Comment on attachment 279021 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279021&action=review > Source/JavaScriptCore/ChangeLog:29 > + keep around at most 128 "shadow" frames that wouldn't have naturally been kept I presume the 128 shadow frames will be the last (aka top) 128 frames. Can you say something here to indicate that explicitly? > Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:77 > + for (auto iter = frames.rbegin(), end = frames.rend(); iter != end; ++iter) { why "rbegin" and "rend" instead of "begin" and "end"? > Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:102 > ASSERT(isValid()); > if (!isValid()) > - return 0; > - > - if (m_caller) > - return m_caller; > - > - FindCallerMidStackFunctor functor(m_callFrame); > - m_callFrame->vm().topCallFrame->iterate(functor); > - > - CallFrame* callerFrame = functor.getCallerFrame(); > - if (!callerFrame) > return nullptr; > > - m_caller = DebuggerCallFrame::create(callerFrame); > return m_caller; I'm a little concerned with us eagerly creating the entire stack on instantiation. I recall the Inspector used to have issues with "hanging" while waiting for the stack of frames to be created when it is debugging a deeply recursive program. I think I made the creation of the caller DebuggerCallFrame incremental like this to specifically mitigate that issue. However, I don't know if the Inspector side was ever changed to not ask for the whole stack in one shot. Ideally, the Inspector will only ask for say the top 20 frames, and leave the rest wait till the user actually asks for them. Is there a reason why we cannot continue to only create the caller as needed? > Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:147 > return 0; Please change this to return nullptr as well like you did for the others above. > Source/JavaScriptCore/debugger/DebuggerCallFrame.h:75 > + JS_EXPORT_PRIVATE TextPosition currentPosition(); > JS_EXPORT_PRIVATE static TextPosition positionForCallFrame(CallFrame*); I suspect that the JS_EXPORT_PRIVATE was only needed way back when the Inspector was still in WebCore. A quick grep of the code seems to suggest that the JS_EXPORT_PRIVATE is no longer needed for these. Can you check? Ditto for other methods for JS_EXPORT_PRIVATE as well. Or we can remove these in a subsequent patch.

Filip Pizlo

Comment 57 2016-05-16 11:47:36 PDT

Comment on attachment 279021 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279021&action=review I like it! >> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:77 >> + for (auto iter = frames.rbegin(), end = frames.rend(); iter != end; ++iter) { > > why "rbegin" and "rend" instead of "begin" and "end"? I think it would be easier to use an index-based loop here: for (unsigned i = frames.size(); i--;)

Joseph Pecoraro

Comment 58 2016-05-16 12:21:46 PDT

Comment on attachment 279021 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279021&action=review Inspector parts look good to me! > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1981 > + DC17E8181C9C91D9008A6AB3 /* ShadowChicken.h in Headers */ = {isa = PBXBuildFile; fileRef = DC17E8141C9C7FD4008A6AB3 /* ShadowChicken.h */; settings = {ATTRIBUTES = (Private, ); }; }; > + DC17E8191C9C91DB008A6AB3 /* ShadowChickenInlines.h in Headers */ = {isa = PBXBuildFile; fileRef = DC17E8151C9C7FD4008A6AB3 /* ShadowChickenInlines.h */; settings = {ATTRIBUTES = (Private, ); }; }; Is this needed? Where is ShadowChicken used outside out JSC? Or is it a transitive include? > Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:107 > + return scope()->globalObject()->globalExec(); scope() can return nullptr if !isValid(). Perhaps we should guard or ASSERT that here. > Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:136 > + if (JSFunction* func = jsDynamicCast<JSFunction*>(m_shadowChickenFrame.callee)) > + return func->name(); I think this can use JSFunction::calculatedDisplayName to get the "friendlyFunctionName". It appears to just need an ExecState for the VM. >> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:147 >> return 0; > > Please change this to return nullptr as well like you did for the others above. Nit: Drive-by make nullptr? > Source/JavaScriptCore/debugger/DebuggerCallFrame.h:81 > + explicit DebuggerCallFrame(CallFrame*, const ShadowChicken::Frame&); Nit: Explicit is not needed if there is more then one parameter. > Source/WebCore/ForwardingHeaders/interpreter/ShadowChicken.h:4 > +#ifndef WebCore_FWD_ShadowChicken_h > +#define WebCore_FWD_ShadowChicken_h > +#include <JavaScriptCore/ShadowChicken.h> > +#endif Is this needed? Where is ShadowChicken used in WebCore? > Source/WebInspectorUI/UserInterface/Views/CallFrameTreeElement.js:68 > + let tailCallSuffix = ""; > + if (this._callFrame.isTailDeleted) > + tailCallSuffix = " " + WebInspector.UIString("(Tail Call Optimized)"); I think the case should be sentence style, and not all capitalized: "(Tail call optimized)" > LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:42 > + //InspectorTest.assert(WebInspector.debuggerManager.callFrames.length >= expectedFrames.length); Either include this assert or drop it. It seems like it would be fine to uncomment. > LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:57 > + InspectorTest.assert(expectedFrame.thisValue[0] === prop.name); > + InspectorTest.assert('' + expectedFrame.thisValue[1] === prop.value); > + InspectorTest.log("call frame at: " + i + " is correct.\n"); A better string might be: InspectorTest.pass(`Call Frame ${i} 'this' value is correct.\n`); // PASS: Call Frame # 'this' value is correct. Or turning these both into expect lines: InspectorTest.expectThat(thisObject.type === "object", "'this' value should be an object."); InspectorTest.expectThat(expectedFrame.thisValue[0] === prop.name, `'this' value object should have expected property (${expectedFrame.thisValue[0]}).`); InspectorTest.expectThat(expectedFrame.thisValue[1] === prop.value, "'this' value object should have expected property value (${expectedFrame.thisValue[1]})."); // PASS: 'this' value should be an object. // PASS: 'this' value object should have expected property (...). // PASS: 'this' value object should have expected property value (...). > LayoutTests/inspector/debugger/tail-deleted-frames.html:42 > + //InspectorTest.assert(WebInspector.debuggerManager.callFrames.length >= expectedFrames.length); Same thing, seems you can uncomment this or remove it.

Saam Barati

Comment 59 2016-05-16 13:11:02 PDT

Comment on attachment 279021 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279021&action=review >> Source/JavaScriptCore/ChangeLog:29 >> + keep around at most 128 "shadow" frames that wouldn't have naturally been kept > > I presume the 128 shadow frames will be the last (aka top) 128 frames. Can you say something here to indicate that explicitly? Sure. >>> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:77 >>> + for (auto iter = frames.rbegin(), end = frames.rend(); iter != end; ++iter) { >> >> why "rbegin" and "rend" instead of "begin" and "end"? > > I think it would be easier to use an index-based loop here: > > for (unsigned i = frames.size(); i--;) I will a regular for loop. >> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:102 >> return m_caller; > > I'm a little concerned with us eagerly creating the entire stack on instantiation. I recall the Inspector used to have issues with "hanging" while waiting for the stack of frames to be created when it is debugging a deeply recursive program. I think I made the creation of the caller DebuggerCallFrame incremental like this to specifically mitigate that issue. However, I don't know if the Inspector side was ever changed to not ask for the whole stack in one shot. Ideally, the Inspector will only ask for say the top 20 frames, and leave the rest wait till the user actually asks for them. > > Is there a reason why we cannot continue to only create the caller as needed? We do eagerly create a stack trace, but we will now lazily create DebuggerCallFrame. So in practice, this is not different than what we're already doing. Mark and I discussed this offline, and Mark is concerned with this limiting us sending a stack trace in batches to the inspector. We ran some experiments and concluded that the most expensive part is not creating DebuggerCallFrame stack trace, but sending the inspector the serialized data. So I don't think this is a preventative design of future Inspector protocol optimization. >> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:107 >> + return scope()->globalObject()->globalExec(); > > scope() can return nullptr if !isValid(). Perhaps we should guard or ASSERT that here. I don't think guarding will help here. And scope() already has an ASSERT. I would want this code to crash if used on an invalid call frame.

Saam Barati

Comment 60 2016-05-16 13:14:28 PDT

Comment on attachment 279021 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279021&action=review >> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1981 >> + DC17E8191C9C91DB008A6AB3 /* ShadowChickenInlines.h in Headers */ = {isa = PBXBuildFile; fileRef = DC17E8151C9C7FD4008A6AB3 /* ShadowChickenInlines.h */; settings = {ATTRIBUTES = (Private, ); }; }; > > Is this needed? Where is ShadowChicken used outside out JSC? Or is it a transitive include? Transitive include. >> Source/JavaScriptCore/debugger/DebuggerCallFrame.h:81 >> + explicit DebuggerCallFrame(CallFrame*, const ShadowChicken::Frame&); > > Nit: Explicit is not needed if there is more then one parameter. Will change. >> Source/WebCore/ForwardingHeaders/interpreter/ShadowChicken.h:4 >> +#endif > > Is this needed? Where is ShadowChicken used in WebCore? Transitive include from DebuggerCallFrame >> Source/WebInspectorUI/UserInterface/Views/CallFrameTreeElement.js:68 >> + tailCallSuffix = " " + WebInspector.UIString("(Tail Call Optimized)"); > > I think the case should be sentence style, and not all capitalized: "(Tail call optimized)" Will change >> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:42 >> + //InspectorTest.assert(WebInspector.debuggerManager.callFrames.length >= expectedFrames.length); > > Either include this assert or drop it. It seems like it would be fine to uncomment. Yeah not sure why I left this commented it out. It must be true or the below loop will fail. >> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:57 >> + InspectorTest.log("call frame at: " + i + " is correct.\n"); > > A better string might be: > > InspectorTest.pass(`Call Frame ${i} 'this' value is correct.\n`); > // PASS: Call Frame # 'this' value is correct. > > Or turning these both into expect lines: > > InspectorTest.expectThat(thisObject.type === "object", "'this' value should be an object."); > InspectorTest.expectThat(expectedFrame.thisValue[0] === prop.name, `'this' value object should have expected property (${expectedFrame.thisValue[0]}).`); > InspectorTest.expectThat(expectedFrame.thisValue[1] === prop.value, "'this' value object should have expected property value (${expectedFrame.thisValue[1]})."); > // PASS: 'this' value should be an object. > // PASS: 'this' value object should have expected property (...). > // PASS: 'this' value object should have expected property value (...). I'll go with this. I like it.

Mark Lam

Comment 61 2016-05-16 13:18:53 PDT

Comment on attachment 279021 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279021&action=review >>> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:107 >>> + return scope()->globalObject()->globalExec(); >> >> scope() can return nullptr if !isValid(). Perhaps we should guard or ASSERT that here. > > I don't think guarding will help here. And scope() already has an ASSERT. I would want this code to crash if used on an invalid call frame. I see that DebuggerCallFrame::globalExec() is only used in ScriptDebugServer::evaluateBreakpointAction(). Based on how it's used, I think the DebuggerCallFrame should always be valid in those cases. Hence, I agree that all we need here is the ASSERT (which is already covered). > Source/JavaScriptCore/jit/CCallHelpers.cpp:56 > -void CCallHelpers::setupShadowChickenPacket() > +void CCallHelpers::setupShadowChickenPacket(GPRReg shadowPacket, GPRReg scratch1NonArgGPR, GPRReg scratch2) > { Can we assert that the passed in scratch1NonArgGPR is not in the Arg GPR set (assuming this is possible)? > Source/JavaScriptCore/jit/CCallHelpers.h:2336 > + // Leaves behind a pointer to the Packet we should write to in shadowPacket. > + void setupShadowChickenPacket(GPRReg shadowPacket, GPRReg scratch1NonArgGPR, GPRReg scratch2); Everywhere else in the code for C++ code, we would have called this "ensureShadowChickenPacket" instead. Would it make sense to rename this to "ensureShadowChickenPacket" or "emitEnsureShadowChickenPacket"?

Saam Barati

Comment 62 2016-05-16 14:44:09 PDT

Created attachment 279049 [details] patch for landing

WebKit Commit Bot

Comment 63 2016-05-16 14:45:51 PDT

Attachment 279049 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:75: 'currentParent' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 1 in 64 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 64 2016-05-16 15:01:15 PDT

Created attachment 279050 [details] what it looks like in inspector

Saam Barati

Comment 65 2016-05-16 15:52:10 PDT

Created attachment 279056 [details] patch for landing

WebKit Commit Bot

Comment 66 2016-05-16 15:54:33 PDT

Attachment 279056 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:75: 'currentParent' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 1 in 64 files If any of these errors are false positives, please file a bug against check-webkit-style.

Geoffrey Garen

Comment 67 2016-05-16 16:06:00 PDT

I think the UI should say "Tail call" and not "Tail call optimized". Tail calls are best thought of as a language feature and not as an optimization. That's how they're described in the language specification and in functional programming discussions.

Saam Barati

Comment 68 2016-05-16 16:31:38 PDT

landed in: http://trac.webkit.org/changeset/200981

Saam Barati

Comment 69 2016-05-16 16:32:02 PDT

(In reply to comment #67) > I think the UI should say "Tail call" and not "Tail call optimized". Tail > calls are best thought of as a language feature and not as an optimization. > That's how they're described in the language specification and in functional > programming discussions. I went with "(Tail Call)" for the tooltip text.

Ryan Haddad

Comment 70 2016-05-16 16:55:39 PDT

It looks like this change broke the CLoop build: <https://build.webkit.org/builders/Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/16045> <https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/6305>

Saam Barati

Comment 71 2016-05-16 17:02:42 PDT

(In reply to comment #70) > It looks like this change broke the CLoop build: > <https://build.webkit.org/builders/ > Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/16045> > <https://build.webkit.org/builders/ > Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/6305> Thanks. I'm looking into it now.

Saam Barati

Comment 72 2016-05-16 17:29:04 PDT

cloop fix: http://trac.webkit.org/changeset/200984

Note You need to log in before you can comment on or make changes to this bug.