Bug 201021 – Add missing exception check in canonicalizeLocaleList

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

[Justin Michaud]

Add missing exception check in canonicalizeLocaleList

[Justin Michaud]

Created attachment 376965 [details]
Patch

[Justin Michaud]

<rdar://54456311>

[Mark Lam]

Comment on attachment 376965 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 376965 [details]
Patch

Clearing flags on attachment: 376965

Committed r249020: <https://trac.webkit.org/changeset/249020>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 376965 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlObject.cpp:552
>              JSString* tag = kValue.toString(&state);
>              RETURN_IF_EXCEPTION(scope, Vector<String>());
>  
> -            String canonicalizedTag = canonicalizeLanguageTag(tag->value(&state));
> +            auto tagValue = tag->value(&state);

Better idiom for this is toWTFString.

[Mark Lam]

Comment on attachment 376965 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlObject.cpp:552
>> +            auto tagValue = tag->value(&state);
> 
> Better idiom for this is toWTFString.

toWTFString() doesn't help here.  We already know that the tag is a JSString above.  toWTFString() would do a lot of busy work before calling JSString::value() anyway.  If your proposal is to replace the toString() call above also, then that would not work because it does not handle exceptions generated by the toString().  Basically, toWTFString() has this issue in general.

[Darin Adler]

Comment on attachment 376965 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/IntlObject.cpp:552
>>> +            auto tagValue = tag->value(&state);
>> 
>> Better idiom for this is toWTFString.
> 
> toWTFString() doesn't help here.  We already know that the tag is a JSString above.  toWTFString() would do a lot of busy work before calling JSString::value() anyway.  If your proposal is to replace the toString() call above also, then that would not work because it does not handle exceptions generated by the toString().  Basically, toWTFString() has this issue in general.

Yes, I meant replace the toString above.

You’re saying that toWTFString doesn’t handle exceptions generated by toString correctly!? That must be fixed or we have to remove toWTFString!