Bug 104370 – [GTK] Cookies' storage path and policy are not set when the WebProcess is killed and relaunched

• Component: WebKit2
• Status: RESOLVED FIXED

[Joaquim Rocha]

The cookies' storage path and policy are not set when the WebProcess is killed and relaunched on Epiphany (and possibly other ports using Soup).

Steps to reproduce the storage path one on Epiphany:
1) Open Epiphany log into some website;
2) Open the Personal Data dialog and check out this website's cookie and eventually others'.
3) Kill the webprocess, reload that website and check out the cookies list in Personal Data.

Expected:
The user is still logged in in the website and the same list of cookies is observed before.

Outcome:
The user is not logged in and no cookies are found in that list.

I will upload a patch to fix this soon.

[Joaquim Rocha]

Forgot to add the steps to reproduce the policy issue:

1) Open Epiphany, go to Personal Data and clear all cookies.
2) Go to Preferences > Privacy and set it to never accept cookies.
3) Access website that has cookies e.g. google.com, open Personal Data again and verify there is no Google cookie
4) Kill the WebProcess and reload google.com
5) Open Personal Data and verify that there's now a Google cookie...

[Joaquim Rocha]

Make [gtk] tag capital case.

[Joaquim Rocha]

Created attachment 178222 [details]
Patch

[Joaquim Rocha]

Comment on attachment 178222 [details]
Patch

Set cq?

[Carlos Garcia Campos]

Comment on attachment 178222 [details]
Patch

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

I've noticed that WIN has WebContext::setInitialHTTPCookieAcceptPolicy() maybe we could reuse that and save the values in the context instead of the cookies proxy.

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:83
> +    encoder << cookieAcceptPolicy;

I think you should use encoder.encodeEnum() for enums.

> Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:140
> +#if USE(SOUP)
> +    setCookieAcceptPolicy(policy);
> +#endif

You can simply do m_cookieAcceptPolicy = policy;

> Source/WebKit2/UIProcess/WebCookieManagerProxy.h:75
> +    void setCookieAcceptPolicy(const HTTPCookieAcceptPolicy&);

This method is confusing, because there's setHTTPCookieAcceptPolicy and setCookieAcceptPolicy, but it's not needed it at all, since we don't expect it to be used by others.

> Source/WebKit2/UIProcess/WebCookieManagerProxy.h:101
> +    HTTPCookieAcceptPolicy m_cookieAcceptPolicy;
> +    pair<String, uint32_t> m_cookiePersistentStorage;

These should be initialized in the constructor.

> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:49
> +    const pair<String, uint32_t>& storage = m_cookieManagerProxy->cookiePersistentStorage();
> +    parameters.cookiePersistentStoragePath = storage.first;
> +    parameters.cookiePersistentStorageType = storage.second;

Maybe this could be simpler if we had a method returning two parameters instead of pair? something like:

m_cookieManagerProxy->getCookiePersistentStorage(parameters.cookiePersistentStoragePath, parameters.cookiePersistentStorageType);

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:93
> +    const pair<String, uint32_t>& storage = m_cookieManagerProxy->cookiePersistentStorage();
> +    parameters.cookiePersistentStoragePath = storage.first;
> +    parameters.cookiePersistentStorageType = storage.second;

Ditto.

> Source/WebKit2/UIProcess/soup/WebCookieManagerProxySoup.cpp:54
> +const pair<String, uint32_t>& WebCookieManagerProxy::cookiePersistentStorage() const
> +{
> +    return m_cookiePersistentStorage;
> +}
> +
> +void WebCookieManagerProxy::setCookieAcceptPolicy(const HTTPCookieAcceptPolicy& policy)
> +{
> +    m_cookieAcceptPolicy = policy;
> +}
> +
> +const HTTPCookieAcceptPolicy& WebCookieManagerProxy::cookieAcceptPolicy() const
> +{
> +    return m_cookieAcceptPolicy;

These could be in the header.

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:173
> +    WebCookieManager::shared().setCookiePersistentStorage(parameters.cookiePersistentStoragePath, parameters.cookiePersistentStorageType);

You should check cookiePersistentStoragePath is not empty before calling setCookiePersistentStorage(), as it will happen the first time the web process is launched.

[Joaquim Rocha]

Created attachment 178507 [details]
Patch

[Early Warning System Bot]

Comment on attachment 178507 [details]
Patch

Attachment 178507 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15227575

[Build Bot]

Comment on attachment 178507 [details]
Patch

Attachment 178507 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15242232

[EFL EWS Bot]

Comment on attachment 178507 [details]
Patch

Attachment 178507 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15222679

[Joaquim Rocha]

Created attachment 178513 [details]
Patch

[EFL EWS Bot]

Comment on attachment 178513 [details]
Patch

Attachment 178513 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15239436

[Joaquim Rocha]

Created attachment 178524 [details]
Patch

[Joaquim Rocha]

Created attachment 178813 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 178813 [details]
Patch

Looks great, thanks!

[WebKit Review Bot]

Comment on attachment 178813 [details]
Patch

Clearing flags on attachment: 178813

Committed r137432: <http://trac.webkit.org/changeset/137432>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.