[webkit-reviews] review denied: [Bug 185261] Adopt new async _savecookies SPI for keeping networking process active during flushing cookies : [Attachment 339782] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 185261: Adopt new async _savecookies SPI for keeping networking process
active during flushing cookies
https://bugs.webkit.org/show_bug.cgi?id=185261

Attachment 339782: Patch

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




--- Comment #10 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 339782
  --> https://bugs.webkit.org/attachment.cgi?id=339782
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.