Bug 31638 – [Qt] fast/history/back-forward-reset-after-error-handling.html failing due to WorkQueue not being un-frozen

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Antonio Gomes]

No current QT tests were using the WorkQueue. Now that we fixed its usage (see bug 30953, bug 31158 and bug 31164), we have to un-freeze it after each test execution for it to actually work.

patch coming ...

[Antonio Gomes]

Created attachment 43449 [details]
(landed in r51299) patch 0.1

[Kenneth Rohde Christiansen]

Ok, this is going to result in a timeout.

In a local patch I have:

-    // Causes timeout, why?
+    // The below line is used in other ports, but for us it results in
+    // a timeout for fast/frames/frame-navigation.html
     //WorkQueue::shared()->setFrozen(false);

Can you investigate that? It only timeouts when running the whole test suite.

[Antonio Gomes]

(In reply to comment #2)
> Ok, this is going to result in a timeout.
> 
> In a local patch I have:
> 
> -    // Causes timeout, why?
> +    // The below line is used in other ports, but for us it results in
> +    // a timeout for fast/frames/frame-navigation.html
>      //WorkQueue::shared()->setFrozen(false);
> 
> Can you investigate that? It only timeouts when running the whole test suite.

thx for looking at the patch.

for me no new timeout's happenned. in fact, i saw one test starting to have incorrent layout and one fixed, which was the test this bug fixed. could you please let me know which started to timeout for you ?


@kenneth: Regardless that, IMHO this solution is the right thing to do. If it affected other tests, revealing hidden bugs, we should investigate in follow up bugs.

[Antonio Gomes]

(In reply to comment #2)
> Ok, this is going to result in a timeout.
> 
> In a local patch I have:
> 
> -    // Causes timeout, why?
> +    // The below line is used in other ports, but for us it results in
> +    // a timeout for fast/frames/frame-navigation.html
>      //WorkQueue::shared()->setFrozen(false);
> 
> Can you investigate that? It only timeouts when running the whole test suite.

ah, i see in your comment now (frame-navigation.html). sorry about that.

for me (linux 32-bit) it did not timeout, but had incorrect layout due to font stuff probably (see diff below):

DIFF

--- /tmp/layout-test-results/fast/frames/frame-navigation-expected.txt	2009-11-18 21:49:42.000000000 -0400
+++ /tmp/layout-test-results/fast/frames/frame-navigation-actual.txt	2009-11-18 21:49:42.000000000 -0400
@@ -14,8 +14,8 @@
                   text run at (0,0) width 374: "This tests that navigating to two pages in a subframe,"
                   text run at (0,20) width 351: "and then going back does not cause an assertion."
               RenderBlock {P} at (0,56) size 381x20
-                RenderText {#text} at (0,0) size 185x20
-                  text run at (0,0) width 185: "SUCCESS - Didn't assert!"
+                RenderText {#text} at (0,0) size 187x20
+                  text run at (0,0) width 187: "SUCCESS - Didn't assert!"
       RenderFrame {FRAME} at (403,0) size 397x600
         layer at (0,0) size 397x600
           RenderView at (0,0) size 397x600





we are maybe free to go here then (?)

[Kenneth Rohde Christiansen]

(In reply to comment #4)

As I said pretty clearly above, it only timeouts when running the whole test suite.

If you run it individually it does not timeout.

> (In reply to comment #2)
> > Ok, this is going to result in a timeout.
> > 
> > In a local patch I have:
> > 
> > -    // Causes timeout, why?
> > +    // The below line is used in other ports, but for us it results in
> > +    // a timeout for fast/frames/frame-navigation.html
> >      //WorkQueue::shared()->setFrozen(false);
> > 
> > Can you investigate that? It only timeouts when running the whole test suite.
> 
> ah, i see in your comment now (frame-navigation.html). sorry about that.
> 
> for me (linux 32-bit) it did not timeout, but had incorrect layout due to font
> stuff probably (see diff below):
> 
> DIFF
> 
> --- /tmp/layout-test-results/fast/frames/frame-navigation-expected.txt   
> 2009-11-18 21:49:42.000000000 -0400
> +++ /tmp/layout-test-results/fast/frames/frame-navigation-actual.txt   
> 2009-11-18 21:49:42.000000000 -0400
> @@ -14,8 +14,8 @@
>                    text run at (0,0) width 374: "This tests that navigating to
> two pages in a subframe,"
>                    text run at (0,20) width 351: "and then going back does not
> cause an assertion."
>                RenderBlock {P} at (0,56) size 381x20
> -                RenderText {#text} at (0,0) size 185x20
> -                  text run at (0,0) width 185: "SUCCESS - Didn't assert!"
> +                RenderText {#text} at (0,0) size 187x20
> +                  text run at (0,0) width 187: "SUCCESS - Didn't assert!"
>        RenderFrame {FRAME} at (403,0) size 397x600
>          layer at (0,0) size 397x600
>            RenderView at (0,0) size 397x600
> 
> 
> 
> 
> 
> we are maybe free to go here then (?)

[Antonio Gomes]

(In reply to comment #5)
> (In reply to comment #4)
> 
> As I said pretty clearly above, it only timeouts when running the whole test
> suite.
> 
> If you run it individually it does not timeout.


kenneth, let me be clearer this time too: no timeout to me running the whole test suite.

please lets considere get this right fix in , skip revealed hidden bugs, and work on them on followups

[Kenneth Rohde Christiansen]

I agree, but we have many side-effects and we need to fix these more than the tests. Please get bbandix to test it on the test buildbot. If there are no timeouts, I r+ it.

> kenneth, let me be clearer this time too: no timeout to me running the whole
> test suite.
> 
> please lets considere get this right fix in , skip revealed hidden bugs, and
> work on them on followups

[Antonio Gomes]

andras, could you help us here ? thx

[Andras Becsi]

(In reply to comment #8)
> andras, could you help us here ? thx

Your patch corrects fast/history/back-forward-reset-after-error-handling.html but fast/frames/frame-navigation.html times out on the buildbot.

[Antonio Gomes]

(In reply to comment #8)
> andras, could you help us here ? thx

kenneth, what do you think ?

we fix the timeout first or skip it for now and fix in followup?

[Kenneth Rohde Christiansen]

I will say, investigate it immediately. Both tests actually seem to be related.

If you fix both, please commit.

[Jakub Wieczorek]

Created attachment 43659 [details]
fix the timeout in fast/frames/frame-navigation.html

It is interesting that the test in question (fast/frames/frame-navigation.html) is passing at all. It turns out, however, that the expected result, which has been introduced in http://trac.webkit.org/changeset/46634, is wrong.

If you compare http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/frames/frame-navigation-expected.txt

with http://trac.webkit.org/browser/trunk/LayoutTests/platform/qt/fast/frames/frame-navigation-expected.txt

you'll notice that the latter is missing the expected tree in the second child frame.

The reason why it is timeouting is that the test performs a load in one of the child frames. The DRT should dump the tree after all tasks in the work queue are proceeded, but it does not, because it waits for the QWebFrame::loadFinished() signal from the main frame, but that obviously does not work with the child frames. The solution is to change the connection to the QWebPage::loadFinished() signal.

Here is a patch that fixes the timeout issue. Unfortunately, for some reason I am getting wrong metrics in all tests in my local environment with Qt 4.6. Therefore, I cannot include the valid result for the test, so it would be nice if someone could try to generate the result.

Feel free to merge this patch with the first one, if you wish.

[Kenneth Rohde Christiansen]

Great catch. Maybe a comment in the DRT is deserved? 

The DRT is complicated and it is easy to break things when fixing other things. For this reason we try to comment why we do what and when, or at least I have been starting to do that.

[Kenneth Rohde Christiansen]

Comment on attachment 43659 [details]
fix the timeout in fast/frames/frame-navigation.html

Let's get this in. Later we can add a comment and fix the results. All in all, it is better than how it is today.

Great work Jakub

[WebKit Commit Bot]

Comment on attachment 43659 [details]
fix the timeout in fast/frames/frame-navigation.html

Clearing flags on attachment: 43659

Committed r51293: <http://trac.webkit.org/changeset/51293>

[Antonio Gomes]

i can confirm that this is the fix.

i had this patch locally, but could not ask for review earlier because i am on vacation and not able to access internet every day, but jakub came first :)

i can confirm that the expected result was wrong and my patch was fixing it too.

i will commit the fix for the expected result...

(In reply to comment #12)
> Created an attachment (id=43659) [details]
> fix the timeout in fast/frames/frame-navigation.html
> 
> It is interesting that the test in question (fast/frames/frame-navigation.html)
> is passing at all. It turns out, however, that the expected result, which has
> been introduced in http://trac.webkit.org/changeset/46634, is wrong.
> 
> If you compare
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/frames/frame-navigation-expected.txt
> 
> with
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/qt/fast/frames/frame-navigation-expected.txt
> 
> you'll notice that the latter is missing the expected tree in the second child
> frame.
> 
> The reason why it is timeouting is that the test performs a load in one of the
> child frames. The DRT should dump the tree after all tasks in the work queue
> are proceeded, but it does not, because it waits for the
> QWebFrame::loadFinished() signal from the main frame, but that obviously does
> not work with the child frames. The solution is to change the connection to the
> QWebPage::loadFinished() signal.
> 
> Here is a patch that fixes the timeout issue. Unfortunately, for some reason I
> am getting wrong metrics in all tests in my local environment with Qt 4.6.
> Therefore, I cannot include the valid result for the test, so it would be nice
> if someone could try to generate the result.
> 
> Feel free to merge this patch with the first one, if you wish.