Bug 156685 – Hook up ShadowChicken to the debugger to show tail deleted frames

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Radar WebKit Bug Importer]

<rdar://problem/25770521>

[Saam Barati]

Created attachment 278465 [details]
WIP

it shows tail deleted frames in the inspector, but it's hacky.

[WebKit Commit Bot]

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 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 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 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]

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

[Saam Barati]

Created attachment 278793 [details]
WIP

[Saam Barati]

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

[Saam Barati]

Created attachment 278797 [details]
WIP

rebased

[WebKit Commit Bot]

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 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]

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 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]

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]

Created attachment 278806 [details]
WIP

[WebKit Commit Bot]

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 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]

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 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]

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 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]

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]

Created attachment 278853 [details]
WIP

[WebKit Commit Bot]

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]

Created attachment 278867 [details]
WIP

some 32-bit code

[WebKit Commit Bot]

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]

Created attachment 278910 [details]
WIP

[Saam Barati]

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]

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 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]

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 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]

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 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]

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]

Created attachment 278992 [details]
WIP

[WebKit Commit Bot]

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]

Created attachment 279004 [details]
patch

[WebKit Commit Bot]

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]

Created attachment 279006 [details]
patch

[WebKit Commit Bot]

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]

Created attachment 279007 [details]
patch

[WebKit Commit Bot]

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 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]

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 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]

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 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]

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]

Created attachment 279021 [details]
patch

[WebKit Commit Bot]

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 on attachment 279021 [details]
patch

The inspector parts look fine to me.

[Filip Pizlo]

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

[Mark Lam]

I'm reading it too.

[Mark Lam]

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 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 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 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 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 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]

Created attachment 279049 [details]
patch for landing

[WebKit Commit Bot]

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]

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

[Saam Barati]

Created attachment 279056 [details]
patch for landing

[WebKit Commit Bot]

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]

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]

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

[Saam Barati]

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

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]

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

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