[WebIDL] Interface prototype objects should define @@toStringTag

• Component: Bindings
• Status: RESOLVED FIXED

[Alexey Shvayka]

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.

[Alexey Shvayka]

Created attachment 397558 [details]
Patch

[Alexey Shvayka]

Created attachment 397560 [details]
Patch

Add JSC namespace in JSDOMWindowProperties.h.

[Alexey Shvayka]

Created attachment 397574 [details]
Patch

Drop WebAssembly[Symbol.toStringTag], fix global objects prototypes, and adjust tests.

[Alexey Shvayka]

Created attachment 397575 [details]
Patch

Add missing "$" in CodeGeneratorJS.pm.

[Alexey Shvayka]

Created attachment 397585 [details]
Patch

Adjust remaining tests and improve ChangeLog.

[Darin Adler]

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"

[Alexey Shvayka]

Created attachment 397689 [details]
Patch

Add assert before jsNontrivialString(), use suffix-style if statement, tweak ChangeLog, rebase on updated cross-realm tests expectations.

[Alexey Shvayka]

(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.

[Darin Adler]

(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.

[Alexey Shvayka]

(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().

[Darin Adler]

(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.

[Alexey Shvayka]

(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.

[Darin Adler]

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.

[Alexey Shvayka]

Created attachment 397855 [details]
Patch

Extract JSC_TO_STRING_TAG_WITHOUT_TRANSITION() macro.

[Darin Adler]

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.

[Alexey Shvayka]

(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.

[Darin Adler]

(In reply to Alexey Shvayka from comment #16)
> It is a macro so we could do `info()->className`

Got it.

[Alexey Shvayka]

Created attachment 397879 [details]
Patch

Adjust expectations for class-string-interface.any.js WPT test.

[Alexey Shvayka]

Created attachment 398126 [details]
Patch

dom/nodes/Document-createEvent.html is obsolete, revert expectations change.

[Alexey Shvayka]

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

[EWS]

Committed r260992: <https://trac.webkit.org/changeset/260992>

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

[Radar WebKit Bug Importer]

<rdar://problem/62714506>

[Alexey Shvayka]

Committed r260995: <https://trac.webkit.org/changeset/260995>

[Darin Adler]

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?

[Alexey Shvayka]

(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.