Crashes in ResourceHandleCFURLConnectionDelegateWithOperationQueue due to unimplement...
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 16 Oct 2014 19:45:31 +0000 (19:45 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 16 Oct 2014 19:45:31 +0000 (19:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=137779
rdar://problem/18679320

Reviewed by Brady Eidson.

* platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp:
(WebCore::ResourceHandleCFURLConnectionDelegate::retain):
(WebCore::ResourceHandleCFURLConnectionDelegate::release):
(WebCore::ResourceHandleCFURLConnectionDelegate::makeConnectionClient):
* platform/network/cf/ResourceHandleCFURLConnectionDelegate.h:
Implemented retain/release. They are necessary, as ResourceHandle goes away when
it's canceled, and there is noone else to keep the client object alive but
CFURLConnection itself.

* platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:
(WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::willSendRequest):
(WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveResponse):
(WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveData):
(WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didFinishLoading):
(WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didFail):
(WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::willCacheResponse):
(WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveChallenge):
(WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didSendBodyData):
(WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::canRespondToProtectionSpace):
(WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveDataArray):
Added a FIXME about potential improvements that I spotted while invsestigating this.

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

Source/WebCore/ChangeLog
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp

index 09dd1e8..05eec7a 100644 (file)
@@ -1,3 +1,33 @@
+2014-10-16  Alexey Proskuryakov  <ap@apple.com>
+
+        Crashes in ResourceHandleCFURLConnectionDelegateWithOperationQueue due to unimplemented retain/release
+        https://bugs.webkit.org/show_bug.cgi?id=137779
+        rdar://problem/18679320
+
+        Reviewed by Brady Eidson.
+
+        * platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp:
+        (WebCore::ResourceHandleCFURLConnectionDelegate::retain):
+        (WebCore::ResourceHandleCFURLConnectionDelegate::release):
+        (WebCore::ResourceHandleCFURLConnectionDelegate::makeConnectionClient):
+        * platform/network/cf/ResourceHandleCFURLConnectionDelegate.h:
+        Implemented retain/release. They are necessary, as ResourceHandle goes away when
+        it's canceled, and there is noone else to keep the client object alive but
+        CFURLConnection itself.
+
+        * platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:
+        (WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::willSendRequest):
+        (WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveResponse):
+        (WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveData):
+        (WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didFinishLoading):
+        (WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didFail):
+        (WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::willCacheResponse):
+        (WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveChallenge):
+        (WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didSendBodyData):
+        (WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::canRespondToProtectionSpace):
+        (WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveDataArray):
+        Added a FIXME about potential improvements that I spotted while invsestigating this.
+
 2014-10-15  Andrei Bucur  <abucur@adobe.com>
 
         ASSERTION  FAILED in WebCore::RenderFlowThread::getRegionRangeForBox
index 71448cf..4f7a792 100644 (file)
@@ -56,6 +56,17 @@ void ResourceHandleCFURLConnectionDelegate::releaseHandle()
     m_handle = nullptr;
 }
 
+const void* ResourceHandleCFURLConnectionDelegate::retain(const void* clientInfo)
+{
+    static_cast<ResourceHandleCFURLConnectionDelegate*>(const_cast<void*>(clientInfo))->ref();
+    return clientInfo;
+}
+
+void ResourceHandleCFURLConnectionDelegate::release(const void* clientInfo)
+{
+    static_cast<ResourceHandleCFURLConnectionDelegate*>(const_cast<void*>(clientInfo))->deref();
+}
+
 CFURLRequestRef ResourceHandleCFURLConnectionDelegate::willSendRequestCallback(CFURLConnectionRef, CFURLRequestRef cfRequest, CFURLResponseRef originalRedirectResponse, const void* clientInfo)
 {
     return static_cast<ResourceHandleCFURLConnectionDelegate*>(const_cast<void*>(clientInfo))->willSendRequest(cfRequest, originalRedirectResponse);
@@ -173,7 +184,10 @@ ResourceRequest ResourceHandleCFURLConnectionDelegate::createResourceRequest(CFU
 
 CFURLConnectionClient_V6 ResourceHandleCFURLConnectionDelegate::makeConnectionClient() const
 {
-    CFURLConnectionClient_V6 client = { 6, this, 0, 0, 0,
+    CFURLConnectionClient_V6 client = { 6, this,
+        &ResourceHandleCFURLConnectionDelegate::retain,
+        &ResourceHandleCFURLConnectionDelegate::release,
+        0, // copyDescription
         &ResourceHandleCFURLConnectionDelegate::willSendRequestCallback,
         &ResourceHandleCFURLConnectionDelegate::didReceiveResponseCallback,
         &ResourceHandleCFURLConnectionDelegate::didReceiveDataCallback,
index adfa9d2..d4965a9 100644 (file)
@@ -59,6 +59,8 @@ protected:
     ResourceRequest createResourceRequest(CFURLRequestRef, CFURLResponseRef);
 
 private:
+    static const void* retain(const void*);
+    static void release(const void*);
     static CFURLRequestRef willSendRequestCallback(CFURLConnectionRef, CFURLRequestRef, CFURLResponseRef, const void* clientInfo);
     static void didReceiveResponseCallback(CFURLConnectionRef, CFURLResponseRef, const void* clientInfo);
     static void didReceiveDataCallback(CFURLConnectionRef, CFDataRef, CFIndex originalLength, const void* clientInfo);
index 55c3df2..d062a54 100644 (file)
@@ -101,6 +101,8 @@ CFURLRequestRef ResourceHandleCFURLConnectionDelegateWithOperationQueue::willSen
 
     ASSERT(!isMainThread());
 
+    // FIXME: The block implicitly copies protector object, which is wasteful. We should just call ref(),
+    // capture "this" by pointer value, and use a C++ lambda to prevent other unintentional capturing.
     RefPtr<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protector(this);
 
     dispatch_async(dispatch_get_main_queue(), ^{
@@ -125,6 +127,8 @@ CFURLRequestRef ResourceHandleCFURLConnectionDelegateWithOperationQueue::willSen
 
 void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveResponse(CFURLConnectionRef connection, CFURLResponseRef cfResponse)
 {
+    // FIXME: The block implicitly copies protector object, which is wasteful. We should just call ref(),
+    // capture "this" by pointer value, and use a C++ lambda to prevent other unintentional capturing.
     RefPtr<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protector(this);
 
     dispatch_async(dispatch_get_main_queue(), ^{
@@ -161,6 +165,8 @@ void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveResponse
 
 void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveData(CFDataRef data, CFIndex originalLength)
 {
+    // FIXME: The block implicitly copies protector object, which is wasteful. We should just call ref(),
+    // capture "this" by pointer value, and use a C++ lambda to prevent other unintentional capturing.
     RefPtr<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protector(this);
     CFRetain(data);
 
@@ -177,6 +183,8 @@ void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveData(CFD
 
 void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didFinishLoading()
 {
+    // FIXME: The block implicitly copies protector object, which is wasteful. We should just call ref(),
+    // capture "this" by pointer value, and use a C++ lambda to prevent other unintentional capturing.
     RefPtr<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protector(this);
     dispatch_async(dispatch_get_main_queue(), ^{
         if (!protector->hasHandle())
@@ -190,6 +198,8 @@ void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didFinishLoading()
 
 void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didFail(CFErrorRef error)
 {
+    // FIXME: The block implicitly copies protector object, which is wasteful. We should just call ref(),
+    // capture "this" by pointer value, and use a C++ lambda to prevent other unintentional capturing.
     RefPtr<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protector(this);
     CFRetain(error);
     dispatch_async(dispatch_get_main_queue(), ^{
@@ -205,6 +215,8 @@ void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didFail(CFErrorRef
 
 CFCachedURLResponseRef ResourceHandleCFURLConnectionDelegateWithOperationQueue::willCacheResponse(CFCachedURLResponseRef cachedResponse)
 {
+    // FIXME: The block implicitly copies protector object, which is wasteful. We should just call ref(),
+    // capture "this" by pointer value, and use a C++ lambda to prevent other unintentional capturing.
     RefPtr<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protector(this);
 
     dispatch_async(dispatch_get_main_queue(), ^{
@@ -223,6 +235,8 @@ CFCachedURLResponseRef ResourceHandleCFURLConnectionDelegateWithOperationQueue::
 
 void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveChallenge(CFURLAuthChallengeRef challenge)
 {
+    // FIXME: The block implicitly copies protector object, which is wasteful. We should just call ref(),
+    // capture "this" by pointer value, and use a C++ lambda to prevent other unintentional capturing.
     RefPtr<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protector(this);
     CFRetain(challenge);
     dispatch_async(dispatch_get_main_queue(), ^{
@@ -238,6 +252,8 @@ void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveChalleng
 
 void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didSendBodyData(CFIndex totalBytesWritten, CFIndex totalBytesExpectedToWrite)
 {
+    // FIXME: The block implicitly copies protector object, which is wasteful. We should just call ref(),
+    // capture "this" by pointer value, and use a C++ lambda to prevent other unintentional capturing.
     RefPtr<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protector(this);
     dispatch_async(dispatch_get_main_queue(), ^{
         if (!protector->hasHandle())
@@ -257,6 +273,8 @@ Boolean ResourceHandleCFURLConnectionDelegateWithOperationQueue::shouldUseCreden
 #if USE(PROTECTION_SPACE_AUTH_CALLBACK)
 Boolean ResourceHandleCFURLConnectionDelegateWithOperationQueue::canRespondToProtectionSpace(CFURLProtectionSpaceRef protectionSpace)
 {
+    // FIXME: The block implicitly copies protector object, which is wasteful. We should just call ref(),
+    // capture "this" by pointer value, and use a C++ lambda to prevent other unintentional capturing.
     RefPtr<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protector(this);
 
     dispatch_async(dispatch_get_main_queue(), ^{
@@ -285,6 +303,8 @@ Boolean ResourceHandleCFURLConnectionDelegateWithOperationQueue::canRespondToPro
 #if USE(NETWORK_CFDATA_ARRAY_CALLBACK)
 void ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveDataArray(CFArrayRef dataArray)
 {
+    // FIXME: The block implicitly copies protector object, which is wasteful. We should just call ref(),
+    // capture "this" by pointer value, and use a C++ lambda to prevent other unintentional capturing.
     RefPtr<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protector(this);
     CFRetain(dataArray);
     dispatch_async(dispatch_get_main_queue(), ^{