No thread safety when passing ThreadableLoaderOptions from a worker thread

https://bugs.webkit.org/show_bug.cgi?id=143790

Reviewed by Geoffrey Garen.

* loader/ThreadableLoader.h:
* loader/ThreadableLoader.cpp: (WebCore::ThreadableLoaderOptions::isolatedCopy): Added.

* loader/WorkerThreadableLoader.cpp:
(WebCore::WorkerThreadableLoader::MainThreadBridge::MainThreadBridge): Don't just send
a structure with strings to a different thread, that's bad.

* platform/CrossThreadCopier.h: I think that this is dead code, but for this bug,
just removing a clearly wrong specialization.

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index fe24ee8..530bc91 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,20 @@
+2015-04-15  Alexey Proskuryakov  <ap@apple.com>
+
+        No thread safety when passing ThreadableLoaderOptions from a worker thread
+        https://bugs.webkit.org/show_bug.cgi?id=143790
+
+        Reviewed by Geoffrey Garen.
+
+        * loader/ThreadableLoader.h:
+        * loader/ThreadableLoader.cpp: (WebCore::ThreadableLoaderOptions::isolatedCopy): Added.
+
+        * loader/WorkerThreadableLoader.cpp:
+        (WebCore::WorkerThreadableLoader::MainThreadBridge::MainThreadBridge): Don't just send
+        a structure with strings to a different thread, that's bad.
+
+        * platform/CrossThreadCopier.h: I think that this is dead code, but for this bug,
+        just removing a clearly wrong specialization.
+
 2015-04-15  Alex Christensen  <achristensen@webkit.org>
 
         Progress towards CMake on Mac.

diff --git a/Source/WebCore/loader/ThreadableLoader.cpp b/Source/WebCore/loader/ThreadableLoader.cpp
index a850552..ff76c3d 100644 (file)
--- a/Source/WebCore/loader/ThreadableLoader.cpp
+++ b/Source/WebCore/loader/ThreadableLoader.cpp
@@ -51,6 +51,17 @@ ThreadableLoaderOptions::~ThreadableLoaderOptions()
 {
 }
 
+std::unique_ptr<ThreadableLoaderOptions> ThreadableLoaderOptions::isolatedCopy() const
+{
+    std::unique_ptr<ThreadableLoaderOptions> copy = std::make_unique<ThreadableLoaderOptions>();
+    copy->preflightPolicy = preflightPolicy;
+    copy->crossOriginRequestPolicy = crossOriginRequestPolicy;
+    if (securityOrigin)
+        copy->securityOrigin = securityOrigin->isolatedCopy();
+    copy->initiator = initiator.string().isolatedCopy();
+    return copy;
+}
+
 PassRefPtr<ThreadableLoader> ThreadableLoader::create(ScriptExecutionContext* context, ThreadableLoaderClient* client, const ResourceRequest& request, const ThreadableLoaderOptions& options)
 {
     ASSERT(client);

diff --git a/Source/WebCore/loader/ThreadableLoader.h b/Source/WebCore/loader/ThreadableLoader.h
index cbb84c4..0bb6cd0 100644 (file)
--- a/Source/WebCore/loader/ThreadableLoader.h
+++ b/Source/WebCore/loader/ThreadableLoader.h
@@ -63,6 +63,8 @@ namespace WebCore {
         ThreadableLoaderOptions();
         ~ThreadableLoaderOptions();
 
+        std::unique_ptr<ThreadableLoaderOptions> isolatedCopy() const;
+
         PreflightPolicy preflightPolicy; // If AccessControl is used, how to determine if a preflight is needed.
         CrossOriginRequestPolicy crossOriginRequestPolicy;
         RefPtr<SecurityOrigin> securityOrigin;

diff --git a/Source/WebCore/loader/WorkerThreadableLoader.cpp b/Source/WebCore/loader/WorkerThreadableLoader.cpp
index e92b89d..74e12ca 100644 (file)
--- a/Source/WebCore/loader/WorkerThreadableLoader.cpp
+++ b/Source/WebCore/loader/WorkerThreadableLoader.cpp
@@ -91,18 +91,21 @@ WorkerThreadableLoader::MainThreadBridge::MainThreadBridge(PassRefPtr<Threadable
     ASSERT(m_workerClientWrapper.get());
 
     auto* requestData = request.copyData().release();
+    auto* optionsCopy = options.isolatedCopy().release();
     StringCapture capturedOutgoingReferrer(outgoingReferrer);
-    m_loaderProxy.postTaskToLoader([this, requestData, options, capturedOutgoingReferrer](ScriptExecutionContext& context) {
+    m_loaderProxy.postTaskToLoader([this, requestData, optionsCopy, capturedOutgoingReferrer](ScriptExecutionContext& context) {
         ASSERT(isMainThread());
         Document& document = downcast<Document>(context);
 
         auto request = ResourceRequest::adopt(std::unique_ptr<CrossThreadResourceRequestData>(requestData));
         request->setHTTPReferrer(capturedOutgoingReferrer.string());
 
+        auto options = std::unique_ptr<ThreadableLoaderOptions>(optionsCopy);
+
         // FIXME: If the a site requests a local resource, then this will return a non-zero value but the sync path
         // will return a 0 value. Either this should return 0 or the other code path should do a callback with
         // a failure.
-        m_mainThreadLoader = DocumentThreadableLoader::create(document, *this, *request, options);
+        m_mainThreadLoader = DocumentThreadableLoader::create(document, *this, *request, *options);
         ASSERT(m_mainThreadLoader);
     });
 }

diff --git a/Source/WebCore/platform/CrossThreadCopier.h b/Source/WebCore/platform/CrossThreadCopier.h
index f38663c..70f9243 100644 (file)
--- a/Source/WebCore/platform/CrossThreadCopier.h
+++ b/Source/WebCore/platform/CrossThreadCopier.h
@@ -92,9 +92,6 @@ namespace WebCore {
 
     // To allow a type to be passed across threads using its copy constructor, add a forward declaration of the type and
     // a CopyThreadCopierBase<false, false, TypeName> : public CrossThreadCopierPassThrough<TypeName> { }; to this file.
-    template<> struct CrossThreadCopierBase<false, false, ThreadableLoaderOptions> : public CrossThreadCopierPassThrough<ThreadableLoaderOptions> {
-    };
-
     template<> struct CrossThreadCopierBase<false, false, IntRect> : public CrossThreadCopierPassThrough<IntRect> {
     };