Bug 32437 – [Qt] [LayoutTestController]Output of document.write() is vanished by waitUntilDone()/notifyDone()

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

[Csaba Osztrogonác]

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>

[Csaba Osztrogonác]

Failing test skipped temporarily by r52002 to keep QtBuildBot green.

[Alexey Proskuryakov]

http/tests/security/escape-form-data-field-names.html uses the same technique, and should also be affected, as far as I can tell.

[Andras Becsi]

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.

[Robert Hogan]

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?

[WebKit Review Bot]

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

[Robert Hogan]

Created attachment 44763 [details]
Patch

With Changelog!

[WebKit Review Bot]

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

[Adam Barth]

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;
+    }

[Robert Hogan]

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

[Andras Becsi]

(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

[Kenneth Rohde Christiansen]

     m_isLoading = true;
+    m_loadFinished = false;

I think it is time that we use some state instead: Initial, Loading, Finished.

[Robert Hogan]

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.

[WebKit Review Bot]

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

[Kenneth Rohde Christiansen]

(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; }

[Robert Hogan]

Created attachment 44791 [details]
Actually fix style error

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44791 [details] without any errors.

[Robert Hogan]

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

[Andras Becsi]

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

[Robert Hogan]

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

[Robert Hogan]

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.

[Robert Hogan]

Added https://bugs.webkit.org/show_bug.cgi?id=32521 to track issue with 
http/tests/xmlhttprequest/access-control-basic-denied-preflight-cache.html.

[Eric Seidel (no email)]

Comment on attachment 44791 [details]
Actually fix style error

Yay!

[WebKit Commit Bot]

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

[Robert Hogan]

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.

[Eric Seidel (no email)]

OK, I've marked bug 32519 for commit.

[Eric Seidel (no email)]

Comment on attachment 44814 [details]
Fix patch for commit

I'll mark this cq+ once the other one lands.

[WebKit Commit Bot]

Comment on attachment 44814 [details]
Fix patch for commit

Clearing flags on attachment: 44814

Committed r52118: <http://trac.webkit.org/changeset/52118>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.