Skip to content

Commit f3d0984

Browse files
JosephPecoraroJosephPecoraro
committed
Web Inspector: Eliminate the crazy code for evaluateOnCallFrame
https://bugs.webkit.org/show_bug.cgi?id=157510 <rdar://problem/26191332> Reviewed by Timothy Hatcher. Source/JavaScriptCore: * debugger/DebuggerCallFrame.cpp: (JSC::DebuggerCallFrame::evaluateWithScopeExtension): Set and clear an optional scope extension object. * inspector/InjectedScriptSource.js: (InjectedScript.prototype.evaluate): (InjectedScript.prototype._evaluateOn): (InjectedScript.prototype.evaluateOnCallFrame): Unify the code to use the passed in evaluate function and object. When evaluating on a call frame the evaluate function ends up being DebuggerCallFrame::evaluateWithScopeExtension. When evaluating globally this ends up being JSInjectedScriptHost::evaluateWithScopeExtension. In both cases "object" is the preferred this object to use. * debugger/DebuggerCallFrame.h: * inspector/JSJavaScriptCallFrame.cpp: (Inspector::JSJavaScriptCallFrame::evaluateWithScopeExtension): (Inspector::JSJavaScriptCallFrame::evaluate): Deleted. * inspector/JSJavaScriptCallFrame.h: * inspector/JSJavaScriptCallFramePrototype.cpp: (Inspector::JSJavaScriptCallFramePrototype::finishCreation): (Inspector::jsJavaScriptCallFramePrototypeFunctionEvaluateWithScopeExtension): * inspector/JavaScriptCallFrame.h: (Inspector::JavaScriptCallFrame::evaluateWithScopeExtension): (Inspector::JavaScriptCallFrame::evaluate): Deleted. Pass through to DebuggerCallFrame with the proper arguments. * debugger/Debugger.cpp: (JSC::Debugger::hasBreakpoint): * inspector/ScriptDebugServer.cpp: (Inspector::ScriptDebugServer::evaluateBreakpointAction): Use the new evaluate on call frame method name and no scope extension object. LayoutTests: * inspector/debugger/evaluateOnCallFrame-CommandLineAPI-expected.txt: Added. * inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html: Added. Canonical link: https://commits.webkit.org/175663@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@200634 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 8f614fc commit f3d0984

13 files changed

Lines changed: 376 additions & 72 deletions

‎LayoutTests/ChangeLog‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
2016-05-10 Joseph Pecoraro <pecoraro@apple.com>
2+
3+
Web Inspector: Eliminate the crazy code for evaluateOnCallFrame
4+
https://bugs.webkit.org/show_bug.cgi?id=157510
5+
<rdar://problem/26191332>
6+
7+
Reviewed by Timothy Hatcher.
8+
9+
* inspector/debugger/evaluateOnCallFrame-CommandLineAPI-expected.txt: Added.
10+
* inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html: Added.
11+
112
2016-05-05 Jer Noble <jer.noble@apple.com>
213

314
Return a Promise from HTMLMediaElement.play()

‎LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI-expected.txt‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
Tests for the Debugger.evaluateOnCallFrame with the Command Line API.
2+
3+
4+
== Running test suite: Debugger.evaluateOnCallFrame.CommandLineAPI
5+
-- Running test case: ValidateCallFrames
6+
PASS: Strict call frame is `bar`.
7+
PASS: Non-strict call frame is `foo`.
8+
PASS: `a` should be 5 in `bar`.
9+
PASS: `b` should be 123 in `bar`.
10+
PASS: `c` should be 0 in `bar`.
11+
PASS: `this` should be undefined in `bar`.
12+
PASS: `a` should be 1 in `foo`.
13+
PASS: `b` should be 2 in `foo`.
14+
PASS: `c` should be 6 in `foo`.
15+
PASS: `this` should be 'my-this-object' in `foo`.
16+
17+
-- Running test case: AccessCommandLineAPI
18+
PASS: CommandLineAPI `keys` can be accessed in the `bar` strict call frame.
19+
PASS: CommandLineAPI `keys` can be accessed in the `foo` non-strict call frame.
20+
PASS: CommandLineAPI `keys` should work with a simple object.
21+
22+
-- Running test case: AccessGlobalVariable
23+
PASS: Should be able to access var in global scope in strict call frame.
24+
PASS: Should be able to access let in global scope in strict call frame.
25+
PASS: Should be able to access const in global scope in strict call frame.
26+
PASS: Should be able to access var in global scope in non-strict call frame.
27+
PASS: Should be able to access let in global scope in non-strict call frame.
28+
PASS: Should be able to access const in global scope in non-strict call frame.
29+
30+
-- Running test case: NoVariablesCreatedOnCallFrame
31+
PASS: Should not be able to access local var created in earlier eval on strict call frame.
32+
PASS: Should not be able to access local let created in earlier eval on strict call frame.
33+
PASS: Should not be able to access local const created in earlier eval on strict call frame.
34+
PASS: Should be able to access local var created in earlier eval on non-strict call frame.
35+
PASS: Should not be able to access local let created in earlier eval on non-strict call frame.
36+
PASS: Should not be able to access local const created in earlier eval on non-strict call frame.
37+
38+
-- Running test case: NonStrictAndStrictEvaluations
39+
PASS: Non-strict evaluation. Should be able to access arguments.callee.
40+
PASS: Strict evaluation. Should not be able to access arguments.callee.
41+
42+
-- Running test case: CommandLineAPIDoesNotShadowLocalVariables
43+
PASS: Local variable `keys` should not be shadowed by CommandLineAPI `keys` function in call frame for `foo`.
44+
45+
-- Running test case: CommandLineAPIDoesNotShadowGlobalVariables
46+
PASS: CommandLineAPI `keys` can be accessed in the `bar` strict call frame.
47+
PASS: Global assignment to `keys` should be okay.
48+
PASS: Global variable `keys` should not be shadowed by CommandLineAPI `keys` function in call frame for `bar`.
49+
PASS: CommandLineAPI `keys` can be accessed in the `bar` strict call frame after deleting global variable `keys`.
50+
51+
-- Running test case: CommandLineAPIDoesNotShadowGlobalObjectProperties
52+
PASS: `values` should be `window.values` and not shadowed by CommandLineAPI `values` function in strict call frame.
53+
PASS: `values` should be `window.values` and not shadowed by CommandLineAPI `values` function in non-strict call frame.
54+
55+
-- Running test case: Complete
56+

‎LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html‎

Lines changed: 230 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,230 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
5+
<script>
6+
var varGlobalVariable = 1;
7+
let letGlobalVariable = 2;
8+
const constGlobalVariable = 3;
9+
10+
function foo(a, b) {
11+
var c = a + b + arguments[2];
12+
var keys = 123;
13+
bar(c, keys, -129);
14+
}
15+
16+
function bar(a, b) {
17+
"use strict";
18+
let c = b + a + arguments[2];
19+
debugger;
20+
return c;
21+
}
22+
23+
function triggerPause() {
24+
foo.call("my-this-object", 1, 2, 3);
25+
}
26+
27+
function test()
28+
{
29+
const objectGroup = "test";
30+
const includeCommandLineAPI = true;
31+
const returnByValue = true;
32+
33+
function testEvaluateOnCallFrame(callFrame, expression, callback) {
34+
let callFrameId = callFrame.id;
35+
DebuggerAgent.evaluateOnCallFrame.invoke({callFrameId, expression, objectGroup, includeCommandLineAPI, returnByValue}, (error, resultValue, wasThrown) => {
36+
InspectorTest.assert(!error, "Should not be a protocol error.");
37+
InspectorTest.assert(!wasThrown, "Should not be a runtime error.");
38+
if (callback)
39+
callback(resultValue.value);
40+
});
41+
}
42+
43+
function testEvaluateOnCallFrameThrows(callFrame, expression, callback) {
44+
let callFrameId = callFrame.id;
45+
DebuggerAgent.evaluateOnCallFrame.invoke({callFrameId, expression, objectGroup, includeCommandLineAPI, returnByValue}, (error, resultValue, wasThrown) => {
46+
InspectorTest.assert(!error, "Should not be a protocol error.");
47+
InspectorTest.assert(wasThrown, "Should be a runtime error.");
48+
if (callback)
49+
callback(wasThrown);
50+
});
51+
}
52+
53+
function testEvaluateOnStrictCallFrame(expression, callback) {
54+
let callFrame = WebInspector.debuggerManager.activeCallFrame; // bar
55+
testEvaluateOnCallFrame(callFrame, expression, callback);
56+
}
57+
58+
function testEvaluateOnStrictCallFrameThrows(expression, callback) {
59+
let callFrame = WebInspector.debuggerManager.activeCallFrame; // bar
60+
testEvaluateOnCallFrameThrows(callFrame, expression, callback);
61+
}
62+
63+
function testEvaluateOnNonStrictCallFrame(expression, callback) {
64+
let callFrame = WebInspector.debuggerManager.callFrames[1]; // foo
65+
testEvaluateOnCallFrame(callFrame, expression, callback);
66+
}
67+
68+
function testEvaluateOnNonStrictCallFrameThrows(expression, callback) {
69+
let callFrame = WebInspector.debuggerManager.callFrames[1]; // foo
70+
testEvaluateOnCallFrameThrows(callFrame, expression, callback);
71+
}
72+
73+
let suite = InspectorTest.createAsyncSuite("Debugger.evaluateOnCallFrame.CommandLineAPI");
74+
75+
suite.addTestCase({
76+
name: "ValidateCallFrames",
77+
description: "Test6 evaluate can access CommandLineAPI methods.",
78+
test: (resolve, reject) => {
79+
InspectorTest.expectThat(WebInspector.debuggerManager.activeCallFrame.functionName === "bar", "Strict call frame is `bar`.");
80+
InspectorTest.expectThat(WebInspector.debuggerManager.callFrames[1].functionName === "foo", "Non-strict call frame is `foo`.");
81+
82+
testEvaluateOnStrictCallFrame("a", (x) => { InspectorTest.expectThat(x === 6, "`a` should be 5 in `bar`."); });
83+
testEvaluateOnStrictCallFrame("b", (x) => { InspectorTest.expectThat(x === 123, "`b` should be 123 in `bar`."); });
84+
testEvaluateOnStrictCallFrame("c", (x) => { InspectorTest.expectThat(x === 0, "`c` should be 0 in `bar`."); });
85+
testEvaluateOnStrictCallFrame("this", (x) => { InspectorTest.expectThat(x === undefined, "`this` should be undefined in `bar`."); });
86+
87+
testEvaluateOnNonStrictCallFrame("a", (x) => { InspectorTest.expectThat(x === 1, "`a` should be 1 in `foo`."); });
88+
testEvaluateOnNonStrictCallFrame("b", (x) => { InspectorTest.expectThat(x === 2, "`b` should be 2 in `foo`."); });
89+
testEvaluateOnNonStrictCallFrame("c", (x) => { InspectorTest.expectThat(x === 6, "`c` should be 6 in `foo`."); });
90+
testEvaluateOnNonStrictCallFrame("this.toString()", (x) => { InspectorTest.expectThat(x === "my-this-object", "`this` should be 'my-this-object' in `foo`."); resolve(); });
91+
}
92+
});
93+
94+
suite.addTestCase({
95+
name: "AccessCommandLineAPI",
96+
description: "Test evaluate can access CommandLineAPI methods.",
97+
test: (resolve, reject) => {
98+
testEvaluateOnStrictCallFrame("keys.toString()", (resultValue) => {
99+
InspectorTest.expectThat(resultValue.includes("[Command Line API]"), "CommandLineAPI `keys` can be accessed in the `bar` strict call frame.");
100+
});
101+
testEvaluateOnNonStrictCallFrame("dir.toString()", (resultValue) => {
102+
InspectorTest.expectThat(resultValue.includes("[Command Line API]"), "CommandLineAPI `keys` can be accessed in the `foo` non-strict call frame.");
103+
});
104+
testEvaluateOnStrictCallFrame("keys({a:1, b:2})", (resultValue) => {
105+
InspectorTest.expectThat(resultValue.length === 2 && resultValue[0] === "a" && resultValue[1] === "b", "CommandLineAPI `keys` should work with a simple object.");
106+
resolve();
107+
});
108+
}
109+
});
110+
111+
suite.addTestCase({
112+
name: "AccessGlobalVariable",
113+
description: "Test evaluate can access global variables.",
114+
test: (resolve, reject) => {
115+
testEvaluateOnStrictCallFrame("varGlobalVariable", (x) => { InspectorTest.expectThat(x === 1, "Should be able to access var in global scope in strict call frame."); });
116+
testEvaluateOnStrictCallFrame("letGlobalVariable", (x) => { InspectorTest.expectThat(x === 2, "Should be able to access let in global scope in strict call frame."); });
117+
testEvaluateOnStrictCallFrame("constGlobalVariable", (x) => { InspectorTest.expectThat(x === 3, "Should be able to access const in global scope in strict call frame."); });
118+
119+
testEvaluateOnNonStrictCallFrame("varGlobalVariable", (x) => { InspectorTest.expectThat(x === 1, "Should be able to access var in global scope in non-strict call frame."); });
120+
testEvaluateOnNonStrictCallFrame("letGlobalVariable", (x) => { InspectorTest.expectThat(x === 2, "Should be able to access let in global scope in non-strict call frame."); });
121+
testEvaluateOnNonStrictCallFrame("constGlobalVariable", (x) => { InspectorTest.expectThat(x === 3, "Should be able to access const in global scope in non-strict call frame."); resolve(); });
122+
}
123+
});
124+
125+
suite.addTestCase({
126+
name: "NoVariablesCreatedOnCallFrame",
127+
description: "Test evaluate may not create new local variables.",
128+
test: (resolve, reject) => {
129+
testEvaluateOnStrictCallFrame(`
130+
var createdVarLocalVariable = 1;
131+
let createdLetLocalVariable = 2;
132+
const createdConstLocalVariable = 3;
133+
`);
134+
testEvaluateOnStrictCallFrameThrows("createdVarLocalVariable", (wasThrown) => { InspectorTest.expectThat(wasThrown, "Should not be able to access local var created in earlier eval on strict call frame."); });
135+
testEvaluateOnStrictCallFrameThrows("createdLetLocalVariable", (wasThrown) => { InspectorTest.expectThat(wasThrown, "Should not be able to access local let created in earlier eval on strict call frame."); });
136+
testEvaluateOnStrictCallFrameThrows("createdConstLocalVariable", (wasThrown) => { InspectorTest.expectThat(wasThrown, "Should not be able to access local const created in earlier eval on strict call frame."); });
137+
138+
testEvaluateOnNonStrictCallFrame(`
139+
var createdVarLocalVariable = 1;
140+
let createdLetLocalVariable = 2;
141+
const createdConstLocalVariable = 3;
142+
`);
143+
testEvaluateOnNonStrictCallFrame("createdVarLocalVariable", (resultValue) => { InspectorTest.expectThat(resultValue === 1, "Should be able to access local var created in earlier eval on non-strict call frame."); });
144+
testEvaluateOnNonStrictCallFrameThrows("createdLetLocalVariable", (wasThrown) => { InspectorTest.expectThat(wasThrown, "Should not be able to access local let created in earlier eval on non-strict call frame."); });
145+
testEvaluateOnNonStrictCallFrameThrows("createdConstLocalVariable", (wasThrown) => { InspectorTest.expectThat(wasThrown, "Should not be able to access local const created in earlier eval on non-strict call frame."); resolve(); });
146+
}
147+
});
148+
149+
suite.addTestCase({
150+
name: "NonStrictAndStrictEvaluations",
151+
description: "Test evaluate can run strict and non-strict programs.",
152+
test: (resolve, reject) => {
153+
// Non strict, this is okay.
154+
testEvaluateOnNonStrictCallFrame("arguments.callee.name", (resultValue) => {
155+
InspectorTest.expectThat(resultValue === "foo", "Non-strict evaluation. Should be able to access arguments.callee.");
156+
});
157+
// Strict, this throw an exception.
158+
testEvaluateOnStrictCallFrameThrows("arguments.callee.name", (wasThrown) => {
159+
InspectorTest.expectThat(wasThrown, "Strict evaluation. Should not be able to access arguments.callee.");
160+
resolve();
161+
});
162+
}
163+
});
164+
165+
suite.addTestCase({
166+
name: "CommandLineAPIDoesNotShadowLocalVariables",
167+
description: "Test CommandLineAPI does not shadow local variables.",
168+
test: (resolve, reject) => {
169+
testEvaluateOnNonStrictCallFrame("keys", (resultValue) => {
170+
InspectorTest.expectThat(resultValue === 123, "Local variable `keys` should not be shadowed by CommandLineAPI `keys` function in call frame for `foo`.");
171+
resolve();
172+
});
173+
}
174+
});
175+
176+
suite.addTestCase({
177+
name: "CommandLineAPIDoesNotShadowGlobalVariables",
178+
description: "Test CommandLineAPI does not shadow global variables.",
179+
test: (resolve, reject) => {
180+
testEvaluateOnStrictCallFrame("keys.toString()", (resultValue) => {
181+
InspectorTest.expectThat(resultValue.includes("[Command Line API]"), "CommandLineAPI `keys` can be accessed in the `bar` strict call frame.");
182+
});
183+
testEvaluateOnStrictCallFrame("window.keys = 999", (resultValue) => {
184+
InspectorTest.expectThat(resultValue === 999, "Global assignment to `keys` should be okay.");
185+
});
186+
testEvaluateOnStrictCallFrame("keys", (resultValue) => {
187+
InspectorTest.expectThat(resultValue === 999, "Global variable `keys` should not be shadowed by CommandLineAPI `keys` function in call frame for `bar`.");
188+
});
189+
testEvaluateOnStrictCallFrame("delete window.keys");
190+
testEvaluateOnStrictCallFrame("keys.toString()", (resultValue) => {
191+
InspectorTest.expectThat(resultValue.includes("[Command Line API]"), "CommandLineAPI `keys` can be accessed in the `bar` strict call frame after deleting global variable `keys`.");
192+
resolve();
193+
});
194+
}
195+
});
196+
197+
suite.addTestCase({
198+
name: "CommandLineAPIDoesNotShadowGlobalObjectProperties",
199+
description: "Test CommandLineAPI does not shadow global object properties.",
200+
test: (resolve, reject) => {
201+
testEvaluateOnStrictCallFrame("values.toString()", (resultValue) => {
202+
InspectorTest.expectThat(resultValue === "[object HTMLDivElement]", "`values` should be `window.values` and not shadowed by CommandLineAPI `values` function in strict call frame.");
203+
});
204+
testEvaluateOnNonStrictCallFrame("values.toString()", (resultValue) => {
205+
InspectorTest.expectThat(resultValue === "[object HTMLDivElement]", "`values` should be `window.values` and not shadowed by CommandLineAPI `values` function in non-strict call frame.");
206+
resolve();
207+
});
208+
}
209+
});
210+
211+
suite.addTestCase({
212+
name: "Complete",
213+
test: (resolve, reject) => {
214+
WebInspector.debuggerManager.resume();
215+
resolve();
216+
}
217+
})
218+
219+
InspectorTest.evaluateInPage("triggerPause()");
220+
WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
221+
suite.runTestCasesAndFinish();
222+
});
223+
}
224+
</script>
225+
</head>
226+
<body onload="runTest()">
227+
<div id="values"></div> <!-- This introduces the named property `window.values` on the window global object. -->
228+
<p>Tests for the Debugger.evaluateOnCallFrame with the Command Line API.</p>
229+
</body>
230+
</html>

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,44 @@
1+
2016-05-10 Joseph Pecoraro <pecoraro@apple.com>
2+
3+
Web Inspector: Eliminate the crazy code for evaluateOnCallFrame
4+
https://bugs.webkit.org/show_bug.cgi?id=157510
5+
<rdar://problem/26191332>
6+
7+
Reviewed by Timothy Hatcher.
8+
9+
* debugger/DebuggerCallFrame.cpp:
10+
(JSC::DebuggerCallFrame::evaluateWithScopeExtension):
11+
Set and clear an optional scope extension object.
12+
13+
* inspector/InjectedScriptSource.js:
14+
(InjectedScript.prototype.evaluate):
15+
(InjectedScript.prototype._evaluateOn):
16+
(InjectedScript.prototype.evaluateOnCallFrame):
17+
Unify the code to use the passed in evaluate function and object.
18+
When evaluating on a call frame the evaluate function ends up being
19+
DebuggerCallFrame::evaluateWithScopeExtension. When evaluating globally
20+
this ends up being JSInjectedScriptHost::evaluateWithScopeExtension.
21+
In both cases "object" is the preferred this object to use.
22+
23+
* debugger/DebuggerCallFrame.h:
24+
* inspector/JSJavaScriptCallFrame.cpp:
25+
(Inspector::JSJavaScriptCallFrame::evaluateWithScopeExtension):
26+
(Inspector::JSJavaScriptCallFrame::evaluate): Deleted.
27+
* inspector/JSJavaScriptCallFrame.h:
28+
* inspector/JSJavaScriptCallFramePrototype.cpp:
29+
(Inspector::JSJavaScriptCallFramePrototype::finishCreation):
30+
(Inspector::jsJavaScriptCallFramePrototypeFunctionEvaluateWithScopeExtension):
31+
* inspector/JavaScriptCallFrame.h:
32+
(Inspector::JavaScriptCallFrame::evaluateWithScopeExtension):
33+
(Inspector::JavaScriptCallFrame::evaluate): Deleted.
34+
Pass through to DebuggerCallFrame with the proper arguments.
35+
36+
* debugger/Debugger.cpp:
37+
(JSC::Debugger::hasBreakpoint):
38+
* inspector/ScriptDebugServer.cpp:
39+
(Inspector::ScriptDebugServer::evaluateBreakpointAction):
40+
Use the new evaluate on call frame method name and no scope extension object.
41+
142
2016-05-10 Saam barati <sbarati@apple.com>
243

344
Make super-property-access.js test run for less time because it was timing out in debug builds.

‎Source/JavaScriptCore/debugger/Debugger.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,8 @@ bool Debugger::hasBreakpoint(SourceID sourceID, const TextPosition& position, Br
453453

454454
NakedPtr<Exception> exception;
455455
DebuggerCallFrame* debuggerCallFrame = currentDebuggerCallFrame();
456-
JSValue result = debuggerCallFrame->evaluate(breakpoint->condition, exception);
456+
JSObject* scopeExtensionObject = nullptr;
457+
JSValue result = debuggerCallFrame->evaluateWithScopeExtension(breakpoint->condition, scopeExtensionObject, exception);
457458

458459
// We can lose the debugger while executing JavaScript.
459460
if (!m_currentCallFrame)

0 commit comments

Comments
 (0)