http/tests/misc/percent-sign-in-form-field-name.html introduced in http://trac.webkit.org/changeset/51973 , which signalled, that our LayoutTesetController has this bug. With this small test, you can reproduce the error: <script> if (window.layoutTestController) { layoutTestController.dumpAsText(); layoutTestController.waitUntilDone(); } document.write('PASS') layoutTestController.notifyDone(); </script>
Failing test skipped temporarily by r52002 to keep QtBuildBot green.
http/tests/security/escape-form-data-field-names.html uses the same technique, and should also be affected, as far as I can tell.
The problem is that if notifyDone() is called the text gets immediately dumped but the evaluation of document.write() hasn't ended yet. Because m_isLoading is set to false maybeDump() returns without doing anything, and the text gets lost. My investigation revealed, that LayoutTestController in general and especially maybeDump() and notifyDone() need a bit of rework. I'll look into that.
Created attachment 44761 [details] Patch This fixes it for me. It also fixes the following from the skipped list on my machine: fast/forms/textarea-linewrap-dynamic.html fast/forms/textarea-setvalue-submit.html fast/forms/textarea-hard-linewrap-empty.html fast/forms/submit-to-url-fragment.html http/tests/misc/percent-sign-in-form-field-name.html http/tests/security/escape-form-data-field-names.html The 'but' is that these 2 need to be added to the skipped list: http/tests/xmlhttprequest/access-control-basic-denied-preflight-cache.html svg/custom/use-instanceRoot-as-event-target.xhtml I think the bug with the drt was masking deeper problems with these two. In the first case, the test returns a 'SUCCESS' *and* an exception. I believe up to now it wasn't catching this exception because notifyDone shut down the page before it had finished loading. In the second case, notifyDone seems to get called three times by WebCore, hence the three sets of identical results for 'test 10'. So in a sense, the test is actually three times more successful than before. ;-) Again, the behaviour of the DRT would have been to shut down the still-loading page before receiving these two extra bits of output. Csaba, can you see if you can reproduce my success with the above tests with this patch?
Attachment 44761 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:101: Missing space before else [whitespace/braces] [5] WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:141: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2
Created attachment 44763 [details] Patch With Changelog!
Attachment 44763 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:101: Missing space before else [whitespace/braces] [5] WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:141: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2
Is there some reason you didn't fix these style nits? They seem like true positives to me: + }else if (!shouldWaitUntilDone()) { [...] + if (!m_loadFinished) { + return; + }
(In reply to comment #8) > Is there some reason you didn't fix these style nits? They seem like true > positives to me: > > + }else if (!shouldWaitUntilDone()) { > > [...] > > + if (!m_loadFinished) { > + return; > + } They are, I missed the stylebot report when adding the changelog but have fixed them locally. Casba suggested yesterday on IRC that he would give the patch a roll on the Qt buildbot server and see if it confirmed my results. Once he's happy I'll re-upload.
(In reply to comment #9) > (In reply to comment #8) > > Is there some reason you didn't fix these style nits? They seem like true > > positives to me: > > > > + }else if (!shouldWaitUntilDone()) { > > > > [...] > > > > + if (!m_loadFinished) { > > + return; > > + } > > They are, I missed the stylebot report when adding the changelog but have fixed > them locally. Casba suggested yesterday on IRC that he would give the patch a > roll on the Qt buildbot server and see if it confirmed my results. Once he's > happy I'll re-upload. >55 void LayoutTestController::reset() >56 { >57 m_isLoading = true; >58 m_loadFinished = false; I think this is a dulication, because if we would refactor LayoutTestController correctly then logically you only have m_loadFinished if m_isLoading is false, so you do not need them both. My plan is to refactor the whole DumpRenderTree in a way which looks at least similar to the other ports implementation. I think leaving things as they are and working around these problems only sweeps them under the carpet. br, bbandix
m_isLoading = true; + m_loadFinished = false; I think it is time that we use some state instead: Initial, Loading, Finished.
Created attachment 44789 [details] Fix style error and renamed m_isLoading to m_hasDumped I think m_isLoading is better off named as m_hasDumped and set to false when the test starts. With the introduction of m_loadFinished, it's purpose is to check whether DRT still needs to dump the results or not.
Attachment 44789 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:141: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1
(In reply to comment #12) > Created an attachment (id=44789) [details] > Fix style error and renamed m_isLoading to m_hasDumped > > I think m_isLoading is better off named as m_hasDumped and set to false when > the test starts. With the introduction of m_loadFinished, it's purpose is to > check whether DRT still needs to dump the results or not. ok then. is m_loadFinished needed then? btw, I don't think we want to add methods for resetting all variables: + void resetLoadFinished() { m_loadFinished = false; }
Created attachment 44791 [details] Actually fix style error
style-queue ran check-webkit-style on attachment 44791 [details] without any errors.
(In reply to comment #14) > (In reply to comment #12) > > Created an attachment (id=44789) [details] [details] > > Fix style error and renamed m_isLoading to m_hasDumped > > > > I think m_isLoading is better off named as m_hasDumped and set to false when > > the test starts. With the introduction of m_loadFinished, it's purpose is to > > check whether DRT still needs to dump the results or not. > > ok then. is m_loadFinished needed then? > Yes, two variables are needed: one to tell DRT if the page has finished loading or not, and another to tell DRT if it's OK to dump. m_isLoading was trying to do both, which I think caused the problems. notifyDone needs to know that the load has finished before it can dump, whereas maybeDump needs to know if notifyDone has already dumped. > btw, I don't think we want to add methods for resetting all variables: > > + void resetLoadFinished() { m_loadFinished = false; } Layout tests can have multiple page loads so m_loadFinished needs to be set to false every time a new one is started, this prevents NotifyDone from dumping prematurely.
(In reply to comment #17) > (In reply to comment #14) > > (In reply to comment #12) > > > Created an attachment (id=44789) [details] [details] [details] > > > Fix style error and renamed m_isLoading to m_hasDumped > > > > > > I think m_isLoading is better off named as m_hasDumped and set to false when > > > the test starts. With the introduction of m_loadFinished, it's purpose is to > > > check whether DRT still needs to dump the results or not. > > > > ok then. is m_loadFinished needed then? > > > > Yes, two variables are needed: one to tell DRT if the page has finished loading > or not, and another to tell DRT if it's OK to dump. m_isLoading was trying to > do both, which I think caused the problems. > > notifyDone needs to know that the load has finished before it can dump, whereas > maybeDump needs to know if notifyDone has already dumped. > > > btw, I don't think we want to add methods for resetting all variables: > > > > + void resetLoadFinished() { m_loadFinished = false; } > > Layout tests can have multiple page loads so m_loadFinished needs to be set to > false every time a new one is started, this prevents NotifyDone from dumping > prematurely. True, they can have multiple page loads. But they also can have a child windows, which is created in QWebPage *DumpRenderTree::createWindow().
(In reply to comment #18) > (In reply to comment #17) > > > > Layout tests can have multiple page loads so m_loadFinished needs to be set to > > false every time a new one is started, this prevents NotifyDone from dumping > > prematurely. > > True, they can have multiple page loads. But they also can have a child > windows, which is created in QWebPage *DumpRenderTree::createWindow(). I think this case is covered by the fact that I connect resetLoadFinished() to QWebPage::loadStarted(). At least my testing here (before/after patch) doesn't introduce any additional failed tests, apart from the two I mention in the patch - and those two appear to be unrelated bugs masked by the current behaviour of notifyDone.
Created https://bugs.webkit.org/show_bug.cgi?id=32519 to track issue with svg/custom/use-instanceRoot-as-event-target.xhtml. Gtk and Win ports skip the test. I ran gtk's DRT against the test and found it has the same problem with the test as Qt's DRT with my patch applied.
Added https://bugs.webkit.org/show_bug.cgi?id=32521 to track issue with http/tests/xmlhttprequest/access-control-basic-denied-preflight-cache.html.
Comment on attachment 44791 [details] Actually fix style error Yay!
Comment on attachment 44791 [details] Actually fix style error Rejecting patch 44791 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel', '--force']" exit_code: 1 Last 500 characters of output: ching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/platform/qt/Skipped Hunk #3 FAILED at 5174. 1 out of 3 hunks FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej patching file WebKitTools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKitTools/DumpRenderTree/qt/DumpRenderTree.cpp patching file WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp patching file WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.h
Created attachment 44814 [details] Fix patch for commit My mistake. I created separate bugs for the additions to the skip list, including bugzilla references. So please review and commit https://bugs.webkit.org/show_bug.cgi?id=32519 before landing this one, otherwise it will break the Qt buildbot.
OK, I've marked bug 32519 for commit.
Comment on attachment 44814 [details] Fix patch for commit I'll mark this cq+ once the other one lands.
Comment on attachment 44814 [details] Fix patch for commit Clearing flags on attachment: 44814 Committed r52118: <http://trac.webkit.org/changeset/52118>
All reviewed patches have been landed. Closing bug.