Move initialHTTPCookieAcceptPolicy to NetworkSession

• Component: WebKit2
• Status: RESOLVED LATER

[Michael Catanzaro]

Hey Alex, this is part two in removing libsoup-specific members from NetworkProcessCreationParameters. Here I move initialHTTPAcceptPolicy from NetworkProcess to NetworkSession. Alex, this one could benefit from more review than usual because there were a few different ways to do this. I decided to make this variable cross-platform, because I think setting the cookie accept policy before the first web process is created is broken on non-libsoup ports without this. Then, accept policy was already tied to SessionID in the UI process, but in the network process it was global. I was only able to completely change this for non-Cocoa ports, because Cocoa has the sharedHTTPCookieStorage. So it's still global, but now only WebCookieManagerMac.cpp knows this, and everywhere else uses SessionID. We could alternatively get rid of SessionID in the UI process and just leave it as global. (We could also keep initialHTTPAcceptPolicy libsoup-specific, but it really doesn't need to be.)

I also tried hard to change it from legacy enum to enum class, but got thwarted by too many errors. We could try that again in another patch.

Oh, and for good measure I change libsoup from using HTTPCookieAcceptPolicyOnlyFromMainDocumentDomain to using HTTPCookieAcceptPolicyExclusivelyFromMainDocumentDomain, to resolve bug #193458. I could split that out if desired, but it's not much code compared to the rest of the patch.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> So it's still global, but now only
> WebCookieManagerMac.cpp knows this, and everywhere else uses SessionID.

Also: WebCookieManagerProxy::persistHTTPCookieAcceptPolicy in the UI process

[Michael Catanzaro]

Created attachment 363175 [details]
Patch

[Michael Catanzaro]

Comment on attachment 363175 [details]
Patch

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

> Source/WebKit/Shared/HTTPCookieAcceptPolicy.h:44
> +#if USE(SOUP) || USE(CURL)
> +const HTTPCookieAcceptPolicy defaultHTTPCookieAcceptPolicy = HTTPCookieAcceptPolicyExclusivelyFromMainDocumentDomain;
> +#else
> +const HTTPCookieAcceptPolicy defaultHTTPCookieAcceptPolicy = HTTPCookieAcceptPolicyAlways;
> +#endif

Oh I wasn't sure about this either. I guess EWS will complain if this is the wrong default for Cocoa....

[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

[EWS Watchlist]

Attachment 363175 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:241:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:266:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Created attachment 363176 [details]
Patch

[EWS Watchlist]

Attachment 363176 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:241:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:266:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Created attachment 363178 [details]
Patch

[EWS Watchlist]

Attachment 363178 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:241:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:266:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Comment on attachment 363178 [details]
Patch

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

> Source/WebKit/Shared/HTTPCookieAcceptPolicy.h:38
>  enum {
>      HTTPCookieAcceptPolicyAlways = 0,
>      HTTPCookieAcceptPolicyNever = 1,
> +    // Misnamed, this means no third-party cookies except from domains that have previously set cookies.
>      HTTPCookieAcceptPolicyOnlyFromMainDocumentDomain = 2,
> +    // No third-party cookies at all.
>      HTTPCookieAcceptPolicyExclusivelyFromMainDocumentDomain = 3,
>  };
>  typedef unsigned HTTPCookieAcceptPolicy;

Wanted to mention https://trac.webkit.org/changeset/242281/webkit which converts to an enum class. I remember you saying that you were having problems with that.

[Michael Catanzaro]

Yeah thanks, I might try that in a follow-up.

[Daniel Bates]

Comment on attachment 363178 [details]
Patch

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

> Source/WebKit/NetworkProcess/Cookies/soup/WebCookieManagerSoup.cpp:73
> +        switch (soup_cookie_jar_get_accept_policy(session->cookieStorage())) {
> +        case SOUP_COOKIE_JAR_ACCEPT_ALWAYS:
> +            return HTTPCookieAcceptPolicyAlways;
> +        case SOUP_COOKIE_JAR_ACCEPT_NEVER:
> +            return HTTPCookieAcceptPolicyNever;
> +        case SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY:
> +            return HTTPCookieAcceptPolicyExclusivelyFromMainDocumentDomain;
> +        }
> +        ASSERT_NOT_REACHED();

Ok as-is. Could move this code into a static non-member function to make this code prettier.

> Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.cpp:51
> +        , defaultHTTPCookieAcceptPolicy, { }, { }, false

Not your problem, but this kind of aggregate initializer usage is what gives uniform initializer syntax (UIS) a bad name. UIS should be used to improve readability, but this initializer is completely incomprehensible. So many ways to improve this code..... I am leaning towards stack allocate the object in the function and set each struct member on its own line (this will be just as performant because any non-dumb compiler will move local to return AND elide the move <— RVO QED). That’s the best in my opinion in terms of readability (don’t believe me? ask I’ll give you a longer argument). A cop out, but improvement nonetheless would be to mention the class name before the UIS braces and add inline comments or local variables for the Boolean. Yeah, as I type this “cop out” I can’t say it with a straight face. stack allocate or walk away.

[Daniel Bates]

Comment on attachment 363178 [details]
Patch

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

>> Source/WebKit/NetworkProcess/Cookies/soup/WebCookieManagerSoup.cpp:73
>> +        switch (soup_cookie_jar_get_accept_policy(session->cookieStorage())) {
>> +        case SOUP_COOKIE_JAR_ACCEPT_ALWAYS:
>> +            return HTTPCookieAcceptPolicyAlways;
>> +        case SOUP_COOKIE_JAR_ACCEPT_NEVER:
>> +            return HTTPCookieAcceptPolicyNever;
>> +        case SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY:
>> +            return HTTPCookieAcceptPolicyExclusivelyFromMainDocumentDomain;
>> +        }
>> +        ASSERT_NOT_REACHED();
> 
> Ok as-is. Could move this code into a static non-member function to make this code prettier.

Ok as-is. Could move this code into a static non-member function to make this code prettier.

> Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.cpp:199
> +        , WTFMove(initialHTTPCookieAcceptPolicy)

Again, not your problem, No love went into this code, but love could be given to it :) struct allocate and return the struct would work. Haven’t read the code, but even better would be to decode directly into struct. Oh well, ship may have sailed for that one.

[Michael Catanzaro]

Comment on attachment 363178 [details]
Patch

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

I agree the struct initialization in NetworkSessionCreationParameters is gnarly, and could stand to be improved in a separate patch.

>>> Source/WebKit/NetworkProcess/Cookies/soup/WebCookieManagerSoup.cpp:73
>>> +        ASSERT_NOT_REACHED();
>> 
>> Ok as-is. Could move this code into a static non-member function to make this code prettier.
> 
> Ok as-is. Could move this code into a static non-member function to make this code prettier.

This should remain a member function because it accesses a member variable (the NetworkProcess). To turn it into a non-member function, we'd need to pass the NetworkProcess as a parameter, which doesn't seem worth it.

[Daniel Bates]

Comment on attachment 363178 [details]
Patch

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

>>>> Source/WebKit/NetworkProcess/Cookies/soup/WebCookieManagerSoup.cpp:73
>>>> +        ASSERT_NOT_REACHED();
>>> 
>>> Ok as-is. Could move this code into a static non-member function to make this code prettier.
>> 
>> Ok as-is. Could move this code into a static non-member function to make this code prettier.
> 
> This should remain a member function because it accesses a member variable (the NetworkProcess). To turn it into a non-member function, we'd need to pass the NetworkProcess as a parameter, which doesn't seem worth it.

I’m talking about the switch. The switch block can be extracted pramrterized by its condition

[Michael Catanzaro]

Comment on attachment 363178 [details]
Patch

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

>>>>> Source/WebKit/NetworkProcess/Cookies/soup/WebCookieManagerSoup.cpp:73
>>>>> +        ASSERT_NOT_REACHED();
>>>> 
>>>> Ok as-is. Could move this code into a static non-member function to make this code prettier.
>>> 
>>> Ok as-is. Could move this code into a static non-member function to make this code prettier.
>> 
>> This should remain a member function because it accesses a member variable (the NetworkProcess). To turn it into a non-member function, we'd need to pass the NetworkProcess as a parameter, which doesn't seem worth it.
> 
> I’m talking about the switch. The switch block can be extracted pramrterized by its condition

Well it could, but I don't think that's necessarily going to be easier to read. This function is little more than that switch block already.

[Michael Catanzaro]

Alex, this NetworkProcess -> NetworkSession work was requested by you; do you want to review it? This is the hardest part of the work, since we have to decide how to handle initialHTTPCookieAcceptPolicy; the remaining parameters should be easier.

[Alex Christensen]

Comment on attachment 363178 [details]
Patch

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

It is not obvious that this patch does not change behavior on Cocoa platforms.  I think this patch should be redone in a simpler way that is clear it only affects lib soup.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:75
> +    networkProcess.supplement<WebCookieManager>()->setHTTPCookieAcceptPolicy(m_sessionID, parameters.initialHTTPCookieAcceptPolicy, OptionalCallbackID());

This seems like it's in the wrong place.

[Michael Catanzaro]

Comment on attachment 363178 [details]
Patch

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

I can make initialHTTPCookieAcceptPolicy soup-specific, sure. But understand that the current patch is actually *not* soup-specific, it's really a behavior change for Mac. Previously, I think an attempt to set the cookie accept policy before the network process is created would fail. Now it should work. And that's when apps are most likely to want to use this setting, so I figured you would want it too.  (For Cocoa, it will affect every NetworkSession, though.)

Now, what I can't make soup-specific are all the cross-platform functions for setting cookie accept policy. Not sure how I'll handle this. Currently those are NetworkSession-specific in the UI process, but global in the network process; i.e. all the NetworkSession parameters in the UI process are actually a lie. So the status quo there is pretty poor. Any preferences for how to handle that?

I actually wonder if perhaps we should leave this as network process global after all, simply because Cocoa ports have the SharedCookieStorage, and so any attempt to make this NetworkSession-specific is going to be a lie there. That would simplify everything a lot, but it runs against your goal of reducing network process globals.

>> Source/WebKit/NetworkProcess/NetworkSession.cpp:75
>> +    networkProcess.supplement<WebCookieManager>()->setHTTPCookieAcceptPolicy(m_sessionID, parameters.initialHTTPCookieAcceptPolicy, OptionalCallbackID());
> 
> This seems like it's in the wrong place.

I think it's the right place; why do you think it's not? It was in platformInitializeNetworkProcess before. We don't have an initializeNetworkSession function, so constructor seems like the best place.

(Of course, if you want it soup-specific, I'll move it to NetworkSessionSoup.cpp.)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #19)
> I actually wonder if perhaps we should leave this as network process global
> after all, simply because Cocoa ports have the SharedCookieStorage, and so
> any attempt to make this NetworkSession-specific is going to be a lie there.
> That would simplify everything a lot, but it runs against your goal of
> reducing network process globals.

Well actually, I think that's the answer right there. If the cross-platform HTTPCookieAcceptPolicy functions are network process globals, then InitialHTTTPCookieAcceptPolicy should be too. If they're NetworkSession-specific, then InitialHTTPCookieAcceptPolicy should follow. The inconsistent status quo is problematic and makes it difficult to decide what behavior is desirable:

(In reply to Michael Catanzaro from comment #19)
> Currently those are NetworkSession-specific in the UI process, but global in the
> network process; i.e. all the NetworkSession parameters in the UI process
> are actually a lie.

Most of my patch here is dedicated to cleaning that up and making it really NetworkSession-specific everywhere, instead of only in the UI process, (at least all the way until we get to Cocoa's SharedCookieStorage, which ruins things there). If you don't want those changes, then we should remove the SessionID parameters from the UI process functions to reflect that it's really global in the network process. OK?

Decisions, decisions....

[Michael Catanzaro]

> (In reply to Michael Catanzaro from comment #19)
> i.e. all the NetworkSession parameters in the UI process
> are actually a lie.

I meant "SessionID parameters"

[Michael Catanzaro]

Alex, if you want me to keep working on moving soup stuff from NetworkProcessCreationParameters to NetworkSessionCreationParameters, I need to know how you want me to handle this.

[Alex Christensen]

This bug has become much less important.  Let's come back to it if needed.

[Michael Catanzaro]

Fair enough!

I'm going to suggest removing the sessionID parameters from the UI process side of this, then, since it's not hooked up on the network process side and it's a weird state to leave trunk in. Happy to cook a patch if desired.