Bug 195140 - Move initialHTTPCookieAcceptPolicy to NetworkSession
Summary: Move initialHTTPCookieAcceptPolicy to NetworkSession
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-27 18:20 PST by Michael Catanzaro
Modified: 2019-06-25 12:29 PDT (History)
9 users (show)

See Also:


Attachments
Patch (37.02 KB, patch)
2019-02-27 18:27 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (39.90 KB, patch)
2019-02-27 18:35 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (39.92 KB, patch)
2019-02-27 18:40 PST, Michael Catanzaro
achristensen: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2019-02-27 18:20:27 PST
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.
Comment 1 Michael Catanzaro 2019-02-27 18:26:45 PST
(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
Comment 2 Michael Catanzaro 2019-02-27 18:27:01 PST
Created attachment 363175 [details]
Patch
Comment 3 Michael Catanzaro 2019-02-27 18:29:44 PST
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....
Comment 4 Build Bot 2019-02-27 18:29:47 PST Comment hidden (obsolete)
Comment 5 Build Bot 2019-02-27 18:30:07 PST Comment hidden (obsolete)
Comment 6 Michael Catanzaro 2019-02-27 18:35:14 PST
Created attachment 363176 [details]
Patch
Comment 7 Build Bot 2019-02-27 18:37:23 PST Comment hidden (obsolete)
Comment 8 Michael Catanzaro 2019-02-27 18:40:50 PST
Created attachment 363178 [details]
Patch
Comment 9 Build Bot 2019-02-27 18:42:54 PST Comment hidden (obsolete)
Comment 10 Don Olmstead 2019-03-01 14:20:06 PST
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.
Comment 11 Michael Catanzaro 2019-03-01 16:25:11 PST
Yeah thanks, I might try that in a follow-up.
Comment 12 Daniel Bates 2019-03-03 13:53:21 PST
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.
Comment 13 Daniel Bates 2019-03-03 14:02:19 PST
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.
Comment 14 Michael Catanzaro 2019-03-04 06:57:12 PST
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.
Comment 15 Daniel Bates 2019-03-04 12:11:39 PST
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
Comment 16 Michael Catanzaro 2019-03-04 13:11:17 PST
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.
Comment 17 Michael Catanzaro 2019-03-19 07:57:00 PDT
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.
Comment 18 Alex Christensen 2019-03-20 12:14:28 PDT
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.
Comment 19 Michael Catanzaro 2019-03-20 15:13:06 PDT
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.)
Comment 20 Michael Catanzaro 2019-03-20 15:20:15 PDT
(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....
Comment 21 Michael Catanzaro 2019-03-20 15:21:36 PDT
> (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"
Comment 22 Michael Catanzaro 2019-04-17 12:21:01 PDT
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.
Comment 23 Alex Christensen 2019-06-25 11:15:43 PDT
This bug has become much less important.  Let's come back to it if needed.
Comment 24 Michael Catanzaro 2019-06-25 12:29:33 PDT
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.