Bug 205323 – [JSC] 8Bit JSRopeString can contain 16Bit string in its rope

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

[Yusuke Suzuki]

[JSC] 8Bit JSRopeString can contain 16Bit string in its rope

[Yusuke Suzuki]

Created attachment 385850 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/57868328>

[Yusuke Suzuki]

Comment on attachment 385850 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSString.cpp:164
>          return;

This is OK. When creating substring JSRopeString, we always resolve the substringBase before creating JSRopeString. This means that we already know the flag of result JSString.
And 8Bit flag of resolved JSString never changes.

[Mark Lam]

Comment on attachment 385850 [details]
Patch

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

r=me.  I wish there's a way to assert that we're not copying 16-bit chars that don't fit into 8-bit chars, but so far, I think that such an assertion is too expensive to have enabled all the time even for a debug build.  Can you file a bug to investigate options for testing this?

> Source/JavaScriptCore/runtime/JSString.h:594
>      void resolveRopeSlowCase8(LChar*) const;

I think this function is now deleted in the implementation.  Please remove this declaration.

[Yusuke Suzuki]

Comment on attachment 385850 [details]
Patch

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

Thanks! Filed https://bugs.webkit.org/show_bug.cgi?id=205355

>> Source/JavaScriptCore/runtime/JSString.h:594
>>      void resolveRopeSlowCase8(LChar*) const;
> 
> I think this function is now deleted in the implementation.  Please remove this declaration.

Fixed.

[Yusuke Suzuki]

Committed r253648: <https://trac.webkit.org/changeset/253648>

[Darin Adler]

Comment on attachment 385850 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.h:1089
> -template<typename CharacterType> inline void StringImpl::copyCharacters(CharacterType* destination, const CharacterType* source, unsigned numCharacters)
> +template<typename SourceCharacterType, typename DestinationCharacterType>
> +inline void StringImpl::copyCharacters(DestinationCharacterType* destination, const SourceCharacterType* source, unsigned numCharacters)
>  {
> -    if (numCharacters == 1) {
> -        *destination = *source;
> -        return;
> +    if constexpr (std::is_same<SourceCharacterType, DestinationCharacterType>::value) {
> +        if (numCharacters == 1) {
> +            *destination = *source;
> +            return;
> +        }
> +        memcpy(destination, source, numCharacters * sizeof(DestinationCharacterType));
> +    } else {
> +        for (unsigned i = 0; i < numCharacters; ++i)
> +            destination[i] = source[i];
>      }
> -    memcpy(destination, source, numCharacters * sizeof(CharacterType));
> -}
> -
> -ALWAYS_INLINE void StringImpl::copyCharacters(UChar* destination, const LChar* source, unsigned numCharacters)
> -{
> -    for (unsigned i = 0; i < numCharacters; ++i)
> -        destination[i] = source[i];
>  }

What’s the rationale for these changes? Do they make the implementation more efficient? Or are they more elegant? I like the old version better and it seems like it generates identical code, but maybe I am missing something.

[Yusuke Suzuki]

Comment on attachment 385850 [details]
Patch

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

>> Source/WTF/wtf/text/StringImpl.h:1089
>>  }
> 
> What’s the rationale for these changes? Do they make the implementation more efficient? Or are they more elegant? I like the old version better and it seems like it generates identical code, but maybe I am missing something.

The rational behind this is adding `copyCharacters(LChar*, UChar*, unsigned)` version of this function, which does not exist previously.
This is necessary to implement JSRopeString's resolution fixed in this patch.

[Darin Adler]

Comment on attachment 385850 [details]
Patch

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

>>> Source/WTF/wtf/text/StringImpl.h:1089
>>>  }
>> 
>> What’s the rationale for these changes? Do they make the implementation more efficient? Or are they more elegant? I like the old version better and it seems like it generates identical code, but maybe I am missing something.
> 
> The rational behind this is adding `copyCharacters(LChar*, UChar*, unsigned)` version of this function, which does not exist previously.
> This is necessary to implement JSRopeString's resolution fixed in this patch.

Could we have named that one differently or does it need to be an overload? I don’t think it’s great to have functions that are guaranteed correct on any data assuming the buffer sizes are correct that have the same name as a function that must only be called if the precondition on the characters is correct. It’s fine to have both functions, but I’d want the one that narrows to have a different name unless there’s a reason it shouldn’t.

[Yusuke Suzuki]

Comment on attachment 385850 [details]
Patch

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

>>>> Source/WTF/wtf/text/StringImpl.h:1089
>>>>  }
>>> 
>>> What’s the rationale for these changes? Do they make the implementation more efficient? Or are they more elegant? I like the old version better and it seems like it generates identical code, but maybe I am missing something.
>> 
>> The rational behind this is adding `copyCharacters(LChar*, UChar*, unsigned)` version of this function, which does not exist previously.
>> This is necessary to implement JSRopeString's resolution fixed in this patch.
> 
> Could we have named that one differently or does it need to be an overload? I don’t think it’s great to have functions that are guaranteed correct on any data assuming the buffer sizes are correct that have the same name as a function that must only be called if the precondition on the characters is correct. It’s fine to have both functions, but I’d want the one that narrows to have a different name unless there’s a reason it shouldn’t.

Make sense! I'll do it in a follow-up patch.