Bug 207450 - [Curl] Implement NetworkStorageSession::get/set/deleteCookie
Summary: [Curl] Implement NetworkStorageSession::get/set/deleteCookie
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Windows 10
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-09 20:47 PST by Pavel Feldman
Modified: 2020-02-14 15:18 PST (History)
9 users (show)

See Also:


Attachments
Patch (5.29 KB, patch)
2020-02-09 20:49 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (7.69 KB, patch)
2020-02-09 20:58 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (7.70 KB, patch)
2020-02-14 10:44 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (7.71 KB, patch)
2020-02-14 13:33 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2020-02-09 20:47:34 PST
Implementing methods for WK2 to operate on cookies.
Comment 1 Pavel Feldman 2020-02-09 20:49:39 PST
Created attachment 390230 [details]
Patch
Comment 2 Pavel Feldman 2020-02-09 20:58:22 PST
Created attachment 390231 [details]
Patch
Comment 3 Fujii Hironori 2020-02-09 21:09:08 PST
Comment on attachment 390231 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Is there any existing layout tests this patch will make green?
Comment 4 Christopher Reid 2020-02-10 13:42:56 PST
Comment on attachment 390231 [details]
Patch

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

Note: I'm not a reviewer so these are just some informal comments.
The patch looks good overall.

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:164
> +    String url = (cookie.secure ? "https" : "http") + ("://" + cookie.domain + cookie.path);

String + String is pretty inefficient in WebKit.
This could be `String url = makeString((cookie.secure ? "https"_s : "http"_s), "://"_s, cookie.domain, cookie.path)`

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:453
> +    const String sql = "SELECT name, value, domain, path, expires, httponly, secure, session FROM Cookie";

String literals should now have a _s suffix.

> Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:66
> +    , m_cookieDatabase(makeUniqueRef<CookieJarDB>(sessionID.isEphemeral() ? ":memory:" : defaultCookieJarPath()))

Ditto ":memory:"_s
Comment 5 Pavel Feldman 2020-02-11 13:10:49 PST
Thanks for looking into it.

This achieves feature parity with the Cocoa API in terms of raw cookie management. We'd like to be able to manage cookies on the datastore / processpool level over the remote debugging protocol. The harness / web inspector client is not there (yet) and it'll take time, so this does not make any tests green yet. But it looks innocent in terms of adding simple yet functional implementations for those.
Comment 6 Pavel Feldman 2020-02-13 14:08:19 PST
I'm happy to follow up to all the nite below privided you accept the patch if I do!
Comment 7 Don Olmstead 2020-02-13 14:35:05 PST
(In reply to Pavel Feldman from comment #6)
> I'm happy to follow up to all the nite below privided you accept the patch
> if I do!

We found https://github.com/microsoft/playwright/blob/master/browser_patches/webkit/patches/bootstrap.diff and see there's a bunch of stuff in there that impact the ports we maintain. We're happy to review any of those changes that directly effect us, like this one, but larger changes that effect everyone would probably be better to have an Apple reviewer.

As a heads up I saw quite a few cases of `if PLATFORM(COCOA) || USE(CURL) || USE(SOUP)` in your diff so in those cases you can just get rid of the guard since that's all the upstream ports.
Comment 8 Pavel Feldman 2020-02-14 10:44:22 PST
Created attachment 390785 [details]
Patch
Comment 9 Pavel Feldman 2020-02-14 10:48:02 PST
Comment on attachment 390231 [details]
Patch

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

>> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:164
>> +    String url = (cookie.secure ? "https" : "http") + ("://" + cookie.domain + cookie.path);
> 
> String + String is pretty inefficient in WebKit.
> This could be `String url = makeString((cookie.secure ? "https"_s : "http"_s), "://"_s, cookie.domain, cookie.path)`

Done.

>> Source/WebCore/platform/network/curl/CookieJarDB.cpp:453
>> +    const String sql = "SELECT name, value, domain, path, expires, httponly, secure, session FROM Cookie";
> 
> String literals should now have a _s suffix.

Done. Note that other strings in this file don't use it. Should I be changing them as well or only update the code I touch?

>> Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:66
>> +    , m_cookieDatabase(makeUniqueRef<CookieJarDB>(sessionID.isEphemeral() ? ":memory:" : defaultCookieJarPath()))
> 
> Ditto ":memory:"_s

Done.
Comment 10 Pavel Feldman 2020-02-14 10:48:33 PST
> We found
> https://github.com/microsoft/playwright/blob/master/browser_patches/webkit/
> patches/bootstrap.diff and see there's a bunch of stuff in there that impact
> the ports we maintain. We're happy to review any of those changes that
> directly effect us, like this one, but larger changes that effect everyone
> would probably be better to have an Apple reviewer.

Yes, that's the plan. There is no rush and we'd like to make sure everyone is happy about the process. All the WebCore and Web Inspector patches will go by the WebCore and Web Inspector teams. When you say ports you maintain, do you mean Windows or some other ports as well?

In the Windows land, we have modified Minibrowser that allows headless operation. If you are interested in seeing headless operation upstream, I'm happy to to upload some patches. Other than that, we don't have much. This network patch is nearly the only change that we need for the win platform.


> As a heads up I saw quite a few cases of `if PLATFORM(COCOA) || USE(CURL) ||
> USE(SOUP)` in your diff so in those cases you can just get rid of the guard
> since that's all the upstream ports.

We are building all the platforms and all these PLATFORM checks are either 3 lines above/below the actual change in diff format or a couple of places where we touched it.
Comment 11 Don Olmstead 2020-02-14 10:56:00 PST
Comment on attachment 390785 [details]
Patch

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

Fix the ":memory:"_s and we're good.

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:467
> +        Cookie cookie;
> +        cookie.name = pstmt->getColumnText(0);
> +        cookie.value = pstmt->getColumnText(1);
> +        cookie.domain = pstmt->getColumnText(2).convertToASCIILowercase();
> +        cookie.path = pstmt->getColumnText(3);
> +        cookie.expires = (double)pstmt->getColumnInt64(4) * 1000;
> +        cookie.httpOnly = (pstmt->getColumnInt(5) == 1);
> +        cookie.secure = (pstmt->getColumnInt(6) == 1);
> +        cookie.session = (pstmt->getColumnInt(7) == 1);

I think this is something we can do in a follow up but I'm just making a note that this is probably something we'd want to do in a local function since I see this code elsewhere in the file.

> Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:66
> +    , m_cookieDatabase(makeUniqueRef<CookieJarDB>(sessionID.isEphemeral() ? ":memory:" : defaultCookieJarPath()))

Missed the ":memory:"_s here
Comment 12 Pavel Feldman 2020-02-14 13:33:14 PST
Created attachment 390806 [details]
Patch
Comment 13 Pavel Feldman 2020-02-14 13:35:47 PST
Comment on attachment 390785 [details]
Patch

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

>> Source/WebCore/platform/network/curl/CookieJarDB.cpp:467
>> +        cookie.session = (pstmt->getColumnInt(7) == 1);
> 
> I think this is something we can do in a follow up but I'm just making a note that this is probably something we'd want to do in a local function since I see this code elsewhere in the file.

Happy to follow up. Do you plan to support the samesite cookies? https://web.dev/samesite-cookies-explained/, https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-03#section-4.1 ? It would touch this code.

>> Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:66
>> +    , m_cookieDatabase(makeUniqueRef<CookieJarDB>(sessionID.isEphemeral() ? ":memory:" : defaultCookieJarPath()))
> 
> Missed the ":memory:"_s here

Ouch. Done.
Comment 14 Don Olmstead 2020-02-14 13:54:10 PST
(In reply to Pavel Feldman from comment #13)
> Comment on attachment 390785 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390785&action=review
> 
> >> Source/WebCore/platform/network/curl/CookieJarDB.cpp:467
> >> +        cookie.session = (pstmt->getColumnInt(7) == 1);
> > 
> > I think this is something we can do in a follow up but I'm just making a note that this is probably something we'd want to do in a local function since I see this code elsewhere in the file.
> 
> Happy to follow up. Do you plan to support the samesite cookies?
> https://web.dev/samesite-cookies-explained/,
> https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-03#section-4.1 ?
> It would touch this code.

Yes that's something we are interested in supporting for the cURL networking layer but haven't had the resources yet.
Comment 15 Don Olmstead 2020-02-14 14:10:31 PST
(In reply to Pavel Feldman from comment #10)
> > We found
> > https://github.com/microsoft/playwright/blob/master/browser_patches/webkit/
> > patches/bootstrap.diff and see there's a bunch of stuff in there that impact
> > the ports we maintain. We're happy to review any of those changes that
> > directly effect us, like this one, but larger changes that effect everyone
> > would probably be better to have an Apple reviewer.
> 
> Yes, that's the plan. There is no rush and we'd like to make sure everyone
> is happy about the process. All the WebCore and Web Inspector patches will
> go by the WebCore and Web Inspector teams. When you say ports you maintain,
> do you mean Windows or some other ports as well?

We at Sony maintain the WinCairo port, which includes the cURL networking backend, and the PlayStation port. The later doesn't have an EWS and we're working towards spinning up a buildbot in the next couple weeks. So its only there if you're looking for it.

Since you're with MS you should be aware that there is also the FTW port which we hope to more or less merge the AppleWin and WinCairo port so there's one Windows port to rule them all. The between that and WinCairo is that the rendering is through Direct2D.

> In the Windows land, we have modified Minibrowser that allows headless
> operation. If you are interested in seeing headless operation upstream, I'm
> happy to to upload some patches. Other than that, we don't have much. This
> network patch is nearly the only change that we need for the win platform.

Headless could be interesting. We have Docker containers for all our build and test bots so that might come in handy later on. If you want to do it we're happy to review it.

> > As a heads up I saw quite a few cases of `if PLATFORM(COCOA) || USE(CURL) ||
> > USE(SOUP)` in your diff so in those cases you can just get rid of the guard
> > since that's all the upstream ports.
> 
> We are building all the platforms and all these PLATFORM checks are either 3
> lines above/below the actual change in diff format or a couple of places
> where we touched it.

Sure I'm just mentioning it for when you land it you might want to look at whether some of the PLATFORM checks can be removed like the above or modified so they don't effect downstream ports.
Comment 16 WebKit Commit Bot 2020-02-14 15:17:45 PST
Comment on attachment 390806 [details]
Patch

Clearing flags on attachment: 390806

Committed r256651: <https://trac.webkit.org/changeset/256651>
Comment 17 WebKit Commit Bot 2020-02-14 15:17:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2020-02-14 15:18:19 PST
<rdar://problem/59474769>