Bug 184952 – fromCharCode is missing some exception checks

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

[Keith Miller]

fromCharCode is missing some exception checks

[Keith Miller]

Created attachment 338699 [details]
Patch

[Saam Barati]

Comment on attachment 338699 [details]
Patch

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

> JSTests/stress/fromCharCode-exception-check.js:5
> +} catch (e) { }

probably worth checking the exception type here

[Keith Miller]

<rdar://problem/39001185>

[Keith Miller]

Comment on attachment 338699 [details]
Patch

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

>> JSTests/stress/fromCharCode-exception-check.js:5
>> +} catch (e) { }
> 
> probably worth checking the exception type here

Done.

[Keith Miller]

Committed r230980: <https://trac.webkit.org/changeset/230980>

[Darin Adler]

Comment on attachment 338699 [details]
Patch

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

> Source/JavaScriptCore/runtime/StringConstructor.cpp:82
> +        RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +        scope.release();

Sad to have to do this branch just to optimize the exception case. Can’t this have the optimization that the old code did, where it just did a little wasteful work to make the single character string that would then be ignored, and let the exception propagate without checking for it explicitly? Or is that impractical now?

[Keith Miller]

Comment on attachment 338699 [details]
Patch

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

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:82
>> +        scope.release();
> 
> Sad to have to do this branch just to optimize the exception case. Can’t this have the optimization that the old code did, where it just did a little wasteful work to make the single character string that would then be ignored, and let the exception propagate without checking for it explicitly? Or is that impractical now?

We could do that. You'd just need to move the scope.release() above the current line.

In practice though, I'd guess that the branch doesn't actually matter much, especially if it's never taken. By the time we get to the DFG/FTL, we already have an intrinsic for fromCharCode so this code doesn't really matter. In the LLInt/Baseline there are so many other branches this one probably doesn't matter.

Either way, since it's safe to just ignore the exception status I'll remove the RETURN_IF_EXCEPTION line.

[Keith Miller]

(In reply to Keith Miller from comment #7)
> Comment on attachment 338699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338699&action=review
> 
> >> Source/JavaScriptCore/runtime/StringConstructor.cpp:82
> >> +        scope.release();
> > 
> > Sad to have to do this branch just to optimize the exception case. Can’t this have the optimization that the old code did, where it just did a little wasteful work to make the single character string that would then be ignored, and let the exception propagate without checking for it explicitly? Or is that impractical now?
> 
> We could do that. You'd just need to move the scope.release() above the
> current line.
> 
> In practice though, I'd guess that the branch doesn't actually matter much,
> especially if it's never taken. By the time we get to the DFG/FTL, we
> already have an intrinsic for fromCharCode so this code doesn't really
> matter. In the LLInt/Baseline there are so many other branches this one
> probably doesn't matter.
> 
> Either way, since it's safe to just ignore the exception status I'll remove
> the RETURN_IF_EXCEPTION line.

Uploaded: https://bugs.webkit.org/show_bug.cgi?id=185083