Bug 42578 – [Qt] DRT sideeffect revealed by r63657 and r75305

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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

[Andreas Kling]

Created attachment 62039 [details]
Proposed patch

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Andreas Kling]

Comment on attachment 62039 [details]
Proposed patch

Clearing flags on attachment: 62039

Committed r63738: <http://trac.webkit.org/changeset/63738>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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: .

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Robert Hogan]

Created attachment 71072 [details]
Patch

[Robert Hogan]

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!

[Andras Becsi]

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.

[Andras Becsi]

(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*.

[Andreas Kling]

How will this affect DRT performance?

[Andras Becsi]

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.

[Csaba Osztrogonác]

(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?

[Csaba Osztrogonác]

Created attachment 71154 [details]
updating navigation tests

Correct expected files for navigation tests.

Feel free to integrate them to your patch.

[Robert Hogan]

(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.

[Simon Hausmann]

(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?

[Robert Hogan]

(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.

[Andras Becsi]

(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.

[Balazs Kelemen]

(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.

[Robert Hogan]

(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.

[Csaba Osztrogonác]

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

[Antonio Gomes]

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).

[Kenneth Rohde Christiansen]

(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?

[Robert Hogan]

Created attachment 78349 [details]
Patch

[Kenneth Rohde Christiansen]

(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.

[Robert Hogan]

(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?

[Csaba Osztrogonác]

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 )

[Kenneth Rohde Christiansen]

(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.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Robert Hogan]

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?

[Robert Hogan]

Created attachment 90073 [details]
Patch

[Antonio Gomes]

Comment on attachment 90073 [details]
Patch

rs=me

[Robert Hogan]

Created attachment 90233 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Comment on attachment 90233 [details]
Patch

Clearing flags on attachment: 90233

Committed r84411: <http://trac.webkit.org/changeset/84411>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.