Keep RefPtr instead of raw pointer to message queue on WebCoreResourceHandleAsOperati...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 16 Jan 2020 02:00:16 +0000 (02:00 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 16 Jan 2020 02:00:16 +0000 (02:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=206261
<rdar://problem/57562592>

Patch by Alex Christensen <achristensen@webkit.org> on 2020-01-15
Reviewed by David Kilzer.

There's no reason to keep a raw pointer when we can keep a smart pointer.
This will make this more robust against someone forgetting to clear this pointer value.

* platform/network/ResourceHandle.h:
* platform/network/SynchronousLoaderClient.cpp:
(WebCore::SynchronousLoaderClient::SynchronousLoaderClient):
(WebCore::SynchronousLoaderClient::didFinishLoading):
(WebCore::SynchronousLoaderClient::didFail):
* platform/network/SynchronousLoaderClient.h:
(WebCore::SynchronousLoaderMessageQueue::create):
(WebCore::SynchronousLoaderMessageQueue::append):
(WebCore::SynchronousLoaderMessageQueue::kill):
(WebCore::SynchronousLoaderMessageQueue::killed const):
(WebCore::SynchronousLoaderMessageQueue::waitForMessage):
* platform/network/mac/ResourceHandleMac.mm:
(WebCore::ResourceHandle::makeDelegate):
* platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.h:
* platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:
(-[WebCoreResourceHandleAsOperationQueueDelegate callFunctionOnMainThread:]):
(-[WebCoreResourceHandleAsOperationQueueDelegate initWithHandle:messageQueue:]):
(-[WebCoreResourceHandleAsOperationQueueDelegate connection:willSendRequest:redirectResponse:]):

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

14 files changed:
Source/WebCore/ChangeLog
Source/WebCore/platform/network/ResourceHandle.h
Source/WebCore/platform/network/ResourceHandleInternal.h
Source/WebCore/platform/network/SynchronousLoaderClient.cpp
Source/WebCore/platform/network/SynchronousLoaderClient.h
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.h
Source/WebCore/platform/network/curl/CurlDownload.cpp
Source/WebCore/platform/network/curl/CurlRequest.cpp
Source/WebCore/platform/network/curl/CurlRequest.h
Source/WebCore/platform/network/mac/ResourceHandleMac.mm
Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.h
Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm

index e6f5431..2a0e586 100644 (file)
@@ -1,3 +1,33 @@
+2020-01-15  Alex Christensen  <achristensen@webkit.org>
+
+        Keep RefPtr instead of raw pointer to message queue on WebCoreResourceHandleAsOperationQueueDelegate
+        https://bugs.webkit.org/show_bug.cgi?id=206261
+        <rdar://problem/57562592>
+
+        Reviewed by David Kilzer.
+
+        There's no reason to keep a raw pointer when we can keep a smart pointer.
+        This will make this more robust against someone forgetting to clear this pointer value.
+
+        * platform/network/ResourceHandle.h:
+        * platform/network/SynchronousLoaderClient.cpp:
+        (WebCore::SynchronousLoaderClient::SynchronousLoaderClient):
+        (WebCore::SynchronousLoaderClient::didFinishLoading):
+        (WebCore::SynchronousLoaderClient::didFail):
+        * platform/network/SynchronousLoaderClient.h:
+        (WebCore::SynchronousLoaderMessageQueue::create):
+        (WebCore::SynchronousLoaderMessageQueue::append):
+        (WebCore::SynchronousLoaderMessageQueue::kill):
+        (WebCore::SynchronousLoaderMessageQueue::killed const):
+        (WebCore::SynchronousLoaderMessageQueue::waitForMessage):
+        * platform/network/mac/ResourceHandleMac.mm:
+        (WebCore::ResourceHandle::makeDelegate):
+        * platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.h:
+        * platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:
+        (-[WebCoreResourceHandleAsOperationQueueDelegate callFunctionOnMainThread:]):
+        (-[WebCoreResourceHandleAsOperationQueueDelegate initWithHandle:messageQueue:]):
+        (-[WebCoreResourceHandleAsOperationQueueDelegate connection:willSendRequest:redirectResponse:]):
+
 2020-01-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         [SVG2]: Implement support for the 'pathLength' attribute
index be81a01..6807404 100644 (file)
@@ -86,6 +86,7 @@ class NetworkLoadMetrics;
 class ResourceRequest;
 class ResourceResponse;
 class SharedBuffer;
+class SynchronousLoaderMessageQueue;
 class Timer;
 
 #if USE(CURL)
@@ -123,7 +124,7 @@ public:
 
 #if PLATFORM(COCOA)
     WEBCORE_EXPORT NSURLConnection *connection() const;
-    id makeDelegate(bool, WTF::MessageQueue<WTF::Function<void()>>*);
+    id makeDelegate(bool, RefPtr<SynchronousLoaderMessageQueue>&&);
     id delegate();
     void releaseDelegate();
 #endif
@@ -232,7 +233,7 @@ private:
 #endif
 
 #if USE(CFURLCONNECTION)
-    void createCFURLConnection(bool shouldUseCredentialStorage, bool shouldContentSniff, bool shouldContentEncodingSniff, WTF::MessageQueue<WTF::Function<void()>>*, CFDictionaryRef clientProperties);
+    void createCFURLConnection(bool shouldUseCredentialStorage, bool shouldContentSniff, bool shouldContentEncodingSniff, RefPtr<SynchronousLoaderMessageQueue>&&, CFDictionaryRef clientProperties);
 #endif
 
 #if PLATFORM(MAC)
index 447e091..728fa2e 100644 (file)
@@ -39,6 +39,7 @@
 
 #if USE(CURL)
 #include "CurlRequest.h"
+#include "SynchronousLoaderClient.h"
 #include <wtf/MessageQueue.h>
 #include <wtf/MonotonicTime.h>
 #endif
@@ -127,7 +128,7 @@ public:
     unsigned m_authFailureCount { 0 };
     bool m_addedCacheValidationHeaders { false };
     RefPtr<CurlRequest> m_curlRequest;
-    MessageQueue<WTF::Function<void()>>* m_messageQueue { };
+    RefPtr<SynchronousLoaderMessageQueue> m_messageQueue;
     MonotonicTime m_startTime;
 #endif
 
index 9ea02c6..c7c12ce 100644 (file)
@@ -33,6 +33,9 @@
 
 namespace WebCore {
 
+SynchronousLoaderClient::SynchronousLoaderClient()
+    : m_messageQueue(SynchronousLoaderMessageQueue::create()) { }
+
 SynchronousLoaderClient::~SynchronousLoaderClient() = default;
 
 void SynchronousLoaderClient::willSendRequestAsync(ResourceHandle* handle, ResourceRequest&& request, ResourceResponse&&, CompletionHandler<void(ResourceRequest&&)>&& completionHandler)
@@ -73,18 +76,30 @@ void SynchronousLoaderClient::didReceiveData(ResourceHandle*, const char* data,
     m_data.append(data, length);
 }
 
-void SynchronousLoaderClient::didFinishLoading(ResourceHandle*)
+void SynchronousLoaderClient::didFinishLoading(ResourceHandle* handle)
 {
-    m_messageQueue.kill();
+    m_messageQueue->kill();
+#if PLATFORM(COCOA)
+    if (handle)
+        handle->releaseDelegate();
+#else
+    UNUSED_PARAM(handle);
+#endif
 }
 
-void SynchronousLoaderClient::didFail(ResourceHandle*, const ResourceError& error)
+void SynchronousLoaderClient::didFail(ResourceHandle* handle, const ResourceError& error)
 {
     ASSERT(m_error.isNull());
 
     m_error = error;
     
-    m_messageQueue.kill();
+    m_messageQueue->kill();
+#if PLATFORM(COCOA)
+    if (handle)
+        handle->releaseDelegate();
+#else
+    UNUSED_PARAM(handle);
+#endif
 }
 
 }
index f5f40d1..8d2a977 100644 (file)
 #include "ResourceResponse.h"
 #include <wtf/Function.h>
 #include <wtf/MessageQueue.h>
+#include <wtf/ThreadSafeRefCounted.h>
 
 namespace WebCore {
 
+class SynchronousLoaderMessageQueue : public ThreadSafeRefCounted<SynchronousLoaderMessageQueue> {
+public:
+    static Ref<SynchronousLoaderMessageQueue> create() { return adoptRef(*new SynchronousLoaderMessageQueue); }
+
+    void append(std::unique_ptr<Function<void()>>&& task) { m_queue.append(WTFMove(task)); }
+    void kill() { m_queue.kill(); }
+    bool killed() const { return m_queue.killed(); }
+    std::unique_ptr<Function<void()>> waitForMessage() { return m_queue.waitForMessage(); }
+
+private:
+    SynchronousLoaderMessageQueue() = default;
+    MessageQueue<Function<void()>> m_queue;
+};
+
 class SynchronousLoaderClient final : public ResourceHandleClient {
 public:
+    SynchronousLoaderClient();
     virtual ~SynchronousLoaderClient();
 
     void setAllowStoredCredentials(bool allow) { m_allowStoredCredentials = allow; }
     const ResourceResponse& response() const { return m_response; }
     Vector<char>& mutableData() { return m_data; }
     const ResourceError& error() const { return m_error; }
-    MessageQueue<Function<void()>>& messageQueue() { return m_messageQueue; }
+    SynchronousLoaderMessageQueue& messageQueue() { return m_messageQueue.get(); }
 
     WEBCORE_EXPORT static ResourceError platformBadResponseError();
 
@@ -61,6 +77,6 @@ private:
     ResourceResponse m_response;
     Vector<char> m_data;
     ResourceError m_error;
-    MessageQueue<Function<void()>> m_messageQueue;
+    Ref<SynchronousLoaderMessageQueue> m_messageQueue;
 };
 }
index 8d14aeb..322a845 100644 (file)
@@ -131,7 +131,7 @@ static void setClientCertificateInSSLProperties(CFMutableDictionaryRef sslProps,
 }
 #endif
 
-void ResourceHandle::createCFURLConnection(bool shouldUseCredentialStorage, bool shouldContentSniff, bool shouldContentEncodingSniff, MessageQueue<Function<void()>>* messageQueue, CFDictionaryRef clientProperties)
+void ResourceHandle::createCFURLConnection(bool shouldUseCredentialStorage, bool shouldContentSniff, bool shouldContentEncodingSniff, RefPtr<SynchronousLoaderMessageQueue>&& messageQueue, CFDictionaryRef clientProperties)
 {
     if ((!d->m_user.isEmpty() || !d->m_pass.isEmpty()) && !firstRequest().url().protocolIsInHTTPFamily()) {
         // Credentials for ftp can only be passed in URL, the didReceiveAuthenticationChallenge delegate call won't be made.
@@ -239,7 +239,7 @@ void ResourceHandle::createCFURLConnection(bool shouldUseCredentialStorage, bool
     CFDictionaryAddValue(propertiesDictionary.get(), kCFURLConnectionSocketStreamProperties, streamProperties);
     CFRelease(streamProperties);
 
-    d->m_connectionDelegate = adoptRef(new ResourceHandleCFURLConnectionDelegateWithOperationQueue(this, messageQueue));
+    d->m_connectionDelegate = adoptRef(new ResourceHandleCFURLConnectionDelegateWithOperationQueue(this, WTFMove(messageQueue)));
     d->m_connectionDelegate->setupRequest(request.get());
 
     CFURLConnectionClient_V6 client = d->m_connectionDelegate->makeConnectionClient();
index 713b4df..9d1e9c4 100644 (file)
@@ -36,6 +36,7 @@
 #include "ResourceHandleClient.h"
 #include "ResourceResponse.h"
 #include "SharedBuffer.h"
+#include "SynchronousLoaderClient.h"
 #if !PLATFORM(WIN)
 #include "WebCoreURLResponse.h"
 #endif
@@ -48,9 +49,9 @@
 
 namespace WebCore {
 
-ResourceHandleCFURLConnectionDelegateWithOperationQueue::ResourceHandleCFURLConnectionDelegateWithOperationQueue(ResourceHandle* handle, MessageQueue<Function<void()>>* messageQueue)
+ResourceHandleCFURLConnectionDelegateWithOperationQueue::ResourceHandleCFURLConnectionDelegateWithOperationQueue(ResourceHandle* handle, RefPtr<SynchronousLoaderMessageQueue>&& messageQueue)
     : ResourceHandleCFURLConnectionDelegate(handle)
-    , m_messageQueue(messageQueue)
+    , m_messageQueue(WTFMove(messageQueue))
 {
 }
 
index 1136896..dfd746f 100644 (file)
 
 namespace WebCore {
 
+class SynchronousLoaderMessageQueue;
+
 class ResourceHandleCFURLConnectionDelegateWithOperationQueue final : public ResourceHandleCFURLConnectionDelegate {
 public:
-    ResourceHandleCFURLConnectionDelegateWithOperationQueue(ResourceHandle*, MessageQueue<Function<void()>>*);
+    ResourceHandleCFURLConnectionDelegateWithOperationQueue(ResourceHandle*, RefPtr<SynchronousLoaderMessageQueue>&&);
     virtual ~ResourceHandleCFURLConnectionDelegateWithOperationQueue();
 
 private:
@@ -63,7 +65,7 @@ private:
 #endif
 
     BinarySemaphore m_semaphore;
-    MessageQueue<Function<void()>>* m_messageQueue { nullptr };
+    RefPtr<SynchronousLoaderMessageQueue> m_messageQueue;
 
     RetainPtr<CFURLRequestRef> m_requestResult;
     RetainPtr<CFCachedURLResponseRef> m_cachedResponseResult;
index c1b912e..c36e1f8 100644 (file)
@@ -34,6 +34,7 @@
 #include "ResourceRequest.h"
 #include "ResourceResponse.h"
 #include "SharedBuffer.h"
+#include "SynchronousLoaderClient.h"
 
 namespace WebCore {
 
index c1d157b..39c3ea0 100644 (file)
 #include "NetworkLoadMetrics.h"
 #include "ResourceError.h"
 #include "SharedBuffer.h"
+#include "SynchronousLoaderClient.h"
 #include <wtf/CrossThreadCopier.h>
 #include <wtf/Language.h>
 #include <wtf/MainThread.h>
 
 namespace WebCore {
 
-CurlRequest::CurlRequest(const ResourceRequest&request, CurlRequestClient* client, ShouldSuspend shouldSuspend, EnableMultipart enableMultipart, CaptureNetworkLoadMetrics captureExtraMetrics, MessageQueue<Function<void()>>* messageQueue)
+CurlRequest::CurlRequest(const ResourceRequest&request, CurlRequestClient* client, ShouldSuspend shouldSuspend, EnableMultipart enableMultipart, CaptureNetworkLoadMetrics captureExtraMetrics, RefPtr<SynchronousLoaderMessageQueue>&& messageQueue)
     : m_client(client)
-    , m_messageQueue(messageQueue)
+    , m_messageQueue(WTFMove(messageQueue))
     , m_request(request.isolatedCopy())
     , m_shouldSuspend(shouldSuspend == ShouldSuspend::Yes)
     , m_enableMultipart(enableMultipart == EnableMultipart::Yes)
index b08ecc1..c8c6690 100644 (file)
@@ -43,6 +43,7 @@ class CurlRequestClient;
 class NetworkLoadMetrics;
 class ResourceError;
 class SharedBuffer;
+class SynchronousLoaderMessageQueue;
 
 class CurlRequest : public ThreadSafeRefCounted<CurlRequest>, public CurlRequestSchedulerClient, public CurlMultipartHandleClient {
     WTF_MAKE_NONCOPYABLE(CurlRequest);
@@ -63,9 +64,9 @@ public:
         Extended
     };
 
-    static Ref<CurlRequest> create(const ResourceRequest& request, CurlRequestClient& client, ShouldSuspend shouldSuspend = ShouldSuspend::No, EnableMultipart enableMultipart = EnableMultipart::No, CaptureNetworkLoadMetrics captureMetrics = CaptureNetworkLoadMetrics::Basic, MessageQueue<Function<void()>>* messageQueue = nullptr)
+    static Ref<CurlRequest> create(const ResourceRequest& request, CurlRequestClient& client, ShouldSuspend shouldSuspend = ShouldSuspend::No, EnableMultipart enableMultipart = EnableMultipart::No, CaptureNetworkLoadMetrics captureMetrics = CaptureNetworkLoadMetrics::Basic, RefPtr<SynchronousLoaderMessageQueue>&& messageQueue = nullptr)
     {
-        return adoptRef(*new CurlRequest(request, &client, shouldSuspend, enableMultipart, captureMetrics, messageQueue));
+        return adoptRef(*new CurlRequest(request, &client, shouldSuspend, enableMultipart, captureMetrics, WTFMove(messageQueue)));
     }
 
     virtual ~CurlRequest() = default;
@@ -105,7 +106,7 @@ private:
         FinishTransfer
     };
 
-    CurlRequest(const ResourceRequest&, CurlRequestClient*, ShouldSuspend, EnableMultipart, CaptureNetworkLoadMetrics, MessageQueue<Function<void()>>*);
+    CurlRequest(const ResourceRequest&, CurlRequestClient*, ShouldSuspend, EnableMultipart, CaptureNetworkLoadMetrics, RefPtr<SynchronousLoaderMessageQueue>&&);
 
     void retain() override { ref(); }
     void release() override { deref(); }
@@ -168,7 +169,7 @@ private:
     Lock m_statusMutex;
     bool m_cancelled { false };
     bool m_completed { false };
-    MessageQueue<Function<void()>>* m_messageQueue { };
+    RefPtr<SynchronousLoaderMessageQueue> m_messageQueue;
 
     ResourceRequest m_request;
     String m_user;
index 5bff074..61a67ef 100644 (file)
@@ -309,15 +309,15 @@ void ResourceHandle::unschedule(SchedulePair& pair)
         [d->m_connection.get() unscheduleFromRunLoop:runLoop forMode:(__bridge NSString *)pair.mode()];
 }
 
-id ResourceHandle::makeDelegate(bool shouldUseCredentialStorage, MessageQueue<Function<void()>>* queue)
+id ResourceHandle::makeDelegate(bool shouldUseCredentialStorage, RefPtr<SynchronousLoaderMessageQueue>&& queue)
 {
     ASSERT(!d->m_delegate);
 
     id <NSURLConnectionDelegate> delegate;
     if (shouldUseCredentialStorage)
-        delegate = [[WebCoreResourceHandleAsOperationQueueDelegate alloc] initWithHandle:this messageQueue:queue];
+        delegate = [[WebCoreResourceHandleAsOperationQueueDelegate alloc] initWithHandle:this messageQueue:WTFMove(queue)];
     else
-        delegate = [[WebCoreResourceHandleWithCredentialStorageAsOperationQueueDelegate alloc] initWithHandle:this messageQueue:queue];
+        delegate = [[WebCoreResourceHandleWithCredentialStorageAsOperationQueueDelegate alloc] initWithHandle:this messageQueue:WTFMove(queue)];
 
     d->m_delegate = delegate;
     [delegate release];
index 249ee43..906da88 100644 (file)
 #include <wtf/Function.h>
 #include <wtf/Lock.h>
 #include <wtf/MessageQueue.h>
+#include <wtf/RefPtr.h>
 #include <wtf/RetainPtr.h>
 #include <wtf/SchedulePair.h>
 #include <wtf/threads/BinarySemaphore.h>
 
 namespace WebCore {
 class ResourceHandle;
+class SynchronousLoaderMessageQueue;
 }
 
 @interface WebCoreResourceHandleAsOperationQueueDelegate : NSObject <NSURLConnectionDelegate> {
@@ -42,7 +44,7 @@ class ResourceHandle;
 
     // Synchronous delegates on operation queue wait until main thread sends an asynchronous response.
     BinarySemaphore m_semaphore;
-    MessageQueue<Function<void()>>* m_messageQueue;
+    RefPtr<WebCore::SynchronousLoaderMessageQueue> m_messageQueue;
     RetainPtr<NSURLRequest> m_requestResult;
     Lock m_mutex;
     RetainPtr<NSCachedURLResponse> m_cachedResponseResult;
@@ -51,7 +53,7 @@ class ResourceHandle;
 }
 
 - (void)detachHandle;
-- (id)initWithHandle:(WebCore::ResourceHandle*)handle messageQueue:(MessageQueue<Function<void()>>*)messageQueue;
+- (id)initWithHandle:(WebCore::ResourceHandle*)handle messageQueue:(RefPtr<WebCore::SynchronousLoaderMessageQueue>&&)messageQueue;
 @end
 
 @interface WebCoreResourceHandleWithCredentialStorageAsOperationQueueDelegate : WebCoreResourceHandleAsOperationQueueDelegate
index f10eb62..830ed06 100644 (file)
@@ -79,7 +79,7 @@ static bool scheduledWithCustomRunLoopMode(const Optional<SchedulePairHashSet>&
         CFRunLoopPerformBlock(pair->runLoop(), pair->mode(), block.get());
 }
 
-- (id)initWithHandle:(ResourceHandle*)handle messageQueue:(MessageQueue<Function<void()>>*)messageQueue
+- (id)initWithHandle:(WebCore::ResourceHandle*)handle messageQueue:(RefPtr<WebCore::SynchronousLoaderMessageQueue>&&)messageQueue
 {
     self = [self init];
     if (!self)
@@ -90,7 +90,7 @@ static bool scheduledWithCustomRunLoopMode(const Optional<SchedulePairHashSet>&
         if (auto* pairs = m_handle->context()->scheduledRunLoopPairs())
             m_scheduledPairs = *pairs;
     }
-    m_messageQueue = messageQueue;
+    m_messageQueue = WTFMove(messageQueue);
 
     return self;
 }