[webkit-reviews] review granted: [Bug 20777] [CURL] memory leak of ResouceHandles : [Attachment 26023] Updated patch with refs() and manager-side refcount handling

[bugzilla-daemon at webkit.org]

Holger Freyther <zecke at selfish.org> has granted Kalle Vahlman <zuh at iki.fi>'s
request for review:
Bug 20777: [CURL] memory leak of ResouceHandles
https://bugs.webkit.org/show_bug.cgi?id=20777

Attachment 26023: Updated patch with refs() and manager-side refcount handling 
https://bugs.webkit.org/attachment.cgi?id=26023&action=review

------- Additional Comments from Holger Freyther <zecke at selfish.org>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> +	   WARNING: NO TEST CASES ADDED OR CHANGED

Just remove this is if you think it is not testable. I would agree that testing
this is incredible hard with a LayoutTest and from within the API... I will run
run-webkit-tests and see if we crash...



> index bbc31d4..ca24ec5 100644
> --- a/WebCore/platform/network/curl/ResourceHandleCurl.cpp
> +++ b/WebCore/platform/network/curl/ResourceHandleCurl.cpp
> @@ -104,7 +104,6 @@ ResourceHandle::~ResourceHandle()
>  bool ResourceHandle::start(Frame* frame)
>  {
>      ASSERT(frame);
> -    ref();
>      ResourceHandleManager::sharedInstance()->add(this);
>      return true;
>  }

yes!



> diff --git a/WebCore/platform/network/curl/ResourceHandleManager.cpp
b/WebCore/platform/network/curl/ResourceHandleManager.cpp
> index 410f9eb..f1a2be4 100644
> --- a/WebCore/platform/network/curl/ResourceHandleManager.cpp
> +++ b/WebCore/platform/network/curl/ResourceHandleManager.cpp
> @@ -350,6 +350,7 @@ void
ResourceHandleManager::removeFromCurl(ResourceHandle* job)
>      curl_multi_remove_handle(m_curlMultiHandle, d->m_handle);
>      curl_easy_cleanup(d->m_handle);
>      d->m_handle = 0;
> +    job->deref();
>  }

deref when the job was started and finished by curl? looks right.



>  
>  void ResourceHandleManager::setupPUT(ResourceHandle*, struct curl_slist**)
> @@ -443,6 +444,7 @@ void ResourceHandleManager::add(ResourceHandle* job)
>  {
>      // we can be called from within curl, so to avoid re-entrancy issues
>      // schedule this job to be added the next time we enter curl download
loop
> +    job->ref();

yupp, ref it when we start to handle it..




>	   if (job == m_resourceHandleList[i]) {
>	       m_resourceHandleList.remove(i);
> +	       job->deref();
>	       return true;

Cool. It looks right. When we add something to m_resourceHandleList as ref the
job, when we cancel/remove a not started job we deref, once curl finished
(which means the job was started and removed from the m_resourceHandleList) we
will deref it.