[webkit-dev] PSA: String shouldn't be a member of a ThreadSafeRefCounted class

Chris Dumez cdumez at apple.com
Sat Feb 24 09:08:42 PST 2018


Actually, it is my understanding that isolatedCopy() does the right thing here. If you look at the implementation:

String String::isolatedCopy() const &
{
    // FIXME: Should this function, and the many others like it, be inlined?
    return m_impl ? m_impl->isolatedCopy() : String { };
}

String String::isolatedCopy() &&
{
    if (isSafeToSendToAnotherThread()) {
        // Since we know that our string is a temporary that will be destroyed
        // we can just steal the m_impl from it, thus avoiding a copy.
        return { WTFMove(*this) };
    }

    return m_impl ? m_impl->isolatedCopy() : String { };
}

The isolatedCopy() optimization (i.e. isSafeToSendToAnotherThread) can only be used if the String is a temporary. If you example, m_name is not a temporary and will be deep copied.

--
 Chris Dumez




> On Feb 23, 2018, at 11:45 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> 
> Hi all,
> 
> This is a remainder that our String class is NOT thread safe, and should NOT be used inside an object shared across multiple threads. In particular, it's not necessarily safe to have it as a member of ThreadSafeRefCounted class, which can be accessed from multiple threads.
> 
> Let's consider the following example.
> 
> class A : public ThreadSafeRefCounted<A> {
>     public:
>         A(const String& name)
>             : m_name(name)
>         { }
>         String name() { return m_name.isolatedCopy(); }
> 
>     private:
>         String m_name;
> }
> 
> This code is NOT thread safe depending on how name() is used.
> 
> For example, if it's ever inserted or looked up in a hash table as the key, or if it's ever converted into an AtomicString, then it would lead to memory corruption. This is because String::hash() would mutate m_hashAndFlags member variable without any lock, and isolatedCopy() doesn't make a copy if there is exactly one reference to a given StringImpl (String is basically just a RefPtr of StringImpl).
> 
> - R. Niwa
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20180224/c5a46e4a/attachment.html>


More information about the webkit-dev mailing list