Bug 201366 – Tail Deleted Frames shown in Web Inspector are sometimes incorrect (Shadow Chicken)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Created attachment 377763 [details]
[TEST PAGE] test.html

Tail Deleted Frames shown in Web Inspector are sometimes incorrect (Shadow Chicken)

Steps to reproduce:
1. Inspect test page
  => Show Call Stack in Web Inspector debugger sidebar

Expected:

  Call Stack:
  [green f] a
  [gray  f] b
  [gray  f] c
  [green f] startABC

Actual:

  Call Stack:
  [green f] a
  [gray  f] b
  [gray  f] c
  [gray  f] THIS_DOES_NOT_CALL_c
  [gray  f] THIS_DOES_NOT_CALL_c
  ...
  [gray  f] THIS_DOES_NOT_CALL_c
  [green f] startABC


Test page:

<script>
"use strict";
function a() {
    let x = 20;
    debugger;
    x;
    return x;
}
function b() {
    let y = 40;
    return a.call({aThis: 2});
}
function c() {
    let z = 60;
    return b.call({bThis: 1}); 
}
function startABC() {
    for (let i = 0; i < 500; ++i)
        THIS_DOES_NOT_CALL_c();
    c.call({cThis: 0});
}
function THIS_DOES_NOT_CALL_c() {
    return Math.random();
}
setInterval(startABC);
</script>

[Joseph Pecoraro]

Created attachment 377767 [details]
[TEST PAGE] test-less-reproducible-issue.html

There is another case we run into with test-less-reproducible-issue.html.

Steps:
1. Open test-less-reproducible-issue.html in a new process
2. Wait about 5 seconds
3. Open inspector (will automatically pause)
4. Reload a few times and look at the stack trace

Expected:

  Call Stack:
  [green f] a
  [gray  f] b
  [gray  f] c
  [green f] startABC

Actual:

  Call Stack:
  [green f] a
  [gray  f] b
  [gray  f] b
  [green f] startABC

This particular issue is very tricky to reproduce, so it could be a different issue.

[Joseph Pecoraro]

Created attachment 377793 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

> [TEST PAGE] test.html

The patch only partially addresses this case. I'm not sure yet how we might be able to more completely address this (Native function calls lack prologue).

> [TEST PAGE] test-less-reproducible-issue.html

The patch should address this case. Ensure we include prologues during an update.

[Joseph Pecoraro]

Created attachment 377794 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

Comment on attachment 377794 [details]
[PATCH] Proposed Fix

Attachment 377794 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12987002

New failing tests:
stress/shadow-chicken-enabled.js.shadow-chicken

[Joseph Pecoraro]

Created attachment 377907 [details]
[PATCH] Proposed Fix

Had to remove one of the cases.

[Saam Barati]

Comment on attachment 377907 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/ChangeLog:8
> +        It is possible for the log buffer to be full right as someone is trying to

Interesting

[WebKit Commit Bot]

Comment on attachment 377907 [details]
[PATCH] Proposed Fix

Rejecting attachment 377907 [details] from commit-queue.

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

Last 5000 characters of output:
viewer', u'Saam Barati']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 33 diffs from patch file(s).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/inspector/debugger/evaluateOnCallFrame-exception.html
patching file LayoutTests/inspector/debugger/resources/tail-deleted-frames-from-vm-entry.js
rm 'LayoutTests/inspector/debugger/resources/tail-deleted-frames-from-vm-entry.js'
patching file LayoutTests/inspector/debugger/resources/tail-deleted-frames-this-value.js
Hunk #1 FAILED at 1.
File LayoutTests/inspector/debugger/resources/tail-deleted-frames-this-value.js is not empty after patch, as expected
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/inspector/debugger/resources/tail-deleted-frames-this-value.js.rej
rm 'LayoutTests/inspector/debugger/resources/tail-deleted-frames-this-value.js'
patching file LayoutTests/inspector/debugger/resources/tail-deleted-frames.js
rm 'LayoutTests/inspector/debugger/resources/tail-deleted-frames.js'
patching file LayoutTests/inspector/debugger/tail-deleted-frames-expected.txt
rm 'LayoutTests/inspector/debugger/tail-deleted-frames-expected.txt'
patching file LayoutTests/inspector/debugger/tail-deleted-frames-from-vm-entry-expected.txt
rm 'LayoutTests/inspector/debugger/tail-deleted-frames-from-vm-entry-expected.txt'
patching file LayoutTests/inspector/debugger/tail-deleted-frames-from-vm-entry.html
rm 'LayoutTests/inspector/debugger/tail-deleted-frames-from-vm-entry.html'
patching file LayoutTests/inspector/debugger/tail-deleted-frames-this-value-expected.txt
Hunk #1 FAILED at 1.
File LayoutTests/inspector/debugger/tail-deleted-frames-this-value-expected.txt is not empty after patch, as expected
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/inspector/debugger/tail-deleted-frames-this-value-expected.txt.rej
rm 'LayoutTests/inspector/debugger/tail-deleted-frames-this-value-expected.txt'
patching file LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html
Hunk #1 FAILED at 1.
File LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html is not empty after patch, as expected
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html.rej
rm 'LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html'
patching file LayoutTests/inspector/debugger/tail-deleted-frames.html
rm 'LayoutTests/inspector/debugger/tail-deleted-frames.html'
patching file LayoutTests/inspector/debugger/tail-deleted-frames/resources/stack-trace-utilities.js
patching file LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-frames.js
patching file LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-native-tail-deleted-calls.js
patching file LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-tail-deleted-frames.js
patching file LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-scopes.js
patching file LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-this-value.js
patching file LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-vm-entry.js
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames-expected.txt
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames.html
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls-expected.txt
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls.html
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames-expected.txt
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames.html
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes-expected.txt
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes.html
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value-expected.txt
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value.html
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry-expected.txt
patching file LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry.html
patching file LayoutTests/platform/mac/TestExpectations
patching file Source/JavaScriptCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/JavaScriptCore/interpreter/ShadowChicken.cpp

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

Full output: https://webkit-queues.webkit.org/results/13000828

[Joseph Pecoraro]

I have concluded that commit-queue doesn't like me. It never works on my patches =(.

[Joseph Pecoraro]

https://trac.webkit.org/changeset/249566/webkit

[Radar WebKit Bug Importer]

<rdar://problem/55104146>

[Radar WebKit Bug Importer]

<rdar://problem/55104147>

[Ryan Haddad]

Reverted r249566 for reason:

Causes inspector layout test crashes under GuardMalloc

Committed r249577: <https://trac.webkit.org/changeset/249577>

[Ryan Haddad]

Details in <rdar://problem/55104147>

[Joseph Pecoraro]

Comment on attachment 377907 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/interpreter/ShadowChicken.cpp:92
> +    m_log = static_cast<Packet*>(fastZeroedMalloc(sizeof(Packet) * m_logSize + 1));

This needed to be: `* (m_logSize + 1)` so the addition happens first!

[Joseph Pecoraro]

https://trac.webkit.org/changeset/249586/webkit