Bug 174942 - [Curl] Use SQLite database in cookie jar implementation for Curl port
Summary: [Curl] Use SQLite database in cookie jar implementation for Curl port
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: Nobody
URL:
Keywords: InRadar
: 14730 (view as bug list)
Depends on: 175084 174943
Blocks: 117300
  Show dependency treegraph
 
Reported: 2017-07-28 10:28 PDT by Basuke Suzuki
Modified: 2018-11-14 11:00 PST (History)
11 users (show)

See Also:


Attachments
Patch (52.78 KB, patch)
2018-01-17 16:04 PST, Christopher Reid
achristensen: review-
Details | Formatted Diff | Diff
Patch (59.11 KB, patch)
2018-01-31 15:38 PST, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch (59.10 KB, patch)
2018-01-31 15:45 PST, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch (69.89 KB, patch)
2018-02-01 02:49 PST, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch (69.89 KB, patch)
2018-02-01 03:10 PST, Christopher Reid
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-07-28 10:28:19 PDT
Current file based implementation is far from ideal. The goal is to replace its backend from file to database.
Comment 1 Christopher Reid 2018-01-17 16:04:31 PST
Created attachment 331552 [details]
Patch
Comment 2 Alex Christensen 2018-01-17 17:41:12 PST
Comment on attachment 331552 [details]
Patch

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

I feel like this patch has a lot of parsers.  Are there not existing parsers for these things?

> Source/WebCore/ChangeLog:3
> +        [Curl] Use SQLite database in cookie jar implementation for Curl port

Cool!

> Source/WebCore/platform/network/curl/CookieJarCurlDatabase.cpp:89
> +    CookieJarDB* cookieJarDB = CookieJarDB::getInstance();

This should be implemented by NetworkStorageSession instead of having a global singleton.

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:76
> +CookieJarDB* CookieJarDB::getInstance()

This could return a reference.

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:80
> +        CookieJarDB::s_instance = new CookieJarDB();

This should use smart pointers or NeverDestroyed to avoid explicit new/delete calls.

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:84
> +void CookieJarDB::releaseInstance()

This is never called.

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:191
> +void CookieJarDB::detectDatabaseCorruption()

Does this really detect anything?

> Source/WebCore/platform/network/curl/CookieUtil.cpp:82
> +static bool parseExpires(const char* expires, time_t& time)

This could return std::optional<time_t>
Comment 3 Christopher Reid 2018-01-18 13:00:50 PST
(In reply to Alex Christensen from comment #2)
> Comment on attachment 331552 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331552&action=review
> 
> I feel like this patch has a lot of parsers.  Are there not existing parsers
> for these things?

I wasn't able to find any parsers for cookie strings within WebCore for other network layers. They all seem to be using a library for parsing.
CookieJarSoup uses soup_cookie_parse
CookieJarCF uses CFHTTPCookieCreateWithResponseHeaderFields
CookieJarMac uses NSHTTPCookie's cookiesWithResponseHeaderFields

getNetscapeCookieFormat in CookieJarCurl does some similar parsing but it can't be used directly since it's just returning a string in a different format.
I can pull out that parsing code and share it with the cookie jar if that's preferred.

> > Source/WebCore/ChangeLog:3
> > +        [Curl] Use SQLite database in cookie jar implementation for Curl port
> 
> Cool!
> 
> > Source/WebCore/platform/network/curl/CookieJarCurlDatabase.cpp:89
> > +    CookieJarDB* cookieJarDB = CookieJarDB::getInstance();
> 
> This should be implemented by NetworkStorageSession instead of having a
> global singleton.
> 

I'll look into using that.

> > Source/WebCore/platform/network/curl/CookieJarDB.cpp:76
> > +CookieJarDB* CookieJarDB::getInstance()
> 
> This could return a reference.
> 

Will do.

> > Source/WebCore/platform/network/curl/CookieJarDB.cpp:80
> > +        CookieJarDB::s_instance = new CookieJarDB();
> 
> This should use smart pointers or NeverDestroyed to avoid explicit
> new/delete calls.
> 

Will do.

> > Source/WebCore/platform/network/curl/CookieJarDB.cpp:84
> > +void CookieJarDB::releaseInstance()
> 
> This is never called.
> 

I'll take that out.

> > Source/WebCore/platform/network/curl/CookieJarDB.cpp:191
> > +void CookieJarDB::detectDatabaseCorruption()
> 
> Does this really detect anything?
> 

flagDatabaseCorruption seems like a more suitable name.

> > Source/WebCore/platform/network/curl/CookieUtil.cpp:82
> > +static bool parseExpires(const char* expires, time_t& time)
> 
> This could return std::optional<time_t>

Will do.


Thanks for the feedback.
Comment 4 Christopher Reid 2018-01-31 15:38:20 PST
Created attachment 332812 [details]
Patch

Updated patch based on Alex's feedback.

I also added a flag to use the old backend with ENABLE(CURL_COOKIE_STORAGE).
I think we'd want to solely use the CookieJarDB backend in curl and remove handling cookies with libcurl, but I'm not sure if the libcurl cookie backend should be removed yet.
Comment 5 EWS Watchlist 2018-01-31 15:40:39 PST
Attachment 332812 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Christopher Reid 2018-01-31 15:45:38 PST
Created attachment 332814 [details]
Patch

Fixing ChangeLog style issues
Comment 7 Alex Christensen 2018-01-31 16:04:53 PST
Comment on attachment 332814 [details]
Patch

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

> Source/WebCore/platform/network/NetworkStorageSession.h:165
> +    std::unique_ptr<CookieJarCurl> m_cookieStorage;

These could be UniqueRef<CookieJarCurl> or just CookieJarCurl.

> Source/WebCore/platform/network/NetworkStorageSession.h:166
> +#if !ENABLE(CURL_COOKIE_STORAGE)

Is there a reason we want to leave the code from both cookie storage formats?  Isn't the database format better?

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:260
> +    std::unique_ptr<SQLiteStatement> pstmt;
> +    pstmt = std::make_unique<SQLiteStatement>(m_database, sql);

You can assign to it on the same line.
Comment 8 Christopher Reid 2018-01-31 16:42:31 PST
(In reply to Alex Christensen from comment #7)
> Comment on attachment 332814 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332814&action=review
> 
> > Source/WebCore/platform/network/NetworkStorageSession.h:165
> > +    std::unique_ptr<CookieJarCurl> m_cookieStorage;
> 
> These could be UniqueRef<CookieJarCurl> or just CookieJarCurl.

Will do.

> > Source/WebCore/platform/network/NetworkStorageSession.h:166
> > +#if !ENABLE(CURL_COOKIE_STORAGE)
> 
> Is there a reason we want to leave the code from both cookie storage
> formats?  Isn't the database format better?

We don't need the old format in, I'll take it out. I was just worried that other projects using curl might not want to use the new format just yet.

> > Source/WebCore/platform/network/curl/CookieJarDB.cpp:260
> > +    std::unique_ptr<SQLiteStatement> pstmt;
> > +    pstmt = std::make_unique<SQLiteStatement>(m_database, sql);
> 
> You can assign to it on the same line.

Will do.
Comment 9 Christopher Reid 2018-01-31 18:04:01 PST
(In reply to Christopher Reid from comment #8)
> (In reply to Alex Christensen from comment #7)
> > Comment on attachment 332814 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=332814&action=review
> > 
> > > Source/WebCore/platform/network/NetworkStorageSession.h:165
> > > +    std::unique_ptr<CookieJarCurl> m_cookieStorage;
> > 
> > These could be UniqueRef<CookieJarCurl> or just CookieJarCurl.
> 
> Will do.

Actually, I'm not sure if I can do this. PlatformCookieJar.h dispatcher functions use a const NetworkStorageSession& and I would have to make all the calls to CookieJarDB const which doesn't seem ideal. I thought UniqueRef might allow modifying the underlying data in a const call but that doesn't seem to be the case. Is there any way around that or should I just stick with unique_ptr?
Comment 10 Christopher Reid 2018-02-01 02:49:29 PST
Created attachment 332865 [details]
Patch

I removed all the old curl cookie code and ended up using mutable to avoid using unique_ptr in NetworkStorageSession.
Comment 11 Christopher Reid 2018-02-01 03:10:00 PST
Created attachment 332866 [details]
Patch

Had the wrong line endings in the last patch.
Comment 12 WebKit Commit Bot 2018-02-01 16:08:46 PST
Comment on attachment 332866 [details]
Patch

Clearing flags on attachment: 332866

Committed r227987: <https://trac.webkit.org/changeset/227987>
Comment 13 WebKit Commit Bot 2018-02-01 16:08:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-02-01 16:11:26 PST
<rdar://problem/37135651>
Comment 15 Basuke Suzuki 2018-11-14 11:00:36 PST
*** Bug 14730 has been marked as a duplicate of this bug. ***