Bug 153679 - [INTL] Intl Constructors not web compatible with Object.create usage
Summary: [INTL] Intl Constructors not web compatible with Object.create usage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy VanWagoner
URL:
Keywords:
Depends on: 155300 155308
Blocks: 90906
  Show dependency treegraph
 
Reported: 2016-01-29 15:39 PST by Andy VanWagoner
Modified: 2016-03-10 17:35 PST (History)
9 users (show)

See Also:


Attachments
Patch (16.03 KB, patch)
2016-02-27 16:19 PST, Andy VanWagoner
no flags Details | Formatted Diff | Diff
Patch (16.68 KB, patch)
2016-03-09 06:58 PST, Andy VanWagoner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy VanWagoner 2016-01-29 15:39:24 PST
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.
Comment 1 Andy VanWagoner 2016-01-29 15:42:12 PST
See https://github.com/tc39/ecma402/issues/57
Comment 3 Mark S. Miller 2016-02-21 08:57:01 PST
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.
Comment 4 Andy VanWagoner 2016-02-21 11:46:24 PST
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!
Comment 5 Andy VanWagoner 2016-02-27 16:19:10 PST
Created attachment 272426 [details]
Patch
Comment 6 Darin Adler 2016-03-01 08:31:54 PST
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.
Comment 7 Andy VanWagoner 2016-03-09 06:06:16 PST
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.
Comment 8 Andy VanWagoner 2016-03-09 06:58:00 PST
Created attachment 273426 [details]
Patch
Comment 9 Darin Adler 2016-03-09 20:37:30 PST
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.
Comment 10 WebKit Commit Bot 2016-03-09 21:28:23 PST
Comment on attachment 273426 [details]
Patch

Clearing flags on attachment: 273426

Committed r197925: <http://trac.webkit.org/changeset/197925>
Comment 11 WebKit Commit Bot 2016-03-09 21:28:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Csaba Osztrogonác 2016-03-10 05:55:00 PST
(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.
Comment 14 Andy VanWagoner 2016-03-10 08:16:44 PST
(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.
Comment 15 Andy VanWagoner 2016-03-10 08:44:50 PST
shouldNotBe isn't defined in the test harness used by run-javascriptcore-tests, but is in the harness use by run-webkit-tests.
Comment 16 Csaba Osztrogonác 2016-03-10 08:53:12 PST
(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.
Comment 17 Andy VanWagoner 2016-03-10 09:01:05 PST
(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.
Comment 18 Andy VanWagoner 2016-03-10 09:39:05 PST
https://bugs.webkit.org/show_bug.cgi?id=155300
Comment 19 WebKit Commit Bot 2016-03-10 11:35:04 PST
Re-opened since this is blocked by bug 155308