Bug 174002 - [Curl] Safe access and life cycle management of bare Curl handle by wrapping with C++ class
Summary: [Curl] Safe access and life cycle management of bare Curl handle by wrapping ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 117300 174003
  Show dependency treegraph
 
Reported: 2017-06-29 18:31 PDT by Basuke Suzuki
Modified: 2017-07-09 12:11 PDT (History)
5 users (show)

See Also:


Attachments
implemented (73.92 KB, patch)
2017-07-07 13:05 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
implemented
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]
implemented

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]
implemented

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.