Bug 145352 – String.prototype.charAt() should use StringView.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andreas Kling]

String.prototype.charAt() currently always reifies JSString. This is wasteful if the JSString is a substring.
It also uses jsSingleCharacterSubstring() which is a counter-productive optimization, so let's remove that.

[Andreas Kling]

Created attachment 253651 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 253651 [details]
Patch

Clearing flags on attachment: 253651

Committed r184865: <http://trac.webkit.org/changeset/184865>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 253651 [details]
Patch

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

> Source/JavaScriptCore/runtime/StringPrototype.cpp:788
> +    StringView string = thisValue.toString(exec)->view(exec);

This code is wrong. It takes a temporary reference to a string's backing store, but nothing prevents that backing store from being deleted or garbage collected.

This code is only possible because JSString::SafeView will automatically convert to StringView, due to its operator StringView(). It is very weird that the safe type automatically converts to the unsafe type. That makes the safe type unsafe too.

[Darin Adler]

Comment on attachment 253651 [details]
Patch

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

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:788
>> +    StringView string = thisValue.toString(exec)->view(exec);
> 
> This code is wrong. It takes a temporary reference to a string's backing store, but nothing prevents that backing store from being deleted or garbage collected.
> 
> This code is only possible because JSString::SafeView will automatically convert to StringView, due to its operator StringView(). It is very weird that the safe type automatically converts to the unsafe type. That makes the safe type unsafe too.

I spotted the same mistake in another patch recently.

[Andreas Kling]

crab

That operator StringView() is kind of a footgun eh.

[Darin Adler]

Oh, all we need to do is to make the return type be SafeView instead of StringView.

[Darin Adler]

(In reply to comment #4)
> It is very weird
> that the safe type automatically converts to the unsafe type. That makes the
> safe type unsafe too.

The job of SafeView is to make code like this OK:

    function(value->view(exec));

It doesn’t make code with local variables safe. Perhaps we can find an even better solution.

[Darin Adler]

(In reply to comment #7)
> Oh, all we need to do is to make the return type be SafeView instead of
> StringView.

local variable type, not return type

Never occurred to me to actually use SafeView as a local variable.