Bug 153679 – [INTL] Intl Constructors not web compatible with Object.create usage

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andy VanWagoner]

The second version of ECMA-402 removed the ability to call Collator, DateTimeFormat, and NumberFormat with an already created `this` value to re-initialize. All calls without `new` instead create a new object, instead of trying to initialize an arbitrary `this`.

The new behavior breaks this pattern used in a actual websites:

    var format = Object.create(Intl.NumberFormat.prototype)
    Intl.NumberFormat.apply(format, args)

The 3rd version will likely re-allow this pattern. While arbitrary objects will not possible to initialize as a Collator, DateTimeFormat, or NumberFormat, objects inheriting from these prototypes can be.

The implementation of Intl constructors in WebKit should be made web-compatible and forward-compatible with this pattern.

[Andy VanWagoner]

See https://github.com/tc39/ecma402/issues/57

[Mark S. Miller]

https://github.com/tc39/ecma402/issues/57#issuecomment-186856109

[Mark S. Miller]

Unlike the github bug thread or what we heard at the tc39 mtg, the first post in this thread uses .apply rather than .call. The code at https://github.com/tc39/ecma402/issues/57#issuecomment-186856109 will need some adjustment to give these objects their own overriding .call and .apply methods without stomping on each other. But https://github.com/tc39/ecma402/issues/57#issuecomment-186856109 , if it otherwise works, gives the idea.

[Andy VanWagoner]

Ok, the gist is essentially that rather than try to swap the object in place, we let it be a proxy of sorts to a real Collator/NumberFormat/DateTimeFormat.

I'll try this week to write a patch that does that.

Thanks!

[Andy VanWagoner]

Created attachment 272426 [details]
Patch

[Darin Adler]

Comment on attachment 272426 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:121
> +    IntlDateTimeFormatConstructor* callee = jsCast<IntlDateTimeFormatConstructor*>(state->callee());

Since callee is used below and thisValue is not, I suggest moving this up one line and maybe initializing vm and callee in a separate paragraph.

> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:124
> +    JSValue prototype = callee->getDirect(vm, vm.propertyNames->prototype);
> +    IntlDateTimeFormat* dateTimeFormat = jsDynamicCast<IntlDateTimeFormat*>(thisValue);
> +    if (!dateTimeFormat && JSObject::defaultHasInstance(state, thisValue, prototype)) {

We it would be nice to not do the work to get the prototype unless thisValue is not a dateTime, so we should be using two nested if statements here. First checking dateTimeFormat for null before getting the prototype.

> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:129
> +        ASSERT(dateTimeFormat);

This assertion seems a little excessive to me; not all that helpful.

> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:132
> +        JSValue locales = state->argument(0);
> +        JSValue options = state->argument(1);
> +        dateTimeFormat->initializeDateTimeFormat(*state, locales, options);

Not all that useful to put these into local variables; I’d suggest just writing this out in a single line. Same in the main code below.

> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:136
> +        if (state->hadException())
> +            return JSValue::encode(jsUndefined());

Where is this exception coming from? If it’s coming from the call to initializeDateTimeFormat, then I suggest doing it before calling putDirect. I am assuming that putDirect cannot return an exception.

> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:144
>      // 3. ReturnIfAbrupt(dateTimeFormat).
>      ASSERT(dateTimeFormat);

I’m a little mystified by this. The standard seems to contemplate an error case here that we assert does not exist!

> Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:118
> +    // FIXME: Workaround to provide compatibility with ECMA-402 1.0 call/apply patterns.

All the same comments apply here as in the IntlDateTimeFormatConstructor.cpp file. It’s also bothering me that we are writing out separate copies of almost identical code for these two classes. Not great factoring.

[Andy VanWagoner]

Comment on attachment 272426 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:121
>> +    IntlDateTimeFormatConstructor* callee = jsCast<IntlDateTimeFormatConstructor*>(state->callee());
> 
> Since callee is used below and thisValue is not, I suggest moving this up one line and maybe initializing vm and callee in a separate paragraph.

will do.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:124
>> +    if (!dateTimeFormat && JSObject::defaultHasInstance(state, thisValue, prototype)) {
> 
> We it would be nice to not do the work to get the prototype unless thisValue is not a dateTime, so we should be using two nested if statements here. First checking dateTimeFormat for null before getting the prototype.

will do.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:129
>> +        ASSERT(dateTimeFormat);
> 
> This assertion seems a little excessive to me; not all that helpful.

yeah, I'll pull it out, and the one below that also just asserts that create worked.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:132
>> +        dateTimeFormat->initializeDateTimeFormat(*state, locales, options);
> 
> Not all that useful to put these into local variables; I’d suggest just writing this out in a single line. Same in the main code below.

will do.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:136
>> +            return JSValue::encode(jsUndefined());
> 
> Where is this exception coming from? If it’s coming from the call to initializeDateTimeFormat, then I suggest doing it before calling putDirect. I am assuming that putDirect cannot return an exception.

I assumed that putDirect can cause an exception on a sealed object. Is that not the case?

>> Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:118
>> +    // FIXME: Workaround to provide compatibility with ECMA-402 1.0 call/apply patterns.
> 
> All the same comments apply here as in the IntlDateTimeFormatConstructor.cpp file. It’s also bothering me that we are writing out separate copies of almost identical code for these two classes. Not great factoring.

Personally, I think writing the same thing twice is better than trying to make an abstraction that makes sense and works in both places, especially when I expect this workaround to just get ripped out after a while. If it were 3 or 4 times, I would feel more pressure to create an abstraction.

[Andy VanWagoner]

Created attachment 273426 [details]
Patch

[Darin Adler]

Comment on attachment 272426 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:136
>>> +            return JSValue::encode(jsUndefined());
>> 
>> Where is this exception coming from? If it’s coming from the call to initializeDateTimeFormat, then I suggest doing it before calling putDirect. I am assuming that putDirect cannot return an exception.
> 
> I assumed that putDirect can cause an exception on a sealed object. Is that not the case?

I do not think it can. I believe that putDirect is only safe to use when you know a sealed object is not involved.

[WebKit Commit Bot]

Comment on attachment 273426 [details]
Patch

Clearing flags on attachment: 273426

Committed r197925: <http://trac.webkit.org/changeset/197925>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Looks like this caused 14 JSC tests to fail:
https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/4090/steps/jscore-test/logs/stdio

[Csaba Osztrogonác]

(In reply to comment #12)
> Looks like this caused 14 JSC tests to fail:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/4090/steps/jscore-
> test/logs/stdio

Please run run-javascriptcore-tests next time before 
landing a JSC change to avoid similar breakages.

[Andy VanWagoner]

(In reply to comment #13)
> (In reply to comment #12)
> > Looks like this caused 14 JSC tests to fail:
> > https://build.webkit.org/builders/
> > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/4090/steps/jscore-
> > test/logs/stdio
> 
> Please run run-javascriptcore-tests next time before 
> landing a JSC change to avoid similar breakages.

It's sad that this is not part of EWS.

[Andy VanWagoner]

shouldNotBe isn't defined in the test harness used by run-javascriptcore-tests, but is in the harness use by run-webkit-tests.

[Csaba Osztrogonác]

(In reply to comment #15)
> shouldNotBe isn't defined in the test harness used by
> run-javascriptcore-tests, but is in the harness use by run-webkit-tests.

In this case you should add it to standalone-pre.js too.

[Andy VanWagoner]

(In reply to comment #16)
> (In reply to comment #15)
> > shouldNotBe isn't defined in the test harness used by
> > run-javascriptcore-tests, but is in the harness use by run-webkit-tests.
> 
> In this case you should add it to standalone-pre.js too.

I'm preparing a patch now.

[Andy VanWagoner]

https://bugs.webkit.org/show_bug.cgi?id=155300

[WebKit Commit Bot]

Re-opened since this is blocked by bug 155308