Bug 201021 - Add missing exception check in canonicalizeLocaleList
Summary: Add missing exception check in canonicalizeLocaleList
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-21 19:13 PDT by Justin Michaud
Modified: 2019-08-22 17:23 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.35 KB, patch)
2019-08-21 19:14 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2019-08-21 19:13:48 PDT
Add missing exception check in canonicalizeLocaleList
Comment 1 Justin Michaud 2019-08-21 19:14:16 PDT
Created attachment 376965 [details]
Patch
Comment 2 Justin Michaud 2019-08-21 19:15:27 PDT
<rdar://54456311>
Comment 3 Mark Lam 2019-08-21 19:17:25 PDT
Comment on attachment 376965 [details]
Patch

r=me
Comment 4 WebKit Commit Bot 2019-08-22 11:18:11 PDT
Comment on attachment 376965 [details]
Patch

Clearing flags on attachment: 376965

Committed r249020: <https://trac.webkit.org/changeset/249020>
Comment 5 WebKit Commit Bot 2019-08-22 11:18:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2019-08-22 16:39:16 PDT
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.
Comment 7 Mark Lam 2019-08-22 16:59:35 PDT
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.
Comment 8 Darin Adler 2019-08-22 17:23:03 PDT
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!