Make API::String copy the underlying strings if needed, for thread safety
authorandersca@apple.com <andersca@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Dec 2014 01:03:15 +0000 (01:03 +0000)
committerandersca@apple.com <andersca@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Dec 2014 01:03:15 +0000 (01:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=139261

Reviewed by Sam Weinig.

Source/WebKit2:

* Shared/API/c/WKString.cpp:
(WKStringCreateWithUTF8CString):
(WKStringCreateWithJSString):
(WKStringCopyJSString):
Move the implementations from API::String and directly into the API functions.

* Shared/APIString.h:
Add a create overload that takes an rvalue reference. Call it from the create overload
that takes an lvalue reference, but explicitly copy the string.
We call isolatedCopy() again on the string in the rvalue reference overload, but that is a no-op
if the string can be sent to another thread. Add assertions in the String constructor that we can
send the string to another thread.

Source/WTF:

* wtf/RefPtr.h:
(WTF::RefPtr<T>::leakRef):
Add a leakRef() to RefPtr so we don't have to go through PassRefPtr.

* wtf/text/WTFString.cpp:
(WTF::String::isSafeToSendToAnotherThread):
Check if the string is empty before checking whether it's in an atomic string table.
It's safe to send empty strings to other threads even if they're in the atomic string table
since they will never be deallocated.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@176826 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/RefPtr.h
Source/WTF/wtf/text/WTFString.cpp
Source/WebKit2/ChangeLog
Source/WebKit2/Shared/API/c/WKString.cpp
Source/WebKit2/Shared/APIString.h

index 40e51c1..a24f9a4 100644 (file)
@@ -1,3 +1,20 @@
+2014-12-04  Anders Carlsson  <andersca@apple.com>
+
+        Make API::String copy the underlying strings if needed, for thread safety
+        https://bugs.webkit.org/show_bug.cgi?id=139261
+
+        Reviewed by Sam Weinig.
+
+        * wtf/RefPtr.h:
+        (WTF::RefPtr<T>::leakRef):
+        Add a leakRef() to RefPtr so we don't have to go through PassRefPtr.
+
+        * wtf/text/WTFString.cpp:
+        (WTF::String::isSafeToSendToAnotherThread):
+        Check if the string is empty before checking whether it's in an atomic string table.
+        It's safe to send empty strings to other threads even if they're in the atomic string table
+        since they will never be deallocated.
+
 2014-12-04  Csaba Osztrogon√°c  <ossy@webkit.org>
 
         Fix cast-align warning in StringImpl.h
index 3213fb1..5de5c73 100644 (file)
@@ -64,6 +64,8 @@ namespace WTF {
         PassRefPtr<T> release() { PassRefPtr<T> tmp = adoptRef(m_ptr); m_ptr = nullptr; return tmp; }
         PassRef<T> releaseNonNull() { ASSERT(m_ptr); PassRef<T> tmp = adoptRef(*m_ptr); m_ptr = nullptr; return tmp; }
 
+        T* leakRef() WARN_UNUSED_RETURN;
+
         T& operator*() const { return *m_ptr; }
         ALWAYS_INLINE T* operator->() const { return m_ptr; }
         
@@ -107,6 +109,15 @@ namespace WTF {
         derefIfNotNull(ptr);
     }
 
+    template<typename T>
+    inline T* RefPtr<T>::leakRef()
+    {
+        T* ptr = m_ptr;
+        m_ptr = nullptr;
+
+        return ptr;
+    }
+
     template<typename T> inline RefPtr<T>& RefPtr<T>::operator=(const RefPtr& o)
     {
         RefPtr ptr = o;
index f69ec84..debd926 100644 (file)
@@ -690,14 +690,14 @@ bool String::isSafeToSendToAnotherThread() const
 {
     if (!impl())
         return true;
+    if (isEmpty())
+        return true;
     // AtomicStrings are not safe to send between threads as ~StringImpl()
     // will try to remove them from the wrong AtomicStringTable.
     if (impl()->isAtomic())
         return false;
     if (impl()->hasOneRef())
         return true;
-    if (isEmpty())
-        return true;
     return false;
 }
 
index 16aa8e0..9e465d7 100644 (file)
@@ -1,3 +1,23 @@
+2014-12-04  Anders Carlsson  <andersca@apple.com>
+
+        Make API::String copy the underlying strings if needed, for thread safety
+        https://bugs.webkit.org/show_bug.cgi?id=139261
+
+        Reviewed by Sam Weinig.
+
+        * Shared/API/c/WKString.cpp:
+        (WKStringCreateWithUTF8CString):
+        (WKStringCreateWithJSString):
+        (WKStringCopyJSString):
+        Move the implementations from API::String and directly into the API functions.
+
+        * Shared/APIString.h:
+        Add a create overload that takes an rvalue reference. Call it from the create overload
+        that takes an lvalue reference, but explicitly copy the string.
+        We call isolatedCopy() again on the string in the rvalue reference overload, but that is a no-op
+        if the string can be sent to another thread. Add assertions in the String constructor that we can
+        send the string to another thread.
+
 2014-12-04  Beth Dakin  <bdakin@apple.com>
 
         Clients disabling action menus sometimes still invoke action menu behaviors
index 9847181..93becd9 100644 (file)
@@ -28,6 +28,9 @@
 #include "WKStringPrivate.h"
 
 #include "WKAPICast.h"
+#include <JavaScriptCore/InitializeThreading.h>
+#include <JavaScriptCore/JSStringRef.h>
+#include <JavaScriptCore/OpaqueJSString.h>
 
 using namespace WebKit;
 
@@ -38,7 +41,7 @@ WKTypeID WKStringGetTypeID()
 
 WKStringRef WKStringCreateWithUTF8CString(const char* string)
 {
-    RefPtr<API::String> apiString = API::String::createFromUTF8String(string);
+    auto apiString = API::String::create(WTF::String::fromUTF8(string));
     return toAPI(apiString.release().leakRef());
 }
 
@@ -85,11 +88,13 @@ bool WKStringIsEqualToUTF8CStringIgnoringCase(WKStringRef aRef, const char* b)
 
 WKStringRef WKStringCreateWithJSString(JSStringRef jsStringRef)
 {
-    RefPtr<API::String> apiString = jsStringRef ? API::String::create(jsStringRef) : API::String::createNull();
+    auto apiString = jsStringRef ? API::String::create(jsStringRef->string()) : API::String::createNull();
+
     return toAPI(apiString.release().leakRef());
 }
 
 JSStringRef WKStringCopyJSString(WKStringRef stringRef)
 {
-    return toImpl(stringRef)->createJSString();
+    JSC::initializeThreading();
+    return OpaqueJSString::create(toImpl(stringRef)->string()).leakRef();
 }
index 5fe603b..2b586cc 100644 (file)
@@ -27,9 +27,6 @@
 #define APIString_h
 
 #include "APIObject.h"
-#include <JavaScriptCore/InitializeThreading.h>
-#include <JavaScriptCore/JSStringRef.h>
-#include <JavaScriptCore/OpaqueJSString.h>
 #include <wtf/PassRefPtr.h>
 #include <wtf/text/StringView.h>
 #include <wtf/text/WTFString.h>
@@ -39,24 +36,19 @@ namespace API {
 
 class String final : public ObjectImpl<Object::Type::String> {
 public:
-    static PassRefPtr<String> createNull()
+    static RefPtr<String> createNull()
     {
         return adoptRef(new String);
     }
 
-    static PassRefPtr<String> create(const WTF::String& string)
+    static RefPtr<String> create(WTF::String&& string)
     {
-        return adoptRef(new String(string));
+        return adoptRef(new String(string.isNull() ? WTF::String(StringImpl::empty()) : string.isolatedCopy()));
     }
 
-    static PassRefPtr<String> create(JSStringRef jsStringRef)
+    static RefPtr<String> create(const WTF::String& string)
     {
-        return adoptRef(new String(jsStringRef->string()));
-    }
-
-    static PassRefPtr<String> createFromUTF8String(const char* string)
-    {
-        return adoptRef(new String(WTF::String::fromUTF8(string)));
+        return create(string.isolatedCopy());
     }
 
     virtual ~String()
@@ -101,24 +93,20 @@ public:
 
     const WTF::String& string() const { return m_string; }
 
-    JSStringRef createJSString() const
-    {
-        JSC::initializeThreading();
-        return OpaqueJSString::create(m_string).leakRef();
-    }
-
 private:
     String()
         : m_string()
     {
     }
 
-    String(const WTF::String& string)
-        : m_string(!string.impl() ? WTF::String(StringImpl::empty()) : string)
+    String(WTF::String&& string)
+        : m_string(WTF::move(string))
     {
+        ASSERT(!m_string.isNull());
+        ASSERT(m_string.isSafeToSendToAnotherThread());
     }
 
-    WTF::String m_string;
+    const WTF::String m_string;
 };
 
 } // namespace WebKit