Bug 42578 - [Qt] DRT sideeffect revealed by r63657 and r75305
Summary: [Qt] DRT sideeffect revealed by r63657 and r75305
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Critical
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 32961
  Show dependency treegraph
 
Reported: 2010-07-19 11:03 PDT by Csaba Osztrogonác
Modified: 2011-04-20 13:29 PDT (History)
9 users (show)

See Also:


Attachments
Proposed patch (1.45 KB, patch)
2010-07-20 01:10 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (11.31 KB, patch)
2010-10-18 12:41 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
updating navigation tests (27.49 KB, patch)
2010-10-19 04:52 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2011-01-09 05:18 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
tests fails with attachment 78349 (diff) (2.67 KB, patch)
2011-01-10 12:12 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
tests fails with attachment 78349 (diff) (2.67 KB, patch)
2011-01-10 12:16 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (3.47 KB, patch)
2011-04-18 12:43 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (2.98 KB, patch)
2011-04-19 11:56 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2010-07-19 11:03:17 PDT
A DRT sideeffect bug revealed by r63657. After this changeset,
1-2 http tests always fail. I can reproduce it locally when
I run all tests. These failing tests work correctly if I run
only one test, all http tests, etc.

some fail:
http://build.webkit.org/results/Qt%20Linux%20Release/r63657%20%2815665%29/results.html
http://build.webkit.org/results/Qt%20Linux%20Release/r63657%20%2815666%29/results.html
http://build.webkit.org/results/Qt%20Linux%20Release/r63659%20%2815668%29/results.html
http://build.webkit.org/results/Qt%20Linux%20Release/r63676%20%2815675%29/results.html
http://build.webkit.org/results/Qt%20Linux%20Release/r63678%20%2815677%29/results.html

I haven't found yet what cause this crazyness. :(

Now I have only one workaround to make buildbot happy: 
skipping tests again, which were unskipped by r63657.
Comment 1 Csaba Osztrogonác 2010-07-19 11:20:45 PDT
Tests skipped by http://trac.webkit.org/changeset/63679.
Comment 2 Andreas Kling 2010-07-20 01:10:59 PDT
Created attachment 62039 [details]
Proposed patch
Comment 3 Csaba Osztrogonác 2010-07-20 01:15:36 PDT
I cc-ed Kenneth, AFAIK he reviewed a bunch of notification related patches before.
Comment 4 Csaba Osztrogonác 2010-07-20 04:08:52 PDT
With this patch http/tests/xmlhttprequest/null-auth.php still fails for me. (tested with r63733)
Comment 5 Andreas Kling 2010-07-20 04:46:12 PDT
Comment on attachment 62039 [details]
Proposed patch

Clearing flags on attachment: 62039

Committed r63738: <http://trac.webkit.org/changeset/63738>
Comment 6 Andreas Kling 2010-07-20 04:46:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Csaba Osztrogonác 2010-07-20 08:05:48 PDT
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: .
Comment 8 Csaba Osztrogonác 2010-07-21 08:30:18 PDT
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.
Comment 9 Csaba Osztrogonác 2010-07-22 04:33:22 PDT
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
Comment 10 Csaba Osztrogonác 2010-07-26 03:10:01 PDT
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
Comment 11 Robert Hogan 2010-10-18 12:41:19 PDT
Created attachment 71072 [details]
Patch
Comment 12 Robert Hogan 2010-10-18 12:52:45 PDT
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 13 Andras Becsi 2010-10-18 14:24:10 PDT
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.
Comment 14 Andras Becsi 2010-10-18 14:32:42 PDT
(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*.
Comment 15 Andreas Kling 2010-10-19 00:43:11 PDT
How will this affect DRT performance?
Comment 16 Andras Becsi 2010-10-19 03:12:45 PDT
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.
Comment 17 Csaba Osztrogonác 2010-10-19 04:50:20 PDT
(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?
Comment 18 Csaba Osztrogonác 2010-10-19 04:52:50 PDT
Created attachment 71154 [details]
updating navigation tests

Correct expected files for navigation tests.

Feel free to integrate them to your patch.
Comment 19 Robert Hogan 2010-10-19 14:06:58 PDT
(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.
Comment 20 Simon Hausmann 2010-10-21 13:20:29 PDT
(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?
Comment 21 Robert Hogan 2010-10-21 13:54:55 PDT
(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.
Comment 22 Andras Becsi 2010-10-22 00:42:47 PDT
(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.
Comment 23 Balazs Kelemen 2010-10-23 15:44:02 PDT
(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.
Comment 24 Robert Hogan 2010-10-25 08:05:42 PDT
(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.
Comment 25 Csaba Osztrogonác 2010-10-26 15:07:58 PDT
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
Comment 26 Antonio Gomes 2011-01-08 18:26:56 PST
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).
Comment 27 Kenneth Rohde Christiansen 2011-01-09 03:50:03 PST
(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?
Comment 28 Robert Hogan 2011-01-09 05:18:14 PST
Created attachment 78349 [details]
Patch
Comment 29 Kenneth Rohde Christiansen 2011-01-09 05:28:56 PST
(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.
Comment 30 Robert Hogan 2011-01-09 05:45:48 PST
(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?
Comment 31 Csaba Osztrogonác 2011-01-10 12:12:19 PST
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 )
Comment 32 Kenneth Rohde Christiansen 2011-01-10 12:14:08 PST
(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.
Comment 33 Csaba Osztrogonác 2011-01-10 12:16:54 PST
Created attachment 78428 [details]
tests fails with attachment 78349 [details] (diff)
Comment 34 Csaba Osztrogonác 2011-01-11 05:28:50 PST
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
Comment 35 Csaba Osztrogonác 2011-01-11 05:30:22 PST
One more strange thing: The 24 tests only fail in xvfb, but pass with X forward.
Comment 36 Robert Hogan 2011-02-08 12:20:04 PST
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?
Comment 37 Robert Hogan 2011-04-18 12:43:57 PDT
Created attachment 90073 [details]
Patch
Comment 38 Antonio Gomes 2011-04-18 12:56:16 PDT
Comment on attachment 90073 [details]
Patch

rs=me
Comment 39 Robert Hogan 2011-04-19 11:56:13 PDT
Created attachment 90233 [details]
Patch
Comment 40 WebKit Commit Bot 2011-04-19 17:37:49 PDT
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 41 WebKit Commit Bot 2011-04-20 13:29:05 PDT
Comment on attachment 90233 [details]
Patch

Clearing flags on attachment: 90233

Committed r84411: <http://trac.webkit.org/changeset/84411>
Comment 42 WebKit Commit Bot 2011-04-20 13:29:13 PDT
All reviewed patches have been landed.  Closing bug.