Bug 174002

Summary: [Curl] Safe access and life cycle management of bare Curl handle by wrapping with C++ class
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: WebCore Misc.Assignee: Basuke Suzuki <Basuke.Suzuki>
Severity: Normal CC: achristensen, Basuke.Suzuki, buildbot, commit-queue, galpeter
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 117300, 174003    
Description Flags
implemented none

Description Basuke Suzuki 2017-06-29 18:31:25 PDT
libcurl is a low-end C API library which is powerful, but easy to make a mistake and hard to find the source of issue. Wrapping by C++ class makes the code confident and accelerate the speed of refactoring, especially for upcoming NetworkLoad implementation.
Comment 1 Basuke Suzuki 2017-07-07 13:05:17 PDT
Created attachment 314865 [details]
Comment 2 Basuke Suzuki 2017-07-07 13:07:55 PDT
This is the first step to make libcurl native resources handling into wrapped class. More refactoring is applying by upcoming patches for other bugs.
Comment 3 Alex Christensen 2017-07-08 19:31:04 PDT
Comment on attachment 314865 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=314865&action=review

Let's land this and do more improvements on top of this.  The curl code was a big mess and is getting much better.  Thanks!

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:271
> +    struct curl_slist* list = curlHandle.getCookieList();

auto* list

> Source/WebCore/platform/network/curl/CurlContext.cpp:334
> +        fastFree(m_url);

Let's use a MallocPtr or std::unique_ptr<char[]> for m_url instead of having to do manual memory management here.

> Source/WebCore/platform/network/curl/CurlContext.cpp:665
> +    // The size of a curl_off_t could be different in WebKit and in cURL depending on
> +    // compilation flags of both.

This doesn't seem good.  Maybe we should get rid of this and require consistent compilation flags.

> Source/WebCore/platform/network/curl/CurlContext.cpp:684
> +#ifndef NDEBUG
> +
> +void CurlHandle::enableVerboseIfUsed()
> +{
> +    if (CurlContext::singleton().isVerbose())
> +        curl_easy_setopt(m_handle, CURLOPT_VERBOSE, 1);
> +}

We could have the functions exist and do nothing in release builds to reduce the number of #ifndef checks.

> Source/WebCore/platform/network/curl/ResourceHandleManager.h:29
>  #ifndef ResourceHandleManager_h
>  #define ResourceHandleManager_h

#pragma once.
Comment 4 WebKit Commit Bot 2017-07-08 19:58:39 PDT
Comment on attachment 314865 [details]

Clearing flags on attachment: 314865

Committed r219279: <http://trac.webkit.org/changeset/219279>
Comment 5 WebKit Commit Bot 2017-07-08 19:58:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Basuke Suzuki 2017-07-09 12:11:10 PDT
Thanks Alex. This makes much safer for us to improve our code base.