RESOLVED FIXED 195337
[JSC] Should check exception for JSString::toExistingAtomicString
https://bugs.webkit.org/show_bug.cgi?id=195337
Summary [JSC] Should check exception for JSString::toExistingAtomicString
Yusuke Suzuki
Reported 2019-03-05 12:58:09 PST
[JSC] Should check exception for JSString::toExistingAtomicString
Attachments
Patch (8.30 KB, patch)
2019-03-05 13:02 PST, Yusuke Suzuki
no flags
Yusuke Suzuki
Comment 1 2019-03-05 13:02:27 PST
Keith Miller
Comment 2 2019-03-05 13:04:07 PST
Comment on attachment 363672 [details] Patch r=me
Saam Barati
Comment 3 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
Mark Lam
Comment 4 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.
Mark Lam
Comment 5 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.
Mark Lam
Comment 6 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.
Yusuke Suzuki
Comment 7 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.
Yusuke Suzuki
Comment 8 2019-03-05 13:20:53 PST
Radar WebKit Bug Importer
Comment 9 2019-03-05 13:21:38 PST
Note You need to log in before you can comment on or make changes to this bug.