Bug 195337 - [JSC] Should check exception for JSString::toExistingAtomicString
Summary: [JSC] Should check exception for JSString::toExistingAtomicString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-05 12:58 PST by Yusuke Suzuki
Modified: 2019-03-05 13:21 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.30 KB, patch)
2019-03-05 13:02 PST, Yusuke Suzuki
sbarati: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>