Summary: | [Qt] Flaky security tests | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Allan Sandfeld Jensen <allan.jensen> | ||||||||||||
Component: | Tools / Tests | Assignee: | Allan Sandfeld Jensen <allan.jensen> | ||||||||||||
Status: | RESOLVED INVALID | ||||||||||||||
Severity: | Normal | CC: | felipe, hausmann, jturcotte, kbalazs, ossy, rafael.lobo, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 420+ | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 100653 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Allan Sandfeld Jensen
2012-10-25 09:32:04 PDT
Created attachment 170680 [details]
Patch
Created attachment 171194 [details]
Patch
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"? 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.) (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'. Committed r132782: <http://trac.webkit.org/changeset/132782> Re-opened since this is blocked by bug 100653 Comment on attachment 171194 [details] Patch Rolled out, because it made zillion tests fail and flakey - http://trac.webkit.org/changeset/132785 (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. (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. 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.
Created attachment 171256 [details]
Patch
Avoid changing existing signal/slot connections
Comment on attachment 171256 [details]
Patch
Still missing a few more early dump output-resets which otherwise causes more failures.
Created attachment 171261 [details]
Patch
Comment on attachment 171261 [details] Patch Clearing flags on attachment: 171261 Committed r132819: <http://trac.webkit.org/changeset/132819> All reviewed patches have been landed. Closing bug. Reverted r132819 for reason: It made layout testing 40% slower and storage tests assert Committed r132873: <http://trac.webkit.org/changeset/132873> 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 (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. (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? 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. === 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. |