Bug 195337

Summary: [JSC] Should check exception for JSString::toExistingAtomicString
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Yusuke Suzuki 2019-03-05 12:58:09 PST
[JSC] Should check exception for JSString::toExistingAtomicString
Comment 1 Yusuke Suzuki 2019-03-05 13:02:27 PST
Created attachment 363672 [details]
Patch
Comment 2 Keith Miller 2019-03-05 13:04:07 PST
Comment on attachment 363672 [details]
Patch

r=me
Comment 3 Saam Barati 2019-03-05 13:04:34 PST
Comment on attachment 363672 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGOperations.cpp:693
> +                RETURN_IF_EXCEPTION(scope, encodedJSValue());

encodedJSValue() => { } ?

> Source/JavaScriptCore/runtime/JSString.cpp:273
> +            RELEASE_AND_RETURN(scope, existingAtomicString);

hmmm, why do you need to release here?

> Source/JavaScriptCore/runtime/JSString.cpp:280
> +            RELEASE_AND_RETURN(scope, existingAtomicString);

ditto

> Source/JavaScriptCore/runtime/JSString.cpp:284
> +    RELEASE_AND_RETURN(scope, nullptr);

ditto
Comment 4 Mark Lam 2019-03-05 13:06:58 PST
Comment on attachment 363672 [details]
Patch

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

r=me

>> Source/JavaScriptCore/runtime/JSString.cpp:273
>> +            RELEASE_AND_RETURN(scope, existingAtomicString);
> 
> hmmm, why do you need to release here?

Because otherwise, the ThrowScope will think you have an unchecked exception.  RELEASE here means we're intentionally letting the caller handle any pending exceptions upon return.
Comment 5 Mark Lam 2019-03-05 13:07:35 PST
(In reply to Mark Lam from comment #4)
> Comment on attachment 363672 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363672&action=review
> 
> r=me
> 
> >> Source/JavaScriptCore/runtime/JSString.cpp:273
> >> +            RELEASE_AND_RETURN(scope, existingAtomicString);
> > 
> > hmmm, why do you need to release here?
> 
> Because otherwise, the ThrowScope will think you have an unchecked
> exception.  RELEASE here means we're intentionally letting the caller handle
> any pending exceptions upon return.

Reminder: the ThrowScope destructor does this checking.
Comment 6 Mark Lam 2019-03-05 13:11:06 PST
Comment on attachment 363672 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/JSString.cpp:273
>>>> +            RELEASE_AND_RETURN(scope, existingAtomicString);
>>> 
>>> hmmm, why do you need to release here?
>> 
>> Because otherwise, the ThrowScope will think you have an unchecked exception.  RELEASE here means we're intentionally letting the caller handle any pending exceptions upon return.
> 
> Reminder: the ThrowScope destructor does this checking.

I was wrong.  The existingAtomicString expression cannot throw.  Saam is right about the RELEASE_AND_RETURN not being needed.
Comment 7 Yusuke Suzuki 2019-03-05 13:12:27 PST
Comment on attachment 363672 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:693
>> +                RETURN_IF_EXCEPTION(scope, encodedJSValue());
> 
> encodedJSValue() => { } ?

I use `encodedJSValue()` here because of consistency in this file.

>> Source/JavaScriptCore/runtime/JSString.cpp:273
>> +            RELEASE_AND_RETURN(scope, existingAtomicString);
> 
> hmmm, why do you need to release here?

Right! We do not need to use RELEASE_AND_RETURN here. Dropped.

>> Source/JavaScriptCore/runtime/JSString.cpp:280
>> +            RELEASE_AND_RETURN(scope, existingAtomicString);
> 
> ditto

Fixed.

>> Source/JavaScriptCore/runtime/JSString.cpp:284
>> +    RELEASE_AND_RETURN(scope, nullptr);
> 
> ditto

Fixed.
Comment 8 Yusuke Suzuki 2019-03-05 13:20:53 PST
Committed r242500: <https://trac.webkit.org/changeset/242500>
Comment 9 Radar WebKit Bug Importer 2019-03-05 13:21:38 PST
<rdar://problem/48611208>