Bug 195337 – [JSC] Should check exception for JSString::toExistingAtomicString

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Should check exception for JSString::toExistingAtomicString

[Yusuke Suzuki]

Created attachment 363672 [details]
Patch

[Keith Miller]

Comment on attachment 363672 [details]
Patch

r=me

[Saam Barati]

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

[Mark Lam]

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.

[Mark Lam]

(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.

[Mark Lam]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r242500: <https://trac.webkit.org/changeset/242500>

[Radar WebKit Bug Importer]

<rdar://problem/48611208>