[JSC] Should check exception for JSString::toExistingAtomicString
Created attachment 363672 [details] Patch
Comment on attachment 363672 [details] Patch r=me
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 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.
(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 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 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.
Committed r242500: <https://trac.webkit.org/changeset/242500>
<rdar://problem/48611208>