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" ```
Created attachment 372719 [details] Patch
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
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
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
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
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
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
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
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
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
Created attachment 372748 [details] Patch Add "WindowProperties" symbol and fix tests.
Created attachment 372771 [details] Patch Adjust bindings tests.
Created attachment 374400 [details] Patch Rebase and mark 4 more tests as passing.
Created attachment 374407 [details] Patch Fix weird type conversion error.
Adding Darin to CC in case he has some feedback on WebCore/bindings changes.
Created attachment 398257 [details] Patch
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?
(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 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.
(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.
(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.
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 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.
(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.
Created attachment 398436 [details] Patch Rebase patch and adjust remaining tests.
Committed r261159: <https://trac.webkit.org/changeset/261159> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398436 [details].
<rdar://problem/62883891>
(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.
(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.
*** Bug 161392 has been marked as a duplicate of this bug. ***