Bug 45455

Created attachment 67029 [details] Patch

Robert, I would likely sr=me this patch, but I always think that making DRT working as close as possible to real world WebKit clients (e.g. a browser) is a good idea. Are you planning on implementing cache policies for DRT at all? It would be a good idea in my option. (Well, since you are moving the tests to the "permanently skipped" section, I would say you are not, but let me ask anyways.) Maybe we should discuss that on webkit-qt mailing lisT?

(In reply to comment #2) > Robert, I would likely sr=me this patch, but I always think that making DRT working as close as possible to real world WebKit clients (e.g. a browser) is a good idea. > > Are you planning on implementing cache policies for DRT at all? It would be a good idea in my option. (Well, since you are moving the tests to the "permanently skipped" section, I would say you are not, but let me ask anyways.) I was going to do exactly that but then I realized that the only thing the layout tests would be validating is my re-implementation of setCookieFromUrl() in DRT. What do you think? Is there a real benefit? Or is it worth it just to get them out of the skipped list?

(In reply to comment #3) > What do you think? Is there a real benefit? Or is it worth it just to get them out of the skipped list? We'd still be testing the code that lets you hook up a cookie policy, no? Unless no, I think we should do it. More coverage + fewer skipped tests: good stuff.

(In reply to comment #4) > (In reply to comment #3) > > What do you think? Is there a real benefit? Or is it worth it just to get them out of the skipped list? > > We'd still be testing the code that lets you hook up a cookie policy, no? > > Unless no, I think we should do it. More coverage + fewer skipped tests: good stuff. Turns out we need to add a QNetworkRequest::FirstPartyForCookies attribute to Qt if clients are to have any hope of blocking third-party cookies without reinventing the world. There's a QtWebKit and a Qt part to that so will open separate bugs for each and make the webkit one block this.

See: http://bugreports.qt.nokia.com/browse/QTBUG-13601 and http://qt.gitorious.org/qt/qt/merge_requests/2473

Created attachment 67457 [details] Patch

Marking for review, but it's dependent on: http://qt.gitorious.org/qt/qt/merge_requests/2473

Attachment 67457 [details] did not build on qt: Build output: http://queues.webkit.org/results/3902448

There is some discussion at http://qt.gitorious.org/qt/qt/merge_requests/2473. The issue is preserving binary compatibility. I need suggestions on alternatives to amending the current API for setCookiesFromUrl() - which is a non-runner. On way is for clients to ensure, in their implementation, that they can access the QNetworkRequest responsible for the call to setCookiesFromUrl() so that they can read the new attribute – that wouldn’t be trivial. Alternatively I could hack QNetworkCookie to make the firstPartyUrl available. Or lastly, introduce a new virtual function that ultimately replaces it. From the comments, that isn’t permissible either. Am I missing any other alternatives?

Jocelyn can you have a look at this?

Are you going to make this compile on Qt? :)

(In reply to comment #12) > Are you going to make this compile on Qt? :) It depends on: http://bugreports.qt.nokia.com/browse/QTBUG-13601 and http://qt.gitorious.org/qt/qt/merge_requests/2473

(In reply to comment #10) Since this whole first-party URL matching logic is pretty much clear and won't be changed by most users, we could also introduce QWebSettings::ThirdPartyCookiesAllowed which would, if false, prevent the call to setCookiesFromUrl in the first place. I would prefer it over adding a separate QMap for first-party URLs in QNetworkCookieJar. The ideal solution would be to pass it in setCookiesFromUrl like you wanted, but since binary compatibility isn't allowing it I think that we need to weight the gain that a complex solution would grant to the user. If we chose this way, it would be nice to know some use cases where the user would need to handle the decision in his subclassed QNetworkCookieJar instead of letting QtWebKit handle the decision.

(In reply to comment #14) > (In reply to comment #10) > > Since this whole first-party URL matching logic is pretty much clear and won't be changed by most users, we could also introduce QWebSettings::ThirdPartyCookiesAllowed which would, if false, prevent the call to setCookiesFromUrl in the first place. > > I would prefer it over adding a separate QMap for first-party URLs in QNetworkCookieJar. The ideal solution would be to pass it in setCookiesFromUrl like you wanted, but since binary compatibility isn't allowing it I think that we need to weight the gain that a complex solution would grant to the user. > If we chose this way, it would be nice to know some use cases where the user would need to handle the decision in his subclassed QNetworkCookieJar instead of letting QtWebKit handle the decision. Thanks Jocelyn - I'll go with that.

Yesterday I looked into this again with a view to making it a QWebSetting and discovered that the major blocker is distinguishing the top-level domain in a url. If you can't do this, you end up blocking cookies from different hosts rather than different registry-controlled domains. Fortunately I discovered Peter Hartmann already done all the hard work by adding a hash table based on the public suffix list to QtNetwork, used privately by QNetworkCookieJar. Since it seems best to keep the policy logic outside QtNetwork (otherwise you end up having to communicate both the policy and the first-party url from QtWebKit via the QNetworkRequest) the most effective way of making use of Peter's TLD table seems to me to be through something like QUrl::topLevelDomain(). This would allow QtWebKit make a proper comparison between urls to establish if they are from the same registry-controlled domain. It would also be a natural fit with QUrl's current API which is able to dissect every other component of the url. The downside is that the hash table has to get included through a wrapper into a QtCore class (ie QUrl) and as a consequence even ends up in the qmake binary, growing it by 100K. This is my first hack at creating the required QUrl::topLevelDomain() and sharing the table between QtNetwork and QtCore: https://gist.github.com/900765 (I've snipped the move of the hash table to a qurltlds_p.h file in that paste so that it isn't thousands of lines long). I've pm'd Peter for his thoughts. If anyone thinks I'm going about it all wrong please shout!

We could have QtCore exporting a function or data structure pointer. QtNetwork would fill it in with the TLD data. That way we don't bloat QtCore. Just an idea..

(In reply to comment #17) > We could have QtCore exporting a function or data structure pointer. QtNetwork would fill it in with the TLD data. > That way we don't bloat QtCore. > Just an idea.. That's a good point, especially if we wan't to continue increasing the isolation of QtNetwork from QtCore. What we need is just a function that returns the TLD from a host/URL if I'm right.

(In reply to comment #17) > We could have QtCore exporting a function or data structure pointer. QtNetwork would fill it in with the TLD data. > That way we don't bloat QtCore. > Just an idea.. OK, so to be sure I'm clear: you're suggesting an exported public API such as QTopLevelDomainDB that contains methods for passing urls and strings and getting back the TLDs or whatever other info might be needed from the public suffix database? Please let me know if I've misunderstood, as I'm not sure I've grokked what you're suggesting by 'QtCore exporting a function or data structure pointer'. I can't see how QtCore could export a function that QtNetwork could populate without making QtCore depend on QtNetwork. Which I thought would be a bad thing.

> OK, so to be sure I'm clear: you're suggesting an exported public API such as QTopLevelDomainDB that contains methods for passing urls and strings and getting back the TLDs or whatever other info might be needed from the public suffix database? > > Please let me know if I've misunderstood, as I'm not sure I've grokked what you're suggesting by 'QtCore exporting a function or data structure pointer'. I can't see how QtCore could export a function that QtNetwork could populate without making QtCore depend on QtNetwork. Which I thought would be a bad thing. I think we should just move the TLD code over to Qt Core (which will grow it 100K), and #ifdef it out for the bootstrap mode so it does not end up in qmake, then we can add the functionality in QUrl, as Robert suggested.

(In reply to comment #20) > > OK, so to be sure I'm clear: you're suggesting an exported public API such as QTopLevelDomainDB that contains methods for passing urls and strings and getting back the TLDs or whatever other info might be needed from the public suffix database? > > > > Please let me know if I've misunderstood, as I'm not sure I've grokked what you're suggesting by 'QtCore exporting a function or data structure pointer'. I can't see how QtCore could export a function that QtNetwork could populate without making QtCore depend on QtNetwork. Which I thought would be a bad thing. > > I think we should just move the TLD code over to Qt Core (which will grow it 100K), and #ifdef it out for the bootstrap mode so it does not end up in qmake, then we can add the functionality in QUrl, as Robert suggested. Thanks Peter - I have a patch for Qt, but gitorious is erroring out when I create merge requests. I'll post it here for now. Might be easier to review/comment line by line that way anyway.

Created attachment 90407 [details] Qt Patch

Created attachment 90412 [details] Qt Patch

Created attachment 91116 [details] Patch

(In reply to comment #21) > > Thanks Peter - I have a patch for Qt, but gitorious is erroring out when I create merge requests. I'll post it here for now. Might be easier to review/comment line by line that way anyway. Merge request for Qt at: https://qt.gitorious.org/qt/qt/merge_requests/1205

Comment on attachment 91116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91116&action=review > Source/WebCore/platform/qt/ThirdPartyCookiesQt.cpp:37 > +static QNetworkCookieJar *cookieJar(QObject *originatingFrame) Style, * on the left, there's a few of these.

Created attachment 91538 [details] Patch

The Qt part of this is in master now (thanks Peter!): http://bugreports.qt.nokia.com/browse/QTBUG-13601 http://gitorious.org/qt/qt/merge_requests/1205 Would someone care to review before I rebase?

Created attachment 95268 [details] Patch

Comment on attachment 95268 [details] Patch Attachment 95268 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8743569

Comment on attachment 95268 [details] Patch Attachment 95268 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8743574 New failing tests: http/tests/cookies/strict-third-party-cookie-blocking.html

Created attachment 95273 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

The Qt part of this patch is in the 4.8 branch. Peter, which version of Qt should I compile-guard against? 4.8.0 ? 4.8.X ?

Created attachment 95463 [details] Patch

(In reply to comment #33) > The Qt part of this patch is in the 4.8 branch. > > Peter, which version of Qt should I compile-guard against? 4.8.0 ? 4.8.X ? I would just use the 4.8 branch from http://qt.gitorious.org/qt/qt/commits/4.8 . I have no idea what the Webkit bot uses, though...

Created attachment 97701 [details] Patch

Comment on attachment 97701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97701&action=review Should we change the default to AllowThirdPartyWithExistingCookies and document it? Do you know if there is a security reason justifying the behavior of Safari? > Source/WebCore/platform/qt/ThirdPartyCookiesQt.cpp:61 > + firstPartyDomain.remove(firstPartyDomain.length() - firstPartyTLD.length(), firstPartyTLD.length()); > + firstPartyDomain.prepend(QLatin1Char('.')); It would be nice to have an inline function to do this. That would make the code naturally self documented. > Source/WebCore/platform/qt/ThirdPartyCookiesQt.h:24 > +#include <QNetworkCookieJar> > +#include <QUrl> You don't need those includes. > Source/WebCore/platform/qt/ThirdPartyCookiesQt.h:29 > +bool thirdPartyCookiePolicyPermits(QNetworkCookieJar* , const QUrl&, const QUrl& firstPartyUrl); > +bool thirdPartyCookiePolicyPermits(QObject* originatingFrame, const QUrl&, const QUrl& firstPartyUrl); QNetworkCookieJar is also a QObject. This is ok but I think that is slightly more confusing than it should be. I think -thirdPartyCookiePolicyPermits should take a QWebFrame, and the cast could be done before -alternatively, thirdPartyCookiePolicyPermits could be renamed to thirdPartyCookiePolicyPermitsForFrame() Just to make sure they are never mixed by accident. > Source/WebKit/qt/Api/qwebsettings.cpp:395 > + \since 4.8 Probably not. Do we do since QtWebKit 2.3? I guess we can add \since 4.8 if the patch is integrated. > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1069 > +bool DumpRenderTreeSupportQt::thirdPartyCookiePolicy(QNetworkCookieJar* jar, const QUrl& url, const QUrl& firstPartyUrl) I don't like the name of the function thirdPartyCookiePolicy(), it returns a bool, the name suggest it return an enum with the policy. > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:92 > +#if QT_VERSION >= 0x040800 Why not >= QT_VERSION_CHECK(4, 8, 0)? > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2936 > +#if QT_VERSION >= 0x040800 Why not >= QT_VERSION_CHECK(4, 8, 0)?

Comment on attachment 97701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97701&action=review > Source/WebCore/platform/qt/ThirdPartyCookiesQt.cpp:42 > + QWebFrame* frame = qobject_cast<QWebFrame*>(originatingFrame); > + if (!frame) If frame is null here, it is a programming mistake somewhere before this call. I don't mind the check to avoid a crash, but I think an ASSERT(frame) would be nice as well.

(In reply to comment #37) > (From update of attachment 97701 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97701&action=review > > Should we change the default to AllowThirdPartyWithExistingCookies and document it? Do you know if there is a security reason justifying the behavior of Safari? You mean to justify selecting this as their default mode for blocking cookies? It's more a usability reason: https://bugs.webkit.org/show_bug.cgi?id=35824#c33 I think the QtWebKit default should be to allow them, and let clients decide on their own default. > > Source/WebKit/qt/Api/qwebsettings.cpp:395 > > + \since 4.8 > > Probably not. > Do we do since QtWebKit 2.3? > I guess we can add \since 4.8 if the patch is integrated. Updated it to QtWebKit 2.3 > > > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:92 > > +#if QT_VERSION >= 0x040800 > > Why not >= QT_VERSION_CHECK(4, 8, 0)? > For some reason the unit test doesn't understand QT_VERSION_CHECK. Couldn't figure out why.

Comment on attachment 97701 [details] Patch Attachment 97701 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8909446

Committed r90341: <http://trac.webkit.org/changeset/90341>

Committed r90343: <http://trac.webkit.org/changeset/90343>

Reopen, because platform/qt/http/tests/cookies/strict-third-party-cookie-blocking.html still fails with Qt 4.8 and Qt5 too (and it is in platform/qt/Skipped list): +CONSOLE MESSAGE: line 5: ReferenceError: Can't find variable: resetCookies +CONSOLE MESSAGE: line 79: TypeError: 'undefined' is not an object (evaluating 'functions.length')

(In reply to comment #43) > Reopen, because platform/qt/http/tests/cookies/strict-third-party-cookie-blocking.html still fails with Qt 4.8 and Qt5 too (and it is in platform/qt/Skipped list): > > +CONSOLE MESSAGE: line 5: ReferenceError: Can't find variable: resetCookies > +CONSOLE MESSAGE: line 79: TypeError: 'undefined' is not an object (evaluating 'functions.length') It looks like platform/ tests are no longer run from the non-platform-specific directories. I'll update the test.

(In reply to comment #44) > (In reply to comment #43) > > Reopen, because platform/qt/http/tests/cookies/strict-third-party-cookie-blocking.html still fails with Qt 4.8 and Qt5 too (and it is in platform/qt/Skipped list): > > > > +CONSOLE MESSAGE: line 5: ReferenceError: Can't find variable: resetCookies > > +CONSOLE MESSAGE: line 79: TypeError: 'undefined' is not an object (evaluating 'functions.length') > > It looks like platform/ tests are no longer run from the non-platform-specific directories. I'll update the test. The bug is still valid.

still valid bug

Created attachment 177251 [details] Patch

Comment on attachment 177251 [details] Patch LGTM, r=me. Please remove qt/http/tests/cookies/strict-third-party-cookie-blocking.html from qt/TestExpectations when you lands the fix.

Committed r137280: <http://trac.webkit.org/changeset/137280>

(In reply to comment #49) > Committed r137280: <http://trac.webkit.org/changeset/137280> The test still fails with the following error message: --- /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/platform/qt/http/tests/cookies/strict-third-party-cookie-blocking-expected.txt +++ /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/platform/qt/http/tests/cookies/strict-third-party-cookie-blocking-actual.txt @@ -1,3 +1,4 @@ +ALERT: Attempt to clear http://:8000/cookies/resources/cookie-utility.php?queryfunction=deleteCookies cookies might have failed. Test results might be off from here on out. (Error: NETWORK_ERR: XMLHttpRequest Exception 101) ALERT: ALERT: Allowing all cookies @@ -44,6 +45,7 @@ ALERT: http://localhost:8000/cookies/resources/cookie-utility.php?queryfunction=setFooAndBarCookie ALERT: XHR response - Set the foo and bar cookies ALERT: Test stage 10 document.cookie is: +ALERT: Attempt to clear http://:8000/cookies/resources/cookie-utility.php?queryfunction=deleteCookies cookies might have failed. Test results might be off from here on out. (Error: NETWORK_ERR: XMLHttpRequest Exception 101) Be default, Safari only blocks third-party cookies if the third-party has no cookies already set. This test is a stricter version of third-party-cookie-relaxing.html that expects third-party cookies to get blocked all the time. Could you check and fix it please?

I skipped it again to paint the bot green. Please unskip it with the proper fix.