Bug 185578

Summary: [INTL] Improve spec & test262 compliance for Intl APIs
Product: WebKit Reporter: Andy VanWagoner <andy>
Component: JavaScriptCoreAssignee: Andy VanWagoner <andy>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, ryanhaddad, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Andy VanWagoner 2018-05-11 22:12:59 PDT 
[INTL] Improve spec & test262 compliance for Intl APIs
Comment 1 Andy VanWagoner 2018-05-11 22:26:26 PDT 
Created attachment 340246 [details]
Patch
Comment 2 Andy VanWagoner 2018-05-12 08:06:30 PDT 
Created attachment 340250 [details]
Patch
Comment 3 Yusuke Suzuki 2018-05-12 09:13:35 PDT 
Comment on attachment 340250 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:92
> +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsString(&vm, "Object"), PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly);

Use vm.propertyNames->Object

> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:229
>      // Category names are always ASCII, so use char[].

Put `index` here.
Comment 4 EWS Watchlist 2018-05-12 09:19:09 PDT 
Comment on attachment 340250 [details]
Patch

Attachment 340250 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7661626

New failing tests:
stress/regress-178385.js.dfg-maximal-flush-validate-no-cjit
stress/regress-178385.js.ftl-eager
stress/regress-178385.js.ftl-eager-no-cjit-b3o1
stress/regress-178385.js.ftl-no-cjit-small-pool
stress/regress-178385.js.ftl-no-cjit-no-put-stack-validate
stress/regress-178385.js.no-cjit-validate-phases
stress/regress-178385.js.ftl-eager-no-cjit
stress/regress-178385.js.ftl-no-cjit-b3o1
stress/regress-178385.js.dfg-eager-no-cjit-validate
stress/regress-178385.js.default
stress/regress-178385.js.dfg-eager
stress/regress-178385.js.no-cjit-collect-continuously
stress/regress-178385.js.no-ftl
stress/regress-178385.js.ftl-no-cjit-no-inline-validate
stress/regress-178385.js.no-llint
stress/regress-178385.js.ftl-no-cjit-validate-sampling-profiler
apiTests
Comment 5 Andy VanWagoner 2018-05-12 09:26:09 PDT 
(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 340250 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340250&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:92
> > +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsString(&vm, "Object"), PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly);
> 
> Use vm.propertyNames->Object

The value needs to be a JSValue. Identifiers are not JSStrings, and I'm not seeing a straightforward way to convert it.

> 
> > Source/JavaScriptCore/runtime/IntlPluralRules.cpp:229
> >      // Category names are always ASCII, so use char[].
> 
> Put `index` here.

Will do.
Comment 6 Andy VanWagoner 2018-05-12 09:55:57 PDT 
Created attachment 340252 [details]
Patch
Comment 7 WebKit Commit Bot 2018-05-13 07:28:45 PDT 
Comment on attachment 340252 [details]
Patch

Clearing flags on attachment: 340252

Committed r231740: <https://trac.webkit.org/changeset/231740>
Comment 8 WebKit Commit Bot 2018-05-13 07:28:46 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-05-13 07:29:20 PDT 
<rdar://problem/40201390>
Comment 10 Ryan Haddad 2018-05-14 11:43:14 PDT 
Debug JSC bots are failing tests due to an unchecked exception after this change.

ERROR: Unchecked JS exception:
    This scope can throw a JS exception: defineOwnProperty @ ./runtime/JSArray.cpp:140
        (ExceptionScope::m_recursionDepth was 10)
    But the exception was unchecked as of this scope: supportedLocales @ ./runtime/IntlObject.cpp:831
        (ExceptionScope::m_recursionDepth was 9)

Unchecked exception detected at:
    1   0x1026ea1f3 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
    2   0x1026c455b JSC::ThrowScope::~ThrowScope()
    3   0x1026c4935 JSC::ThrowScope::~ThrowScope()
    4   0x1024d2d7c JSC::supportedLocales(JSC::ExecState&, WTF::HashSet<WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String> > const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&, JSC::JSValue)
    5   0x1024cb7ce JSC::IntlNumberFormatConstructorFuncSupportedLocalesOf(JSC::ExecState*)
    6   0xb6d07cc185
    7   0x10138b73c llint_entry
    8   0x101382f42 vmEntryToJavaScript
    9   0x10221606a JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
    10  0x1021b0f9b JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*)
    11  0x1021af972 JSC::eval(JSC::ExecState*)
    12  0x10229b955 llint_slow_path_call_eval
    13  0x10138c077 llint_entry
    14  0x10138b73c llint_entry
    15  0x101382f42 vmEntryToJavaScript
    16  0x10221606a JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
    17  0x1021b53b3 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
    18  0x102463347 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
    19  0x1011dd329 runWithOptions(GlobalObject*, CommandLine&, bool&)
    20  0x1011b520c jscmain(int, char**)::$_3::operator()(JSC::VM&, GlobalObject*, bool&) const
    21  0x10119cdb4 int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&)
    22  0x10119b89f jscmain(int, char**)
    23  0x10119b7fe main
    24  0x7fff6e830015 start
    25  0xc

https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1893
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1007
Comment 11 Andy VanWagoner 2018-05-14 11:46:39 PDT 
(In reply to Ryan Haddad from comment #10)
> Debug JSC bots are failing tests due to an unchecked exception after this
> change.
> 
> ERROR: Unchecked JS exception:
>     This scope can throw a JS exception: defineOwnProperty @
> ./runtime/JSArray.cpp:140
>         (ExceptionScope::m_recursionDepth was 10)
>     But the exception was unchecked as of this scope: supportedLocales @
> ./runtime/IntlObject.cpp:831
>         (ExceptionScope::m_recursionDepth was 9)
> 
> Unchecked exception detected at:
>     1   0x1026ea1f3 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned
> int, JSC::ExceptionEventLocation&)
>     2   0x1026c455b JSC::ThrowScope::~ThrowScope()
>     3   0x1026c4935 JSC::ThrowScope::~ThrowScope()
>     4   0x1024d2d7c JSC::supportedLocales(JSC::ExecState&,
> WTF::HashSet<WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String> >
> const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&,
> JSC::JSValue)
>     5   0x1024cb7ce
> JSC::IntlNumberFormatConstructorFuncSupportedLocalesOf(JSC::ExecState*)
>     6   0xb6d07cc185
>     7   0x10138b73c llint_entry
>     8   0x101382f42 vmEntryToJavaScript
>     9   0x10221606a JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
>     10  0x1021b0f9b JSC::Interpreter::execute(JSC::EvalExecutable*,
> JSC::ExecState*, JSC::JSValue, JSC::JSScope*)
>     11  0x1021af972 JSC::eval(JSC::ExecState*)
>     12  0x10229b955 llint_slow_path_call_eval
>     13  0x10138c077 llint_entry
>     14  0x10138b73c llint_entry
>     15  0x101382f42 vmEntryToJavaScript
>     16  0x10221606a JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
>     17  0x1021b53b3 JSC::Interpreter::executeProgram(JSC::SourceCode const&,
> JSC::ExecState*, JSC::JSObject*)
>     18  0x102463347 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&,
> JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
>     19  0x1011dd329 runWithOptions(GlobalObject*, CommandLine&, bool&)
>     20  0x1011b520c jscmain(int, char**)::$_3::operator()(JSC::VM&,
> GlobalObject*, bool&) const
>     21  0x10119cdb4 int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool,
> jscmain(int, char**)::$_3 const&)
>     22  0x10119b89f jscmain(int, char**)
>     23  0x10119b7fe main
>     24  0x7fff6e830015 start
>     25  0xc
> 
> https://build.webkit.org/builders/Apple%20High%20Sierra%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/1893
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1007

Oops, looks like a simple missing `RETURN_IF_EXCEPTION(scope, JSValue());`. Should I push another patch to this issue, or open a new one?
Comment 12 Andy VanWagoner 2018-05-14 14:01:47 PDT 
https://bugs.webkit.org/show_bug.cgi?id=185623