Bug 207450 – [Curl] Implement NetworkStorageSession::get/set/deleteCookie

• Component: WebKit2
• Status: RESOLVED FIXED

[Pavel Feldman]

Implementing methods for WK2 to operate on cookies.

[Pavel Feldman]

Created attachment 390230 [details]
Patch

[Pavel Feldman]

Created attachment 390231 [details]
Patch

[Fujii Hironori]

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?

[Christopher Reid]

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

[Pavel Feldman]

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.

[Pavel Feldman]

I'm happy to follow up to all the nite below privided you accept the patch if I do!

[Don Olmstead]

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

[Pavel Feldman]

Created attachment 390785 [details]
Patch

[Pavel Feldman]

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.

[Pavel Feldman]

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

[Don Olmstead]

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

[Pavel Feldman]

Created attachment 390806 [details]
Patch

[Pavel Feldman]

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.

[Don Olmstead]

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

[Don Olmstead]

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

[WebKit Commit Bot]

Comment on attachment 390806 [details]
Patch

Clearing flags on attachment: 390806

Committed r256651: <https://trac.webkit.org/changeset/256651>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59474769>