Bug 42578

Tests skipped by http://trac.webkit.org/changeset/63679.

Created attachment 62039 [details] Proposed patch

I cc-ed Kenneth, AFAIK he reviewed a bunch of notification related patches before.

With this patch http/tests/xmlhttprequest/null-auth.php still fails for me. (tested with r63733)

Comment on attachment 62039 [details] Proposed patch Clearing flags on attachment: 62039 Committed r63738: <http://trac.webkit.org/changeset/63738>

All reviewed patches have been landed. Closing bug.

When I removed canvas tests from Skipped list, 2 test still fail: --- /tmp/layout-test-results/http/tests/xmlhttprequest/authorization-header-expected.txt 2010-07-20 07:49:41.106304973 -0700 +++ /tmp/layout-test-results/http/tests/xmlhttprequest/authorization-header-actual.txt 2010-07-20 07:49:41.106304973 -0700 @@ -1,3 +1,3 @@ Test that Authorization header can be set via setRequestHeader. -PASS +FAIL. Unexpected response: Authentication canceled --- /tmp/layout-test-results/http/tests/xmlhttprequest/null-auth-expected.txt 2010-07-20 07:49:45.526332169 -0700 +++ /tmp/layout-test-results/http/tests/xmlhttprequest/null-auth-actual.txt 2010-07-20 07:49:45.526332169 -0700 @@ -2,4 +2,4 @@ No auth tokens should be sent with this request. -No authentication +User: , password: .

Trunk worked correctly, but after http://trac.webkit.org/changeset/63809 http/tests/xmlhttprequest/null-auth.php fails. :( Unskipping canvas tests resolved it: http://trac.webkit.org/changeset/63818 It is the strangest sideeffect bug what I have ever seen.

r63738 solved the "DESKTOP NOTIFICATION CLOSED: New E-mail" problem, now "only" http/tests/xmlhttprequest/authorization-header.html and http/tests/xmlhttprequest/null-auth.php fail when we are unlucky. old-run-webkit-tests restart DRT after 1000 tests, if you add/remove/skip/unskip a test, the restart position will be shifted. If you are lucky, the tests pass. But if you aren't, the tests fail. :( I can reproduced the fails with this minimalist set of tests: (If I remove one test, all of them pass.) http/tests/navigation/redirect302-basic.html http/tests/navigation/redirect302-frames.html http/tests/navigation/redirect302-goback.html http/tests/navigation/redirect302-subframeload.html http/tests/navigation/redirect-cycle.html http/tests/navigation/redirect-load-no-form-restoration.html http/tests/navigation/redirect-preserves-referrer.html http/tests/navigation/relativeanchor-basic.html http/tests/navigation/relativeanchor-frames.html http/tests/navigation/relativeanchor-goback.html http/tests/navigation/reload-subframe-frame.html http/tests/navigation/reload-subframe-iframe.html http/tests/navigation/replacestate-updates-referrer.html http/tests/navigation/restore-form-state-https.html http/tests/navigation/slowmetaredirect-basic.html http/tests/navigation/slowtimerredirect-basic.html http/tests/navigation/success200-basic.html http/tests/navigation/success200-goback.html http/tests/navigation/success200-loadsame.html http/tests/navigation/success200-reload.html http/tests/navigation/timerredirect-basic.html http/tests/navigation/timerredirect-frames.html http/tests/navigation/window-open-adds-history-item2.html http/tests/navigation/window-open-adds-history-item.html http/tests/notifications/icon-exists-cancel.html http/tests/notifications/icon-exists.html http/tests/notifications/icon-requires-auth.html http/tests/xmlhttprequest/authorization-header.html http/tests/xmlhttprequest/null-auth.php

I skipped http/tests/xmlhttprequest/authorization-header.html and http/tests/xmlhttprequest/null-auth.php , because http/tests/xmlhttprequest/null-auth.php fails after r64034 again: http://trac.webkit.org/changeset/64038

Created attachment 71072 [details] Patch

We need to reset QNetworkAccessManager for every new layout test, otherwise the http authenticators from one test are cached and used in the next. This happens most in the xmlhttprequest layout tests. Since Qt does not allow you to set a new QNAM once a QWebPage has used the original one we have to delete the old QWebPage and create a new one for each layout test. Ossy, can you test this patch offline first? Want to be sure it doesn't clobber anything important!

Comment on attachment 71072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71072&action=review > WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:509 > + // create our primary testing page/view. > + if (isGraphicsBased()) { > + m_page = new WebPage(static_cast<QWebView*>(m_mainView), this); > + static_cast<QWebView*>(m_mainView)->setPage(m_page); > + } else { > + m_page = new WebPage(static_cast<QWebView*>(m_mainView), this); > + static_cast<QWebView*>(m_mainView)->setPage(m_page); > + } The two paths are the same (copy-paste). I think the test for isGraphicsBased() can be omitted here. LGTM otherwise. If Ossy hasn't the time to check this on the bot environment, I'll do it tomorrow.

(In reply to comment #13) > (From update of attachment 71072 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71072&action=review > > > WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:509 > > + // create our primary testing page/view. > > + if (isGraphicsBased()) { > > + m_page = new WebPage(static_cast<QWebView*>(m_mainView), this); > > + static_cast<QWebView*>(m_mainView)->setPage(m_page); > > + } else { > > + m_page = new WebPage(static_cast<QWebView*>(m_mainView), this); > > + static_cast<QWebView*>(m_mainView)->setPage(m_page); > > + } > > The two paths are the same (copy-paste). I think the test for isGraphicsBased() can be omitted here. > LGTM otherwise. If Ossy hasn't the time to check this on the bot environment, I'll do it tomorrow. I just realized: the first one has to be casted to WebViewGraphicsBased*.

How will this affect DRT performance?

After changing: WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:509 + // create our primary testing page/view. + if (isGraphicsBased()) { + m_page = new WebPage(static_cast<QWebView*>(m_mainView), this); + static_cast<QWebView*>(m_mainView)->setPage(m_page); + } to WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:509 + // create our primary testing page/view. + if (isGraphicsBased()) { + m_page = new WebPage(static_cast<WebViewGraphicsBased*>(m_mainView), this); + static_cast<WebViewGraphicsBased*>(m_mainView)->setPage(m_page); + } 8 tests fail: http/tests/navigation/error404-basic.html http/tests/navigation/error404-goback.html http/tests/navigation/error404-subframeload.html http/tests/navigation/javascriptlink-frames.html http/tests/navigation/postredirect-basic.html http/tests/navigation/postredirect-frames.html http/tests/navigation/postredirect-goback1.html http/tests/security/mixedContent/insecure-css-in-main-frame.html The navigation tests seem to have wrong expected results, the security test was flakey before, now it seems to fail always. The culprit is that the whole testing session is more than 1 minute slower on our bot after this patch. We should discuss whether this is feasible, or if there is eventually a better way to achieve the goal. My opinion is that it should be worth it especially if there are some other tests in the Skipped list which become correct after this change.

(In reply to comment #16) > 8 tests fail: > > http/tests/navigation/error404-basic.html > http/tests/navigation/error404-goback.html > http/tests/navigation/error404-subframeload.html > http/tests/navigation/javascriptlink-frames.html > http/tests/navigation/postredirect-basic.html > http/tests/navigation/postredirect-frames.html > http/tests/navigation/postredirect-goback1.html > http/tests/security/mixedContent/insecure-css-in-main-frame.html > > > The navigation tests seem to have wrong expected results, the security test was flakey before, now it seems to fail always. I can confirm. I tried to run navigation tests with --singly option and I got same fails as after this patch. When the fix for this sideeffect bug landed, we have to update expected results. http/tests/security/mixedContent/insecure-css-in-main-frame.html is flakey because of a sideeffect, which will be fixed by this patch. To make it pass we have to remove the Qt specific wrong expected result: LayoutTests/platform/qt/http/tests/security/mixedContent/insecure-css-in-main-frame-expected.txt I worry about the performance regression a little bit. Can we make it faster?

Created attachment 71154 [details] updating navigation tests Correct expected files for navigation tests. Feel free to integrate them to your patch.

(In reply to comment #17) > (In reply to comment #16) > > 8 tests fail: > > > > http/tests/navigation/error404-basic.html > > http/tests/navigation/error404-goback.html > > http/tests/navigation/error404-subframeload.html > > http/tests/navigation/javascriptlink-frames.html > > http/tests/navigation/postredirect-basic.html > > http/tests/navigation/postredirect-frames.html > > http/tests/navigation/postredirect-goback1.html > > http/tests/security/mixedContent/insecure-css-in-main-frame.html > > > > > > The navigation tests seem to have wrong expected results, the security test was flakey before, now it seems to fail always. The differences are all rendertext-related rather than 'real' differences. Strange. > > I can confirm. I tried to run navigation tests with --singly option > and I got same fails as after this patch. When the fix for this > sideeffect bug landed, we have to update expected results. > > http/tests/security/mixedContent/insecure-css-in-main-frame.html is > flakey because of a sideeffect, which will be fixed by this patch. > To make it pass we have to remove the Qt specific wrong expected result: > LayoutTests/platform/qt/http/tests/security/mixedContent/insecure-css-in-main-frame-expected.txt > > I worry about the performance regression a little bit. Can we make it faster? This was the only way I could think of. I'll update per Andras' comment and commit over the weekend if r+'d. That will allow me to time to watch the failures, if any, on the bot and correct them.

(In reply to comment #19) > > I worry about the performance regression a little bit. Can we make it faster? > > This was the only way I could think of. You're right, that's currently the only way. There is clearly a function missing in QNetworkAccessManager to clear the cache. Actually, isn't there a way to make QWebPage::setNetworkAccessManager() work? What's the problem with using that instead? How long does the DRT run take for you with and without your change, i.e. what's the performance impact?

(In reply to comment #20) > (In reply to comment #19) > > > I worry about the performance regression a little bit. Can we make it faster? > > > > This was the only way I could think of. > > You're right, that's currently the only way. There is clearly a function missing in QNetworkAccessManager to clear the cache. > > Actually, isn't there a way to make QWebPage::setNetworkAccessManager() work? What's the problem with using that instead? > I tried that, but it crashed a lot. Then I read: http://doc.qt.nokia.com/4.6/qwebpage.html#setNetworkAccessManager "Note: It is currently not supported to change the network access manager after the QWebPage has used it. The results of doing this are undefined." And gave up! > How long does the DRT run take for you with and without your change, i.e. what's the performance impact? Ossy says it takes about a minute extra to perform the entire set of layout tests, so definitely slower.

(In reply to comment #20) > How long does the DRT run take for you with and without your change, i.e. what's the performance impact? In the ideal case (average load) the bot runs the tests in about 6 minutes, with this patch the session lasts approximately one minute longer. So the regression is a bit less than 20%. I personally do not see this as a big problem since the patch should fix a lot more tests which are currently in the skipped list, but this has to be confirmed yet.

(In reply to comment #20) > (In reply to comment #19) > > > I worry about the performance regression a little bit. Can we make it faster? > > > > This was the only way I could think of. > > You're right, that's currently the only way. There is clearly a function missing in QNetworkAccessManager to clear the cache. > Maybe setCache would do the job.

(In reply to comment #23) > > Maybe setCache would do the job. You're onto something there. setCache() won't do the job: QNAM uses QNetworkAccessCache rather than QNetworkCache for caching connections and their associated authenticators. But there is a QNetworkAccessManager::clearCache() that is exported for auto-tests, and this clears down the QNetworkAccessCache. However, this will only work if DRT is built against a Qt that has been compiled with "DEFINES += QT_BUILD_INTERNAL". Which will not be the case for most people hacking on QtWebKit I think. I also suspect that there are other side-effects from not using a new QWebPage for each test - I just don't know what they are.

I added http/tests/security/mixedContent/insecure-css-in-main-frame.html to the skipped list to avoid flakey failing in the future and removed the incorrect Qt specifix expected file: http://trac.webkit.org/changeset/70498

maybe we could only reset QNAM if test has "http/" in the url? It is a hack, but wouldn't affect the overall running speed (or affect it less).

(In reply to comment #24) > (In reply to comment #23) > > > > Maybe setCache would do the job. > > You're onto something there. setCache() won't do the job: QNAM uses QNetworkAccessCache rather than QNetworkCache for caching connections and their associated authenticators. But there is a QNetworkAccessManager::clearCache() that is exported for auto-tests, and this clears down the QNetworkAccessCache. > > However, this will only work if DRT is built against a Qt that has been compiled with "DEFINES += QT_BUILD_INTERNAL". Which will not be the case for most people hacking on QtWebKit I think. We could try to make this API public in Qt, and start using it now on the build bots, I think. Simon?

Created attachment 78349 [details] Patch

(In reply to comment #28) > Created an attachment (id=78349) [details] > Patch I don't like this at all. Qt is ours, so we shouldn't have to do things like this. This is going to make layout testing useless on actual devices.

(In reply to comment #29) > (In reply to comment #28) > > Created an attachment (id=78349) [details] [details] > > Patch > > I don't like this at all. Qt is ours, so we shouldn't have to do things like this. Fair enought! >This is going to make layout testing useless on actual devices. I don't understand this part. What do you mean exactly?

Created attachment 78426 [details] tests fails with attachment 78349 [details] (diff) I tested the patch, but unfortunately 4 tests fail with it and the testing time is so slow. (412 sec -> 595 sec )

(In reply to comment #30) > (In reply to comment #29) > > (In reply to comment #28) > > > Created an attachment (id=78349) [details] [details] [details] > > > Patch > > > > I don't like this at all. Qt is ours, so we shouldn't have to do things like this. > > Fair enought! > > >This is going to make layout testing useless on actual devices. > > I don't understand this part. What do you mean exactly? It is already so slow that we can barely run the tests on actual devices, making it even slower would make it near to impossible.

Created attachment 78428 [details] tests fails with attachment 78349 [details] (diff)

After r75305 24 tests fail because of this bug: http://build.webkit.org/results/Qt%20Linux%20Release/r75488%20%2826299%29/results.html I added them to the Skipped list temporarily to make buildbot happy: http://trac.webkit.org/changeset/75492

One more strange thing: The 24 tests only fail in xvfb, but pass with X forward.

I've created http://bugreports.qt.nokia.com/browse/QTBUG-17312 Any volunteers for running layout tests against a QT_BUILD_INTERNAL compile of Qt and using QNAM::clearCache() in the DRT to see if it actually does fix things?

Created attachment 90073 [details] Patch

Comment on attachment 90073 [details] Patch rs=me

Created attachment 90233 [details] Patch

Comment on attachment 90233 [details] Patch Rejecting attachment 90233 [details] from commit-queue. Failed to run "[u'zip', u'-r', u'../commit-queue-logs/42578-layout-test-results-3.zip', u'/tmp/layout-test-resul..." exit_code: 12 zip warning: name not matched: /tmp/layout-test-results zip error: Nothing to do! (try: zip -r ../commit-queue-logs/42578-layout-test-results-3.zip . -i /tmp/layout-test-results) Full output: http://queues.webkit.org/results/8473474

Comment on attachment 90233 [details] Patch Clearing flags on attachment: 90233 Committed r84411: <http://trac.webkit.org/changeset/84411>

All reviewed patches have been landed. Closing bug.