Bug 201366 - Tail Deleted Frames shown in Web Inspector are sometimes incorrect (Shadow Chicken)
Summary: Tail Deleted Frames shown in Web Inspector are sometimes incorrect (Shadow Ch...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-30 16:19 PDT by Joseph Pecoraro
Modified: 2019-09-06 12:11 PDT (History)
11 users (show)

See Also:


Attachments
[TEST PAGE] test.html (429 bytes, text/html)
2019-08-30 16:19 PDT, Joseph Pecoraro
no flags Details
[TEST PAGE] test-less-reproducible-issue.html (380 bytes, text/html)
2019-08-30 16:32 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (66.71 KB, patch)
2019-08-31 01:33 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (66.73 KB, patch)
2019-08-31 01:37 PDT, Joseph Pecoraro
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (64.37 KB, patch)
2019-09-03 10:59 PDT, Joseph Pecoraro
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-08-30 16:19:56 PDT
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>
Comment 1 Joseph Pecoraro 2019-08-30 16:32:11 PDT
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.
Comment 2 Joseph Pecoraro 2019-08-31 01:33:08 PDT
Created attachment 377793 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2019-08-31 01:36:19 PDT
> [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.
Comment 4 Joseph Pecoraro 2019-08-31 01:37:28 PDT
Created attachment 377794 [details]
[PATCH] Proposed Fix
Comment 5 EWS Watchlist 2019-08-31 03:53:02 PDT
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
Comment 6 Joseph Pecoraro 2019-09-03 10:59:08 PDT
Created attachment 377907 [details]
[PATCH] Proposed Fix

Had to remove one of the cases.
Comment 7 Saam Barati 2019-09-04 23:03:23 PDT
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
Comment 8 WebKit Commit Bot 2019-09-05 10:45:44 PDT
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
Comment 9 Joseph Pecoraro 2019-09-05 13:47:23 PDT
I have concluded that commit-queue doesn't like me. It never works on my patches =(.
Comment 10 Joseph Pecoraro 2019-09-06 01:15:53 PDT
https://trac.webkit.org/changeset/249566/webkit
Comment 11 Radar WebKit Bug Importer 2019-09-06 01:16:18 PDT
<rdar://problem/55104146>
Comment 12 Radar WebKit Bug Importer 2019-09-06 01:16:19 PDT
<rdar://problem/55104147>
Comment 13 Ryan Haddad 2019-09-06 10:03:33 PDT
Reverted r249566 for reason:

Causes inspector layout test crashes under GuardMalloc

Committed r249577: <https://trac.webkit.org/changeset/249577>
Comment 14 Ryan Haddad 2019-09-06 11:51:48 PDT
Details in <rdar://problem/55104147>
Comment 15 Joseph Pecoraro 2019-09-06 12:09:40 PDT
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!
Comment 16 Joseph Pecoraro 2019-09-06 12:11:40 PDT
https://trac.webkit.org/changeset/249586/webkit