Bug 199138 – Object.prototype.toString is not spec-perfect

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Shvayka]

ECMA262: https://tc39.es/ecma262/#sec-object.prototype.tostring
Test262: https://test262.report/browse/built-ins/Object/prototype/toString/proxy-function.js

1. Proxied functions:

```
toString.call(new Proxy(function() {}, {}))
// actual: "[object Object]"
// expected: "[object Function]"
```

2. Proxy constructor:

```
toString.call(Proxy)
// actual: "[object Proxy]"
// expected: "[object Function]"
```

3. Post ES5 built-ins and DOM objects with deleted @@toStringTag are treated specially:

Test262: https://github.com/tc39/test262/pull/2202

```
delete Math[Symbol.toStringTag]
toString.call(Math)
// actual: "[object Math]"
// expected: "[object Object]"
```

4. @@toStringTag symbols are missing on WebAssembly API and DOM objects:

```
document[Symbol.toStringTag]
// actual: undefined
// expected: "HTMLDocument"
```

[Alexey Shvayka]

Created attachment 372719 [details]
Patch

[EWS Watchlist]

Comment on attachment 372719 [details]
Patch

Attachment 372719 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12557900

New failing tests:
inspector/model/remote-object-get-properties.html
inspector/model/remote-object-dom.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/window-prototype-chain.html

[EWS Watchlist]

Created attachment 372722 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 372719 [details]
Patch

Attachment 372719 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12557916

New failing tests:
inspector/model/remote-object-get-properties.html
inspector/model/remote-object-dom.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/window-prototype-chain.html

[EWS Watchlist]

Created attachment 372723 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 372719 [details]
Patch

Attachment 372719 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12557902

New failing tests:
wasm.yaml/wasm/js-api/test_basic_api.js.wasm-no-tls-context
wasm.yaml/wasm/js-api/test_basic_api.js.wasm-collect-continuously
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-eager-jettison
wasm.yaml/wasm/spec-tests/jsapi.js.default-wasm
wasm.yaml/wasm/js-api/test_basic_api.js.wasm-no-call-ic
wasm.yaml/wasm/js-api/test_basic_api.js.wasm-slow-memory
wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline
wasm.yaml/wasm/js-api/test_basic_api.js.wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-air
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-tls-context
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-collect-continuously
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/js-api/test_basic_api.js.wasm-no-air
wasm.yaml/wasm/js-api/test_basic_api.js.wasm-eager-jettison
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-call-ic
apiTests

[EWS Watchlist]

Comment on attachment 372719 [details]
Patch

Attachment 372719 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12557934

New failing tests:
inspector/model/remote-object-get-properties.html
inspector/model/remote-object-dom.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/window-prototype-chain.html

[EWS Watchlist]

Created attachment 372725 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 372719 [details]
Patch

Attachment 372719 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12557981

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/window-prototype-chain.html

[EWS Watchlist]

Created attachment 372726 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[Alexey Shvayka]

Created attachment 372748 [details]
Patch

Add "WindowProperties" symbol and fix tests.

[Alexey Shvayka]

Created attachment 372771 [details]
Patch

Adjust bindings tests.

[Alexey Shvayka]

Created attachment 374400 [details]
Patch

Rebase and mark 4 more tests as passing.

[Alexey Shvayka]

Created attachment 374407 [details]
Patch

Fix weird type conversion error.

[Alexey Shvayka]

Adding Darin to CC in case he has some feedback on WebCore/bindings changes.

[Alexey Shvayka]

Created attachment 398257 [details]
Patch

[Darin Adler]

Comment on attachment 398257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398257&action=review

If any of these are particular hot in performance benchmarks we could probably optimize this further. Not sure we need to make a rope every time.

Also, if toStringName is always returning an ASCIILiteral we should consider changing its return type from String to ASCIILiteral.

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:341
> +        jsTag = jsNontrivialString(vm, WTFMove(tag));

I think this is likely wrong. What guarantees this tag is never a single character?

[Alexey Shvayka]

(In reply to Darin Adler from comment #17)

Thank you for review, Darin!

> If any of these are particular hot in performance benchmarks we could
> probably optimize this further. Not sure we need to make a rope every time.

Please note that the rope is cached by StructureRareData::setObjectToStringValue(): new one is created only if there is a Proxy/getter/__proto__ change.

> Also, if toStringName is always returning an ASCIILiteral we should consider
> changing its return type from String to ASCIILiteral.

That would be nice improvement. I would prefer to implement this in a follow-up to keep this patch small.

> > Source/JavaScriptCore/runtime/ObjectPrototype.cpp:341
> > +        jsTag = jsNontrivialString(vm, WTFMove(tag));
> 
> I think this is likely wrong. What guarantees this tag is never a single
> character?

This case handles tags from toStringName(), a very small set of pre-ES5 built-ins names that will never grow.

[Keith Miller]

Comment on attachment 398257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398257&action=review

r=me too.

> Source/JavaScriptCore/ChangeLog:15
> +        "Object", while @@toStringTag was set to correct value. This is quite error-prone approach and

Nit: quite *an* error-prone

> Source/JavaScriptCore/ChangeLog:16
> +        observable spec discrepancy if @@toStringTag is deleted or set to non-string.

Nit: set to *a* non-string.

> Source/JavaScriptCore/ChangeLog:19
> +        For Object.prototype.toString to work threw DebuggerScope and JSProxy, we perform all checks

Nit: threw => through.

> Source/JavaScriptCore/jsc.cpp:497
> +        JSC_TO_STRING_TAG_WITHOUT_TRANSITION();

Why make this change?

>> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:341
>> +        jsTag = jsNontrivialString(vm, WTFMove(tag));
> 
> I think this is likely wrong. What guarantees this tag is never a single character?

Yeah, it might just so happen that they're not single characters. If so, we should at least ASSERT_WITH_MESSAGE() as much here.

[Alexey Shvayka]

(In reply to Keith Miller from comment #19)
> > Source/JavaScriptCore/jsc.cpp:497
> > +        JSC_TO_STRING_TAG_WITHOUT_TRANSITION();
> 
> Why make this change?

Since we are removing className() from JSObject::toStringName(), we need to make sure it is not observable. Without this change, we will end up with `toString.call(globalThis)` returning "[object Object]" that will most likely break some code/tests. Same with ConsoleObject.

[Darin Adler]

(In reply to Alexey Shvayka from comment #18)
> (In reply to Darin Adler from comment #17)
> > If any of these are particular hot in performance benchmarks we could
> > probably optimize this further. Not sure we need to make a rope every time.
> 
> Please note that the rope is cached by
> StructureRareData::setObjectToStringValue(): new one is created only if
> there is a Proxy/getter/__proto__ change.

If the rope is cached for a long time, might consider making it as a single string in the first place, slightly cutting down on memory cost.

[Alexey Shvayka]

Created attachment 398293 [details]
Patch

Remove JSDOMConstructorBase::className(), add ASSERT_WITH_MESSAGE, add exception check, reword ChangeLog, adjust tests, set @@toStringTag on all WebIDL prototypes.

[Alexey Shvayka]

Comment on attachment 398293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398293&action=review

> LayoutTests/imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/class-string-iterator-prototype-object.any-expected.txt:5
>  FAIL Object.prototype.toString applied after nulling the prototype assert_equals: expected "[object Object]" but got "[object URLSearchParams Iterator]"

This test is incorrect, Chrome fails it too. Upstream PR: https://github.com/web-platform-tests/wpt/pull/23371.

[Alexey Shvayka]

(In reply to Darin Adler from comment #17)
> Also, if toStringName is always returning an ASCIILiteral we should consider
> changing its return type from String to ASCIILiteral.

Since ClassInfo.className is a char*, we might change return type of className() as well.

> If the rope is cached for a long time, might consider making it as a single
> string in the first place, slightly cutting down on memory cost.

I'm a little hesitant to do so w/o extensive benchmarking, hoping that performance of cold runs is also important for code in the wild.

[Alexey Shvayka]

Created attachment 398436 [details]
Patch

Rebase patch and adjust remaining tests.

[EWS]

Committed r261159: <https://trac.webkit.org/changeset/261159>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398436 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62883891>

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #24)
> (In reply to Darin Adler from comment #17)
> > Also, if toStringName is always returning an ASCIILiteral we should consider
> > changing its return type from String to ASCIILiteral.
> 
> Since ClassInfo.className is a char*, we might change return type of
> className() as well.

It doesn't seem like we can change return type of className() to ASCIILiteral because of its usages in:
* Source/JavaScriptCore/API/JSCallbackObject.h
* Source/WebCore/svg/SVGElement.h

We could do this for toStringName(), but it would require tweaking JSCallbackObject<Parent>::getOwnPropertySlot() to end with:

```
bool hasProperty = Parent::getOwnPropertySlot(thisObject, globalObject, propertyName, slot);
if (!hasProperty && propertyName == vm.propertyNames->toStringTagSymbol) {
    slot.setValue(thisObject, PropertyAttribute::None, jsNontrivialString(vm, /* name from classInfo */));
    return true;
}
return hasProperty;
```

which seems quite hacky. We could remove JSCallbackObject<Parent>::toStringName() though.

[Alexey Shvayka]

(In reply to Darin Adler from comment #17)
> Also, if toStringName is always returning an ASCIILiteral we should consider
> changing its return type from String to ASCIILiteral.

https://bugs.webkit.org/show_bug.cgi?id=224247 removes toStringName() and refines builtin tag's type to `const char*`, as in ClassInfo.

[Alexey Shvayka]

*** Bug 161392 has been marked as a duplicate of this bug. ***