Title: [174787] trunk/Source/WebCore
Revision
174787
Author
[email protected]
Date
2014-10-16 12:45:31 -0700 (Thu, 16 Oct 2014)

Log Message

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.

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (174786 => 174787)


--- trunk/Source/WebCore/ChangeLog	2014-10-16 19:14:42 UTC (rev 174786)
+++ trunk/Source/WebCore/ChangeLog	2014-10-16 19:45:31 UTC (rev 174787)
@@ -1,3 +1,33 @@
+2014-10-16  Alexey Proskuryakov  <[email protected]>
+
+        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  <[email protected]>
 
         ASSERTION  FAILED in WebCore::RenderFlowThread::getRegionRangeForBox

Modified: trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp (174786 => 174787)


--- trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp	2014-10-16 19:14:42 UTC (rev 174786)
+++ trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp	2014-10-16 19:45:31 UTC (rev 174787)
@@ -56,6 +56,17 @@
     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 @@
 
 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,

Modified: trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h (174786 => 174787)


--- trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h	2014-10-16 19:14:42 UTC (rev 174786)
+++ trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h	2014-10-16 19:45:31 UTC (rev 174787)
@@ -59,6 +59,8 @@
     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);

Modified: trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp (174786 => 174787)


--- trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp	2014-10-16 19:14:42 UTC (rev 174786)
+++ trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp	2014-10-16 19:45:31 UTC (rev 174787)
@@ -101,6 +101,8 @@
 
     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 @@
 
 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::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::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::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 @@
 
 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 @@
 
 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::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 @@
 #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 @@
 #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(), ^{
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to