Bug 65309 – [Qt] [WK2] Implement a persistent cookie storage.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Luiz Agostini]

The objective is to extend the classes WebCookieManager and WebCookieManagerProxy providing a way for the UI process to request the current cookie list from the web process and for the UI process to reset the current cookie list in use by the web process.

This will enable the UI process to present the full cookie list to the user, and will enable the UI process to be responsible for the persistent storage of the cookies.

[Luiz Agostini]

Created attachment 102244 [details]
work in progress

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Tools/MiniBrowser/qt/CookieManager.h:1:  #ifndef header guard has wrong style, please use: CookieManager_h  [build/header_guard] [5]
Tools/MiniBrowser/qt/CookieManager.h:9:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/MiniBrowser/qt/CookieManager.h:24:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/qt/qwkcontext.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 34 files


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

[WebKit Review Bot]

Comment on attachment 102244 [details]
work in progress

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

[WebKit Review Bot]

Comment on attachment 102244 [details]
work in progress

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

[Rafael Brandao]

I feel like I've never used C/C++ like this before. It looks like functional programming but a bit more complicated to understand, but very interesting anyway. :)

Just one thing before I forget, on "qwkcontext_p.h", you've wrote "Callbak" instead of "Callback". Probably a typo.

Hope to provide more relevant feedback soon.

[Alexey Proskuryakov]

>     // This struct is currently only used to provide more cookies information
>     // to the Web Inspector.

This doesn't seem to be true any more.

My understanding is that for Mac and Windows ports of WebKit2, we tried hard to avoid the need to ever send the list of cookies to another process. Perhaps this functionality shouldn't be needed by any port.

[Luiz Agostini]

(In reply to comment #6)
> >     // This struct is currently only used to provide more cookies information
> >     // to the Web Inspector.
> 
> This doesn't seem to be true any more.
> 
> My understanding is that for Mac and Windows ports of WebKit2, we tried hard to avoid the need to ever send the list of cookies to another process. Perhaps this functionality shouldn't be needed by any port.

We have been discussing about the need of sending cookies from UI to web process and I must say that I am not that sure about it, but the other way around seems to be needed to show to the user the current list of cookies.

How do you plan to deal with cookies considering that we will have more then one web process?

[Alexey Proskuryakov]

FWIW, Safari 5.1 can show the list of cookies in UI.

[Jessie Berlin]

(In reply to comment #7)
> (In reply to comment #6)
> > >     // This struct is currently only used to provide more cookies information
> > >     // to the Web Inspector.
> > 
> > This doesn't seem to be true any more.
> > 
> > My understanding is that for Mac and Windows ports of WebKit2, we tried hard to avoid the need to ever send the list of cookies to another process. Perhaps this functionality shouldn't be needed by any port.
> 
> We have been discussing about the need of sending cookies from UI to web process and I must say that I am not that sure about it, but the other way around seems to be needed to show to the user the current list of cookies.
> 
> How do you plan to deal with cookies considering that we will have more then one web process?

In 5.1, Safari shares the CFURLStorageSession that is used by the UI Process with the Web Process. That means that the same CFHTTPCookieStorage is used by both the UI and Web processes.

Is something similar possible for your port?

[Benjamin Poulain]

> In 5.1, Safari shares the CFURLStorageSession that is used by the UI Process with the Web Process. That means that the same CFHTTPCookieStorage is used by both the UI and Web processes.
> 
> Is something similar possible for your port?

No, not really.
Cookies will not be shared between applications, and currently nothing in our network stack is shared between processes.

Personnally I am not a fan of resetAllCookies() but I think the message GetAllCookies will be necessary for presenting the cookies in the UI.

[Alexis Menard (darktears)]

Created attachment 106333 [details]
Patch

[Noam Rosenthal]

Comment on attachment 106333 [details]
Patch

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

The implementation seems good, though I feel that it should either be renamed or moved to the WebKit2 directory and initialized as a factory.

> Source/WebCore/WebCore.pri:9
> +QT *= network sql

I'm not a fan of this hard dependency of WebCore on QtSQL. If this feature is decoupled enough, can we protect its compilation with 
contains(QT_CONFIG, sql) or something like that?

> Source/WebCore/platform/Cookie.h:42
> +            : expires(0)

Implicit MS-from-epoch :(
But that's been there before your patch.

> Source/WebCore/platform/qt/CookieJarQt.cpp:165
> +    WebKit2SharedCookieJar::shared()->getHostnamesWithCookies(hostnames);
>  }
>  
>  void deleteCookiesForHostname(const String& hostname)
>  {
> -    // FIXME: Not yet implemented
> +    WebKit2SharedCookieJar::shared()->deleteCookiesForHostname(hostname);
>  }
>  
>  void deleteAllCookies()
>  {
> -    // FIXME: Not yet implemented
> +    WebKit2SharedCookieJar::shared()->deleteAllCookies();

WebKit2-specific code inside WebCore? Doesn't look right.
Maybe instead do this factory style, and initialize the factory from WebProcess main? That way the implementation can live inside WebKit2 if it's WebKit2 specific.
btw what's WebKit2-specific about this apart from the name?

> Source/WebCore/platform/qt/CookieJarQt.h:40
> +    void deleteCookiesForHostname(const QString&);

For readability, maybe use String here and not QString, as this is one of the functions called from WebCore.

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:52
> +    jar->setParent(0);

This line is a bit puzzling. Could you add a comment?

[Jocelyn Turcotte]

Comment on attachment 106333 [details]
Patch

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

> Source/WebCore/platform/Cookie.h:75
> +#if PLATFORM(QT)
> +        Cookie(const QNetworkCookie&);
> +        operator QNetworkCookie() const;
> +#endif

Why do you need the Cookie struct? I only see you use QNetworkCookie directly in this patch.

> Source/WebCore/platform/qt/CookieJarQt.cpp:155
> +    WebKit2SharedCookieJar::shared()->getHostnamesWithCookies(hostnames);

Any call to those function will have the side effect of creating the SQLite database under the storage path which by default is empty.
To make it future proof and prevent unwanted wasted resources it would be nice having it created less magically. Would be cool as a factory as No'am suggested.

> Source/WebCore/platform/qt/CookieJarQt.cpp:256
> +bool WebKit2SharedCookieJar::setCookiesFromUrl(const QList<QNetworkCookie>& cookieList, const QUrl& url)
> +{

Maybe it's not worth worrying about it now, but if you have more than one process accessing the same SQLite db at the same time you might run into issues there (same app opened multiple times, or more than one WebProcess). Overall it would be good and free to wrap all updates in this function in a transaction, and it would solve this problem, so it might be worth trying it out.

> Source/WebCore/platform/qt/CookieJarQt.cpp:265
> +    sqlQuery.prepare(QLatin1String("DELETE FROM cookies WHERE domain=:domainvalue"));
> +    sqlQuery.bindValue(QLatin1String(":domainvalue"), url.host());
> +    sqlQuery.exec();
> +
> +    sqlQuery.prepare(QLatin1String("INSERT INTO cookies (cookie, domain) VALUES (:cookievalue, :domainvalue)"));
> +    foreach (const QNetworkCookie &cookie, cookiesForUrl(url)) {

This is going to be run synchronously for each HTTP request, I wonder if this will slow down resource fetching.
It's difficult to judge without benchmarking anyway.

> Source/WebCore/platform/qt/CookieJarQt.cpp:268
> +            sqlQuery.bindValue(QLatin1String(":domainvalue"), url.host());

You should use cookie.domain() here to handle correctly domains like ".google.com" when queried though getHostnamesWithCookies and deleteCookiesForHostname. If you do so you should also use it in the delete SQL query above, but I guess that means you would have to itterate over all cookies in the list in case there is more than one different domain used. I'm not sure what is the best approach there.

> Source/WebCore/platform/qt/CookieJarQt.cpp:283
> +    sqlQuery.prepare(QLatin1String("CREATE TABLE IF NOT EXISTS cookies (id INTEGER PRIMARY KEY AUTOINCREMENT,"
> +                       "cookie BLOB, domain VARCHAR);"));

Might be worth creating an index on "domain".

> Source/WebCore/platform/qt/CookieJarQt.cpp:291
> +    QDir().mkpath(databasePath + QLatin1String(".QtWebKit/"));
> +    const QString dataBaseName = databasePath + QLatin1String(".QtWebKit/cookies.db");

Since you use QDesktopServices::DataLocation by default, I don't think that the intermediate directory should be hidden.
I think cookies.db could also be put directly in cookieStorageDirectory instead.

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:50
> +    WebCore::WebKit2SharedCookieJar* jar = WebCore::sharedCookieJar();
> +    jar->setStoragePath(parameters.cookieStorageDirectory);

It should not be created/used if the UI process didn't set the storage path, or else assert that the storage path isn't empty.

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:66
> +    WebCore::sharedCookieJar()->destroy();

This should be done after you destroy the QNAM, since it still has a reference to it.

[Alexis Menard (darktears)]

(In reply to comment #13)
> (From update of attachment 106333 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106333&action=review
> 
> > Source/WebCore/platform/Cookie.h:75
> > +#if PLATFORM(QT)
> > +        Cookie(const QNetworkCookie&);
> > +        operator QNetworkCookie() const;
> > +#endif
> 
> Why do you need the Cookie struct? I only see you use QNetworkCookie directly in this patch.

Some rest of a next patch.

> 
> > Source/WebCore/platform/qt/CookieJarQt.cpp:155
> > +    WebKit2SharedCookieJar::shared()->getHostnamesWithCookies(hostnames);
> 
> Any call to those function will have the side effect of creating the SQLite database under the storage path which by default is empty.
> To make it future proof and prevent unwanted wasted resources it would be nice having it created less magically. Would be cool as a factory as No'am suggested.

How this use case can happen? In order to have the cookie jar created and this functions called you need to create the WebProcess which then will create the shared object first. But agree for the magic

> 
> > Source/WebCore/platform/qt/CookieJarQt.cpp:256
> > +bool WebKit2SharedCookieJar::setCookiesFromUrl(const QList<QNetworkCookie>& cookieList, const QUrl& url)
> > +{
> 
> Maybe it's not worth worrying about it now, but if you have more than one process accessing the same SQLite db at the same time you might run into issues there (same app opened multiple times, or more than one WebProcess). Overall it would be good and free to wrap all updates in this function in a transaction, and it would solve this problem, so it might be worth trying it out.

SQLite3 handle the concurrency very well. We may have some tweaks if we experience issues but I'd say for now it should be fine.

http://www.sqlite.org/lockingv3.html

> 
> > Source/WebCore/platform/qt/CookieJarQt.cpp:265
> > +    sqlQuery.prepare(QLatin1String("DELETE FROM cookies WHERE domain=:domainvalue"));
> > +    sqlQuery.bindValue(QLatin1String(":domainvalue"), url.host());
> > +    sqlQuery.exec();
> > +
> > +    sqlQuery.prepare(QLatin1String("INSERT INTO cookies (cookie, domain) VALUES (:cookievalue, :domainvalue)"));
> > +    foreach (const QNetworkCookie &cookie, cookiesForUrl(url)) {
> 
> This is going to be run synchronously for each HTTP request, I wonder if this will slow down resource fetching.
> It's difficult to judge without benchmarking anyway.
> 
> > Source/WebCore/platform/qt/CookieJarQt.cpp:268
> > +            sqlQuery.bindValue(QLatin1String(":domainvalue"), url.host());
> 
> You should use cookie.domain() here to handle correctly domains like ".google.com" when queried though getHostnamesWithCookies and deleteCookiesForHostname. If you do so you should also use it in the delete SQL query above, but I guess that means you would have to itterate over all cookies in the list in case there is more than one different domain used. I'm not sure what is the best approach there.

The implementation of setCookiesFromUrl uses the host as comparison. Not sure neither. 

> 
> > Source/WebCore/platform/qt/CookieJarQt.cpp:283
> > +    sqlQuery.prepare(QLatin1String("CREATE TABLE IF NOT EXISTS cookies (id INTEGER PRIMARY KEY AUTOINCREMENT,"
> > +                       "cookie BLOB, domain VARCHAR);"));
> 
> Might be worth creating an index on "domain".
> 
> > Source/WebCore/platform/qt/CookieJarQt.cpp:291
> > +    QDir().mkpath(databasePath + QLatin1String(".QtWebKit/"));
> > +    const QString dataBaseName = databasePath + QLatin1String(".QtWebKit/cookies.db");
> 
> Since you use QDesktopServices::DataLocation by default, I don't think that the intermediate directory should be hidden.
> I think cookies.db could also be put directly in cookieStorageDirectory instead.
> 
> > Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:50
> > +    WebCore::WebKit2SharedCookieJar* jar = WebCore::sharedCookieJar();
> > +    jar->setStoragePath(parameters.cookieStorageDirectory);
> 
> It should not be created/used if the UI process didn't set the storage path, or else assert that the storage path isn't empty.

Will modify

> 
> > Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:66
> > +    WebCore::sharedCookieJar()->destroy();
> 
> This should be done after you destroy the QNAM, since it still has a reference to it.

Agree.

[Alexis Menard (darktears)]

Created attachment 106361 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 106361 [details]
Patch

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

> Source/WebCore/WebCore.pri:9
> +QT *= network sql

QtSql is built by default on Qt, and there is no way to deactivate the module so I think for now it's fine depending on it.

[Alexis Menard (darktears)]

Created attachment 106363 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 106364 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 106364 [details]
Patch

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

> Source/WebCore/platform/qt/CookieJarQt.cpp:255
> +        sqlQuery.bindValue(QLatin1String(":domainvalue"), url.host());

Though about it and you need to use cookie.domain() here or else:
- deleteCookiesForHostname won't work properly when asked to delete cookies for ".google.com", which will be in the list returned by getHostnamesWithCookies if www.google.com created such a cookie (since getHostnamesWithCookies uses cookie.domain()).
- If www.google.com create a cookie for the domain ".google.com", it won't be overwritten in the DB if mail.google.com ask to set the same cookie name later. But it will be overwritten in QNetworkCookieJar's storage.

[Alexis Menard (darktears)]

Created attachment 106427 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 106427 [details]
Patch

I think it looks fine!

[WebKit Review Bot]

Comment on attachment 106427 [details]
Patch

Attachment 106427 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9595652

[Alexis Menard (darktears)]

Comment on attachment 106427 [details]
Patch

cr-mac is sick, the problem has nothing to do with this path, clearing cq-

[Jesus Sanchez-Palencia]

(In reply to comment #20)
> Created an attachment (id=106427) [details]

It still bugs me to see: "+QT *= network sql"
but I guess we have no other option... LGTM.

[Siddharth Mathur]

Thanks for doing this!

- Do we need to call m_database.close() somewhere? 

- setCookiesFromUrl() : is it possible to combine many calls to sqlQuery.exec() with one longer query that adds multiple records? It will very likely be better for performance due to lower SQL engine as well as underlying file-system costs. 

- Also, since cookie handling is a highly exercised area of a browser, it would be nice to write to performance test for SharedCookieJarQt doing insert/read/replace. Here is a sample for QNetworkDiskCache: https://qt.gitorious.org/qt/qt/blobs/4.8/tests/benchmarks/network/access/qnetworkdiskcache/tst_qnetworkdiskcache.cpp

I would probably have saved the cookies to file(s) directly instead of depending on SQL as this is only a 2 column schema, but that's just IMHO. 

Thanks again for taking this up.

[Alexis Menard (darktears)]

(In reply to comment #25)
> Thanks for doing this!
> 
> - Do we need to call m_database.close() somewhere? 

Yep. Good point.

> 
> - setCookiesFromUrl() : is it possible to combine many calls to sqlQuery.exec() with one longer query that adds multiple records? It will very likely be better for performance due to lower SQL engine as well as underlying file-system costs. 
> 

Well I could improve it later. Let's make this land and I can do a separate patch which create a session, batch the queries and commit them. I'm not sure yet if that's doable with QtSql and it's worth it.

> - Also, since cookie handling is a highly exercised area of a browser, it would be nice to write to performance test for SharedCookieJarQt doing insert/read/replace. Here is a sample for QNetworkDiskCache: https://qt.gitorious.org/qt/qt/blobs/4.8/tests/benchmarks/network/access/qnetworkdiskcache/tst_qnetworkdiskcache.cpp

I may add that in a later commit.

> 
> I would probably have saved the cookies to file(s) directly instead of depending on SQL as this is only a 2 column schema, but that's just IMHO.

That will not work as it will be hard to handle concurrency access (which sqlite do fine) and also the fact that the WebProcess can be kill at any time so I'd like to let an underlying framework handles it.

> 
> Thanks again for taking this up.

[Alexis Menard (darktears)]

Created attachment 106624 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 106648 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 106648 [details]
Patch

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

> Source/WebCore/platform/qt/CookieJarQt.cpp:239
> +    m_database.close();

QDatabase's destructor takes care of that, but I guess it doesn't hurt.

[Chang Shu]

It seems to me the patch is missing tests. Would it help to pass some existing layout tests?

[Alexis Menard (darktears)]

(In reply to comment #30)
> It seems to me the patch is missing tests. Would it help to pass some existing layout tests?

The existing layout tests covers the new code. There is one test case that I can't find a way to test is the actual persistence, e.g. log in a website, close the browser and open it again and you should be logged. I don't see how we could do that with the existing layout-tests as it will require to run a http layout tests, make sure the cookie store and re-run the layout tests (but make sure the WebProcess is shutdown). I don't see how this could fit with the way the layout tests are run. We can't also easily write a API tests as it would require an http server to run.

[Chang Shu]

(In reply to comment #31)
> (In reply to comment #30)
> > It seems to me the patch is missing tests. Would it help to pass some existing layout tests?
> 
> The existing layout tests covers the new code. There is one test case that I can't find a way to test is the actual persistence, e.g. log in a website, close the browser and open it again and you should be logged. I don't see how we could do that with the existing layout-tests as it will require to run a http layout tests, make sure the cookie store and re-run the layout tests (but make sure the WebProcess is shutdown). I don't see how this could fit with the way the layout tests are run. We can't also easily write a API tests as it would require an http server to run.

Are you saying the existing layout tests cover most of the code? If so, we should see some updates on the Skipped file. I also suggest you put the above comments in the Changelog so people know some test cases have to be tested manually.

[Alexis Menard (darktears)]

Created attachment 107998 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > It seems to me the patch is missing tests. Would it help to pass some existing layout tests?
> > 
> > The existing layout tests covers the new code. There is one test case that I can't find a way to test is the actual persistence, e.g. log in a website, close the browser and open it again and you should be logged. I don't see how we could do that with the existing layout-tests as it will require to run a http layout tests, make sure the cookie store and re-run the layout tests (but make sure the WebProcess is shutdown). I don't see how this could fit with the way the layout tests are run. We can't also easily write a API tests as it would require an http server to run.
> 
> Are you saying the existing layout tests cover most of the code? If so, we should see some updates on the Skipped file. I also suggest you put the above comments in the Changelog so people know some test cases have to be tested manually.

The tests were already enabled as the previous implementation was storing the cookies in memory. This implementation covers it and it works as I ran them and couldn't find any regressions. The only thing that is not tested and I can't find a way to make it automatic is what I described in the new patch attached : log in to a website that store persistent cookie, close the entire app, open it again and go to the website and make sure you are logged. So this patch will not make things worst, will not remove features, will not enable new tests (as they were working before) but add the persistent storage that we can't test easily. I hope this clarified your concern with the patch.

[Chang Shu]

Comment on attachment 107998 [details]
Patch

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

just have some minor questions. address them if they make sense, otherwise, just cq+. thx

> Source/WebCore/platform/qt/CookieJarQt.cpp:204
> +    QList<QNetworkCookie>::Iterator end = cookies.end();

can just move cookies.end() in while().

> Source/WebCore/platform/qt/CookieJarQt.cpp:244
> +    bool status = QNetworkCookieJar::setCookiesFromUrl(cookieList, url);

do we have to do all the stuff below when status==false?

> Source/WebCore/platform/qt/CookieJarQt.cpp:263
> +    if (!m_database.open()) {

not sure if we have to check the state of m_database every time we use it.

[Alexis Menard (darktears)]

Comment on attachment 107998 [details]
Patch

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

>> Source/WebCore/platform/qt/CookieJarQt.cpp:204
>> +    QList<QNetworkCookie>::Iterator end = cookies.end();
> 
> can just move cookies.end() in while().

It's to avoid re-evaluating the end() for the iterator at each loop. Optimization.

>> Source/WebCore/platform/qt/CookieJarQt.cpp:244

> 
> do we have to do all the stuff below when status==false?

Good point I will do an early return.

>> Source/WebCore/platform/qt/CookieJarQt.cpp:263
>> +    if (!m_database.open()) {
> 
> not sure if we have to check the state of m_database every time we use it.

This method is only called on the constructor so yes when creating the jar is nice to check that we could open the cookie database.

[Chang Shu]

> >> Source/WebCore/platform/qt/CookieJarQt.cpp:263
> >> +    if (!m_database.open()) {
> > 
> > not sure if we have to check the state of m_database every time we use it.
> 
> This method is only called on the constructor so yes when creating the jar is nice to check that we could open the cookie database.

I mean, if database failed to open, all the subsequent db operations may cause problem since the patch does not check the validation of database when using it. But it could be overcautious. One way to test it is you don't call m_database.open() and see if you will get any crash. If operations failed silently, it's ok.

[Alexis Menard (darktears)]

Created attachment 108040 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 108040 [details]
Patch for landing

Clearing flags on attachment: 108040

Committed r95568: <http://trac.webkit.org/changeset/95568>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.