Tail Deleted Frames shown in Web Inspector are sometimes incorrect (Shadow Chicken)
authorjoepeck@webkit.org <joepeck@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 6 Sep 2019 08:14:08 +0000 (08:14 +0000)
committerjoepeck@webkit.org <joepeck@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 6 Sep 2019 08:14:08 +0000 (08:14 +0000)
https://bugs.webkit.org/show_bug.cgi?id=201366

Reviewed by Saam Barati.

Source/JavaScriptCore:

It is possible for the log buffer to be full right as someone is trying to
log a function prologue. In such a case the machine stack has already been
updated to include the new JavaScript call frame, but the prologue packet
cannot be included in the update because the log is full. This would mean
that the update fails to rationalize the machine stack with the shadow
log / stack. Namely, the current JavaScript call frame is unable to
find a matching prologue (the one we are holding to include after the update)
and inserts a questionable value into the stack; and in the process
missing and removing real potential tail calls.

For example:

    "use strict";
    function third() { return 1; }
    function second() { return third(); }
    function first() { return second(); }
    function start() { return first(); }

If the the log fills up just as we are entering `b` then we may have a list
full log of packets looking like:

  Shadow Log:
    ...
    { prologue-packet: entering `start` ... }
    { prologue-packet: entering `first` ... }
    { tail-packet: leaving `first` with a tail call }

  Incoming Packet:
    { prologue-packet: entering `second` ... }

  Current JS Stack:
    second
    start

Since the Current JavaScript stack already has `second`, if we process the
log without the prologue for `second` then we push a confused entry on the
shadow stack and clear the log such that we eventually lose the tail-call
information for `first` to `second`.

This patch solves this issue by providing enough extra space in the log
to always process the incoming packet when that forces an update. This way
clients can continue to behave exactly as they are.

--

We also document a corner case in some circumstances where the shadow
log may currently be insufficient to know how to reconcile:

For example:

    "use strict";
    function third() { return 1; }
    function second() { return third(); }
    function first() { return second(); }
    function doNothingTail() { return Math.random() }
    function start() {
        for (i=0;i<1000;++i) doNothingTail();
        return first();
    }

In this case the ShadowChicken log may be processed multiple times due
to the many calls to `doNothingTail` / `Math.random()`. When calling the
Native function no prologue packet is emitted, so it is unclear that we
temporarly go deeper and come back out on the stack, so the log appears
to have lots of doNothingTail calls reusing the same frame:

  Shadow Log:
    ...
    , [123] {callee = 0x72a21aee0, frame = 0x7ffeef897270, callerFrame = 0x7ffeef8972e0, name = start}
    , [124] {callee = 0x72a21af10, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = doNothingTail}
    , [125] tail-packet:{frame = 0x7ffeef8971f0}
    , [126] {callee = 0x72a21af10, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = doNothingTail}
    , [127] tail-packet:{frame = 0x7ffeef8971f0}
    ...
    , [140] {callee = 0x72a21af10, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = doNothingTail}
    , [141] tail-packet:{frame = 0x7ffeef8971f0}
    , [142] {callee = 0x72a21af10, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = doNothingTail}
    , [143] tail-packet:{frame = 0x7ffeef8971f0}
    , [144] {callee = 0x72a21aeb0, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = first}
    , [145] tail-packet:{frame = 0x7ffeef8971f0}
    , [146] {callee = 0x72a21ae80, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = second}
    ...

This log would seem to be indistinguishable from real tail recursion, such as:

    "use strict";
    function third() { return 1; }
    function second() { return third(); }
    function first() { return second(); }
    function doNothingTail(n) {
        return n ? doNothingTail(n-1) : first();
    }
    function start() {
        return doNothingTail(1000);
    }

Likewise there are more cases where the shadow log appears to be ambiguous with determining
the appropriate parent call frame with intermediate function calls. In practice this may
not be too problematic, as this is a best effort reconstruction of tail deleted frames.
It seems likely we would only show additional frames that did in fact happen serially
between JavaScript call frames, but may not actually be the proper parent frames
heirachy in the stack.

* interpreter/ShadowChicken.cpp:
(JSC::ShadowChicken::Packet::dump const):
(JSC::ShadowChicken::Frame::dump const):
(JSC::ShadowChicken::dump const):
Improved debugging output. Especially for functions.

(JSC::ShadowChicken::ShadowChicken):
Make space in the log for 1 additional packet to process when we slow log.

(JSC::ShadowChicken::log):
Include this packet in our update.

(JSC::ShadowChicken::update):
Address an edge case where we can eliminate tail-deleted frames that don't make sense.

LayoutTests:

* inspector/debugger/tail-deleted-frames-expected.txt: Removed.
* inspector/debugger/tail-deleted-frames-from-vm-entry-expected.txt: Removed.
* inspector/debugger/tail-deleted-frames-from-vm-entry.html: Removed.
* inspector/debugger/tail-deleted-frames-this-value-expected.txt: Removed.
* inspector/debugger/tail-deleted-frames-this-value.html: Removed.
* inspector/debugger/tail-deleted-frames.html: Removed.
Remove legacy tests that are difficult to read.

* inspector/debugger/tail-deleted-frames/resources/stack-trace-utilities.js: Added.
(TestPage.registerInitializer.window.getAsyncStackTrace):
(TestPage.registerInitializer.async.logThisObject):
(TestPage.registerInitializer.async.logScope):
(TestPage.registerInitializer.async.logCallFrame):
(TestPage.registerInitializer):
* inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-frames.js: Added.
* inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-native-tail-deleted-calls.js: Added.
* inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-tail-deleted-frames.js: Added.
* inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-scopes.js: Added.
* inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-this-value.js: Added.
* inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-vm-entry.js: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames-expected.txt: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames.html: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames-expected.txt: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames.html: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes-expected.txt: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes.html: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value-expected.txt: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value.html: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry-expected.txt: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry.html: Added.
Include modern tests that are easier to read.

* inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls-expected.txt: Added.
* inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls.html: Added.
Include a test that is known to produce bad output, since we have reproductive steps.

* platform/mac/TestExpectations:
Updated pathes.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@249566 268f45cc-cd09-0410-ab3c-d52691b4dbfc

28 files changed:
LayoutTests/ChangeLog
LayoutTests/inspector/debugger/evaluateOnCallFrame-exception.html
LayoutTests/inspector/debugger/tail-deleted-frames-expected.txt [deleted file]
LayoutTests/inspector/debugger/tail-deleted-frames-from-vm-entry-expected.txt [deleted file]
LayoutTests/inspector/debugger/tail-deleted-frames-from-vm-entry.html [deleted file]
LayoutTests/inspector/debugger/tail-deleted-frames.html [deleted file]
LayoutTests/inspector/debugger/tail-deleted-frames/resources/stack-trace-utilities.js [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-frames.js [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-native-tail-deleted-calls.js [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-tail-deleted-frames.js [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-scopes.js [moved from LayoutTests/inspector/debugger/resources/tail-deleted-frames.js with 85% similarity]
LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-this-value.js [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-vm-entry.js [moved from LayoutTests/inspector/debugger/resources/tail-deleted-frames-from-vm-entry.js with 81% similarity]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames-expected.txt [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames.html [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls-expected.txt [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls.html [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames-expected.txt [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames.html [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes-expected.txt [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes.html [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value-expected.txt [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value.html [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry-expected.txt [new file with mode: 0644]
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry.html [new file with mode: 0644]
LayoutTests/platform/mac/TestExpectations
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/interpreter/ShadowChicken.cpp

index 3bd7813..c90ed6c 100644 (file)
@@ -1,3 +1,49 @@
+2019-09-05  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Tail Deleted Frames shown in Web Inspector are sometimes incorrect (Shadow Chicken)
+        https://bugs.webkit.org/show_bug.cgi?id=201366
+
+        Reviewed by Saam Barati.
+
+        * inspector/debugger/tail-deleted-frames-expected.txt: Removed.
+        * inspector/debugger/tail-deleted-frames-from-vm-entry-expected.txt: Removed.
+        * inspector/debugger/tail-deleted-frames-from-vm-entry.html: Removed.
+        * inspector/debugger/tail-deleted-frames-this-value-expected.txt: Removed.
+        * inspector/debugger/tail-deleted-frames-this-value.html: Removed.
+        * inspector/debugger/tail-deleted-frames.html: Removed.
+        Remove legacy tests that are difficult to read.
+
+        * inspector/debugger/tail-deleted-frames/resources/stack-trace-utilities.js: Added.
+        (TestPage.registerInitializer.window.getAsyncStackTrace):
+        (TestPage.registerInitializer.async.logThisObject):
+        (TestPage.registerInitializer.async.logScope):
+        (TestPage.registerInitializer.async.logCallFrame):
+        (TestPage.registerInitializer):
+        * inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-frames.js: Added.
+        * inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-native-tail-deleted-calls.js: Added.
+        * inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-tail-deleted-frames.js: Added.
+        * inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-scopes.js: Added.
+        * inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-this-value.js: Added.
+        * inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-vm-entry.js: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames-expected.txt: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames.html: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames-expected.txt: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames.html: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes-expected.txt: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes.html: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value-expected.txt: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value.html: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry-expected.txt: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry.html: Added.
+        Include modern tests that are easier to read.
+
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls-expected.txt: Added.
+        * inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls.html: Added.
+        Include a test that is known to produce bad output, since we have reproductive steps.
+
+        * platform/mac/TestExpectations:
+        Updated pathes.
+
 2019-09-06  Andres Gonzalez  <andresg_22@apple.com>
 
         AccessibilityRenderObject::setSelectedTextRange fails to set the selection passed an empty line.
index 71d6ede..2898a61 100644 (file)
@@ -13,7 +13,6 @@ function test()
     const includeCommandLineAPI = true;
     const returnByValue = true;
 
-    InspectorTest.debug();
     let suite = InspectorTest.createAsyncSuite("Debugger.evaluateOnCallFrame.Exception");
 
     suite.addTestCase({
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames-expected.txt b/LayoutTests/inspector/debugger/tail-deleted-frames-expected.txt
deleted file mode 100644 (file)
index 5451131..0000000
+++ /dev/null
@@ -1,19 +0,0 @@
-Testing that we keep around tail deleted frames in the inspector.
-
-Starting Test
-
-
-------------------------------------
-Hit breakpoint at line: 3, column: 4
-------------------------------------
-Expected frame: {"functionName":"a","scope":["x",20],"isTailDeleted":false}
-Expected frame: {"functionName":"b","scope":["y",40],"isTailDeleted":true}
-Expected frame: {"functionName":"c","scope":["z",60],"isTailDeleted":true}
-Looking at frame number: 0
-    variable 'x': {"_type":"number","_description":"20","_hasChildren":false,"_value":20}
-Looking at frame number: 1
-    variable 'y': {"_type":"number","_description":"40","_hasChildren":false,"_value":40}
-Looking at frame number: 2
-    variable 'z': {"_type":"number","_description":"60","_hasChildren":false,"_value":60}
-Tests done
-
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames-from-vm-entry-expected.txt b/LayoutTests/inspector/debugger/tail-deleted-frames-from-vm-entry-expected.txt
deleted file mode 100644 (file)
index 3f9095e..0000000
+++ /dev/null
@@ -1,98 +0,0 @@
-Testing that we keep around tail deleted frames that are entry frames.
-
-Starting Test
-
-
-------------------------------------
-Hit breakpoint at line: 7, column: 4
-------------------------------------
-Expected frame: {"functionName":"bar","scope":["i",0],"isTailDeleted":false}
-PASS: Function name: bar is correct.
-PASS: Tail deleted expectation correct: false
-Expected frame: {"functionName":"bar","scope":["i",1],"isTailDeleted":true}
-PASS: Function name: bar is correct.
-PASS: Tail deleted expectation correct: true
-Expected frame: {"functionName":"bar","scope":["i",2],"isTailDeleted":true}
-PASS: Function name: bar is correct.
-PASS: Tail deleted expectation correct: true
-Expected frame: {"functionName":"bar","scope":["i",3],"isTailDeleted":true}
-PASS: Function name: bar is correct.
-PASS: Tail deleted expectation correct: true
-Expected frame: {"functionName":"bar","scope":["i",4],"isTailDeleted":true}
-PASS: Function name: bar is correct.
-PASS: Tail deleted expectation correct: true
-Expected frame: {"functionName":"bar","scope":["i",5],"isTailDeleted":true}
-PASS: Function name: bar is correct.
-PASS: Tail deleted expectation correct: true
-Expected frame: {"functionName":"bar","scope":["i",6],"isTailDeleted":true}
-PASS: Function name: bar is correct.
-PASS: Tail deleted expectation correct: true
-Expected frame: {"functionName":"bar","scope":["i",7],"isTailDeleted":true}
-PASS: Function name: bar is correct.
-PASS: Tail deleted expectation correct: true
-Expected frame: {"functionName":"bar","scope":["i",8],"isTailDeleted":true}
-PASS: Function name: bar is correct.
-PASS: Tail deleted expectation correct: true
-Expected frame: {"functionName":"bar","scope":["i",9],"isTailDeleted":true}
-PASS: Function name: bar is correct.
-PASS: Tail deleted expectation correct: true
-Expected frame: {"functionName":"timeout","scope":["foo",25],"isTailDeleted":true}
-PASS: Function name: timeout is correct.
-PASS: Tail deleted expectation correct: true
-Looking at frame number: 0
-    variable 'i': {"_type":"number","_description":"0","_hasChildren":false,"_value":0}
-PASS: Variable is a number.
-PASS: Found scope value: 0
-PASS: Did not find variable we were looking for: i
-Looking at frame number: 1
-    variable 'i': {"_type":"number","_description":"1","_hasChildren":false,"_value":1}
-PASS: Variable is a number.
-PASS: Found scope value: 1
-PASS: Did not find variable we were looking for: i
-Looking at frame number: 2
-    variable 'i': {"_type":"number","_description":"2","_hasChildren":false,"_value":2}
-PASS: Variable is a number.
-PASS: Found scope value: 2
-PASS: Did not find variable we were looking for: i
-Looking at frame number: 3
-    variable 'i': {"_type":"number","_description":"3","_hasChildren":false,"_value":3}
-PASS: Variable is a number.
-PASS: Found scope value: 3
-PASS: Did not find variable we were looking for: i
-Looking at frame number: 4
-    variable 'i': {"_type":"number","_description":"4","_hasChildren":false,"_value":4}
-PASS: Variable is a number.
-PASS: Found scope value: 4
-PASS: Did not find variable we were looking for: i
-Looking at frame number: 5
-    variable 'i': {"_type":"number","_description":"5","_hasChildren":false,"_value":5}
-PASS: Variable is a number.
-PASS: Found scope value: 5
-PASS: Did not find variable we were looking for: i
-Looking at frame number: 6
-    variable 'i': {"_type":"number","_description":"6","_hasChildren":false,"_value":6}
-PASS: Variable is a number.
-PASS: Found scope value: 6
-PASS: Did not find variable we were looking for: i
-Looking at frame number: 7
-    variable 'i': {"_type":"number","_description":"7","_hasChildren":false,"_value":7}
-PASS: Variable is a number.
-PASS: Found scope value: 7
-PASS: Did not find variable we were looking for: i
-Looking at frame number: 8
-    variable 'i': {"_type":"number","_description":"8","_hasChildren":false,"_value":8}
-PASS: Variable is a number.
-PASS: Found scope value: 8
-PASS: Did not find variable we were looking for: i
-Looking at frame number: 9
-    variable 'i': {"_type":"number","_description":"9","_hasChildren":false,"_value":9}
-PASS: Variable is a number.
-PASS: Found scope value: 9
-PASS: Did not find variable we were looking for: i
-Looking at frame number: 10
-    variable 'foo': {"_type":"number","_description":"25","_hasChildren":false,"_value":25}
-PASS: Variable is a number.
-PASS: Found scope value: 25
-PASS: Did not find variable we were looking for: foo
-Tests done
-
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames-from-vm-entry.html b/LayoutTests/inspector/debugger/tail-deleted-frames-from-vm-entry.html
deleted file mode 100644 (file)
index f87b8c2..0000000
+++ /dev/null
@@ -1,105 +0,0 @@
-<!doctype html>
-<html>
-<head>
-<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
-<script src="../../http/tests/inspector/debugger/debugger-test.js"></script>
-<script src="./resources/tail-deleted-frames-from-vm-entry.js"></script>
-<script>
-
-function test()
-{
-    let scriptObject;
-
-    function remoteObjectJSONFilter(key, value) {
-        if (key === "_target" || key === "_listeners")
-            return undefined;
-        if (key === "_objectId" || key === "_stackTrace")
-            return "<filtered>";
-        return value;
-    }
-
-    function startTest() {
-        InspectorTest.log("Starting Test");
-        // 0 based indices.
-        let testInfo = {line: 7, column: 4};
-        let location = scriptObject.createSourceCodeLocation(testInfo.line, testInfo.column);
-        let breakpoint = new WI.Breakpoint(location);
-        WI.debuggerManager.addBreakpoint(breakpoint);
-        InspectorTest.evaluateInPage("setTimeout(timeout, 0);");
-    }
-
-    WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.CallFramesDidChange, function(event) {
-        var activeCallFrame = WI.debuggerManager.activeCallFrame;
-
-        if (!activeCallFrame)
-            return;
-
-        var stopLocation = "line: " + activeCallFrame.sourceCodeLocation.lineNumber + ", column: " + activeCallFrame.sourceCodeLocation.columnNumber;
-
-        InspectorTest.log("\n\n------------------------------------");
-        InspectorTest.log("Hit breakpoint at " + stopLocation);
-        InspectorTest.log("------------------------------------");
-
-        // top down list
-        let expectedFrames = [];
-        for (let i = 0; i < 10; i++)
-            expectedFrames.push({functionName: 'bar', scope: ['i', i], isTailDeleted: i > 0 ? true : false});
-        expectedFrames.push({functionName: 'timeout', scope: ['foo', 25], isTailDeleted: true});
-
-        let targetData = WI.debuggerManager.dataForTarget(WI.debuggerManager.activeCallFrame.target);
-        let callFrames = targetData.callFrames;
-
-        InspectorTest.assert(callFrames.length >= expectedFrames.length);
-
-        for (let i = 0; i < expectedFrames.length; i++) {
-            let callFrame = callFrames[i];
-            let expectedFrame = expectedFrames[i];
-            InspectorTest.log("Expected frame: " + JSON.stringify(expectedFrame));
-            InspectorTest.expectThat(callFrame.functionName === expectedFrame.functionName, `Function name: ${callFrame.functionName} is correct.`);
-
-            InspectorTest.expectThat(callFrame.isTailDeleted === expectedFrame.isTailDeleted, `Tail deleted expectation correct: ${callFrame.isTailDeleted}`);
-            let scope = callFrame.scopeChain[1];
-
-            scope.objects[0].getPropertyDescriptors(function(properties) {
-                let found = false;
-                let variableName = expectedFrame.scope[0];
-                let variableValue = expectedFrame.scope[1];
-                for (let propertyDescriptor of properties) {
-                    if (propertyDescriptor.name === variableName) {
-                        found = true;
-                        InspectorTest.log("Looking at frame number: " + i);
-                        InspectorTest.log(`    variable '${variableName}': ${JSON.stringify(propertyDescriptor.value, remoteObjectJSONFilter)}`);
-                        InspectorTest.expectThat(propertyDescriptor.value.type === 'number', "Variable is a number.");
-                        InspectorTest.expectThat(propertyDescriptor.value.value === variableValue, `Found scope value: ${variableValue}`);
-                    }
-                }
-                InspectorTest.expectThat(!!found, `Did not find variable we were looking for: ${variableName}`);
-            });
-        }
-
-        WI.debuggerManager.resume();
-    });
-
-    WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.Resumed, function(event) {
-        InspectorTest.log("Tests done");
-        InspectorTest.completeTest();
-    });
-
-    WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.ScriptAdded, function(event) {
-        let eventScriptObject = event.data.script;
-        if (/tail-deleted-frames-from-vm-entry\.js$/.test(eventScriptObject.url)) {
-            scriptObject = eventScriptObject;
-            startTest();
-            return;
-        }
-
-    });
-
-    InspectorTest.reloadPage();
-}
-</script>
-</head>
-<body onload="runTest()">
-    <p>Testing that we keep around tail deleted frames that are entry frames. </p>
-</body>
-</html>
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames.html b/LayoutTests/inspector/debugger/tail-deleted-frames.html
deleted file mode 100644 (file)
index daaaca9..0000000
+++ /dev/null
@@ -1,107 +0,0 @@
-<!doctype html>
-<html>
-<head>
-<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
-<script src="../../http/tests/inspector/debugger/debugger-test.js"></script>
-<script src="./resources/tail-deleted-frames.js"></script>
-<script>
-
-function test()
-{
-    var scriptObject;
-
-    function remoteObjectJSONFilter(key, value) {
-        if (key === "_target" || key === "_listeners")
-            return undefined;
-        if (key === "_objectId" || key === "_stackTrace")
-            return "<filtered>";
-        return value;
-    }
-
-    function startTest() {
-        InspectorTest.log("Starting Test");
-        // 0 based indices.
-        let testInfo = {line: 3, column: 4};
-        let location = scriptObject.createSourceCodeLocation(testInfo.line, testInfo.column);
-        let breakpoint = new WI.Breakpoint(location);
-        WI.debuggerManager.addBreakpoint(breakpoint);
-        InspectorTest.evaluateInPage("startABC()");
-    }
-
-    WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.CallFramesDidChange, function(event) {
-        var activeCallFrame = WI.debuggerManager.activeCallFrame;
-
-        if (!activeCallFrame)
-            return;
-
-        var stopLocation = "line: " + activeCallFrame.sourceCodeLocation.lineNumber + ", column: " + activeCallFrame.sourceCodeLocation.columnNumber;
-
-        InspectorTest.log("\n\n------------------------------------");
-        InspectorTest.log("Hit breakpoint at " + stopLocation);
-        InspectorTest.log("------------------------------------");
-
-        // top down list
-        let expectedFrames = [
-            {functionName: 'a', scope: ['x', 20], isTailDeleted: false},
-            {functionName: 'b', scope: ['y', 40], isTailDeleted: true},
-            {functionName: 'c', scope: ['z', 60], isTailDeleted: true}
-        ];
-
-        let targetData = WI.debuggerManager.dataForTarget(WI.debuggerManager.activeCallFrame.target);
-        let callFrames = targetData.callFrames;
-
-        InspectorTest.assert(callFrames.length >= expectedFrames.length);
-
-        for (let i = 0; i < expectedFrames.length; i++) {
-            let callFrame = callFrames[i];
-            let expectedFrame = expectedFrames[i];
-            InspectorTest.log("Expected frame: " + JSON.stringify(expectedFrame));
-            InspectorTest.assert(callFrame.functionName === expectedFrame.functionName);
-
-            InspectorTest.assert(callFrame.isTailDeleted === expectedFrame.isTailDeleted);
-            let topScope = callFrame.scopeChain[0];
-
-            topScope.objects[0].getPropertyDescriptors(function(properties) {
-                let found = false;
-                let variableName = expectedFrame.scope[0];
-                let variableValue = expectedFrame.scope[1];
-                for (let propertyDescriptor of properties) {
-                    if (propertyDescriptor.name === variableName) {
-                        found = true;
-                        InspectorTest.log("Looking at frame number: " + i);
-                        InspectorTest.log(`    variable '${variableName}': ${JSON.stringify(propertyDescriptor.value, remoteObjectJSONFilter)}`);
-                        InspectorTest.assert(propertyDescriptor.value.type === 'number');
-                        InspectorTest.assert(propertyDescriptor.value.value === variableValue);
-                    }
-                }
-                InspectorTest.assert(found);
-            });
-        }
-
-        WI.debuggerManager.resume();
-    });
-
-    WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.Resumed, function(event) {
-        InspectorTest.log("Tests done");
-        InspectorTest.completeTest();
-    });
-
-    WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.ScriptAdded, function(event) {
-        eventScriptObject = event.data.script;
-        
-        if (/tail-deleted-frames\.js$/.test(eventScriptObject.url)) {
-            scriptObject = eventScriptObject;
-            startTest();
-            return;
-        }
-
-    });
-
-    InspectorTest.reloadPage();
-}
-</script>
-</head>
-<body onload="runTest()">
-    <p>Testing that we keep around tail deleted frames in the inspector. </p>
-</body>
-</html>
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/resources/stack-trace-utilities.js b/LayoutTests/inspector/debugger/tail-deleted-frames/resources/stack-trace-utilities.js
new file mode 100644 (file)
index 0000000..220f835
--- /dev/null
@@ -0,0 +1,97 @@
+TestPage.registerInitializer(() => {
+    window.getAsyncStackTrace = function() {
+        InspectorTest.assert(WI.debuggerManager.activeCallFrame, "Active call frame should exist.");
+        if (!WI.debuggerManager.activeCallFrame)
+            return null;
+
+        let targetData = WI.debuggerManager.dataForTarget(WI.debuggerManager.activeCallFrame.target);
+        InspectorTest.assert(targetData, "Data for active call frame target should exist.");
+        if (!targetData)
+            return null;
+
+        const topCallFrameIsBoundary = false;
+        const truncated = false;
+        return new WI.StackTrace(targetData.callFrames, topCallFrameIsBoundary, truncated, targetData.asyncStackTrace);
+    };
+
+    window.logAsyncStackTrace = async function(options = {}) {
+        let foundAsyncBoundary = false;
+        let callFrameIndex = 0;
+
+        async function logThisObject(callFrame) {
+            if (callFrame.thisObject.canLoadPreview()) {
+                await new Promise((resolve) => { callFrame.thisObject.updatePreview(resolve); });
+                let preview = callFrame.thisObject.preview.propertyPreviews[0];
+                InspectorTest.log(`  this: ${preview.name} - ${preview.value}`);
+            } else
+                InspectorTest.log(`  this: undefined`);
+        }
+
+        async function logScope(callFrame, scopeChainIndex) {
+            InspectorTest.log(`  ----Scope----`);
+            return new Promise((resolve, reject) => {
+                let scope = callFrame.scopeChain[scopeChainIndex];
+                if (!scope) {
+                    resolve();
+                    return;
+                }
+                scope.objects[0].getPropertyDescriptors((properties) => {
+                    for (let propertyDescriptor of properties)
+                        InspectorTest.log(`  ${propertyDescriptor.name}: ${propertyDescriptor.value.description}`);
+                    if (!properties.length)
+                        InspectorTest.log(`  --  empty  --`);
+                    InspectorTest.log(`  -------------`);
+                    resolve();
+                });
+            });
+        }
+
+        async function logCallFrame(callFrame, isAsyncBoundary) {
+            let label = callFrame.functionName;
+            if (isAsyncBoundary)
+                InspectorTest.log(`${callFrameIndex}: --- ${label} ---`);
+            else {
+                let code = callFrame.nativeCode ? "N" : (callFrame.programCode ? "P" : "F");
+                let icon = callFrame.isTailDeleted ? `-${code}-` : `[${code}]`;
+                InspectorTest.log(`${callFrameIndex}: ${icon} ${label}`);
+                if ("includeThisObject" in options)
+                    await logThisObject(callFrame);
+                if ("includeScope" in options)
+                    await logScope(callFrame, options.includeScope);
+            }
+            callFrameIndex++;
+        }
+
+        InspectorTest.log("CALL STACK:");
+
+        let stackTrace = getAsyncStackTrace();
+        while (stackTrace) {
+            let callFrames = stackTrace.callFrames;
+            let topCallFrameIsBoundary = stackTrace.topCallFrameIsBoundary;
+            let truncated = stackTrace.truncated;
+            stackTrace = stackTrace.parentStackTrace;
+            if (!callFrames || !callFrames.length)
+                continue;
+
+            if (topCallFrameIsBoundary) {
+                if (!foundAsyncBoundary) {
+                    InspectorTest.log("ASYNC CALL STACK:");
+                    foundAsyncBoundary = true;
+                }
+                await logCallFrame(callFrames[0], true);
+            }
+
+            for (let i = topCallFrameIsBoundary ? 1 : 0; i < callFrames.length; ++i) {
+                let callFrame = callFrames[i];
+                await logCallFrame(callFrame);
+
+                // Skip call frames after the test harness entry point.
+                if (callFrame.programCode)
+                    break;
+            }
+
+            if (truncated)
+                InspectorTest.log("(remaining call frames truncated)");
+        }
+    };
+});
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-frames.js b/LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-frames.js
new file mode 100644 (file)
index 0000000..d4ab12d
--- /dev/null
@@ -0,0 +1,23 @@
+"use strict";
+function a() {
+    let x = 20;
+    debugger;
+    return x;
+}
+function b() {
+    let y = 40;
+    return a();
+}
+function c() {
+    let z = 60;
+    return b();
+}
+function startABC() {
+    for (let i = 0; i < 5; ++i)
+        THIS_DOES_NOT_CALL_c();
+    c();
+}
+function doNothing() { return 1; }
+function THIS_DOES_NOT_CALL_c() {
+    return doNothing();
+}
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-native-tail-deleted-calls.js b/LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-native-tail-deleted-calls.js
new file mode 100644 (file)
index 0000000..bcbd5a0
--- /dev/null
@@ -0,0 +1,22 @@
+"use strict";
+function a() {
+    let x = 20;
+    debugger;
+    return x;
+}
+function b() {
+    let y = 40;
+    return a();
+}
+function c() {
+    let z = 60;
+    return b();
+}
+function startABC() {
+    for (let i = 0; i < 5; ++i)
+        THIS_DOES_NOT_CALL_c();
+    c();
+}
+function THIS_DOES_NOT_CALL_c() {
+    return Math.random();
+}
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-tail-deleted-frames.js b/LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-intermediate-tail-deleted-frames.js
new file mode 100644 (file)
index 0000000..d4ab12d
--- /dev/null
@@ -0,0 +1,23 @@
+"use strict";
+function a() {
+    let x = 20;
+    debugger;
+    return x;
+}
+function b() {
+    let y = 40;
+    return a();
+}
+function c() {
+    let z = 60;
+    return b();
+}
+function startABC() {
+    for (let i = 0; i < 5; ++i)
+        THIS_DOES_NOT_CALL_c();
+    c();
+}
+function doNothing() { return 1; }
+function THIS_DOES_NOT_CALL_c() {
+    return doNothing();
+}
@@ -1,7 +1,7 @@
 "use strict";
 function a() {
     let x = 20;
-    x;
+    debugger;
     return x;
 }
 function b() {
@@ -10,7 +10,7 @@ function b() {
 }
 function c() {
     let z = 60;
-    return b(); 
+    return b();
 }
 function startABC() {
     c();
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-this-value.js b/LayoutTests/inspector/debugger/tail-deleted-frames/resources/tail-deleted-frames-this-value.js
new file mode 100644 (file)
index 0000000..85680df
--- /dev/null
@@ -0,0 +1,17 @@
+"use strict";
+function a() {
+    let x = 20;
+    debugger;
+    return x;
+}
+function b() {
+    let y = 40;
+    return a.call({aThis: 2});
+}
+function c() {
+    let z = 60;
+    return b.call({bThis: 1});
+}
+function startABC() {
+    c.call({cThis: 0});
+}
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames-expected.txt b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames-expected.txt
new file mode 100644 (file)
index 0000000..0b648f6
--- /dev/null
@@ -0,0 +1,14 @@
+Ensure proper tail deleted frames with different call stacks.
+
+
+== Running test suite: Debugger.TailDeletedFrames.IntermediateFrames
+-- Running test case: Debugger.TailDeletedFrames.IntermediateFrames
+PAUSED
+CALL STACK:
+0: [F] a
+1: -F- b
+2: -F- c
+3: [F] startABC
+4: [P] Global Code
+RESUMED
+
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames.html b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-frames.html
new file mode 100644 (file)
index 0000000..7c41b74
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../../http/tests/inspector/resources/inspector-test.js"></script>
+<script src="resources/stack-trace-utilities.js"></script>
+<script src="resources/tail-deleted-frames-intermediate-frames.js"></script>
+<script>
+function test()
+{
+    let suite = InspectorTest.createAsyncSuite("Debugger.TailDeletedFrames.IntermediateFrames");
+
+    suite.addTestCase({
+        name: "Debugger.TailDeletedFrames.IntermediateFrames",
+        description: "Ensure intermediate frames in a log do not show up in the call stack",
+        async test() {
+            // Trigger breakpoint.
+            InspectorTest.evaluateInPage(`startABC()`);
+
+            // Dump stack when paused.
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused);
+            InspectorTest.log("PAUSED");
+            await logAsyncStackTrace();
+
+            // Resume and end test.
+            WI.debuggerManager.resume();
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Resumed);
+            InspectorTest.log("RESUMED");
+        }
+    });
+
+    suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body onload="runTest()">
+<p>Ensure proper tail deleted frames with different call stacks.</p>
+</body>
+</html>
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls-expected.txt b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls-expected.txt
new file mode 100644 (file)
index 0000000..9c3d6a9
--- /dev/null
@@ -0,0 +1,20 @@
+Ensure proper tail deleted frames with different call stacks.
+
+
+== Running test suite: Debugger.TailDeletedFrames.IntermediateTailDeletedFrames
+-- Running test case: Debugger.TailDeletedFrames.IntermediateTailDeletedFrames
+FAIL: This should not have `THIS_DOES_NOT_CALL_c` in the call stack.
+PAUSED
+CALL STACK:
+0: [F] a
+1: -F- b
+2: -F- c
+3: -F- THIS_DOES_NOT_CALL_c
+4: -F- THIS_DOES_NOT_CALL_c
+5: -F- THIS_DOES_NOT_CALL_c
+6: -F- THIS_DOES_NOT_CALL_c
+7: -F- THIS_DOES_NOT_CALL_c
+8: [F] startABC
+9: [P] Global Code
+RESUMED
+
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls.html b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-native-tail-deleted-calls.html
new file mode 100644 (file)
index 0000000..3d0d328
--- /dev/null
@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../../http/tests/inspector/resources/inspector-test.js"></script>
+<script src="resources/stack-trace-utilities.js"></script>
+<script src="resources/tail-deleted-frames-intermediate-native-tail-deleted-calls.js"></script>
+<script>
+function test()
+{
+    let suite = InspectorTest.createAsyncSuite("Debugger.TailDeletedFrames.IntermediateTailDeletedFrames");
+
+    suite.addTestCase({
+        name: "Debugger.TailDeletedFrames.IntermediateTailDeletedFrames",
+        description: "Ensure intermediate tail deleted frames in a log do not show up in the call stack",
+        async test() {
+            // Trigger breakpoint.
+            InspectorTest.evaluateInPage(`startABC()`);
+
+            // FIXME: ShadowChicken only works if each tail call function emits a prologue
+            // on entry. Unfortunately native function calls do not emit a prologue, so the
+            // ShadowChicken stack cannot properly asses the tail call from `THIS_DOES_NOT_CALL_c`.
+            InspectorTest.fail("This should not have `THIS_DOES_NOT_CALL_c` in the call stack.")
+
+            // Dump stack when paused.
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused);
+            InspectorTest.log("PAUSED");
+            await logAsyncStackTrace();
+
+            // Resume and end test.
+            WI.debuggerManager.resume();
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Resumed);
+            InspectorTest.log("RESUMED");
+        }
+    });
+
+    suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body onload="runTest()">
+<p>Ensure proper tail deleted frames with different call stacks.</p>
+</body>
+</html>
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames-expected.txt b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames-expected.txt
new file mode 100644 (file)
index 0000000..9f184bc
--- /dev/null
@@ -0,0 +1,14 @@
+Ensure proper tail deleted frames with different call stacks.
+
+
+== Running test suite: Debugger.TailDeletedFrames.IntermediateTailDeletedFrames
+-- Running test case: Debugger.TailDeletedFrames.IntermediateTailDeletedFrames
+PAUSED
+CALL STACK:
+0: [F] a
+1: -F- b
+2: -F- c
+3: [F] startABC
+4: [P] Global Code
+RESUMED
+
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames.html b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-intermediate-tail-deleted-frames.html
new file mode 100644 (file)
index 0000000..fce932d
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../../http/tests/inspector/resources/inspector-test.js"></script>
+<script src="resources/stack-trace-utilities.js"></script>
+<script src="resources/tail-deleted-frames-intermediate-tail-deleted-frames.js"></script>
+<script>
+function test()
+{
+    let suite = InspectorTest.createAsyncSuite("Debugger.TailDeletedFrames.IntermediateTailDeletedFrames");
+
+    suite.addTestCase({
+        name: "Debugger.TailDeletedFrames.IntermediateTailDeletedFrames",
+        description: "Ensure intermediate tail deleted frames in a log do not show up in the call stack",
+        async test() {
+            // Trigger breakpoint.
+            InspectorTest.evaluateInPage(`startABC()`);
+
+            // Dump stack when paused.
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused);
+            InspectorTest.log("PAUSED");
+            await logAsyncStackTrace();
+
+            // Resume and end test.
+            WI.debuggerManager.resume();
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Resumed);
+            InspectorTest.log("RESUMED");
+        }
+    });
+
+    suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body onload="runTest()">
+<p>Ensure proper tail deleted frames with different call stacks.</p>
+</body>
+</html>
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes-expected.txt b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes-expected.txt
new file mode 100644 (file)
index 0000000..9cfd2c9
--- /dev/null
@@ -0,0 +1,29 @@
+Ensure proper scopes in tail deleted frames.
+
+
+== Running test suite: Debugger.TailDeletedFrames.Scopes
+-- Running test case: Debugger.TailDeletedFrames.Scopes
+PAUSED
+CALL STACK:
+0: [F] a
+  ----Scope----
+  x: 20
+  -------------
+1: -F- b
+  ----Scope----
+  y: 40
+  -------------
+2: -F- c
+  ----Scope----
+  z: 60
+  -------------
+3: [F] startABC
+  ----Scope----
+  --  empty  --
+  -------------
+4: [P] Global Code
+  ----Scope----
+  --  empty  --
+  -------------
+RESUMED
+
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes.html b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-scopes.html
new file mode 100644 (file)
index 0000000..f3fed94
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../../http/tests/inspector/resources/inspector-test.js"></script>
+<script src="resources/stack-trace-utilities.js"></script>
+<script src="resources/tail-deleted-frames-scopes.js"></script>
+<script>
+function test()
+{
+    let suite = InspectorTest.createAsyncSuite("Debugger.TailDeletedFrames.Scopes");
+
+    suite.addTestCase({
+        name: "Debugger.TailDeletedFrames.Scopes",
+        description: "Ensure scopes in tail deleted frames",
+        async test() {
+            // Trigger breakpoint.
+            InspectorTest.evaluateInPage(`startABC()`);
+
+            // Dump stack when paused.
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused);
+            InspectorTest.log("PAUSED");
+            await logAsyncStackTrace({includeScope: 0});
+
+            // Resume and end test.
+            WI.debuggerManager.resume();
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Resumed);
+            InspectorTest.log("RESUMED");
+        }
+    });
+
+    suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body onload="runTest()">
+<p>Ensure proper scopes in tail deleted frames.</p>
+</body>
+</html>
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value-expected.txt b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value-expected.txt
new file mode 100644 (file)
index 0000000..e82a799
--- /dev/null
@@ -0,0 +1,19 @@
+Ensure proper this value in tail deleted frames.
+
+
+== Running test suite: Debugger.TailDeletedFrames.ThisValue
+-- Running test case: Debugger.TailDeletedFrames.Scopes
+PAUSED
+CALL STACK:
+0: [F] a
+  this: aThis - 2
+1: -F- b
+  this: bThis - 1
+2: -F- c
+  this: cThis - 0
+3: [F] startABC
+  this: undefined
+4: [P] Global Code
+  this: test - 
+RESUMED
+
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value.html b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value.html
new file mode 100644 (file)
index 0000000..fe0e0b7
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../../http/tests/inspector/resources/inspector-test.js"></script>
+<script src="resources/stack-trace-utilities.js"></script>
+<script src="resources/tail-deleted-frames-this-value.js"></script>
+<script>
+function test()
+{
+    let suite = InspectorTest.createAsyncSuite("Debugger.TailDeletedFrames.ThisValue");
+
+    suite.addTestCase({
+        name: "Debugger.TailDeletedFrames.Scopes",
+        description: "Ensure thisObject in tail deleted frames",
+        async test() {
+            // Trigger breakpoint.
+            InspectorTest.evaluateInPage(`startABC()`);
+
+            // Dump stack when paused.
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused);
+            InspectorTest.log("PAUSED");
+            await logAsyncStackTrace({includeThisObject:true});
+
+            // Resume and end test.
+            WI.debuggerManager.resume();
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Resumed);
+            InspectorTest.log("RESUMED");
+        }
+    });
+
+    suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body onload="runTest()">
+<p>Ensure proper this value in tail deleted frames.</p>
+</body>
+</html>
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry-expected.txt b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry-expected.txt
new file mode 100644 (file)
index 0000000..d9c743e
--- /dev/null
@@ -0,0 +1,57 @@
+Ensure proper values in tail deleted frames.
+
+
+== Running test suite: Debugger.TailDeletedFrames.VMEntry
+-- Running test case: Debugger.TailDeletedFrames.Scopes
+PAUSED
+CALL STACK:
+0: [F] bar
+  ----Scope----
+  i: 0
+  -------------
+1: -F- bar
+  ----Scope----
+  i: 1
+  -------------
+2: -F- bar
+  ----Scope----
+  i: 2
+  -------------
+3: -F- bar
+  ----Scope----
+  i: 3
+  -------------
+4: -F- bar
+  ----Scope----
+  i: 4
+  -------------
+5: -F- bar
+  ----Scope----
+  i: 5
+  -------------
+6: -F- bar
+  ----Scope----
+  i: 6
+  -------------
+7: -F- bar
+  ----Scope----
+  i: 7
+  -------------
+8: -F- bar
+  ----Scope----
+  i: 8
+  -------------
+9: -F- bar
+  ----Scope----
+  i: 9
+  -------------
+10: -F- timeout
+  ----Scope----
+  foo: 25
+  -------------
+ASYNC CALL STACK:
+11: --- setTimeout ---
+12: [P] Global Code
+  ----Scope----
+RESUMED
+
diff --git a/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry.html b/LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry.html
new file mode 100644 (file)
index 0000000..5b2be0a
--- /dev/null
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../../http/tests/inspector/resources/inspector-test.js"></script>
+<script src="resources/stack-trace-utilities.js"></script>
+<script src="resources/tail-deleted-frames-vm-entry.js"></script>
+<script>
+function test()
+{
+    InspectorTest.debug();
+    let suite = InspectorTest.createAsyncSuite("Debugger.TailDeletedFrames.VMEntry");
+
+    suite.addTestCase({
+        name: "Debugger.TailDeletedFrames.Scopes",
+        description: "Ensure proper values in tail deleted frames",
+        async test() {
+            // Trigger breakpoint.
+            InspectorTest.evaluateInPage(`setTimeout(timeout)`);
+
+            // Dump stack when paused.
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused);
+            InspectorTest.log("PAUSED");
+            await logAsyncStackTrace({includeScope: 1});
+
+            // Resume and end test.
+            WI.debuggerManager.resume();
+            await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Resumed);
+            InspectorTest.log("RESUMED");
+        }
+    });
+
+    suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body onload="runTest()">
+<p>Ensure proper values in tail deleted frames.</p>
+</body>
+</html>
index 3675649..ec77b62 100644 (file)
@@ -1059,8 +1059,7 @@ webkit.org/b/168090 [ Debug ] inspector/debugger/breakpoint-columns.html [ Pass
 webkit.org/b/161951 [ Release ] inspector/debugger/breakpoints/resolved-dump-each-line.html [ Pass Timeout ]
 webkit.org/b/167711 [ Debug ] inspector/debugger/probe-manager-add-remove-actions.html [ Slow ]
 webkit.org/b/168399 [ Debug ] inspector/debugger/search-scripts.html [ Pass Timeout ]
-webkit.org/b/181952 [ Debug ] inspector/debugger/tail-deleted-frames-from-vm-entry.html [ Slow ]
-webkit.org/b/169119 [ Debug ] inspector/debugger/tail-deleted-frames-this-value.html [ Pass Timeout ]
+webkit.org/b/181952 [ Debug ] inspector/debugger/tail-deleted-frames/tail-deleted-frames-vm-entry.html [ Slow ]
 webkit.org/b/168387 [ Debug ] inspector/debugger/tail-recursion.html [ Pass Timeout ]
 webkit.org/b/170127 inspector/dom-debugger/dom-breakpoints.html [ Pass Timeout ]
 webkit.org/b/148636 inspector/dom/getAccessibilityPropertiesForNode.html [ Pass Timeout ]
index 631fa8e..49f6ae8 100644 (file)
@@ -1,3 +1,128 @@
+2019-09-05  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Tail Deleted Frames shown in Web Inspector are sometimes incorrect (Shadow Chicken)
+        https://bugs.webkit.org/show_bug.cgi?id=201366
+
+        Reviewed by Saam Barati.
+
+        It is possible for the log buffer to be full right as someone is trying to
+        log a function prologue. In such a case the machine stack has already been
+        updated to include the new JavaScript call frame, but the prologue packet
+        cannot be included in the update because the log is full. This would mean
+        that the update fails to rationalize the machine stack with the shadow
+        log / stack. Namely, the current JavaScript call frame is unable to
+        find a matching prologue (the one we are holding to include after the update)
+        and inserts a questionable value into the stack; and in the process
+        missing and removing real potential tail calls.
+
+        For example:
+        
+            "use strict";
+            function third() { return 1; }
+            function second() { return third(); }
+            function first() { return second(); }
+            function start() { return first(); }
+
+        If the the log fills up just as we are entering `b` then we may have a list
+        full log of packets looking like:
+
+          Shadow Log:
+            ...
+            { prologue-packet: entering `start` ... }
+            { prologue-packet: entering `first` ... }
+            { tail-packet: leaving `first` with a tail call }
+
+          Incoming Packet:
+            { prologue-packet: entering `second` ... }
+
+          Current JS Stack:
+            second
+            start
+
+        Since the Current JavaScript stack already has `second`, if we process the
+        log without the prologue for `second` then we push a confused entry on the
+        shadow stack and clear the log such that we eventually lose the tail-call
+        information for `first` to `second`.
+
+        This patch solves this issue by providing enough extra space in the log
+        to always process the incoming packet when that forces an update. This way
+        clients can continue to behave exactly as they are.
+
+        --
+
+        We also document a corner case in some circumstances where the shadow
+        log may currently be insufficient to know how to reconcile:
+        
+        For example:
+
+            "use strict";
+            function third() { return 1; }
+            function second() { return third(); }
+            function first() { return second(); }
+            function doNothingTail() { return Math.random() }
+            function start() {
+                for (i=0;i<1000;++i) doNothingTail();
+                return first();
+            }
+
+        In this case the ShadowChicken log may be processed multiple times due
+        to the many calls to `doNothingTail` / `Math.random()`. When calling the
+        Native function no prologue packet is emitted, so it is unclear that we
+        temporarly go deeper and come back out on the stack, so the log appears
+        to have lots of doNothingTail calls reusing the same frame:
+
+          Shadow Log:
+            ...
+            , [123] {callee = 0x72a21aee0, frame = 0x7ffeef897270, callerFrame = 0x7ffeef8972e0, name = start}
+            , [124] {callee = 0x72a21af10, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = doNothingTail}
+            , [125] tail-packet:{frame = 0x7ffeef8971f0}
+            , [126] {callee = 0x72a21af10, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = doNothingTail}
+            , [127] tail-packet:{frame = 0x7ffeef8971f0}
+            ...
+            , [140] {callee = 0x72a21af10, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = doNothingTail}
+            , [141] tail-packet:{frame = 0x7ffeef8971f0}
+            , [142] {callee = 0x72a21af10, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = doNothingTail}
+            , [143] tail-packet:{frame = 0x7ffeef8971f0}
+            , [144] {callee = 0x72a21aeb0, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = first}
+            , [145] tail-packet:{frame = 0x7ffeef8971f0}
+            , [146] {callee = 0x72a21ae80, frame = 0x7ffeef8971f0, callerFrame = 0x7ffeef897270, name = second}
+            ...
+
+        This log would seem to be indistinguishable from real tail recursion, such as:
+
+            "use strict";
+            function third() { return 1; }
+            function second() { return third(); }
+            function first() { return second(); }
+            function doNothingTail(n) {
+                return n ? doNothingTail(n-1) : first();
+            }
+            function start() {
+                return doNothingTail(1000);
+            }
+
+        Likewise there are more cases where the shadow log appears to be ambiguous with determining
+        the appropriate parent call frame with intermediate function calls. In practice this may
+        not be too problematic, as this is a best effort reconstruction of tail deleted frames.
+        It seems likely we would only show additional frames that did in fact happen serially
+        between JavaScript call frames, but may not actually be the proper parent frames
+        heirachy in the stack.
+
+        * interpreter/ShadowChicken.cpp:
+        (JSC::ShadowChicken::Packet::dump const):
+        (JSC::ShadowChicken::Frame::dump const):
+        (JSC::ShadowChicken::dump const):
+        Improved debugging output. Especially for functions.
+
+        (JSC::ShadowChicken::ShadowChicken):
+        Make space in the log for 1 additional packet to process when we slow log.
+
+        (JSC::ShadowChicken::log):
+        Include this packet in our update.
+
+        (JSC::ShadowChicken::update):
+        Address an edge case where we can eliminate tail-deleted frames that don't make sense.
+
 2019-09-05  Mark Lam  <mark.lam@apple.com>
 
         Refactor the Gigacage code to require less pointer casting.
index 6c50a32..f73264c 100644 (file)
@@ -45,9 +45,16 @@ void ShadowChicken::Packet::dump(PrintStream& out) const
     }
     
     if (isPrologue()) {
+        String name = "?"_s;
+        if (auto* function = jsDynamicCast<JSFunction*>(callee->vm(), callee)) {
+            name = function->name(callee->vm());
+            if (name.isEmpty())
+                name = "?"_s;
+        }
+
         out.print(
             "{callee = ", RawPointer(callee), ", frame = ", RawPointer(frame), ", callerFrame = ",
-            RawPointer(callerFrame), "}");
+            RawPointer(callerFrame), ", name = ", name, "}");
         return;
     }
     
@@ -62,15 +69,27 @@ void ShadowChicken::Packet::dump(PrintStream& out) const
 
 void ShadowChicken::Frame::dump(PrintStream& out) const
 {
+    String name = "?"_s;
+    if (auto* function = jsDynamicCast<JSFunction*>(callee->vm(), callee)) {
+        name = function->name(callee->vm());
+        if (name.isEmpty())
+            name = "?"_s;
+    }
+
     out.print(
-        "{callee = ", RawPointer(callee), ", frame = ", RawPointer(frame), ", isTailDeleted = ",
-        isTailDeleted, "}");
+        "{callee = ", *callee, ", frame = ", RawPointer(frame), ", isTailDeleted = ",
+        isTailDeleted, ", name = ", name, "}");
 }
 
 ShadowChicken::ShadowChicken()
     : m_logSize(Options::shadowChickenLogSize())
 {
-    m_log = static_cast<Packet*>(fastZeroedMalloc(sizeof(Packet) * m_logSize));
+    // Allow one additional packet beyond m_logEnd. This is useful for the moment we
+    // log a packet when the log is full and force an update. At that moment the packet
+    // that is being logged should be included in the update because it may be
+    // a critical prologue needed to rationalize the current machine stack with the
+    // shadow stack.
+    m_log = static_cast<Packet*>(fastZeroedMalloc(sizeof(Packet) * m_logSize + 1));
     m_logCursor = m_log;
     m_logEnd = m_log + m_logSize;
 }
@@ -82,8 +101,9 @@ ShadowChicken::~ShadowChicken()
 
 void ShadowChicken::log(VM& vm, ExecState* exec, const Packet& packet)
 {
-    update(vm, exec);
+    // This write is allowed because we construct the log with space for 1 additional packet.
     *m_logCursor++ = packet;
+    update(vm, exec);
 }
 
 void ShadowChicken::update(VM& vm, ExecState* exec)
@@ -142,7 +162,6 @@ void ShadowChicken::update(VM& vm, ExecState* exec)
         }
     }
 
-    
     if (ShadowChickenInternal::verbose)
         dataLog("    Revised stack: ", listDump(m_stack), "\n");
     
@@ -288,11 +307,21 @@ void ShadowChicken::update(VM& vm, ExecState* exec)
             }
 
             CallFrame* callFrame = visitor->callFrame();
-            if (ShadowChickenInternal::verbose)
-                dataLog("    Examining ", RawPointer(callFrame), "\n");
+            if (ShadowChickenInternal::verbose) {
+                dataLog("    Examining callFrame:", RawPointer(callFrame), ", callee:", RawPointer(callFrame->jsCallee()), ", callerFrame:", RawPointer(callFrame->callerFrame()), "\n");
+                JSObject* callee = callFrame->jsCallee();
+                if (auto* function = jsDynamicCast<JSFunction*>(callee->vm(), callee))
+                    dataLog("      Function = ", function->name(callee->vm()), "\n");
+            }
+
             if (callFrame == highestPointSinceLastTime) {
                 if (ShadowChickenInternal::verbose)
-                    dataLog("    Bailing at ", RawPointer(callFrame), " because it's the highest point since last time.\n");
+                    dataLog("    Bailing at ", RawPointer(callFrame), " because it's the highest point since last time\n");
+
+                // FIXME: At this point the shadow stack may still have tail deleted frames
+                // that do not run into the current call frame but are left in the shadow stack.
+                // Those tail deleted frames should be validated somehow.
+
                 return StackVisitor::Done;
             }
 
@@ -318,7 +347,7 @@ void ShadowChicken::update(VM& vm, ExecState* exec)
                 // anything.
                 && m_log[indexInLog].frame == toPush.last().frame) {
                 if (ShadowChickenInternal::verbose)
-                    dataLog("    Going to loop through to find tail deleted frames with indexInLog = ", indexInLog, " and push-stack top = ", toPush.last(), "\n");
+                    dataLog("    Going to loop through to find tail deleted frames using ", RawPointer(callFrame), " with indexInLog = ", indexInLog, " and push-stack top = ", toPush.last(), "\n");
                 for (;;) {
                     ASSERT(m_log[indexInLog].frame == toPush.last().frame);
                     
@@ -340,6 +369,10 @@ void ShadowChicken::update(VM& vm, ExecState* exec)
                         break;
                     }
                     indexInLog--; // Skip over the tail packet.
+
+                    // FIXME: After a few iterations the tail packet referenced frame may not be the
+                    // same as the original callFrame for the real stack frame we started with.
+                    // It is unclear when we should break.
                     
                     if (!advanceIndexInLogTo(tailPacket.frame, nullptr, nullptr)) {
                         if (ShadowChickenInternal::verbose)
@@ -379,7 +412,7 @@ void ShadowChicken::update(VM& vm, ExecState* exec)
         m_logCursor = m_log;
 
     if (ShadowChickenInternal::verbose)
-        dataLog("    After pushing: ", *this, "\n");
+        dataLog("    After pushing: ", listDump(m_stack), "\n");
 
     // Remove tail frames until the number of tail deleted frames is small enough.
     const unsigned maxTailDeletedFrames = Options::shadowChickenMaxTailDeletedFramesSize();
@@ -447,7 +480,7 @@ void ShadowChicken::dump(PrintStream& out) const
     unsigned limit = static_cast<unsigned>(m_logCursor - m_log);
     out.print("\n");
     for (unsigned i = 0; i < limit; ++i)
-        out.print("\t", comma, m_log[i], "\n");
+        out.print("\t", comma, "[", i, "] ", m_log[i], "\n");
     out.print("]}");
 }