Bug 174942 – [Curl] Use SQLite database in cookie jar implementation for Curl port

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Basuke Suzuki]

Current file based implementation is far from ideal. The goal is to replace its backend from file to database.

[Christopher Reid]

Created attachment 331552 [details]
Patch

[Alex Christensen]

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>

[Christopher Reid]

(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.

[Christopher Reid]

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.

[EWS Watchlist]

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.

[Christopher Reid]

Created attachment 332814 [details]
Patch

Fixing ChangeLog style issues

[Alex Christensen]

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.

[Christopher Reid]

(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.

[Christopher Reid]

(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?

[Christopher Reid]

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.

[Christopher Reid]

Created attachment 332866 [details]
Patch

Had the wrong line endings in the last patch.

[WebKit Commit Bot]

Comment on attachment 332866 [details]
Patch

Clearing flags on attachment: 332866

Committed r227987: <https://trac.webkit.org/changeset/227987>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37135651>

[Basuke Suzuki]

*** Bug 14730 has been marked as a duplicate of this bug. ***