Bug 185261 – Adopt new async _savecookies SPI for keeping networking process active during flushing cookies

• Component: WebKit API
• Status: RESOLVED FIXED

[Sihui Liu]

When network process is flushing cookies to disk, we want it to be active/not suspended before the syncing completes. CFNetwork has provided a new SPI for this.

[Sihui Liu]

<rdar://problem/37214391>

[Sihui Liu]

Created attachment 339454 [details]
Patch

[Geoffrey Garen]

Comment on attachment 339454 [details]
Patch

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

To fix the build, I think you need something like this:

#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000)
// new stuff
#else
// old stuff
#endif

> Source/WebCore/platform/network/NetworkStorageSession.cpp:65
> +    return globalSessionMap().size() + 1;

It would be nice to add a small comment explaining that the + 1 is for the default session, like so: // + 1 for the default session

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:232
> +    m_syncingCookiesResultsToCollect = WebCore::NetworkStorageSession::sessionCount();

In order to handle multiple calls to syncAllCookies, where call #2 might happen before call #1 completes its async activity, this needs to be += instead of =.

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:235
> +    auto *resultsToCollectPtr = &m_syncingCookiesResultsToCollect;
> +    WebCore::NetworkStorageSession::forEach([&] (const WebCore::NetworkStorageSession& networkStorageSession) {

We don't need to use a pointer in order to do this capture. What we're trying to capture is this->m_syncingCookiesResultsToCollect. I would do this by capture "this" in the C++ capture statement, and then using the implicitly in-scope value "m_syncingCookiesResultsToCollect" inside the Objective-C block.

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:239
> +#if PLATFORM(IOS)

I don't think we need to go out of our way to make iOS and macOS different in this regard. Might as well make everything cross-platform if we can.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:551
> +void NetworkProcessProxy::setIsSyncingCookies(bool isSyncingCookies)

You can early return here with "if (isSyncingCookies == !!m_tokenForSyncingCookies) return;".

> Source/WebKit/UIProcess/WebProcessPool.cpp:1600
> +#if PLATFORM(IOS)
> +    if (m_tokenForSyncingCookies)
> +        return;
> +    m_networkprocess->setIsSyncingCookies(true);
> +#endif

You mentioned that it's a bug that a new call to this function while an existing call is in flight will not sync cookies again. I think the best way to fix this is to make setIsSyncingCookies() return early if you set the same value, and then call it unconditionally, and remove the early return.

[Chris Dumez]

Comment on attachment 339454 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:232
>> +    m_syncingCookiesResultsToCollect = WebCore::NetworkStorageSession::sessionCount();
> 
> In order to handle multiple calls to syncAllCookies, where call #2 might happen before call #1 completes its async activity, this needs to be += instead of =.

We have a CallbackAggregator class in WTF just for this kind of use case:
auto completionAggregator = CallbackAggregator::create([this] {
    didSyncAllCookies();
});

WebCore::NetworkStorageSession::forEach([&] (const WebCore::NetworkStorageSession& networkStorageSession) {
    [networkStorageSession.nsCookieStorage() _saveCookies:[completionAggregator = completionAggregator.copyRef()] { }]);
});

Then you don't need m_syncingCookiesResultsToCollect or sessionCount.

[Chris Dumez]

Comment on attachment 339454 [details]
Patch

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

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:37
>      SetIsHoldingLockedFiles(bool isHoldingLockedFiles)

This IPC makes sense because it is called with both true and false by the Network process.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:38
> +    SetIsSyncingCookies(bool isSyncingCookies)

However, this is weird since this is only ever called with false from the NetworkProcess.

Since this is the completion for the NetworkProcess::SyncAllCookies, I guess we could have a DidSyncAllCookies IPC back.

> Source/WebKit/UIProcess/WebProcessPool.cpp:1594
>  void WebProcessPool::syncNetworkProcessCookies()

This method is lying now. Even though the IPC is synchronous, the method returns before cookies are done sync'ing.

If we really want this method to hang until all cookies are syncd, then I'd suggest we make SyncAllCookies IPC a DelayedReply and we have the NetworkProcess only reply in the aggregator completion handler.

[Chris Dumez]

Comment on attachment 339454 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:1594
>>  void WebProcessPool::syncNetworkProcessCookies()
> 
> This method is lying now. Even though the IPC is synchronous, the method returns before cookies are done sync'ing.
> 
> If we really want this method to hang until all cookies are syncd, then I'd suggest we make SyncAllCookies IPC a DelayedReply and we have the NetworkProcess only reply in the aggregator completion handler.

And this raises the question, do we want an asynchronous alternative to _syncNetworkProcessCookies SPI?

[Geoffrey Garen]

Chris pointed out that WebProcessPool::syncNetworkProcessCookies should also change away from using sendSyncToNetworkingProcess because this is an async operation, so it's pointless to synchronously wait for it to complete.

If our clients cared to wait for this operation to complete, we would add a completion handler to it. But since no client cares, we can just make it async with no completion handler.

[Sihui Liu]

Created attachment 339782 [details]
Patch

[Geoffrey Garen]

Comment on attachment 339782 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:944
> +    

Please revert this whitespace change to keep svn history simple.

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:235
> +    RefPtr<CallbackAggregator> completionAggregator = CallbackAggregator::create([&] {

I would call this variable "callbackAggregator". It is nice to avoid giving two names to one thing.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:173
> +    ProcessThrottler::BackgroundActivityToken m_tokenForSyncingCookies;

I would call this "m_syncAllCookiesToken".

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:175
> +    unsigned m_numPendingSyncCookiesCall { 0 };

I would call this "m_syncAllCookiesCounter".

> Source/WebKit/UIProcess/WebProcessPool.cpp:1603
> +    m_networkProcess->send(Messages::NetworkProcess::SyncAllCookies(), 0);

I think this call to send() would make more sense inside NetworkProcessProxy::syncAllCookies() now.

[Chris Dumez]

Comment on attachment 339782 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:235
>> +    RefPtr<CallbackAggregator> completionAggregator = CallbackAggregator::create([&] {
> 
> I would call this variable "callbackAggregator". It is nice to avoid giving two names to one thing.

Since this callback is calling asynchronously, I would not use & for capture. If we start capturing a local variable by mistake, it would be bad. I would just capture |this| explicitly here.

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:238
> +    WebCore::NetworkStorageSession::forEach([&] (const WebCore::NetworkStorageSession& networkStorageSession) {

Here & is fine since forEach() is synchronous.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:551
> +void NetworkProcessProxy::syncAllCookies()

This is very confusing. This method does not sync any cookies.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:566
> +        m_tokenForSyncingCookies = nullptr;

We really do not want to leak process assertions. I think it would be a lot safer to null out m_tokenForSyncingCookies in NetworkProcessProxy::didClose(), like we already do for m_tokenForHoldingLockedFiles. This is to deal with the network process crashing. We probably want to reset m_numPendingSyncCookiesCall to 0 too.

> Source/WebKit/UIProcess/WebProcessPool.cpp:1602
> +    m_networkProcess->syncAllCookies();

This is wrong, you cannot assume m_networkProcess is non-null. This would be ensureNetworkProcess().syncAllCookies();

>> Source/WebKit/UIProcess/WebProcessPool.cpp:1603
>> +    m_networkProcess->send(Messages::NetworkProcess::SyncAllCookies(), 0);
> 
> I think this call to send() would make more sense inside NetworkProcessProxy::syncAllCookies() now.

Agreed and this would address my earlier comment about NetworkProcess::syncAllCookies() not syncing cookies.

[Chris Dumez]

Comment on attachment 339782 [details]
Patch

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

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:563
> +    --m_numPendingSyncCookiesCall;

We tend to ASSERT(m_numPendingSyncCookiesCall); before such a call.

[Sihui Liu]

Created attachment 339801 [details]
Patch

[EWS Watchlist]

Comment on attachment 339801 [details]
Patch

Attachment 339801 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7604782

New failing tests:
http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients.html

[EWS Watchlist]

Created attachment 339807 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Comment on attachment 339801 [details]
Patch

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

r=me with nits.

> Source/WebKit/ChangeLog:10
> +        before cookies are fully synced to disk with process assertion.

s/before/until

> Source/WebKit/NetworkProcess/NetworkProcess.h:33
> +#include "NetworkProcessMessages.h"

This include should not be needed.

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:238
> +    WebCore::NetworkStorageSession::forEach([&] (const WebCore::NetworkStorageSession& networkStorageSession) {

I think we can use "auto& networkStorageSession" here since C++14.

[Sihui Liu]

Created attachment 339825 [details]
Patch

[Chris Dumez]

Comment on attachment 339825 [details]
Patch

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

r=me

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:238
> +    WebCore::NetworkStorageSession::forEach([&] (const auto& networkStorageSession) {

out of curiosity, was the const really needed? We tend to omit it and let the auto take care of it.

[Sihui Liu]

(In reply to Chris Dumez from comment #17)
> Comment on attachment 339825 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339825&action=review
> 
> r=me
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:238
> > +    WebCore::NetworkStorageSession::forEach([&] (const auto& networkStorageSession) {
> 
> out of curiosity, was the const really needed? We tend to omit it and let
> the auto take care of it.

In this case I think it's same. It's just a sign of "we are not going to modify it". I will remove it if you think it's better.

[Chris Dumez]

(In reply to Sihui Liu from comment #18)
> (In reply to Chris Dumez from comment #17)
> > Comment on attachment 339825 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=339825&action=review
> > 
> > r=me
> > 
> > > Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:238
> > > +    WebCore::NetworkStorageSession::forEach([&] (const auto& networkStorageSession) {
> > 
> > out of curiosity, was the const really needed? We tend to omit it and let
> > the auto take care of it.
> 
> In this case I think it's same. It's just a sign of "we are not going to
> modify it". I will remove it if you think it's better.

We usually do not have const variable in WebKit, we omit it whenever possible.

[Sihui Liu]

Created attachment 339831 [details]
Patch

[Daniel Bates]

Comment on attachment 339831 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:239
> +        [networkStorageSession.nsCookieStorage() _saveCookies:[callbackAggregator] { }];

This will break the public SDK build in the future :) We need to forward declare this SPI in Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h.

[Chris Dumez]

(In reply to Daniel Bates from comment #21)
> Comment on attachment 339831 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339831&action=review
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:239
> > +        [networkStorageSession.nsCookieStorage() _saveCookies:[callbackAggregator] { }];
> 
> This will break the public SDK build in the future :) We need to forward
> declare this SPI in Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h.

Completely forgot about that, thanks for catching it Daniel. Sihui, could you please fix before landing?

[Sihui Liu]

Created attachment 339904 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339904 [details]
Patch

Clearing flags on attachment: 339904

Committed r231536: <https://trac.webkit.org/changeset/231536>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change introduced an API test failure on Sierra bots:

20:06:51.862 92941 Failed
20:06:51.862 92941 
20:06:51.862 92941     TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths
20:06:51.862 92941         
20:06:51.862 92941         /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:146
20:06:51.862 92941         Value of: [[NSFileManager defaultManager] fileExistsAtPath:cookieStorageFile.path]
20:06:51.862 92941           Actual: false
20:06:51.862 92941         Expected: true
20:06:51.862 92941         

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/9269

[Chris Dumez]

(In reply to Ryan Haddad from comment #26)
> This change introduced an API test failure on Sierra bots:
> 
> 20:06:51.862 92941 Failed
> 20:06:51.862 92941 
> 20:06:51.862 92941     TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths
> 20:06:51.862 92941         
> 20:06:51.862 92941        
> /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/WebsiteDataStoreCustomPaths.mm:146
> 20:06:51.862 92941         Value of: [[NSFileManager defaultManager]
> fileExistsAtPath:cookieStorageFile.path]
> 20:06:51.862 92941           Actual: false
> 20:06:51.862 92941         Expected: true
> 20:06:51.862 92941         
> 
> https://build.webkit.org/builders/
> Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/9269

Hmm, does not look directly related though. This is odd.

[Chris Dumez]

(In reply to Chris Dumez from comment #27)
> (In reply to Ryan Haddad from comment #26)
> > This change introduced an API test failure on Sierra bots:
> > 
> > 20:06:51.862 92941 Failed
> > 20:06:51.862 92941 
> > 20:06:51.862 92941     TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths
> > 20:06:51.862 92941         
> > 20:06:51.862 92941        
> > /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/WebsiteDataStoreCustomPaths.mm:146
> > 20:06:51.862 92941         Value of: [[NSFileManager defaultManager]
> > fileExistsAtPath:cookieStorageFile.path]
> > 20:06:51.862 92941           Actual: false
> > 20:06:51.862 92941         Expected: true
> > 20:06:51.862 92941         
> > 
> > https://build.webkit.org/builders/
> > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/9269
> 
> Hmm, does not look directly related though. This is odd.

I take that back, definitely related. The API test calls [[[webView configuration] processPool] _syncNetworkProcessCookies]; and then synchronously expects the cookie path to exist. This won't work anymore because the operation is now async, without any callback to the client.

We previously checked that no client expected the synchronous behavior but it looks like we failed to check the API tests.

[Chris Dumez]

(In reply to Chris Dumez from comment #28)
> (In reply to Chris Dumez from comment #27)
> > (In reply to Ryan Haddad from comment #26)
> > > This change introduced an API test failure on Sierra bots:
> > > 
> > > 20:06:51.862 92941 Failed
> > > 20:06:51.862 92941 
> > > 20:06:51.862 92941     TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths
> > > 20:06:51.862 92941         
> > > 20:06:51.862 92941        
> > > /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> > > WebKitCocoa/WebsiteDataStoreCustomPaths.mm:146
> > > 20:06:51.862 92941         Value of: [[NSFileManager defaultManager]
> > > fileExistsAtPath:cookieStorageFile.path]
> > > 20:06:51.862 92941           Actual: false
> > > 20:06:51.862 92941         Expected: true
> > > 20:06:51.862 92941         
> > > 
> > > https://build.webkit.org/builders/
> > > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/9269
> > 
> > Hmm, does not look directly related though. This is odd.
> 
> I take that back, definitely related. The API test calls [[[webView
> configuration] processPool] _syncNetworkProcessCookies]; and then
> synchronously expects the cookie path to exist. This won't work anymore
> because the operation is now async, without any callback to the client.
> 
> We previously checked that no client expected the synchronous behavior but
> it looks like we failed to check the API tests.

One way to fix the API test may be:

while (![[NSFileManager defaultManager] fileExistsAtPath:cookieStorageFile.path])
    TestWebKitAPI::Util::spinRunLoop(1);

[Daniel Bates]

(In reply to Chris Dumez from comment #29)
> (In reply to Chris Dumez from comment #28)
> > (In reply to Chris Dumez from comment #27)
> > > (In reply to Ryan Haddad from comment #26)
> > > > This change introduced an API test failure on Sierra bots:
> > > > 
> > > > 20:06:51.862 92941 Failed
> > > > 20:06:51.862 92941 
> > > > 20:06:51.862 92941     TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths
> > > > 20:06:51.862 92941         
> > > > 20:06:51.862 92941        
> > > > /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> > > > WebKitCocoa/WebsiteDataStoreCustomPaths.mm:146
> > > > 20:06:51.862 92941         Value of: [[NSFileManager defaultManager]
> > > > fileExistsAtPath:cookieStorageFile.path]
> > > > 20:06:51.862 92941           Actual: false
> > > > 20:06:51.862 92941         Expected: true
> > > > 20:06:51.862 92941         
> > > > 
> > > > https://build.webkit.org/builders/
> > > > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/9269
> > > 
> > > Hmm, does not look directly related though. This is odd.
> > 
> > I take that back, definitely related. The API test calls [[[webView
> > configuration] processPool] _syncNetworkProcessCookies]; and then
> > synchronously expects the cookie path to exist. This won't work anymore
> > because the operation is now async, without any callback to the client.
> > 
> > We previously checked that no client expected the synchronous behavior but
> > it looks like we failed to check the API tests.
> 
> One way to fix the API test may be:
> 
> while (![[NSFileManager defaultManager]
> fileExistsAtPath:cookieStorageFile.path])
>     TestWebKitAPI::Util::spinRunLoop(1);

I suggest that we update the existing SPI or add new SPI that takes a completion handler. Then we do not need to busy wait.

[Chris Dumez]

(In reply to Daniel Bates from comment #30)
> (In reply to Chris Dumez from comment #29)
> > (In reply to Chris Dumez from comment #28)
> > > (In reply to Chris Dumez from comment #27)
> > > > (In reply to Ryan Haddad from comment #26)
> > > > > This change introduced an API test failure on Sierra bots:
> > > > > 
> > > > > 20:06:51.862 92941 Failed
> > > > > 20:06:51.862 92941 
> > > > > 20:06:51.862 92941     TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths
> > > > > 20:06:51.862 92941         
> > > > > 20:06:51.862 92941        
> > > > > /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> > > > > WebKitCocoa/WebsiteDataStoreCustomPaths.mm:146
> > > > > 20:06:51.862 92941         Value of: [[NSFileManager defaultManager]
> > > > > fileExistsAtPath:cookieStorageFile.path]
> > > > > 20:06:51.862 92941           Actual: false
> > > > > 20:06:51.862 92941         Expected: true
> > > > > 20:06:51.862 92941         
> > > > > 
> > > > > https://build.webkit.org/builders/
> > > > > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/9269
> > > > 
> > > > Hmm, does not look directly related though. This is odd.
> > > 
> > > I take that back, definitely related. The API test calls [[[webView
> > > configuration] processPool] _syncNetworkProcessCookies]; and then
> > > synchronously expects the cookie path to exist. This won't work anymore
> > > because the operation is now async, without any callback to the client.
> > > 
> > > We previously checked that no client expected the synchronous behavior but
> > > it looks like we failed to check the API tests.
> > 
> > One way to fix the API test may be:
> > 
> > while (![[NSFileManager defaultManager]
> > fileExistsAtPath:cookieStorageFile.path])
> >     TestWebKitAPI::Util::spinRunLoop(1);
> 
> I suggest that we update the existing SPI or add new SPI that takes a
> completion handler. Then we do not need to busy wait.

1. If we update the existing API, then we have to update existing clients. Client apps currently do not need to know about completion.
2. If We add a new SPI, then the API test will no longer be testing the SPI clients are using.

I think "busy" looping is acceptable in the context on an API test.

[Geoffrey Garen]

> I think "busy" looping is acceptable in the context on an API test.

I tend to agree in the context of this API test. But can we put some kind of limit on the busy wait, so that a test failure doesn't prevent future tests from running?

[Daniel Bates]

(In reply to Chris Dumez from comment #31)
> > I suggest that we update the existing SPI or add new SPI that takes a
> > completion handler. Then we do not need to busy wait.
> 
> 1. If we update the existing API, then we have to update existing clients.

We're talking about the SPI _syncNetworkProcessCookies, right? There are exactly 3 invocations of this SPI by exactly 2 clients: 2 x TestWebKitAPI and 1 x MobileSafari. It does not seem like it would be a huge burden to update existing clients.

> Client apps currently do not need to know about completion.
> 2. If We add a new SPI, then the API test will no longer be testing the SPI
> clients are using.
> 

Then how about we do (1). See remarks above.

> I think "busy" looping is acceptable in the context on an API test.

Maybe Alexey has some thoughts on this?

[Daniel Bates]

(In reply to Geoffrey Garen from comment #32)
> > I think "busy" looping is acceptable in the context on an API test.
> 
> I tend to agree in the context of this API test. But can we put some kind of
> limit on the busy wait, so that a test failure doesn't prevent future tests
> from running?

OK.

[Alexey Proskuryakov]

Is the discussion about what happened in bug 185486, not here?

I don't have much problem with polling in an API test (as long as thought is given to whether run loop needs to be active while waiting). But given the kind of unfortunate name of the SPI that became async, this may be a good opportunity to update it anyway. Either add a completion handler, or change the name to something like "start syncing cookies" instead of "sync cookies".

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #35)
> Is the discussion about what happened in bug 185486, not here?
> 
> I don't have much problem with polling in an API test (as long as thought is
> given to whether run loop needs to be active while waiting). But given the
> kind of unfortunate name of the SPI that became async, this may be a good
> opportunity to update it anyway. Either add a completion handler, or change
> the name to something like "start syncing cookies" instead of "sync cookies".

I have been told that the CFNetwork SPI we relied on was already async, it merely did not take a completion block. The new SPI takes such a block. The behavior change we made is that we now also ask CFNetwork asynchronously via async IPC instead of sync IPC. So things are a bit more async now but our SPI was never synchronous.

[Geoffrey Garen]

> > 1. If we update the existing API, then we have to update existing clients.
> 
> Then how about we do (1). See remarks above.

I do generally prefer that async APIs tell you when they're done. That said, this is a rare case where our SPI client did not care to be told.

If we made this SPI API, I would recommend that change. But I don't think we plan to.

It feels upside-down to change the API design to match the test we want. Usually, we change the test to match the API we have.

[Sihui Liu]

(In reply to Daniel Bates from comment #33)
> (In reply to Chris Dumez from comment #31)
> > > I suggest that we update the existing SPI or add new SPI that takes a
> > > completion handler. Then we do not need to busy wait.
> > 
> > 1. If we update the existing API, then we have to update existing clients.
> 
> We're talking about the SPI _syncNetworkProcessCookies, right? There are
> exactly 3 invocations of this SPI by exactly 2 clients: 2 x TestWebKitAPI
> and 1 x MobileSafari. It does not seem like it would be a huge burden to
> update existing clients.
> 
> > Client apps currently do not need to know about completion.
> > 2. If We add a new SPI, then the API test will no longer be testing the SPI
> > clients are using.
> > 
> 
> Then how about we do (1). See remarks above.
> 

Talked to Dan & Geoff offline. Though I agree it's good to make API usable and intuitive, I tend to leave the SPI same for now, as _syncNetworkProcessCookies is only used by one client, which doesn't care when the function is completed, and changing the SPI(with changes to Safari) may make it harder to test older version Safaris that still uses the old SPI. We could add a new SPI, but it increases complexity of this problem which seems unnecessary now.

I will file a radar/bug to track this, in case someday we think it has a higher priority.

[Daniel Bates]

(In reply to Geoffrey Garen from comment #37)
> 
> It feels upside-down to change the API design to match the test we want.

API is an overloaded term. You seem to be using it in its most general form to refer to any application programming interface regardless of its release policy (public, private, et. cetera). Can you please explain why you feel obligated to afford the same guarantees we (Apple) bestow on public API [1] to private API?

For completeness, I agree that it would be "upside-down to change [a public] API design to match [a] test".

[1] <https://en.wikipedia.org/wiki/Application_programming_interface#Release_policies>

[Geoffrey Garen]

> > It feels upside-down to change the API design to match the test we want.

In this sentence, by "API" I meant "the interface Safari uses".