[webkit-reviews] review granted: [Bug 137202] Make StringView check the lifetime of the StringImpl it's created from : [Attachment 239264] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Darin Adler
<darin at apple.com>'s request for review:
Bug 137202: Make StringView check the lifetime of the StringImpl it's created
from
https://bugs.webkit.org/show_bug.cgi?id=137202

Attachment 239264: Patch
https://bugs.webkit.org/attachment.cgi?id=239264&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239264&action=review


> Source/WTF/wtf/text/StringView.cpp:41
> +    std::atomic_uint refCount { 1 };

I think this should be std::atomic<unsigned> instead.

> Source/WTF/wtf/text/StringView.cpp:56
> +static std::mutex& underlyingStringsMutex()
> +{
> +    static NeverDestroyed<std::mutex> mutex;
> +    return mutex;
> +}

This mutex initialization still isn't thread safe. You need to do something
like:

    static std::once_flag onceFlag;
    static LazyNeverDestroyed<std::mutex> mutex;

    std::call_once(onceFlag, []{
	mutex.construct();
    });

    return mutex;

> Source/WTF/wtf/text/StringView.cpp:107
> +	   auto result = underlyingStrings().add(string, nullptr);
> +	   if (result.isNewEntry)
> +	       result.iterator->value = new UnderlyingString(*string);
> +	   else
> +	       ++result.iterator->value->refCount;

I'd write this as

auto& underlyingStringSlot = underlyingStrings().add(string,
nullptr).iterator->value;
if (!underlyingStringSlot)
    underlyingStringSlot = new UnderlyingString(*string);
else
    ++underlyingString->refCount;
underlyingStringSlot = underlyingStringSlot;

> Source/WTF/wtf/text/StringView.h:142
> +    // FIXME: It's peculiar that null strings are 16-bit and empty strings
return 8-bit (according to the is8Bit function).

I agree - we should make them 8-bit strings as well.

> Source/WTF/wtf/text/StringView.h:154
> +    ASSERT(other.underlyingStringIsValid());

Should probably ASSERT that this != &other here.

> Source/WTF/wtf/text/StringView.h:174
> +    ASSERT(other.underlyingStringIsValid());

Should probably ASSERT(this != &other) here since that's already broken.