Bug 32437

Summary: [Qt] [LayoutTestController]Output of document.write() is vanished by waitUntilDone()/notifyDone()
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, abecsi, ap, commit-queue, eric, kenneth, robert, tonikitoo, webkit.review.bot
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 32519    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Fix style error and renamed m_isLoading to m_hasDumped
none
Actually fix style error
eric: review+, commit-queue: commit-queue-
Fix patch for commit none

Description Csaba Osztrogonác 2009-12-11 09:54:28 PST
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>
Comment 1 Csaba Osztrogonác 2009-12-11 10:08:02 PST
Failing test skipped temporarily by r52002 to keep QtBuildBot green.
Comment 2 Alexey Proskuryakov 2009-12-11 10:43:55 PST
http/tests/security/escape-form-data-field-names.html uses the same technique, and should also be affected, as far as I can tell.
Comment 3 Andras Becsi 2009-12-13 02:48:50 PST
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.
Comment 4 Robert Hogan 2009-12-13 13:55:27 PST
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?
Comment 5 WebKit Review Bot 2009-12-13 13:59:48 PST
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
Comment 6 Robert Hogan 2009-12-13 14:01:24 PST
Created attachment 44763 [details]
Patch

With Changelog!
Comment 7 WebKit Review Bot 2009-12-13 14:05:33 PST
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
Comment 8 Adam Barth 2009-12-13 14:09:00 PST
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;
+    }
Comment 9 Robert Hogan 2009-12-14 05:28:36 PST
(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.
Comment 10 Andras Becsi 2009-12-14 05:37:06 PST
(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
Comment 11 Kenneth Rohde Christiansen 2009-12-14 05:43:27 PST
     m_isLoading = true;
+    m_loadFinished = false;

I think it is time that we use some state instead: Initial, Loading, Finished.
Comment 12 Robert Hogan 2009-12-14 06:12:44 PST
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.
Comment 13 WebKit Review Bot 2009-12-14 06:17:33 PST
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
Comment 14 Kenneth Rohde Christiansen 2009-12-14 06:32:33 PST
(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; }
Comment 15 Robert Hogan 2009-12-14 06:34:16 PST
Created attachment 44791 [details]
Actually fix style error
Comment 16 WebKit Review Bot 2009-12-14 06:38:24 PST
style-queue ran check-webkit-style on attachment 44791 [details] without any errors.
Comment 17 Robert Hogan 2009-12-14 06:41:22 PST
(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.
Comment 18 Andras Becsi 2009-12-14 07:06:27 PST
(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().
Comment 19 Robert Hogan 2009-12-14 07:33:49 PST
(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.
Comment 20 Robert Hogan 2009-12-14 08:16:20 PST
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.
Comment 21 Robert Hogan 2009-12-14 08:55:38 PST
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 22 Eric Seidel (no email) 2009-12-14 11:28:18 PST
Comment on attachment 44791 [details]
Actually fix style error

Yay!
Comment 23 WebKit Commit Bot 2009-12-14 12:50:26 PST
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
Comment 24 Robert Hogan 2009-12-14 13:11:15 PST
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.
Comment 25 Eric Seidel (no email) 2009-12-14 13:13:01 PST
OK, I've marked bug 32519 for commit.
Comment 26 Eric Seidel (no email) 2009-12-14 13:13:49 PST
Comment on attachment 44814 [details]
Fix patch for commit

I'll mark this cq+ once the other one lands.
Comment 27 WebKit Commit Bot 2009-12-14 14:09:25 PST
Comment on attachment 44814 [details]
Fix patch for commit

Clearing flags on attachment: 44814

Committed r52118: <http://trac.webkit.org/changeset/52118>
Comment 28 WebKit Commit Bot 2009-12-14 14:09:38 PST
All reviewed patches have been landed.  Closing bug.