Bug 199138 - Object.prototype.toString is not spec-perfect
Summary: Object.prototype.toString is not spec-perfect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on: 211020
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-23 15:37 PDT by Alexey Shvayka
Modified: 2020-06-11 12:10 PDT (History)
14 users (show)

See Also:


Attachments
Patch (36.85 KB, patch)
2019-06-23 16:37 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.23 MB, application/zip)
2019-06-23 18:01 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.80 MB, application/zip)
2019-06-23 18:09 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (3.04 MB, application/zip)
2019-06-23 18:38 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.65 MB, application/zip)
2019-06-23 18:47 PDT, EWS Watchlist
no flags Details
Patch (45.59 KB, patch)
2019-06-24 05:53 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (122.64 KB, patch)
2019-06-24 10:47 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (123.40 KB, patch)
2019-07-18 11:43 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (123.40 KB, patch)
2019-07-18 13:18 PDT, Alexey Shvayka
shvaikalesh: review-
shvaikalesh: commit-queue-
Details | Formatted Diff | Diff
Patch (38.64 KB, patch)
2020-05-01 16:20 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (47.25 KB, patch)
2020-05-02 13:07 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (47.70 KB, patch)
2020-05-04 17:04 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2019-06-23 15:37:58 PDT
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"
```
Comment 1 Alexey Shvayka 2019-06-23 16:37:33 PDT Comment hidden (obsolete)
Comment 2 EWS Watchlist 2019-06-23 18:01:34 PDT Comment hidden (obsolete)
Comment 3 EWS Watchlist 2019-06-23 18:01:36 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-06-23 18:09:15 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-06-23 18:09:16 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-06-23 18:34:51 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-06-23 18:38:08 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-06-23 18:38:10 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2019-06-23 18:47:21 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2019-06-23 18:47:23 PDT Comment hidden (obsolete)
Comment 11 Alexey Shvayka 2019-06-24 05:53:47 PDT Comment hidden (obsolete)
Comment 12 Alexey Shvayka 2019-06-24 10:47:37 PDT Comment hidden (obsolete)
Comment 13 Alexey Shvayka 2019-07-18 11:43:28 PDT Comment hidden (obsolete)
Comment 14 Alexey Shvayka 2019-07-18 13:18:14 PDT Comment hidden (obsolete)
Comment 15 Alexey Shvayka 2019-08-14 13:28:28 PDT Comment hidden (obsolete)
Comment 16 Alexey Shvayka 2020-05-01 16:20:00 PDT
Created attachment 398257 [details]
Patch
Comment 17 Darin Adler 2020-05-01 16:59:37 PDT
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?
Comment 18 Alexey Shvayka 2020-05-01 17:16:42 PDT
(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.
Comment 19 Keith Miller 2020-05-01 17:17:00 PDT
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.
Comment 20 Alexey Shvayka 2020-05-01 17:20:56 PDT
(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.
Comment 21 Darin Adler 2020-05-01 18:01:06 PDT
(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.
Comment 22 Alexey Shvayka 2020-05-02 13:07:55 PDT
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.
Comment 23 Alexey Shvayka 2020-05-02 13:09:20 PDT
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.
Comment 24 Alexey Shvayka 2020-05-02 13:22:19 PDT
(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.
Comment 25 Alexey Shvayka 2020-05-04 17:04:15 PDT
Created attachment 398436 [details]
Patch

Rebase patch and adjust remaining tests.
Comment 26 EWS 2020-05-05 04:33:26 PDT
Committed r261159: <https://trac.webkit.org/changeset/261159>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398436 [details].
Comment 27 Radar WebKit Bug Importer 2020-05-05 04:34:19 PDT
<rdar://problem/62883891>
Comment 28 Alexey Shvayka 2020-06-11 12:10:59 PDT
(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.