Bug 185624

Summary: Session cookies aren't reliably set when using default WKWebSiteDataStore
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: WebKit APIAssignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, commit-queue, ews-watchlist, ggaren, herr.ernst, realdawei, sihui_liu, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews204 for win-future
none
Patch
none
Patch none

Description Sihui Liu 2018-05-14 14:23:16 PDT
We could set session cookies through default WKWebSiteDataStore, but we may not get those cookies reliably after creating a WebView.
Comment 1 Sihui Liu 2018-05-14 14:24:32 PDT
<rdar://problem/39111626>
Comment 2 Sihui Liu 2018-05-14 14:42:31 PDT
Created attachment 340362 [details]
Patch
Comment 3 Chris Dumez 2018-05-14 15:47:43 PDT
Comment on attachment 340362 [details]
Patch

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

I cannot comment about the overall approach but I have a few comments:

> Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:51
> +            auto* session = NetworkStorageSession::storageSession(sessionID);

Double looking seems unfortunate here. Why not update the if above to look like:
if (auto* session = NetworkStorageSession::storageSession(sessionID)) {

> Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:-61
> -            allCookies = WebCore::NetworkStorageSession::defaultStorageSession().getAllCookies();

Doesn't this mean we're only returning the cookies added by the API for the default session now? Previously, we'd ask the default storage session which presumably could return cookies saved on disk, no?

> Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:-103
> -            WebCore::NetworkStorageSession::defaultStorageSession().deleteCookie(cookie);

Ditto. Doesn't this mean that in the common case (default session), the API can only delete cookies that were added via the API?
Comment 4 EWS Watchlist 2018-05-14 16:13:29 PDT
Comment on attachment 340362 [details]
Patch

Attachment 340362 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7682434

New failing tests:
transitions/interrupted-transition-hardware.html
Comment 5 EWS Watchlist 2018-05-14 16:13:30 PDT
Created attachment 340373 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 6 Geoffrey Garen 2018-05-14 17:23:12 PDT
Comment on attachment 340362 [details]
Patch

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

> Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:50
> +        if (sessionID == PAL::SessionID::defaultSessionID()) {

Why do we check for the default session here?
Comment 7 Sihui Liu 2018-05-15 03:34:21 PDT
Created attachment 340403 [details]
Patch
Comment 8 Sihui Liu 2018-05-15 09:21:56 PDT
(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 340362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340362&action=review
> 
> > Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:50
> > +        if (sessionID == PAL::SessionID::defaultSessionID()) {
> 
> Why do we check for the default session here?

It seems only the default session could be created without considering parameter pendingcookies, as it used uiProcessCookieStorageIdentifier to specify cookie storage to sync. Also in APIHTTPCookieStore, we only used pendingcookies param for sessions other than default session.
Comment 9 Sihui Liu 2018-05-15 10:37:10 PDT
(In reply to Chris Dumez from comment #3)
> Comment on attachment 340362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340362&action=review
> 
> I cannot comment about the overall approach but I have a few comments:
> 
> > Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:51
> > +            auto* session = NetworkStorageSession::storageSession(sessionID);
> 
> Double looking seems unfortunate here. Why not update the if above to look
> like:
> if (auto* session = NetworkStorageSession::storageSession(sessionID)) {
> 
> > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:-61
> > -            allCookies = WebCore::NetworkStorageSession::defaultStorageSession().getAllCookies();
> 
> Doesn't this mean we're only returning the cookies added by the API for the
> default session now? Previously, we'd ask the default storage session which
> presumably could return cookies saved on disk, no?
> 
Yes! I've updated the patch to still use default storage session to read cookies from persistent storage.
Comment 10 Geoffrey Garen 2018-05-15 12:11:27 PDT
Comment on attachment 340403 [details]
Patch

I don't understand the pending cookie design in general. It seems like pending cookies + processPoolForCookieStorageOperations() are an attempt to avoid launching the network process? But in what world do we expect our client to add cookies and then not do any networking?

I guess this is a workaround for the fact that the API client can create a WKWebsiteDatastore and a WKProcessPool separately, and pair arbitrary combinations of data store and process pool?

In addition to not understanding the design in general, I don't understand in particular why pending cookies used to apply only to the default session, or in particular why we now want pending cookies to apply only to the default session only if they are not session cookies.

I guess we need to discuss in person with Brady and/or Alex.
Comment 11 Geoffrey Garen 2018-05-15 14:05:40 PDT
Comment on attachment 340403 [details]
Patch

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

> Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:49
> +    if (auto* session = NetworkStorageSession::storageSession(sessionID)) {

I think the UI process needs to clear pending cookies after it first sends them to the network process. Otherwise, if the cookies get deleted or overwritten, then a crash that reinitializes the network process will restore the pending cookies, which is not correct.

Also, I think that clearing pending cookies is a better design because continuing to send them when they should not be set is very surprising.

Once we make that change, we should also be able to change this code. We don't need any check, and instead we can unconditionally set pending cookies. If you like, you can also ASSERT (parameters.pendingCookies().isEmpty() || sessionID == PAL::SessionID::defaultSessionID()), which will verify this check I'm asking you to remove.

> Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:80
> +        if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID() && !cookie.session)

It would be nice to file a Radar about the session cookie syncing issue in CFNetwork, and then comment that this session cookie check is an exploit workaround for that bug. Otherwise, the check seems out of the blue and unnecessary.

I don't understand why non-default data store sessions skip using NetworkStorageSession's setCookie API. I'd like to understand that.

> Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:101
> +        if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID() && !cookie.session)

Ditto.

> Source/WebKit/UIProcess/WebProcessPool.cpp:520
> +    if (m_websiteDataStore)
> +        m_networkProcess->send(Messages::NetworkProcess::AddWebsiteDataStore(m_websiteDataStore->websiteDataStore().parameters()), 0);

Let's move this code down a little bit so it can be next to the other call to AddWebsiteDataStore.

I don't understand why clients that specify 'withWebsiteDataStore' also get a separate m_websiteDataStore. I'd like to know why.
Comment 12 Geoffrey Garen 2018-05-15 15:04:24 PDT
> I don't understand why non-default data store sessions skip using
> NetworkStorageSession's setCookie API. I'd like to understand that.

I believe you should be able to write a test where you make a non-default data store, set a persistent cookie in it, release the data store, then make another non-default data store for the same path, and you'll find that your cookie did not get saved. That seems like a bug.
Comment 13 Sihui Liu 2018-05-15 16:24:16 PDT
Created attachment 340441 [details]
Patch
Comment 14 Sihui Liu 2018-05-15 16:55:31 PDT
(In reply to Geoffrey Garen from comment #11)
> Comment on attachment 340403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340403&action=review
> 
> > Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:49
> > +    if (auto* session = NetworkStorageSession::storageSession(sessionID)) {
> 
> I think the UI process needs to clear pending cookies after it first sends
> them to the network process. Otherwise, if the cookies get deleted or
> overwritten, then a crash that reinitializes the network process will
> restore the pending cookies, which is not correct.
> 
> Also, I think that clearing pending cookies is a better design because
> continuing to send them when they should not be set is very surprising.
> 
> Once we make that change, we should also be able to change this code. We
> don't need any check, and instead we can unconditionally set pending
> cookies. If you like, you can also ASSERT
> (parameters.pendingCookies().isEmpty() || sessionID ==
> PAL::SessionID::defaultSessionID()), which will verify this check I'm asking
> you to remove.
> 
Great advice. Adopted in the new patch.

> > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:80
> > +        if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID() && !cookie.session)
> 
> It would be nice to file a Radar about the session cookie syncing issue in
> CFNetwork, and then comment that this session cookie check is an exploit
> workaround for that bug. Otherwise, the check seems out of the blue and
> unnecessary.
> 
> I don't understand why non-default data store sessions skip using
> NetworkStorageSession's setCookie API. I'd like to understand that.
> 
Seems like a bug. Tracking in https://bugs.webkit.org/show_bug.cgi?id=185666.

> > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:101
> > +        if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID() && !cookie.session)
> 
> Ditto.
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:520
> > +    if (m_websiteDataStore)
> > +        m_networkProcess->send(Messages::NetworkProcess::AddWebsiteDataStore(m_websiteDataStore->websiteDataStore().parameters()), 0);
> 
> Let's move this code down a little bit so it can be next to the other call
> to AddWebsiteDataStore.
> 
> I don't understand why clients that specify 'withWebsiteDataStore' also get
> a separate m_websiteDataStore. I'd like to know why.
It seems 'withWebsiteDataStore' in ensureNetworkProcess is only used in WebsiteDataStore::fetchDataAndApply and WebsiteDataStore::removeData, where we want to add another webSiteDataStore to Network Process before fetching/removing data from it. And m_websiteDataStore is defaultDataStore.
Comment 15 EWS Watchlist 2018-05-16 04:26:24 PDT
Comment on attachment 340441 [details]
Patch

Attachment 340441 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7697206

New failing tests:
http/tests/preload/onload_event.html
Comment 16 EWS Watchlist 2018-05-16 04:26:35 PDT
Created attachment 340484 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 17 Geoffrey Garen 2018-05-16 09:52:15 PDT
Comment on attachment 340441 [details]
Patch

This seems like an improvement, so I'll say r=me.

I think we need to follow up on the potential design problems that we discovered while working on this.
Comment 18 WebKit Commit Bot 2018-05-16 10:19:23 PDT
Comment on attachment 340441 [details]
Patch

Clearing flags on attachment: 340441

Committed r231850: <https://trac.webkit.org/changeset/231850>
Comment 19 WebKit Commit Bot 2018-05-16 10:19:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Dawei Fenton (:realdawei) 2018-05-18 10:24:46 PDT
API test TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool is flaky on the bots after this change.  Here is the failure output:
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:379
        Value of: message.UTF8String
          Actual: "cookie:SessionCookieName=CookieValue; PersistentCookieName=CookieValue"
        Expected: "cookie:PersistentCookieName=CookieValue; SessionCookieName=CookieValue"


https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/4580
Comment 21 Sihui Liu 2018-05-18 15:09:46 PDT
Reopened as API test is flaky.
Comment 22 Sihui Liu 2018-05-18 15:15:37 PDT
Created attachment 340746 [details]
Patch
Comment 23 Chris Dumez 2018-05-18 15:28:51 PDT
Comment on attachment 340746 [details]
Patch

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

> Tools/ChangeLog:3
> +        Session cookies aren't reliably set when using default WKWebSiteDataStore

Title seems wrong (unrelated to this change). The cookies are set, merely returned in a different order, right?

> Tools/ChangeLog:9
> +        Modified test to set correct expection.

typo: expection

Also, I would clarify that you're making the API test robust to cookies being returned in a different order.
Comment 24 Sihui Liu 2018-05-18 15:48:08 PDT
Created attachment 340750 [details]
Patch
Comment 25 Geoffrey Garen 2018-05-19 13:29:32 PDT
Comment on attachment 340750 [details]
Patch

cq+
Comment 26 WebKit Commit Bot 2018-05-19 13:57:07 PDT
Comment on attachment 340750 [details]
Patch

Clearing flags on attachment: 340750

Committed r231999: <https://trac.webkit.org/changeset/231999>
Comment 27 WebKit Commit Bot 2018-05-19 13:57:09 PDT
All reviewed patches have been landed.  Closing bug.