Bug 37334 – [Qt] fast/frames/flattening/frameset-flattening-subframesets.html fails intermittently on Qt bot

• Status: CLOSED
• Resolution: FIXED
• Component: Layout and Rendering

[Csaba Osztrogonác]

http://build.webkit.org/results/Qt%20Linux%20Release/r57338%20%289846%29/fast/frames/flattening/frameset-flattening-subframesets-pretty-diff.html

We should fix it to make our bot more stable and green.

[Kenneth Rohde Christiansen]

That new result is wrong, strange, I dont get this locally. It should be 900 and not 800, or it didnt do the flattening.

[Csaba Osztrogonác]

(In reply to comment #1)
> That new result is wrong, strange, I dont get this locally. It should be 900
> and not 800, or it didnt do the flattening.

You misinterpreted, it is a flakey test. 
It passes usually, but sometimes we get this wrong result.

[Kenneth Rohde Christiansen]

Is that also the case for the mac?

[Csaba Osztrogonác]

(In reply to comment #3)
> Is that also the case for the mac?
I dont't know, I didn't see this fail on Mac bots before, but is is possible.

[Eric Seidel (no email)]

Failed again just now:
http://build.webkit.org/results/Qt%20Linux%20Release/r57583%20(10079)/fast/frames/flattening/frameset-flattening-subframesets-diffs.txt

[Eric Seidel (no email)]

Actually, failed twice in a row:
http://build.webkit.org/results/Qt%20Linux%20Release/r57584%20(10080)/fast/frames/flattening/iframe-flattening-offscreen-diffs.txt

Someone probably got a "you broke it" sheriff-bot email as a result. :(

[Eric Seidel (no email)]

Nevermind, similar failure, different test.

[Csaba Osztrogonác]

I'm worry because of this falkiness, because it occured 5 times during the last 20 builds. We should fix it asap not to block commit queue or put this test into skipped list.

[Eric Seidel (no email)]

It's time to skip this test.   Too many false positives blocking the commit-queue and confusing folks looking at the bots.

[Csaba Osztrogonác]

(In reply to comment #9)
> It's time to skip this test.   Too many false positives blocking the
> commit-queue and confusing folks looking at the bots.

I agree with you, I'll do it.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58588 might have broken Tiger Intel Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/58587
http://trac.webkit.org/changeset/58588

[Csaba Osztrogonác]

(In reply to comment #11)
> http://trac.webkit.org/changeset/58588 might have broken Tiger Intel Release
> The following changes are on the blame list:
> http://trac.webkit.org/changeset/58587
> http://trac.webkit.org/changeset/58588

58587 caused it, not me.

[Csaba Osztrogonác]

I forgot to mention, that I skipped the test until fix.
http://trac.webkit.org/changeset/58588

[Jesus Sanchez-Palencia]

I've just finish running this test. I did a 100,000 iterations round and then a 10 million one and I got no error at all. Not a single time...

I did this on an environment quite similar to the build bot (Debian Lenny, Qt 4.6, the package list provided by Ossy, etc) and I was going to debug it and try to fix, but without fails I can't be of any help. =/

I have no idea of what is causing the random crashes.

[Jesus Sanchez-Palencia]

(In reply to comment #14)
> I've just finish running this test. I did a 100,000 iterations round and then a
> 10 million one and I got no error at all. Not a single time...
> 
> I did this on an environment quite similar to the build bot (Debian Lenny, Qt
> 4.6, the package list provided by Ossy, etc) and I was going to debug it and
> try to fix, but without fails I can't be of any help. =/
> 
> I have no idea of what is causing the random crashes.


After talking to Ossy I ran a new round of tests with "run-webkit-tests fast/frames/flattening --iterations 10000 --exit-after-n-failures 1"

And I had no failures again. I'll let it running a 1 million iterations round during the night...

[Antonio Gomes]

> After talking to Ossy I ran a new round of tests with "run-webkit-tests
> fast/frames/flattening --iterations 10000 --exit-after-n-failures 1"
> 
> And I had no failures again. I'll let it running a 1 million iterations round
> during the night...

jesus, if it is a sideeffect of any other test, you wont catch it by running this test alone repeatedly. How are you running?

Maybe running some tests (folders) that are known to be ran before it, so it can trigger the bad effect(?)

[Csaba Osztrogonác]

Hooray, I catched the bug. :))

layoutTestController.display() in fast/frames/flattening/frameset-flattening-subframesets.html is the culprit.

layoutTestController.display() needs communication with the X server,
and I think this delay might cause failing sometimes. (If I increased
the nice value of X server, fail occured more often.)

I commented layoutTestController.display() in fast/frames/flattening/frameset-flattening-subframesets.html, and it works correctly, without any fail. I ran
fast/frames/flattening tests with--iterations 10000 option. Do we need
display() for this test? Or can we simple remove it from the test case?

[Csaba Osztrogonác]

(In reply to comment #17)
> Hooray, I catched the bug. :))
I meant fast/frames/flattening/frameset-flattening-subframe-resize.html is the culprit.

fast/frames/flattening/frameset-flattening-subframe-resize.html skipped until fix: http://trac.webkit.org/changeset/58711

[Kenneth Rohde Christiansen]

(In reply to comment #18)
> (In reply to comment #17)
> > Hooray, I catched the bug. :))
> I meant fast/frames/flattening/frameset-flattening-subframe-resize.html is the
> culprit.
> 
> fast/frames/flattening/frameset-flattening-subframe-resize.html skipped until
> fix: http://trac.webkit.org/changeset/58711

I think I raed you fixed this one by removing the call to display()? Can we close if that is right?

[Csaba Osztrogonác]

(In reply to comment #19)
> I think I raed you fixed this one by removing the call to display()? Can we
> close if that is right?
I asked, because I'm not sure if removing the call to display is the correct solution. It works for me locally, but is it correct? If it's OK, we can remove it, and unskip fast/frames/flattening/frameset-flattening-subframe-resize.html and then close the bug.

[Kenneth Rohde Christiansen]

display was used to force the layout, but it seems that it is not needed, so it should be OK to remove. At least I think it is worth a try.

[Antonio Gomes]

(In reply to comment #21)
> display was used to force the layout, but it seems that it is not needed, so it
> should be OK to remove. At least I think it is worth a try.

there are many other ways to "force a layout" used throughout LayoutTests/*

document.body.offsetTop

... is one of the common ones.

$ grep -nHR layout LayoutTests/ --include=*.html | grep -i force

maybe switching "LayoutTestController.display()" in favor of one of them?

[Kenneth Rohde Christiansen]

(In reply to comment #22)
> (In reply to comment #21)
> > display was used to force the layout, but it seems that it is not needed, so it
> > should be OK to remove. At least I think it is worth a try.
> 
> there are many other ways to "force a layout" used throughout LayoutTests/*
> 
> document.body.offsetTop
> 
> ... is one of the common ones.
> 
> $ grep -nHR layout LayoutTests/ --include=*.html | grep -i force
> 
> maybe switching "LayoutTestController.display()" in favor of one of them?

document.body.offsetTop is already called, that invalidates the layout, but doesn't necessarily to a relayout, which is only done on paint.

[Eric Seidel (no email)]

(In reply to comment #23)
> document.body.offsetTop is already called, that invalidates the layout, but
> doesn't necessarily to a relayout, which is only done on paint.

That's not correct.  Accessing many javascript properties, including document.body.offsetTop will guarantee that a layout (and thus also a style resolve) has completed.

We should not be using .display() for ensuring layout.  It's used for ensuring paint for for testing specific paint bugs.

That said, if .display() is broken on these bots, then other tests will break.

[Kenneth Rohde Christiansen]

(In reply to comment #24)
> (In reply to comment #23)
> > document.body.offsetTop is already called, that invalidates the layout, but
> > doesn't necessarily to a relayout, which is only done on paint.
> 
> That's not correct.  Accessing many javascript properties, including
> document.body.offsetTop will guarantee that a layout (and thus also a style
> resolve) has completed.
> 
> We should not be using .display() for ensuring layout.  It's used for ensuring
> paint for for testing specific paint bugs.
> 
> That said, if .display() is broken on these bots, then other tests will break.

Doesn't it just result in a call to setNeedsLayout(), thus marking a layout? Qt calls layoutIfNeeded() during paint. If you are right, that is great, because then we can just remove the display() call, but as you said, that won't fix other tests using display().

[Antonio Gomes]

Created attachment 55740 [details]
patch

Committed r59165: <http://trac.webkit.org/changeset/59165>

[Antonio Gomes]

I uploded the patch to make cherry-picking easier

[WebKit Review Bot]

http://trac.webkit.org/changeset/59165 might have broken Chromium Win Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/59168
http://trac.webkit.org/changeset/59169
http://trac.webkit.org/changeset/59170
http://trac.webkit.org/changeset/59165
http://trac.webkit.org/changeset/59166
http://trac.webkit.org/changeset/59167

[Simon Hausmann]

Revision r59165 cherry-picked into qtwebkit-2.0 with commit 79d424c6674d65561b1d926d04b9c8c52fd51384