Bug 211020 - [WebIDL] Interface prototype objects should define @@toStringTag
Summary: [WebIDL] Interface prototype objects should define @@toStringTag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Alexey Shvayka
URL: https://github.com/heycam/webidl/pull...
Keywords: InRadar
Depends on:
Blocks: 199138
  Show dependency treegraph
 
Reported: 2020-04-25 05:33 PDT by Alexey Shvayka
Modified: 2020-05-11 17:37 PDT (History)
13 users (show)

See Also:


Attachments
Patch (1.81 MB, patch)
2020-04-25 05:56 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (1.81 MB, patch)
2020-04-25 06:20 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (229.34 KB, patch)
2020-04-25 12:40 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (251.50 KB, patch)
2020-04-25 12:47 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (273.54 KB, patch)
2020-04-25 14:46 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (327.88 KB, patch)
2020-04-27 09:19 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (395.71 KB, patch)
2020-04-28 10:50 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (395.94 KB, patch)
2020-04-28 13:47 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (393.83 KB, patch)
2020-04-30 17:27 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 2020-04-25 05:33:28 PDT
WebIDL spec was recently updated (https://github.com/heycam/webidl/pull/357) to define Symbol.toStringTag on interface prototype objects.
This change aligns WebIDL with ECMA-262 built-ins and Blink's implementation that have been shipping since Q2 2016.
Gecko have also expressed implementation commitment.

r253855 introduced class strings (https://heycam.github.io/webidl/#dfn-class-string) for iterator prototypes only.
Comment 1 Alexey Shvayka 2020-04-25 05:56:48 PDT
Created attachment 397558 [details]
Patch
Comment 2 Alexey Shvayka 2020-04-25 06:20:16 PDT
Created attachment 397560 [details]
Patch

Add JSC namespace in JSDOMWindowProperties.h.
Comment 3 Alexey Shvayka 2020-04-25 12:40:40 PDT
Created attachment 397574 [details]
Patch

Drop WebAssembly[Symbol.toStringTag], fix global objects prototypes, and adjust tests.
Comment 4 Alexey Shvayka 2020-04-25 12:47:27 PDT
Created attachment 397575 [details]
Patch

Add missing "$" in CodeGeneratorJS.pm.
Comment 5 Alexey Shvayka 2020-04-25 14:46:05 PDT
Created attachment 397585 [details]
Patch

Adjust remaining tests and improve ChangeLog.
Comment 6 Darin Adler 2020-04-26 22:16:38 PDT
Comment on attachment 397585 [details]
Patch

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

Looks great.

Is there a more efficient way of getting this behavior without explicitly putting the property in the prototype object at runtime? The change of the name seems fine, but I am a little sad to see all the putDirectWithoutTransition calls. Not really up to date enough on the latest about performance of the JavaScript runtime, so not really sure if this is a bad thing or not.

I also have feelings about code being written out over and over again by the code generator. I’d like to see us put a function somewhere that does this without always repeating toStringTagSymbol and DontEnum and ReadOnly, etc. Once we start generating code, it’s tempting to write out a lot, but I would have wanted the generated code to be more like this:

    setUpToStringTag(vm, "CSSValueList"_s);

That function would be in a plain old C++ source file.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4381
> +        push(@implContent, "    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsNontrivialString(vm, \"${visibleInterfaceName}\"_s), JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);\n");

This assumes that the visibleInterfaceName will never be a single character. If it is a single character, then we need to call jsString rather than jsNontrivialString. Probably a safe assumption -- what’s the chance of a single character DOM class name? -- but maybe we should put something in the perl code to check for this so it’s a compile time failure rather than a runtime failure.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7321
>          if (IsGlobalInterface($interface)) {
>              $structureFlags{"JSC::HasStaticPropertyTable"} = 1;

Could make this a one-liner with perl's suffix-style if statement.

> LayoutTests/ChangeLog:36
> +        * resources/idlharness.js: Remove asserts that were are commented out in upstream.

small mistake: "were are"
Comment 7 Alexey Shvayka 2020-04-27 09:19:58 PDT
Created attachment 397689 [details]
Patch

Add assert before jsNontrivialString(), use suffix-style if statement, tweak ChangeLog, rebase on updated cross-realm tests expectations.
Comment 8 Alexey Shvayka 2020-04-27 09:20:06 PDT
(In reply to Darin Adler from comment #6)

I appreciate the review and comprehensive comments!

> Is there a more efficient way of getting this behavior without explicitly
> putting the property in the prototype object at runtime? The change of the
> name seems fine, but I am a little sad to see all the
> putDirectWithoutTransition calls. Not really up to date enough on the latest
> about performance of the JavaScript runtime, so not really sure if this is a
> bad thing or not.

Given that WebIDL requires @@toStringTag as own property on interface prototypes (https://heycam.github.io/webidl/#dfn-class-string) to enable developers checking for cross-realm instances via `obj[Symbol.toStringTag]` instead of `Object.prototype.toString.call(obj).slice(8, -1)`, there is not much we could do.
If interface prototypes extended a common class, we could have define @@toStringTag as CustomValue though.

> I also have feelings about code being written out over and over again by the
> code generator. I’d like to see us put a function somewhere that does this
> without always repeating toStringTagSymbol and DontEnum and ReadOnly, etc.
> Once we start generating code, it’s tempting to write out a lot, but I would
> have wanted the generated code to be more like this:

I agree that code generator will benefit from changes like this and code removal in general (I have a change in mind that would simplify callback interfaces a bit),
yet I am not sure it worth extracting @@toStringTag due to its size/complexity, and to keep it consistent with other well-known symbols we emit in CodeGeneratorJS.pm.
Comment 9 Darin Adler 2020-04-27 09:27:37 PDT
(In reply to Alexey Shvayka from comment #8)
> I am not sure it worth extracting @@toStringTag due to its
> size/complexity, and to keep it consistent with other well-known symbols we
> emit in CodeGeneratorJS.pm.

My feedback also applies to other well-known symbols we emit in CodeGeneratorJS.pm. I think we should be putting as much of the code as possible in C++ source files rather than in the perl code generator as long as it doesn’t cause problems for complexity, performance, or clarity.
Comment 10 Alexey Shvayka 2020-04-27 11:24:38 PDT
(In reply to Darin Adler from comment #6)
>     setUpToStringTag(vm, "CSSValueList"_s);
> 
> That function would be in a plain old C++ source file.

Since WebIDL interface prototypes extend JSNonFinalObject and are fully generated by Perl script, we can't define setUpToStringTag() as a method and would have to pass an additional argument: `setUpToStringTag(vm, this, "CSSValueList"_s);`. It doesn't feel right to me (in terms of OOP) that a function merely manipulates its object parameter.

Furthermore, I believe it is important to use putDirectWithoutTransition() performance-wise, which does not seem like a good fit inside a helper function (I could not `grep` any usages) as it expects certain object state that we can't guarantee outside of finishCreation().
Comment 11 Darin Adler 2020-04-27 11:32:37 PDT
(In reply to Alexey Shvayka from comment #10)
> Since WebIDL interface prototypes extend JSNonFinalObject and are fully
> generated by Perl script, we can't define setUpToStringTag() as a method and
> would have to pass an additional argument: `setUpToStringTag(vm, this,
> "CSSValueList"_s);`. It doesn't feel right to me (in terms of OOP) that a
> function merely manipulates its object parameter.

OK, but this seems like a straw man. Why can’t we create an abstract base class that’s derived from JSNonFinalObject instead of deriving directly from it?

> Furthermore, I believe it is important to use putDirectWithoutTransition()
> performance-wise, which does not seem like a good fit inside a helper
> function (I could not `grep` any usages) as it expects certain object state
> that we can't guarantee outside of finishCreation().

To me that just means we need an appropriately specific function name.
Comment 12 Alexey Shvayka 2020-04-27 13:17:28 PDT
(In reply to Darin Adler from comment #11)
> Why can’t we create an abstract base class that’s derived from JSNonFinalObject instead of deriving directly from it?

With abstract base class, we can set correct `className` and define @@toStringTag without additional method:

  void JSPrototypeAbstract::finishCreation(VM& vm)
  {
      Base::finishCreation(vm);
      ASSERT(inherits(vm, info()));
      putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsNontrivialString(vm, info()->className), PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly);
  }


> To me that just means we need an appropriately specific function name.

Alternatively, we can define JSObject::putToStringTagWithoutTransition(VM&), which will also expect correctly set `className`, and use it across Source/JSC (~30 call sites).
This helper will ensure not only correct descriptor attributes and usage of jsNontrivialString(), but also a consistent `className` value.
Comment 13 Darin Adler 2020-04-27 13:28:35 PDT
I think those are both good ideas. Thanks for considering some of these approaches that are a little heavier on code outside the code generator, and lighter on code inside it.
Comment 14 Alexey Shvayka 2020-04-28 10:50:27 PDT
Created attachment 397855 [details]
Patch

Extract JSC_TO_STRING_TAG_WITHOUT_TRANSITION() macro.
Comment 15 Darin Adler 2020-04-28 11:08:32 PDT
Comment on attachment 397855 [details]
Patch

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

Looks good.

> Source/JavaScriptCore/runtime/JSObject.h:1604
> +#define JSC_TO_STRING_TAG_WITHOUT_TRANSITION() \
> +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, \
> +        jsNontrivialString(vm, info()->className), PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly)

Why is this a macro rather than a function? The other macros in this file have arguments, and so I see why they are not just functions, but this seems different.
Comment 16 Alexey Shvayka 2020-04-28 13:13:39 PDT
(In reply to Darin Adler from comment #15)
> Why is this a macro rather than a function? The other macros in this file
> have arguments, and so I see why they are not just functions, but this seems
> different.

It is a macro so we could do `info()->className` without resorting to methodTable and dynamic dispatch. If it was a function, it would not perfectly fit in with other putDirect* methods signature-wise as well IMO.
Comment 17 Darin Adler 2020-04-28 13:38:16 PDT
(In reply to Alexey Shvayka from comment #16)
> It is a macro so we could do `info()->className`

Got it.
Comment 18 Alexey Shvayka 2020-04-28 13:47:28 PDT
Created attachment 397879 [details]
Patch

Adjust expectations for class-string-interface.any.js WPT test.
Comment 19 Alexey Shvayka 2020-04-30 17:27:29 PDT
Created attachment 398126 [details]
Patch

dom/nodes/Document-createEvent.html is obsolete, revert expectations change.
Comment 20 Alexey Shvayka 2020-05-01 02:33:26 PDT
Comment on attachment 398126 [details]
Patch

stress/array-buffer-view-watchpoint-can-be-fired-in-really-add-in-dfg.js failure on ARMv7 looks unrelated: https://bugs.webkit.org/show_bug.cgi?id=210717
Comment 21 EWS 2020-05-01 02:48:41 PDT
Committed r260992: <https://trac.webkit.org/changeset/260992>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398126 [details].
Comment 22 Radar WebKit Bug Importer 2020-05-01 02:49:19 PDT
<rdar://problem/62714506>
Comment 23 Alexey Shvayka 2020-05-01 04:26:12 PDT
Committed r260995: <https://trac.webkit.org/changeset/260995>
Comment 24 Darin Adler 2020-05-01 09:50:58 PDT
Are there any places where we can remove code that used to implement strings like "[object Window]" that are now correctly inherited from the prototype?
Comment 25 Alexey Shvayka 2020-05-01 10:55:36 PDT
(In reply to Darin Adler from comment #24)
> Are there any places where we can remove code that used to implement strings
> like "[object Window]" that are now correctly inherited from the prototype?

Not exactly remove, but rather simplify: strings like "[object Window]" for instances are implemented by JSObject::toStringName(), which returns `className`. Now that strings are inherited from prototypes, we can just return "Object" or "Function" from JSObject::toStringName(), making sure pre-ES6 classes redefine it as per steps 5-13 of https://tc39.es/ecma262/#sec-object.prototype.tostring.

Once we do this, remaining tests cases in wpt/WebIDL/ecmascript-binding/class-string-interface.any.js will be fixed, as well as few test262 failures. Please follow the progress at https://bugs.webkit.org/show_bug.cgi?id=199138.