Bug 206450 - Frequent NetworkConnectionToWebProcess::CookiesEnabled sync IPC when browsing reddit.com
Summary: Frequent NetworkConnectionToWebProcess::CookiesEnabled sync IPC when browsing...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks: 206442
  Show dependency treegraph
 
Reported: 2020-01-17 15:30 PST by Chris Dumez
Modified: 2020-01-22 15:58 PST (History)
21 users (show)

See Also:


Attachments
Patch (11.35 KB, patch)
2020-01-17 15:35 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.96 KB, patch)
2020-01-17 16:00 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (76.66 KB, patch)
2020-01-21 13:49 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (78.85 KB, patch)
2020-01-21 14:08 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (77.90 KB, patch)
2020-01-21 14:10 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (77.90 KB, patch)
2020-01-21 14:13 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (77.91 KB, patch)
2020-01-21 14:39 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (80.40 KB, patch)
2020-01-21 14:53 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (80.94 KB, patch)
2020-01-21 15:01 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (81.27 KB, patch)
2020-01-21 15:13 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (87.77 KB, patch)
2020-01-22 09:11 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-01-17 15:30:57 PST
Frequent NetworkConnectionToWebProcess::CookiesEnabled sync IPC when browsing reddit.com. Happens every time the page script calls navigator.cookieEnabled.
Comment 1 Chris Dumez 2020-01-17 15:35:46 PST
Created attachment 388099 [details]
Patch
Comment 2 Chris Dumez 2020-01-17 16:00:15 PST
Created attachment 388104 [details]
Patch
Comment 3 Sam Weinig 2020-01-18 12:03:19 PST
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).
Comment 4 Alexey Proskuryakov 2020-01-18 20:27:09 PST
That would mean waking up all WebContent processes just for bookkeeping. Which may be frozen.
Comment 5 Sam Weinig 2020-01-19 10:32:52 PST
(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.
Comment 6 Darin Adler 2020-01-20 12:02:46 PST
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.
Comment 7 Chris Dumez 2020-01-21 08:13:30 PST
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.
Comment 8 Chris Dumez 2020-01-21 08:14:16 PST
(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.
Comment 9 Sam Weinig 2020-01-21 08:28:38 PST
(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.
Comment 10 Chris Dumez 2020-01-21 08:39:15 PST
(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.
Comment 11 Chris Dumez 2020-01-21 13:49:06 PST
Created attachment 388343 [details]
Patch
Comment 12 Chris Dumez 2020-01-21 14:08:10 PST
Created attachment 388345 [details]
Patch
Comment 13 Chris Dumez 2020-01-21 14:10:31 PST
Created attachment 388346 [details]
Patch
Comment 14 Chris Dumez 2020-01-21 14:13:14 PST
Created attachment 388347 [details]
Patch
Comment 15 Chris Dumez 2020-01-21 14:39:34 PST
Created attachment 388353 [details]
Patch
Comment 16 Chris Dumez 2020-01-21 14:53:46 PST
Created attachment 388354 [details]
Patch
Comment 17 EWS Watchlist 2020-01-21 14:54:32 PST
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
Comment 18 Chris Dumez 2020-01-21 15:01:52 PST
Created attachment 388358 [details]
Patch
Comment 19 Chris Dumez 2020-01-21 15:13:10 PST
Created attachment 388359 [details]
Patch
Comment 20 Darin Adler 2020-01-21 17:11:37 PST
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.
Comment 21 Chris Dumez 2020-01-22 08:37:42 PST
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.
Comment 22 Chris Dumez 2020-01-22 09:11:45 PST
Created attachment 388426 [details]
Patch
Comment 23 WebKit Commit Bot 2020-01-22 11:31:20 PST
Comment on attachment 388426 [details]
Patch

Clearing flags on attachment: 388426

Committed r254931: <https://trac.webkit.org/changeset/254931>
Comment 24 WebKit Commit Bot 2020-01-22 11:31:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2020-01-22 15:57:41 PST
<rdar://problem/58814695>