Bug 100388 – [Qt] Flaky security tests

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Allan Sandfeld Jensen]

Several tests in http/tests/security are flaky. 

If you first run  http/tests/security/contentSecurityPolicy/object-src-none-blocked.html and then http/tests/security/contentSecurityPolicy/object-src-url-allowed.html, you will notice the flaky error in the top of the second test refers to the first one. The reason being that the plugin from the first test is not fully cancelled before the second is started.

[Allan Sandfeld Jensen]

Created attachment 170680 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 171194 [details]
Patch

[Rafael Brandao]

Comment on attachment 171194 [details]
Patch

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

Nice!

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:518
> +    m_page->mainFrame()->setUrl(QUrl("about:blank"));

Should you wait here for "loadFinished"?

[Jocelyn Turcotte]

Comment on attachment 171194 [details]
Patch

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

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:521
> +

r=me if you remove this extra empty line and if you can add a short comment on why we do this setUrl (and maybe adapt the comment above that says we stop.)

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 171194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171194&action=review
> 
> Nice!
> 
> > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:518
> > +    m_page->mainFrame()->setUrl(QUrl("about:blank"));
> 
> Should you wait here for "loadFinished"?

Actually I only care about the clean-up which happens when setUrl is run, but not when the page loading is merely 'stopped'.

[Allan Sandfeld Jensen]

Committed r132782: <http://trac.webkit.org/changeset/132782>

[WebKit Review Bot]

Re-opened since this is blocked by bug 100653

[Csaba Osztrogonác]

Comment on attachment 171194 [details]
Patch

Rolled out, because it made zillion tests fail and flakey - http://trac.webkit.org/changeset/132785

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 171194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171194&action=review
> 
> Nice!
> 
> > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:518
> > +    m_page->mainFrame()->setUrl(QUrl("about:blank"));
> 
> Should you wait here for "loadFinished"?

Yeah, this is actually what WTR does. It loads the empty url and then waits for the finished signal.

[Rafael Brandao]

(In reply to comment #9)
> (In reply to comment #3)
> > (From update of attachment 171194 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171194&action=review
> > 
> > Nice!
> > 
> > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:518
> > > +    m_page->mainFrame()->setUrl(QUrl("about:blank"));
> > 
> > Should you wait here for "loadFinished"?
> 
> Yeah, this is actually what WTR does. It loads the empty url and then waits for the finished signal.

I've thought so... my concern previously was that you were asking to load "blank:url" expecting to clear the state, but you would go to the next test with a few signals pending, which is most probably not what you want.

[Allan Sandfeld Jensen]

Created attachment 171248 [details]
Patch

Wait until resetting is done, similar to what WebKitTestRunner does. Note QSignalSpy::wait could not be used since there are race-conditions on the flag.

[Allan Sandfeld Jensen]

Created attachment 171256 [details]
Patch

Avoid changing existing signal/slot connections

[Allan Sandfeld Jensen]

Comment on attachment 171256 [details]
Patch

Still missing a few more early dump output-resets which otherwise causes more failures.

[Allan Sandfeld Jensen]

Created attachment 171261 [details]
Patch

[WebKit Review Bot]

Comment on attachment 171261 [details]
Patch

Clearing flags on attachment: 171261

Committed r132819: <http://trac.webkit.org/changeset/132819>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reverted r132819 for reason:

It made layout testing 40% slower and storage tests assert

Committed r132873: <http://trac.webkit.org/changeset/132873>

[Csaba Osztrogonác]

runtime before the patch: 19 mins, 5 secs - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/44437

runtime after the patch: 26 mins, 18 secs - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/44438

asserting tests on 64 bit debug bot: http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/26049

[Csaba Osztrogonác]

(In reply to comment #18)
> runtime before the patch: 19 mins, 5 secs - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/44437
> 
> runtime after the patch: 26 mins, 18 secs - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/44438
> 
> asserting tests on 64 bit debug bot: http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/26049

storage tests only assert if you run all of them, but pass with --run-singly.
Maybe this patch only unhided a crazy GC related bug.

[Allan Sandfeld Jensen]

(In reply to comment #19)
> (In reply to comment #18)
> > runtime before the patch: 19 mins, 5 secs - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/44437
> > 
> > runtime after the patch: 26 mins, 18 secs - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/44438
> > 
> > asserting tests on 64 bit debug bot: http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/26049
> 
> storage tests only assert if you run all of them, but pass with --run-singly.
> Maybe this patch only unhided a crazy GC related bug.

It is likely more state that need to be reset before we reset the URL. Not sure what we can do about the extra time - but perhaps we hit the 5 second timeout due to an oversight, did the list of slow tests change?

[Balazs Kelemen]

One reason of the slowdown could be the runUntil function. I don't think it's a good idea to reintroduce it here. Actually it does busy waiting which produce high cpu usage. (The busy waiting itself could be fixed by using a qeventloop instead, but generally I would avoid using nested loops if not absolutely necessary.) In WTR the general design was to using a nested loop so we cannot get rid of it, but in DRT we can simply convert it to use a signal-slot connection.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.