Bug 206450 – Frequent NetworkConnectionToWebProcess::CookiesEnabled sync IPC when browsing reddit.com

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Frequent NetworkConnectionToWebProcess::CookiesEnabled sync IPC when browsing reddit.com. Happens every time the page script calls navigator.cookieEnabled.

[Chris Dumez]

Created attachment 388099 [details]
Patch

[Chris Dumez]

Created attachment 388104 [details]
Patch

[Sam Weinig]

Comment on attachment 388104 [details]
Patch

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

> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:234
> +bool NetworkProcessConnection::cookiesEnabled()
> +{
> +    if (m_cachedCookiedEnabled)
> +        return *m_cachedCookiedEnabled;
> +
> +    bool result = false;
> +    connection().sendSync(Messages::NetworkConnectionToWebProcess::CookiesEnabled(), Messages::NetworkConnectionToWebProcess::CookiesEnabled::Reply(result), 0);
> +    m_cachedCookiedEnabled = result;
> +    return result;
> +}

What is the value of keeping this sync vs. having the network process proactively update the WebProcesses whenever the policy changes? e.g. cookieAcceptPolicyMayHaveChanged becomes cookieAcceptPolicyChanged(newPolicy).

[Alexey Proskuryakov]

That would mean waking up all WebContent processes just for bookkeeping. Which may be frozen.

[Sam Weinig]

(In reply to Alexey Proskuryakov from comment #4)
> That would mean waking up all WebContent processes just for bookkeeping.
> Which may be frozen.

It seems like for cache invalidation via cookieAcceptPolicyMayHaveChanged you also have to wake up all WebContent processes just for book keeping, no? It that's something to optimize for, seems like having a stardarized delayed updating mechanism might be useful.

[Darin Adler]

Comment on attachment 388104 [details]
Patch

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

>> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:234
>> +}
> 
> What is the value of keeping this sync vs. having the network process proactively update the WebProcesses whenever the policy changes? e.g. cookieAcceptPolicyMayHaveChanged becomes cookieAcceptPolicyChanged(newPolicy).

I was going to ask the same thing.

[Chris Dumez]

Comment on attachment 388104 [details]
Patch

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

>>> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:234
>>> +}
>> 
>> What is the value of keeping this sync vs. having the network process proactively update the WebProcesses whenever the policy changes? e.g. cookieAcceptPolicyMayHaveChanged becomes cookieAcceptPolicyChanged(newPolicy).
> 
> I was going to ask the same thing.

We would need to both send the initial value to the WebContent process (maybe when it gets the IPC connection to the network process) and then keep the value in sync, whenever it changes. It is a bit of extra code complexity. My patch gets most of the perf benefit with a bit less complexity. That's not to say that we cannot do better but I believe my patch was a step in the right direction.

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #4)
> That would mean waking up all WebContent processes just for bookkeeping.
> Which may be frozen.

Sending an IPC to a suspended process does not wake it up, unless you actually explicitly take a process assertion to do so when sending the IPC.

[Sam Weinig]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 388104 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388104&action=review
> 
> >>> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:234
> >>> +}
> >> 
> >> What is the value of keeping this sync vs. having the network process proactively update the WebProcesses whenever the policy changes? e.g. cookieAcceptPolicyMayHaveChanged becomes cookieAcceptPolicyChanged(newPolicy).
> > 
> > I was going to ask the same thing.
> 
> We would need to both send the initial value to the WebContent process
> (maybe when it gets the IPC connection to the network process) and then keep
> the value in sync, whenever it changes. It is a bit of extra code
> complexity. My patch gets most of the perf benefit with a bit less
> complexity. That's not to say that we cannot do better but I believe my
> patch was a step in the right direction.

I generally think getting rid of the sync IPC rather than adding caching, where possible, should be done, even at the cost of a little more complexity (though in this case I don't really think it would be any more complex). The sync IPC tends to come back and bite you at an inopportune time.

Given you already have to do cache invalidation via CookieAcceptPolicyMayHaveChanged, I would expect setting an initial value when getting the network process connection would be all that remains (famous last words on my part there). Probably could add the bit to NetworkProcessConnectionInfo.

[Chris Dumez]

(In reply to Sam Weinig from comment #9)
> (In reply to Chris Dumez from comment #7)
> > Comment on attachment 388104 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=388104&action=review
> > 
> > >>> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:234
> > >>> +}
> > >> 
> > >> What is the value of keeping this sync vs. having the network process proactively update the WebProcesses whenever the policy changes? e.g. cookieAcceptPolicyMayHaveChanged becomes cookieAcceptPolicyChanged(newPolicy).
> > > 
> > > I was going to ask the same thing.
> > 
> > We would need to both send the initial value to the WebContent process
> > (maybe when it gets the IPC connection to the network process) and then keep
> > the value in sync, whenever it changes. It is a bit of extra code
> > complexity. My patch gets most of the perf benefit with a bit less
> > complexity. That's not to say that we cannot do better but I believe my
> > patch was a step in the right direction.
> 
> I generally think getting rid of the sync IPC rather than adding caching,
> where possible, should be done, even at the cost of a little more complexity
> (though in this case I don't really think it would be any more complex). The
> sync IPC tends to come back and bite you at an inopportune time.
> 
> Given you already have to do cache invalidation via
> CookieAcceptPolicyMayHaveChanged, I would expect setting an initial value
> when getting the network process connection would be all that remains
> (famous last words on my part there). Probably could add the bit to
> NetworkProcessConnectionInfo.

Ok, I will look into this.

[Chris Dumez]

Created attachment 388343 [details]
Patch

[Chris Dumez]

Created attachment 388345 [details]
Patch

[Chris Dumez]

Created attachment 388346 [details]
Patch

[Chris Dumez]

Created attachment 388347 [details]
Patch

[Chris Dumez]

Created attachment 388353 [details]
Patch

[Chris Dumez]

Created attachment 388354 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Chris Dumez]

Created attachment 388358 [details]
Patch

[Chris Dumez]

Created attachment 388359 [details]
Patch

[Darin Adler]

Comment on attachment 388359 [details]
Patch

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

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:272
> +    if (policy == CFHTTPCookieStorageAcceptPolicyAlways)
> +        return HTTPCookieAcceptPolicy::AlwaysAccept;
> +    if (policy == CFHTTPCookieStorageAcceptPolicyOnlyFromMainDocumentDomain)
> +        return HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain;
> +    if (policy == CFHTTPCookieStorageAcceptPolicyExclusivelyFromMainDocumentDomain)
> +        return HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain;
> +
> +    return HTTPCookieAcceptPolicy::Never;

Why not use a switch? I guess because it’s not an enum we don’t get that extra compiler checking, but I’d still be tempted to write it that way.

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:464
> +    if (cookieAcceptPolicy == NSHTTPCookieAcceptPolicyAlways)
> +        return HTTPCookieAcceptPolicy::AlwaysAccept;
> +    if (cookieAcceptPolicy == NSHTTPCookieAcceptPolicyOnlyFromMainDocumentDomain)
> +        return HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain;
> +    if (cookieAcceptPolicy == NSHTTPCookieAcceptPolicyExclusivelyFromMainDocumentDomain)
> +        return HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain;
>      END_BLOCK_OBJC_EXCEPTIONS;
> -    return false;
> +
> +    return HTTPCookieAcceptPolicy::Never;

Ditto.

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:252
> +    if (policy == SOUP_COOKIE_JAR_ACCEPT_ALWAYS)
> +        return HTTPCookieAcceptPolicy::AlwaysAccept;
> +    if (policy == SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY)
> +        return HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain;
> +    return HTTPCookieAcceptPolicy::Never;

Ditto.

> Source/WebKit/NetworkProcess/Cookies/WebCookieManager.cpp:158
> +#if PLATFORM(COCOA)
> +    completionHandler(WebCookieManager::platformGetHTTPCookieAcceptPolicy());
> +#else
> +    completionHandler(m_process.defaultStorageSession().cookieAcceptPolicy());
> +#endif

I don’t understand why Cocoa is different here. Maybe it should be obvious but somehow it’s not.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:390
> +    HTTPCookieAcceptPolicy cookieAcceptPolicy = HTTPCookieAcceptPolicy::Never;
> +    if (auto* storage = storageSession(sessionID))
> +        cookieAcceptPolicy = storage->cookieAcceptPolicy();
> +
> +    completionHandler(WTFMove(ipcConnection->second), cookieAcceptPolicy);

Consider:

    auto* storage = storageSession(sessionID);
    completionHandler(WTFMove(ipcConnection->second),
        storage ? storage->cookieAcceptPolicy() : HTTPCookieAcceptPolicy::Never);

Maybe you still like the if better?

> Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm:203
> +static WebCore::HTTPCookieAcceptPolicy toHTTPCookieAcceptPolicy(NSHTTPCookieAcceptPolicy policy)

Seems to duplicate code in NetworkStorageSession::cookieAcceptPolicy. Might be nice to find a way to share these conversion functions.

> Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:84
> +    bool cookiesEnabled();

Maybe const.

[Chris Dumez]

Comment on attachment 388359 [details]
Patch

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

>> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:272
>> +    return HTTPCookieAcceptPolicy::Never;
> 
> Why not use a switch? I guess because it’s not an enum we don’t get that extra compiler checking, but I’d still be tempted to write it that way.

Because it was not an enum I did not use a switch, but I guess you're right, it does not hurt to use an enum, we simply don't get the extra validation.

>> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:464
>> +    return HTTPCookieAcceptPolicy::Never;
> 
> Ditto.

Ok.

>> Source/WebKit/NetworkProcess/Cookies/WebCookieManager.cpp:158
>> +#endif
> 
> I don’t understand why Cocoa is different here. Maybe it should be obvious but somehow it’s not.

I will double check. I don't remember exactly but I think it was me trying to be conservative because the Cocoa implementation was slightly different (used Cocoa directly instead of m_process.defaultStorageSession()). I was worried about constructing the DefaultStorageSession unnecessarily, which may not be an issue.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:390
>> +    completionHandler(WTFMove(ipcConnection->second), cookieAcceptPolicy);
> 
> Consider:
> 
>     auto* storage = storageSession(sessionID);
>     completionHandler(WTFMove(ipcConnection->second),
>         storage ? storage->cookieAcceptPolicy() : HTTPCookieAcceptPolicy::Never);
> 
> Maybe you still like the if better?

I like your proposal, will update.

>> Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm:203
>> +static WebCore::HTTPCookieAcceptPolicy toHTTPCookieAcceptPolicy(NSHTTPCookieAcceptPolicy policy)
> 
> Seems to duplicate code in NetworkStorageSession::cookieAcceptPolicy. Might be nice to find a way to share these conversion functions.

Oh, indeed.

>> Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:84
>> +    bool cookiesEnabled();
> 
> Maybe const.

Ok.

[Chris Dumez]

Created attachment 388426 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 388426 [details]
Patch

Clearing flags on attachment: 388426

Committed r254931: <https://trac.webkit.org/changeset/254931>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58814695>