Skip to content

Commit 432718f

Browse files
dcroussodcrousso
committed
Web Inspector: toString is called twice by console log
https://bugs.webkit.org/show_bug.cgi?id=306712 Reviewed by Timothy Hatcher. Web Inspector attempts to stringify the first argument passed to `console` methods as the primary "message". If the first argument is an object, however, this happens again when generating a `description` for the `Runtime.RemoteObject`. This second can be used to observe whether Web Inspector is open as the latter only happens then. To avoid this, we can simply save and reuse the result of the first instead of doing it again. * Source/JavaScriptCore/inspector/ConsoleMessage.h: (Inspector::ConsoleMessage::setMessageIsFirstArgument): Added. * Source/JavaScriptCore/inspector/ConsoleMessage.cpp: (Inspector::ConsoleMessage::addToFrontend): * Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp: (Inspector::JSGlobalObjectConsoleClient::messageWithTypeAndLevel): * Source/WebCore/page/FrameConsoleClient.cpp: (WebCore::FrameConsoleClient::messageWithTypeAndLevel): * Source/WebCore/workers/WorkerConsoleClient.cpp: (WebCore::WorkerConsoleClient::messageWithTypeAndLevel): * Source/JavaScriptCore/inspector/InjectedScript.h: * Source/JavaScriptCore/inspector/InjectedScript.cpp: (Inspector::InjectedScript::wrapObject const): * Source/JavaScriptCore/inspector/InjectedScriptSource.js: (InjectedScript.prototype.wrapObject): (RemoteObject): (RemoteObject.create): (RemoteObject.describe): * LayoutTests/inspector/console/console-log-single-tostring-invocation.html: Added. * LayoutTests/inspector/console/console-log-single-tostring-invocation-expected.txt: Added. * LayoutTests/inspector/worker/console-basic.html: * LayoutTests/inspector/worker/console-basic-expected.txt: Canonical link: https://commits.webkit.org/323668@main
1 parent c9a4302 commit 432718f

12 files changed

Lines changed: 147 additions & 19 deletions

‎LayoutTests/inspector/console/console-log-single-tostring-invocation-expected.txt‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
CONSOLE MESSAGE: counting-1
2+
CONSOLE MESSAGE: counting-1
3+
Tests that console.log invokes an object's toString exactly once when the inspector is open (bug 306712).
4+
5+
6+
== Running test suite: Console.log.SingleToStringInvocation
7+
-- Running test case: Console.log.Date
8+
toString invocations: 1
9+
PASS: toString should be invoked exactly once.
10+
11+
-- Running test case: Console.log.Function
12+
toString invocations: 1
13+
PASS: toString should be invoked exactly once.
14+

‎LayoutTests/inspector/console/console-log-single-tostring-invocation.html‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<script src="../../http/tests/inspector/resources/protocol-test.js"></script>
5+
<script>
6+
function test()
7+
{
8+
let suite = ProtocolTest.createAsyncSuite("Console.log.SingleToStringInvocation");
9+
10+
async function countToStringInvocations(expression) {
11+
let {result} = await InspectorProtocol.awaitCommand({
12+
method: "Runtime.evaluate",
13+
params: {
14+
expression: `
15+
window.__toStringCount = 0;
16+
${expression}
17+
window.__toStringCount;
18+
`,
19+
},
20+
});
21+
return result.value;
22+
}
23+
24+
suite.addTestCase({
25+
name: "Console.log.Date",
26+
description: "console.log of a Date subclass should invoke its toString exactly once while the inspector is open.",
27+
async test() {
28+
let count = await countToStringInvocations(`
29+
class CountingDate extends Date { toString() { window.__toStringCount++; return "counting-" + window.__toStringCount; } }
30+
console.log(new CountingDate());
31+
`);
32+
ProtocolTest.log("toString invocations: " + count);
33+
ProtocolTest.expectEqual(count, 1, "toString should be invoked exactly once.");
34+
},
35+
});
36+
37+
suite.addTestCase({
38+
name: "Console.log.Function",
39+
description: "console.log of a function should invoke its toString exactly once while the inspector is open.",
40+
async test() {
41+
let count = await countToStringInvocations(`
42+
let fn = function() {};
43+
fn.toString = function() { window.__toStringCount++; return "counting-" + window.__toStringCount; };
44+
console.log(fn);
45+
`);
46+
ProtocolTest.log("toString invocations: " + count);
47+
ProtocolTest.expectEqual(count, 1, "toString should be invoked exactly once.");
48+
},
49+
});
50+
51+
InspectorProtocol.awaitCommand({method: "Console.enable", params: {}}).then(() => {
52+
suite.runTestCasesAndFinish();
53+
});
54+
}
55+
</script>
56+
</head>
57+
<body onload="runTest()">
58+
<p>Tests that console.log invokes an object's toString exactly once when the inspector is open (bug 306712).</p>
59+
</body>
60+
</html>

‎LayoutTests/inspector/worker/console-basic-expected.txt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,3 +171,7 @@ PASS: ConsoleMessage parameter RemoteObjects should be from the Worker target.
171171
"_timestamp": "<filtered>"
172172
}
173173

174+
-- Running test case: Worker.Console.SingleToStringInvocation
175+
PASS: Worker Date toString should be invoked exactly once.
176+
PASS: Worker Function toString should be invoked exactly once.
177+

‎LayoutTests/inspector/worker/console-basic.html‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,42 @@
120120
message: "count",
121121
});
122122

123+
suite.addTestCase({
124+
name: "Worker.Console.SingleToStringInvocation",
125+
async test() {
126+
for (let {name, source} of [
127+
{
128+
name: "Date",
129+
source: `let value = new (class extends Date {
130+
toString() {
131+
++count;
132+
return "worker-date";
133+
}
134+
});`,
135+
},
136+
{
137+
name: "Function",
138+
source: `let value = function() {};
139+
value.toString = function() {
140+
++count;
141+
return "worker-function";
142+
};`,
143+
},
144+
]) {
145+
let {result} = await workerTarget.RuntimeAgent.evaluate.invoke({
146+
expression: `(function() {
147+
let count = 0;
148+
${source}
149+
console.log(value);
150+
return count;
151+
})()`,
152+
returnByValue: true,
153+
});
154+
InspectorTest.expectEqual(result.value, 1, `Worker ${name} toString should be invoked exactly once.`);
155+
}
156+
},
157+
});
158+
123159
suite.runTestCasesAndFinish();
124160
}
125161
</script>

‎Source/JavaScriptCore/inspector/ConsoleMessage.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,11 @@ void ConsoleMessage::addToFrontend(ConsoleFrontendDispatcher& consoleFrontendDis
283283
}
284284
} else {
285285
for (unsigned i = 0; i < m_arguments->argumentCount(); ++i) {
286-
auto inspectorValue = injectedScript.wrapObject(m_arguments->argumentAt(i), "console"_s, generatePreview);
286+
String descriptionHint;
287+
if (i == 0 && m_messageIsFirstArgument)
288+
descriptionHint = m_message.isNull() ? emptyString() : m_message;
289+
290+
auto inspectorValue = injectedScript.wrapObject(m_arguments->argumentAt(i), "console"_s, generatePreview, descriptionHint);
287291
if (!inspectorValue) {
288292
ASSERT_NOT_REACHED();
289293
return;

‎Source/JavaScriptCore/inspector/ConsoleMessage.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,8 @@ class ConsoleMessage {
8383
ScriptArguments* arguments() const { return m_arguments.get(); }
8484
unsigned LEGACY_NODELETE argumentCount() const;
8585

86+
void setMessageIsFirstArgument(bool value) { m_messageIsFirstArgument = value; }
87+
8688
bool isEqual(ConsoleMessage* msg) const;
8789

8890
JS_EXPORT_PRIVATE void clear();
@@ -106,6 +108,7 @@ class ConsoleMessage {
106108
unsigned m_repeatCount { 1 };
107109
String m_requestId;
108110
WallTime m_timestamp;
111+
bool m_messageIsFirstArgument { false };
109112
};
110113

111114
} // namespace Inspector

‎Source/JavaScriptCore/inspector/InjectedScript.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,14 +276,16 @@ Ref<JSON::ArrayOf<Protocol::Debugger::CallFrame>> InjectedScript::wrapCallFrames
276276
return JSON::ArrayOf<Protocol::Debugger::CallFrame>::create();
277277
}
278278

279-
RefPtr<Protocol::Runtime::RemoteObject> InjectedScript::wrapObject(JSC::JSValue value, const String& groupName, bool generatePreview) const
279+
RefPtr<Protocol::Runtime::RemoteObject> InjectedScript::wrapObject(JSC::JSValue value, const String& groupName, bool generatePreview, const String& descriptionHint) const
280280
{
281281
ASSERT(!hasNoValue());
282282
ScriptFunctionCall wrapFunction(globalObject(), injectedScriptObject(), "wrapObject"_s, protect(inspectorEnvironment())->functionCallHandler());
283283
wrapFunction.appendArgument(value);
284284
wrapFunction.appendArgument(groupName);
285285
wrapFunction.appendArgument(hasAccessToInspectedScriptState());
286286
wrapFunction.appendArgument(generatePreview);
287+
if (!descriptionHint.isNull())
288+
wrapFunction.appendArgument(descriptionHint);
287289

288290
auto callResult = callFunctionWithEvalEnabled(wrapFunction);
289291
if (!callResult || !callResult.value())

‎Source/JavaScriptCore/inspector/InjectedScript.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ class InjectedScript final : public InjectedScriptBase {
7878
void saveResult(Protocol::ErrorString&, const String& callArgumentJSON, std::optional<int>& savedResultIndex);
7979

8080
Ref<JSON::ArrayOf<Protocol::Debugger::CallFrame>> wrapCallFrames(JSC::JSValue) const;
81-
JS_EXPORT_PRIVATE RefPtr<Protocol::Runtime::RemoteObject> wrapObject(JSC::JSValue, const String& groupName, bool generatePreview = false) const;
81+
JS_EXPORT_PRIVATE RefPtr<Protocol::Runtime::RemoteObject> wrapObject(JSC::JSValue, const String& groupName, bool generatePreview = false, const String& descriptionHint = { }) const;
8282
RefPtr<Protocol::Runtime::RemoteObject> wrapJSONString(const String& json, const String& groupName, bool generatePreview = false) const;
8383
RefPtr<Protocol::Runtime::RemoteObject> wrapTable(JSC::JSValue table, JSC::JSValue columns) const;
8484
RefPtr<Protocol::Runtime::ObjectPreview> previewValue(JSC::JSValue) const;

‎Source/JavaScriptCore/inspector/InjectedScriptSource.js‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -378,12 +378,12 @@ let InjectedScript = class InjectedScript extends PrototypelessObjectBase
378378
return result;
379379
}
380380

381-
wrapObject(object, groupName, canAccessInspectedGlobalObject, generatePreview)
381+
wrapObject(object, groupName, canAccessInspectedGlobalObject, generatePreview, descriptionHint)
382382
{
383383
if (!canAccessInspectedGlobalObject)
384384
return this._fallbackWrapper(object);
385385

386-
return RemoteObject.create(object, groupName, false, generatePreview);
386+
return RemoteObject.create(object, groupName, false, generatePreview, @undefined, descriptionHint);
387387
}
388388

389389
wrapJSONString(jsonString, groupName, generatePreview)
@@ -1047,7 +1047,7 @@ var injectedScript = new InjectedScript;
10471047

10481048
let RemoteObject = class RemoteObject extends PrototypelessObjectBase
10491049
{
1050-
constructor(object, objectGroupName, forceValueType, generatePreview, columnNames)
1050+
constructor(object, objectGroupName, forceValueType, generatePreview, columnNames, descriptionHint)
10511051
{
10521052
super();
10531053

@@ -1080,7 +1080,7 @@ let RemoteObject = class RemoteObject extends PrototypelessObjectBase
10801080
this.subtype = subtype;
10811081

10821082
this.className = InjectedScriptHost.internalConstructorName(object);
1083-
this.description = RemoteObject.describe(object);
1083+
this.description = RemoteObject.describe(object, descriptionHint);
10841084

10851085
if (subtype === "array")
10861086
this.size = typeof object.length === "number" ? object.length : 0;
@@ -1109,10 +1109,10 @@ let RemoteObject = class RemoteObject extends PrototypelessObjectBase
11091109

11101110
// Static
11111111

1112-
static create(object, objectGroupName, forceValueType, generatePreview, columnNames)
1112+
static create(object, objectGroupName, forceValueType, generatePreview, columnNames, descriptionHint)
11131113
{
11141114
try {
1115-
return new RemoteObject(object, objectGroupName, forceValueType, generatePreview, columnNames);
1115+
return new RemoteObject(object, objectGroupName, forceValueType, generatePreview, columnNames, descriptionHint);
11161116
} catch (e) {
11171117
let description;
11181118
try {
@@ -1159,7 +1159,7 @@ let RemoteObject = class RemoteObject extends PrototypelessObjectBase
11591159
return null;
11601160
}
11611161

1162-
static describe(value)
1162+
static describe(value, hint)
11631163
{
11641164
if (isPrimitiveValue(value))
11651165
return null;
@@ -1173,13 +1173,13 @@ let RemoteObject = class RemoteObject extends PrototypelessObjectBase
11731173
let subtype = RemoteObject.subtype(value);
11741174

11751175
if (subtype === "regexp")
1176-
return toString(value);
1176+
return hint ?? toString(value);
11771177

11781178
if (subtype === "date")
1179-
return toString(value);
1179+
return hint ?? toString(value);
11801180

11811181
if (subtype === "error")
1182-
return toString(value);
1182+
return hint ?? toString(value);
11831183

11841184
if (subtype === "proxy")
11851185
return "Proxy";
@@ -1199,11 +1199,11 @@ let RemoteObject = class RemoteObject extends PrototypelessObjectBase
11991199

12001200
// NodeList in JSC is a function, check for array prior to this.
12011201
if (typeof value === "function")
1202-
return value.toString();
1202+
return hint ?? value.toString();
12031203

12041204
// If Object, try for a better name from the constructor.
12051205
if (className === "Object") {
1206-
let constructorName = value.constructor && value.constructor.name;
1206+
let constructorName = value?.constructor?.name;
12071207
if (constructorName)
12081208
return constructorName;
12091209
}

‎Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,11 @@ void JSGlobalObjectConsoleClient::messageWithTypeAndLevel(MessageType type, Mess
7373
return;
7474

7575
String message;
76-
arguments->getFirstArgumentAsString(message);
77-
m_consoleAgent->addMessageToConsole(makeUnique<ConsoleMessage>(MessageSource::ConsoleAPI, type, level, message, WTF::move(arguments), globalObject));
76+
bool messageIsFirstArgument = arguments->getFirstArgumentAsString(message);
77+
78+
auto consoleMessage = makeUnique<ConsoleMessage>(MessageSource::ConsoleAPI, type, level, message, WTF::move(arguments), globalObject);
79+
consoleMessage->setMessageIsFirstArgument(messageIsFirstArgument);
80+
m_consoleAgent->addMessageToConsole(WTF::move(consoleMessage));
7881

7982
if (type == MessageType::Assert) {
8083
if (CheckedPtr agent = m_debuggerAgent)

0 commit comments

Comments
 (0)