Skip to content

Commit 22a1e11

Browse files
shvaikaleshshvaikalesh
committed
Remove className() and toStringName() from the method table
https://bugs.webkit.org/show_bug.cgi?id=224247 Reviewed by Darin Adler. Source/JavaScriptCore: ES6 introduced Symbol.toStringTag to customize Object.prototype.toString return value. It was adopted by WebIDL spec, Chrome's DevTools, Node.js etc. There is no reason to keep 2 method table methods, each with only 1 call site, instead of using the symbol. Also, it's a bit confusing that for some objects, method table's className() returns different result than JSCell::className(VM&). This change: 1. Removes JSProxy's className() / toStringName() methods because its target() is a global object that never has these overrides and uses Symbol.toStringTag instead. 2. Removes DebuggerScope's className() / toStringName() overrides because its objectAtScope() has these methods extremely rarely (e.g. `with (new Date) {}`), and its not displayed by Web Inspector. 3. Merges JSCallbackObject's className() / toStringName() methods into Symbol.toStringTag branch of getOwnPropertySlot(), with permissive property attributes. To avoid any possible breakage, we make sure that it will be shadowed by a structure property. 4. Reworks JSObject::calculatedClassName() to rely on Symbol.toStringTag, matching Chrome's DevTools behavior. On its own, it's a nice change for Web Inspector. We make sure to lookup Symbol.toStringTag if `constructor.name` inference fails to avoid confusion when extending builtins. 5. Removes now unused className() from the method table. 6. Removes toStringName() override from JSFinalizationRegistry because its builtin tag [1] is already "Object". 7. Introduces BooleanObjectType for Boolean wrapper object, and Boolean.prototype as it's also required to have a [[BooleanData]] internal slot [2]. 8. Reworks Object.prototype.toString to determine builtin tag [1] based on JSType rather than performing method table call. It's guaranteed that a) the set of types we are checking against won't be expanded, and b) objects with these types have correct `className`. 9. Removes now unused toStringTag() from the method table. This patch is performance-neutral and carefully preserves current behavior for API objects, including isPokerBros() hack. [1]: https://tc39.es/ecma262/#sec-object.prototype.tostring (steps 5-14) [2]: https://tc39.es/ecma262/#sec-properties-of-the-boolean-prototype-object * API/JSCallbackObject.h: * API/JSCallbackObjectFunctions.h: (JSC::JSCallbackObject<Parent>::getOwnPropertySlot): (JSC::JSCallbackObject<Parent>::className): Deleted. (JSC::JSCallbackObject<Parent>::toStringName): Deleted. * API/tests/testapiScripts/testapi.js: * debugger/DebuggerScope.cpp: (JSC::DebuggerScope::className): Deleted. (JSC::DebuggerScope::toStringName): Deleted. * debugger/DebuggerScope.h: * runtime/BooleanObject.cpp: (JSC::BooleanObject::toStringName): Deleted. * runtime/BooleanObject.h: (JSC::BooleanObject::createStructure): * runtime/BooleanPrototype.h: * runtime/ClassInfo.h: * runtime/DateInstance.cpp: (JSC::DateInstance::toStringName): Deleted. * runtime/DateInstance.h: * runtime/ErrorInstance.cpp: (JSC::ErrorInstance::toStringName): Deleted. * runtime/ErrorInstance.h: * runtime/JSCell.cpp: (JSC::JSCell::className): Deleted. (JSC::JSCell::toStringName): Deleted. * runtime/JSCell.h: * runtime/JSFinalizationRegistry.cpp: (JSC::JSFinalizationRegistry::toStringName): Deleted. * runtime/JSFinalizationRegistry.h: * runtime/JSObject.cpp: (JSC::JSObject::calculatedClassName): (JSC::JSObject::className): Deleted. (JSC::isPokerBros): Deleted. (JSC::JSObject::toStringName): Deleted. * runtime/JSObject.h: * runtime/JSProxy.cpp: (JSC::JSProxy::className): Deleted. (JSC::JSProxy::toStringName): Deleted. * runtime/JSProxy.h: * runtime/JSType.cpp: (WTF::printInternal): * runtime/JSType.h: * runtime/NumberObject.cpp: (JSC::NumberObject::toStringName): Deleted. * runtime/NumberObject.h: (JSC::NumberObject::createStructure): * runtime/ObjectPrototype.cpp: (JSC::isPokerBros): (JSC::inferBuiltinTag): (JSC::objectPrototypeToString): 1. Removes jsNontrivialString() because it's assertion may fail in case of iOS hack. 2. Utilizes AtomStringImpl to avoid allocating StringImpl for a small fixed set of strings. * runtime/RegExpObject.cpp: (JSC::RegExpObject::toStringName): Deleted. * runtime/RegExpObject.h: * runtime/StringObject.cpp: (JSC::StringObject::toStringName): Deleted. * runtime/StringObject.h: LayoutTests: * inspector/model/remote-object-get-properties-expected.txt: * inspector/model/remote-object-get-properties.html: Canonical link: https://commits.webkit.org/236359@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@275788 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 0c00364 commit 22a1e11

34 files changed

Lines changed: 289 additions & 192 deletions

‎LayoutTests/ChangeLog‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
2021-04-09 Alexey Shvayka <shvaikalesh@gmail.com>
2+
3+
Remove className() and toStringName() from the method table
4+
https://bugs.webkit.org/show_bug.cgi?id=224247
5+
6+
Reviewed by Darin Adler.
7+
8+
* inspector/model/remote-object-get-properties-expected.txt:
9+
* inspector/model/remote-object-get-properties.html:
10+
111
2021-04-09 Jean-Yves Avenard <jya@apple.com>
212

313
Media Session action should default to the MediaElement's default when no MediaSession handler are set

‎LayoutTests/inspector/model/remote-object-get-properties-expected.txt‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,7 @@ OWN PROPERTIES:
467467
Symbol(sym)
468468
Symbol(sym)
469469
Symbol()
470+
Symbol(Symbol.toStringTag)
470471
__proto__
471472

472473
DISPLAYABLE PROPERTIES:
@@ -476,6 +477,7 @@ DISPLAYABLE PROPERTIES:
476477
Symbol(sym)
477478
Symbol(sym)
478479
Symbol()
480+
Symbol(Symbol.toStringTag)
479481
__proto__
480482

481483
ALL PROPERTIES:
@@ -485,6 +487,7 @@ ALL PROPERTIES:
485487
Symbol(sym)
486488
Symbol(sym)
487489
Symbol()
490+
Symbol(Symbol.toStringTag)
488491
toString
489492
toLocaleString
490493
valueOf
@@ -499,6 +502,65 @@ ALL PROPERTIES:
499502
__proto__
500503
-----------------------------------------------------
501504

505+
-----------------------------------------------------
506+
EXPRESSION: window.objectWithSymbolToStringTag
507+
type: object
508+
description: Foo
509+
510+
OWN PROPERTIES:
511+
Symbol(Symbol.toStringTag)
512+
__proto__
513+
514+
DISPLAYABLE PROPERTIES:
515+
Symbol(Symbol.toStringTag)
516+
__proto__
517+
518+
ALL PROPERTIES:
519+
Symbol(Symbol.toStringTag)
520+
toString
521+
toLocaleString
522+
valueOf
523+
hasOwnProperty
524+
propertyIsEnumerable
525+
isPrototypeOf
526+
__defineGetter__
527+
__defineSetter__
528+
__lookupGetter__
529+
__lookupSetter__
530+
constructor
531+
__proto__
532+
-----------------------------------------------------
533+
534+
-----------------------------------------------------
535+
EXPRESSION: window.objectWithShadowedSymbolToStringTag
536+
type: object
537+
description: Foo
538+
539+
OWN PROPERTIES:
540+
__proto__
541+
542+
DISPLAYABLE PROPERTIES:
543+
__proto__
544+
545+
ALL PROPERTIES:
546+
constructor
547+
addEventListener
548+
removeEventListener
549+
dispatchEvent
550+
Symbol(Symbol.toStringTag)
551+
toString
552+
toLocaleString
553+
valueOf
554+
hasOwnProperty
555+
propertyIsEnumerable
556+
isPrototypeOf
557+
__defineGetter__
558+
__defineSetter__
559+
__lookupGetter__
560+
__lookupSetter__
561+
__proto__
562+
-----------------------------------------------------
563+
502564
-----------------------------------------------------
503565
EXPRESSION: document.getElementById('my-select').options
504566
type: object

‎LayoutTests/inspector/model/remote-object-get-properties.html‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,9 @@
4545
var badGetterObject = new ClassWithBadGetter;
4646
var unboundFunction = function() { console.log(arguments); }
4747
var boundFunction = unboundFunction.bind(document.body, 1, 2, 3);
48-
var objectWithSymbolProperties = {prop:1, [Symbol()]:2, [Symbol('sym')]:3, [Symbol('sym')]:4, [Symbol()]: Symbol(), prop2: 5};
48+
var objectWithSymbolProperties = {prop:1, [Symbol()]:2, [Symbol('sym')]:3, [Symbol('sym')]:4, [Symbol()]: Symbol(), [Symbol.toStringTag]: new String("IgnoredTag"), prop2: 5};
49+
var objectWithSymbolToStringTag = {[Symbol.toStringTag]: "Foo"};
50+
var objectWithShadowedSymbolToStringTag = new (class Foo extends EventTarget {});
4951

5052
// --------
5153
// test
@@ -64,6 +66,8 @@
6466
{expression: "window.unboundFunction"},
6567
{expression: "window.boundFunction"},
6668
{expression: "window.objectWithSymbolProperties"},
69+
{expression: "window.objectWithSymbolToStringTag"},
70+
{expression: "window.objectWithShadowedSymbolToStringTag"},
6771
{expression: "document.getElementById('my-select').options"},
6872
]
6973

‎Source/JavaScriptCore/API/JSCallbackObject.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,9 +199,6 @@ class JSCallbackObject final : public Parent {
199199
void finishCreation(VM&);
200200

201201
static IsoSubspace* subspaceForImpl(VM&, SubspaceAccess);
202-
static String className(const JSObject*, VM&);
203-
static String toStringName(const JSObject*, JSGlobalObject*);
204-
205202
static JSValue defaultValue(const JSObject*, JSGlobalObject*, PreferredPrimitiveType);
206203

207204
static bool getOwnPropertySlot(JSObject*, JSGlobalObject*, PropertyName, PropertySlot&);

‎Source/JavaScriptCore/API/JSCallbackObjectFunctions.h‎

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -130,26 +130,6 @@ void JSCallbackObject<Parent>::init(JSGlobalObject* globalObject)
130130
m_classInfo = this->classInfo(getVM(globalObject));
131131
}
132132

133-
template <class Parent>
134-
String JSCallbackObject<Parent>::className(const JSObject* object, VM& vm)
135-
{
136-
const JSCallbackObject* thisObject = jsCast<const JSCallbackObject*>(object);
137-
String thisClassName = thisObject->classRef()->className();
138-
if (!thisClassName.isEmpty())
139-
return thisClassName;
140-
141-
return Parent::className(object, vm);
142-
}
143-
144-
template <class Parent>
145-
String JSCallbackObject<Parent>::toStringName(const JSObject* object, JSGlobalObject* globalObject)
146-
{
147-
VM& vm = getVM(globalObject);
148-
const ClassInfo* info = object->classInfo(vm);
149-
ASSERT(info);
150-
return info->methodTable.className(object, vm);
151-
}
152-
153133
template <class Parent>
154134
bool JSCallbackObject<Parent>::getOwnPropertySlot(JSObject* object, JSGlobalObject* globalObject, PropertyName propertyName, PropertySlot& slot)
155135
{
@@ -213,7 +193,20 @@ bool JSCallbackObject<Parent>::getOwnPropertySlot(JSObject* object, JSGlobalObje
213193
}
214194
}
215195

216-
RELEASE_AND_RETURN(scope, Parent::getOwnPropertySlot(thisObject, globalObject, propertyName, slot));
196+
bool found = Parent::getOwnPropertySlot(thisObject, globalObject, propertyName, slot);
197+
RETURN_IF_EXCEPTION(scope, false);
198+
if (found)
199+
return true;
200+
201+
if (propertyName.uid() == vm.propertyNames->toStringTagSymbol.impl()) {
202+
String className = thisObject->classRef()->className();
203+
if (className.isEmpty())
204+
className = thisObject->className(vm);
205+
slot.setValue(thisObject, static_cast<unsigned>(PropertyAttribute::DontEnum), jsString(vm, WTFMove(className)));
206+
return true;
207+
}
208+
209+
return false;
217210
}
218211

219212
template <class Parent>

‎Source/JavaScriptCore/API/tests/testapiScripts/testapi.js‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,16 @@ shouldBe("MyObject.nullGetSet", 1);
198198
shouldThrow("MyObject.nullCall()");
199199
shouldThrow("MyObject.hasPropertyLie");
200200

201+
var symbolToStringTagDescriptor = Object.getOwnPropertyDescriptor(MyObject, Symbol.toStringTag);
202+
shouldBe("typeof symbolToStringTagDescriptor", "object");
203+
shouldBe("symbolToStringTagDescriptor.value", "MyObject");
204+
shouldBe("symbolToStringTagDescriptor.writable", true);
205+
shouldBe("symbolToStringTagDescriptor.enumerable", false);
206+
shouldBe("symbolToStringTagDescriptor.configurable", true);
207+
208+
MyObject[Symbol.toStringTag] = "Foo";
209+
shouldBe("Object.prototype.toString.call(MyObject)", "[object Foo]");
210+
201211
derived = new Derived();
202212

203213
shouldBe("derived instanceof Derived", true);
@@ -282,6 +292,16 @@ shouldBe("typeof console.log", "function");
282292

283293
shouldBe("EmptyObject", "[object CallbackObject]");
284294

295+
var symbolToStringTagDescriptor = Object.getOwnPropertyDescriptor(EmptyObject, Symbol.toStringTag);
296+
shouldBe("typeof symbolToStringTagDescriptor", "object");
297+
shouldBe("symbolToStringTagDescriptor.value", "CallbackObject");
298+
shouldBe("symbolToStringTagDescriptor.writable", true);
299+
shouldBe("symbolToStringTagDescriptor.enumerable", false);
300+
shouldBe("symbolToStringTagDescriptor.configurable", true);
301+
302+
EmptyObject[Symbol.toStringTag] = "Foo";
303+
shouldBe("Object.prototype.toString.call(EmptyObject)", "[object Foo]");
304+
285305
for (var i = 0; i < 6; ++i)
286306
PropertyCatchalls.x = i;
287307
shouldBe("PropertyCatchalls.x", 4);

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,115 @@
1+
2021-04-09 Alexey Shvayka <shvaikalesh@gmail.com>
2+
3+
Remove className() and toStringName() from the method table
4+
https://bugs.webkit.org/show_bug.cgi?id=224247
5+
6+
Reviewed by Darin Adler.
7+
8+
ES6 introduced Symbol.toStringTag to customize Object.prototype.toString return value.
9+
It was adopted by WebIDL spec, Chrome's DevTools, Node.js etc. There is no reason to
10+
keep 2 method table methods, each with only 1 call site, instead of using the symbol.
11+
12+
Also, it's a bit confusing that for some objects, method table's className() returns
13+
different result than JSCell::className(VM&).
14+
15+
This change:
16+
17+
1. Removes JSProxy's className() / toStringName() methods because its target() is a
18+
global object that never has these overrides and uses Symbol.toStringTag instead.
19+
20+
2. Removes DebuggerScope's className() / toStringName() overrides because its objectAtScope()
21+
has these methods extremely rarely (e.g. `with (new Date) {}`), and its not displayed
22+
by Web Inspector.
23+
24+
3. Merges JSCallbackObject's className() / toStringName() methods into Symbol.toStringTag
25+
branch of getOwnPropertySlot(), with permissive property attributes. To avoid any possible
26+
breakage, we make sure that it will be shadowed by a structure property.
27+
28+
4. Reworks JSObject::calculatedClassName() to rely on Symbol.toStringTag, matching Chrome's
29+
DevTools behavior. On its own, it's a nice change for Web Inspector. We make sure to
30+
lookup Symbol.toStringTag if `constructor.name` inference fails to avoid confusion when
31+
extending builtins.
32+
33+
5. Removes now unused className() from the method table.
34+
35+
6. Removes toStringName() override from JSFinalizationRegistry because its builtin tag [1]
36+
is already "Object".
37+
38+
7. Introduces BooleanObjectType for Boolean wrapper object, and Boolean.prototype as it's
39+
also required to have a [[BooleanData]] internal slot [2].
40+
41+
8. Reworks Object.prototype.toString to determine builtin tag [1] based on JSType rather than
42+
performing method table call. It's guaranteed that a) the set of types we are checking
43+
against won't be expanded, and b) objects with these types have correct `className`.
44+
45+
9. Removes now unused toStringTag() from the method table.
46+
47+
This patch is performance-neutral and carefully preserves current behavior for API objects,
48+
including isPokerBros() hack.
49+
50+
[1]: https://tc39.es/ecma262/#sec-object.prototype.tostring (steps 5-14)
51+
[2]: https://tc39.es/ecma262/#sec-properties-of-the-boolean-prototype-object
52+
53+
* API/JSCallbackObject.h:
54+
* API/JSCallbackObjectFunctions.h:
55+
(JSC::JSCallbackObject<Parent>::getOwnPropertySlot):
56+
(JSC::JSCallbackObject<Parent>::className): Deleted.
57+
(JSC::JSCallbackObject<Parent>::toStringName): Deleted.
58+
* API/tests/testapiScripts/testapi.js:
59+
* debugger/DebuggerScope.cpp:
60+
(JSC::DebuggerScope::className): Deleted.
61+
(JSC::DebuggerScope::toStringName): Deleted.
62+
* debugger/DebuggerScope.h:
63+
* runtime/BooleanObject.cpp:
64+
(JSC::BooleanObject::toStringName): Deleted.
65+
* runtime/BooleanObject.h:
66+
(JSC::BooleanObject::createStructure):
67+
* runtime/BooleanPrototype.h:
68+
* runtime/ClassInfo.h:
69+
* runtime/DateInstance.cpp:
70+
(JSC::DateInstance::toStringName): Deleted.
71+
* runtime/DateInstance.h:
72+
* runtime/ErrorInstance.cpp:
73+
(JSC::ErrorInstance::toStringName): Deleted.
74+
* runtime/ErrorInstance.h:
75+
* runtime/JSCell.cpp:
76+
(JSC::JSCell::className): Deleted.
77+
(JSC::JSCell::toStringName): Deleted.
78+
* runtime/JSCell.h:
79+
* runtime/JSFinalizationRegistry.cpp:
80+
(JSC::JSFinalizationRegistry::toStringName): Deleted.
81+
* runtime/JSFinalizationRegistry.h:
82+
* runtime/JSObject.cpp:
83+
(JSC::JSObject::calculatedClassName):
84+
(JSC::JSObject::className): Deleted.
85+
(JSC::isPokerBros): Deleted.
86+
(JSC::JSObject::toStringName): Deleted.
87+
* runtime/JSObject.h:
88+
* runtime/JSProxy.cpp:
89+
(JSC::JSProxy::className): Deleted.
90+
(JSC::JSProxy::toStringName): Deleted.
91+
* runtime/JSProxy.h:
92+
* runtime/JSType.cpp:
93+
(WTF::printInternal):
94+
* runtime/JSType.h:
95+
* runtime/NumberObject.cpp:
96+
(JSC::NumberObject::toStringName): Deleted.
97+
* runtime/NumberObject.h:
98+
(JSC::NumberObject::createStructure):
99+
* runtime/ObjectPrototype.cpp:
100+
(JSC::isPokerBros):
101+
(JSC::inferBuiltinTag):
102+
(JSC::objectPrototypeToString):
103+
1. Removes jsNontrivialString() because it's assertion may fail in case of iOS hack.
104+
2. Utilizes AtomStringImpl to avoid allocating StringImpl for a small fixed set of strings.
105+
106+
* runtime/RegExpObject.cpp:
107+
(JSC::RegExpObject::toStringName): Deleted.
108+
* runtime/RegExpObject.h:
109+
* runtime/StringObject.cpp:
110+
(JSC::StringObject::toStringName): Deleted.
111+
* runtime/StringObject.h:
112+
1113
2021-04-08 Khem Raj <raj.khem@gmail.com>
2114

3115
[WPE] Build fixes for musl C library on Linux

‎Source/JavaScriptCore/debugger/DebuggerScope.cpp‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -68,28 +68,6 @@ void DebuggerScope::visitChildrenImpl(JSCell* cell, Visitor& visitor)
6868

6969
DEFINE_VISIT_CHILDREN(DebuggerScope);
7070

71-
String DebuggerScope::className(const JSObject* object, VM& vm)
72-
{
73-
const DebuggerScope* scope = jsCast<const DebuggerScope*>(object);
74-
// We cannot assert that scope->isValid() because the TypeProfiler may encounter an invalidated
75-
// DebuggerScope in its log entries. We just need to handle it appropriately as below.
76-
if (!scope->isValid())
77-
return String();
78-
JSObject* thisObject = JSScope::objectAtScope(scope->jsScope());
79-
return thisObject->methodTable(vm)->className(thisObject, vm);
80-
}
81-
82-
String DebuggerScope::toStringName(const JSObject* object, JSGlobalObject* globalObject)
83-
{
84-
const DebuggerScope* scope = jsCast<const DebuggerScope*>(object);
85-
// We cannot assert that scope->isValid() because the TypeProfiler may encounter an invalidated
86-
// DebuggerScope in its log entries. We just need to handle it appropriately as below.
87-
if (!scope->isValid())
88-
return String();
89-
JSObject* thisObject = JSScope::objectAtScope(scope->jsScope());
90-
return thisObject->methodTable(globalObject->vm())->toStringName(thisObject, globalObject);
91-
}
92-
9371
bool DebuggerScope::getOwnPropertySlot(JSObject* object, JSGlobalObject* globalObject, PropertyName propertyName, PropertySlot& slot)
9472
{
9573
DebuggerScope* scope = jsCast<DebuggerScope*>(object);

‎Source/JavaScriptCore/debugger/DebuggerScope.h‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,6 @@ class DebuggerScope final : public JSNonFinalObject {
4747
JS_EXPORT_PRIVATE static DebuggerScope* create(VM& vm, JSScope* scope);
4848

4949
DECLARE_VISIT_CHILDREN;
50-
static String className(const JSObject*, VM&);
51-
static String toStringName(const JSObject*, JSGlobalObject*);
5250
static bool getOwnPropertySlot(JSObject*, JSGlobalObject*, PropertyName, PropertySlot&);
5351
static bool put(JSCell*, JSGlobalObject*, PropertyName, JSValue, PutPropertySlot&);
5452
static bool deleteProperty(JSCell*, JSGlobalObject*, PropertyName, DeletePropertySlot&);

‎Source/JavaScriptCore/runtime/BooleanObject.cpp‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,4 @@ void BooleanObject::finishCreation(VM& vm)
4040
ASSERT(inherits(vm, info()));
4141
}
4242

43-
String BooleanObject::toStringName(const JSObject*, JSGlobalObject*)
44-
{
45-
return "Boolean"_s;
46-
}
47-
4843
} // namespace JSC

0 commit comments

Comments
 (0)