Bug 143375 – [ES6] Enable Symbol in web pages

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Originally, it is hidden in web pages because it causes Facebook's comment input.
When landing Object.getOwnPropertySymbols, original issue is solved, so I think it's time to enable Symbols in web pages.

And I suggest exposing only effective well-known symbols.
Here's all well-known symbols.

Specification Name	[[Description]]	Value and Purpose
@@hasInstance	"Symbol.hasInstance"	A method that determines if a constructor object recognizes an object as one of the constructor’s instances. Called by the semantics of the instanceof operator.
@@isConcatSpreadable	"Symbol.isConcatSpreadable"	A Boolean valued property that if true indicates that an object should be flattened to its array elements by Array.prototype.concat.
@@iterator	"Symbol.iterator"	A method that returns the default Iterator for an object. Called by the semantics of the for-of statement.
@@match	"Symbol.match"	A regular expression method that matches the regular expression against a string. Called by the String.prototype.match method.
@@replace	"Symbol.replace"	A regular expression method that replaces matched substrings of a string. Called by the String.prototype.replace method.
@@search	"Symbol.search"	A regular expression method that returns the index within a string that matches the regular expression. Called by the String.prototype.search method.
@@species	"Symbol.species"	A function valued property that is the constructor function that is used to create derived objects.
@@split	"Symbol.split"	A regular expression method that splits a string at the indices that match the regular expression. Called by the String.prototype.split method.
@@toPrimitive	"Symbol.toPrimitive"	A method that converts an object to a corresponding primitive value. Called by the ToPrimitive abstract operation.
@@toStringTag	"Symbol.toStringTag"	A String valued property that is used in the creation of the default string description of an object. Accessed by the built-in method Object.prototype.toString.
@@unscopables	"Symbol.unscopables"	An object valued property whose own property names are property names that are excluded from the with environment bindings of the associated object.

In them, JSC supports the following 2 symbols. So at this time, exposing them.

@@iterator
@@unscopables

[Yusuke Suzuki]

Created attachment 250160 [details]
Patch

[Yusuke Suzuki]

After https://bugs.webkit.org/show_bug.cgi?id=143404, I'm planning to land it and enable symbols in web pages.

[Yusuke Suzuki]

Comment on attachment 250160 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:18
> +        and makes enabling symbols by default.

Or is it preferable to drop this runtime flag completely?
This runtime flag will affect when adding Array.prototype.values.

[Darin Adler]

Comment on attachment 250160 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:18
>> +        and makes enabling symbols by default.
> 
> Or is it preferable to drop this runtime flag completely?
> This runtime flag will affect when adding Array.prototype.values.

Not sure about others, but I think we should dump it completely.

[Ryosuke Niwa]

Comment on attachment 250160 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:18
>>> +        and makes enabling symbols by default.
>> 
>> Or is it preferable to drop this runtime flag completely?
>> This runtime flag will affect when adding Array.prototype.values.
> 
> Not sure about others, but I think we should dump it completely.

I think we should keep it for a while given the previous attempt to enable this feature broke Facebook.com
although I agree we should remove it eventually.

[Geoffrey Garen]

> I think we should keep it for a while given the previous attempt to enable
> this feature broke Facebook.com
> although I agree we should remove it eventually.

+1

[Ryosuke Niwa]

Are you going to land this patch?

[Yusuke Suzuki]

I was planning to land it after https://bugs.webkit.org/show_bug.cgi?id=143404 is landed.
But maybe Symbol.for is not so used.
So I'll land it first ;)

[WebKit Commit Bot]

Comment on attachment 250160 [details]
Patch

Clearing flags on attachment: 250160

Committed r182653: <http://trac.webkit.org/changeset/182653>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

JSC tests fail.
However, it's very curious. "weak value == nil" is failed, but in this patch, changes in JSC shell is very few.

Now in this patch, we only attached implemented well known symbols.
So, before this patch, Symbol has the following well known symbols property.
    macro(hasInstance) \
    macro(isConcatSpreadable) \
    macro(iterator) \
    macro(match) \
    macro(replace) \
    macro(search) \
    macro(species) \
    macro(split) \
    macro(toPrimitive) \
    macro(toStringTag) \
    macro(unscopables)

After this patch, we only attached
    macro(iterator) \
    macro(unscopables)

After investigating the cause of JSC tests failure, I've found that when adding not implemented well known symbols, JSC tests pass.
    macro(hasInstance) \
    macro(isConcatSpreadable) \
    macro(iterator) \
    macro(match) \
    macro(replace) \
    macro(search) \
    macro(species) \
    macro(split) \
    macro(toPrimitive) \
    macro(toStringTag) \
    macro(unscopables)

But I think it's storage...

[Yusuke Suzuki]

After investigation, I've found the test sometimes pass / fail when inserting breakpoint in lldb.
And this failure is caused because the previous JSGlobalObject is not collected.
I suppose that because previous JSGlobalObject's pointer is still alive in registers or C stacks, it's not released and it causes this failure.

[Yusuke Suzuki]

OK, so, since it's related to conservative GC (stack/register values) issue, now tests pass.

[Yusuke Suzuki]

Opened issue for this API test failure.
https://bugs.webkit.org/show_bug.cgi?id=143634

[Alexey Proskuryakov]

Comment on attachment 250160 [details]
Patch

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

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:124
>      m_frontendPage->settings().setJavaScriptRuntimeFlags({
> -        JSC::RuntimeFlags::SymbolEnabled
>      });

Does this function need to be called at all with the empty arguments?

> Source/WebKit/mac/WebView/WebPreferencesPrivate.h:56
> -    WebKitJavaScriptRuntimeFlagsSymbolEnabled = 1u << 0,
> +    WebKitJavaScriptRuntimeFlagsSymbolDisabled = 1u << 0,

Why do we have an SPI flag for this, does any WebKit client want to disable Symbol?

"Private" headers are SPI, meaning that internal Apple clients can rely on them. So, exposing more than we need has a long-term cost, and once they are exposed, they generally cannot be changed.

> Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:53
> -    kWKJavaScriptRuntimeFlagsSymbolEnabled = 1 << 0,
> +    kWKJavaScriptRuntimeFlagsSymbolDisabled = 1 << 0,

Ditto.

[Ryosuke Niwa]

(In reply to comment #15)
>
> > Source/WebKit/mac/WebView/WebPreferencesPrivate.h:56
> > -    WebKitJavaScriptRuntimeFlagsSymbolEnabled = 1u << 0,
> > +    WebKitJavaScriptRuntimeFlagsSymbolDisabled = 1u << 0,
> 
> Why do we have an SPI flag for this, does any WebKit client want to disable
> Symbol?
> 
> "Private" headers are SPI, meaning that internal Apple clients can rely on
> them. So, exposing more than we need has a long-term cost, and once they are
> exposed, they generally cannot be changed.
> 
> > Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:53
> > -    kWKJavaScriptRuntimeFlagsSymbolEnabled = 1 << 0,
> > +    kWKJavaScriptRuntimeFlagsSymbolDisabled = 1 << 0,
> 
> Ditto.

That's a good point. We should probably remove these runtime flags from private headers right now since we don't need them to disable the feature on websites.

[Yusuke Suzuki]

Comment on attachment 250160 [details]
Patch

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

>> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:124
>>      });
> 
> Does this function need to be called at all with the empty arguments?

To search runtime flags when changing it, calling it here is nice I think.

[Yusuke Suzuki]

(In reply to comment #16)
> (In reply to comment #15)
> >
> > > Source/WebKit/mac/WebView/WebPreferencesPrivate.h:56
> > > -    WebKitJavaScriptRuntimeFlagsSymbolEnabled = 1u << 0,
> > > +    WebKitJavaScriptRuntimeFlagsSymbolDisabled = 1u << 0,
> > 
> > Why do we have an SPI flag for this, does any WebKit client want to disable
> > Symbol?
> > 
> > "Private" headers are SPI, meaning that internal Apple clients can rely on
> > them. So, exposing more than we need has a long-term cost, and once they are
> > exposed, they generally cannot be changed.
> > 
> > > Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:53
> > > -    kWKJavaScriptRuntimeFlagsSymbolEnabled = 1 << 0,
> > > +    kWKJavaScriptRuntimeFlagsSymbolDisabled = 1 << 0,
> > 
> > Ditto.
> 
> That's a good point. We should probably remove these runtime flags from
> private headers right now since we don't need them to disable the feature on
> websites.

Is it better moving them to public headers?

[Ryosuke Niwa]

No, we shouldn't expose it as SPI or API.
Random app embedding WebKit shouldn't be able to change this runtime flag at all since we want to remove it soon. For binary compatibility, we can't easily remove SPI or API once added.