| Summary: | [Qt] fast/frames/flattening/frameset-flattening-subframesets.html fails intermittently on Qt bot | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||
| Component: | Layout and Rendering | Assignee: | QtWebKit Unassigned <webkit-qt-unassigned> | ||||
| Status: | CLOSED FIXED | ||||||
| Severity: | Critical | CC: | abarth, eric, hausmann, jesus, kenneth, tonikitoo, webkit.review.bot | ||||
| Priority: | P1 | Keywords: | Qt | ||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | PC | ||||||
| OS: | All | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 33297, 35784 | ||||||
| Attachments: |
|
||||||
|
Description
Csaba Osztrogonác
2010-04-09 08:37:48 PDT
That new result is wrong, strange, I dont get this locally. It should be 900 and not 800, or it didnt do the flattening. (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. Is that also the case for the mac? (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. Failed again just now: http://build.webkit.org/results/Qt%20Linux%20Release/r57583%20(10079)/fast/frames/flattening/frameset-flattening-subframesets-diffs.txt 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. :( Nevermind, similar failure, different test. 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. It's time to skip this test. Too many false positives blocking the commit-queue and confusing folks looking at the bots. (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. 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 (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. I forgot to mention, that I skipped the test until fix. http://trac.webkit.org/changeset/58588 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. (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...
> 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(?)
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? (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 (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? (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. 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. (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? (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. (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. (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(). Created attachment 55740 [details] patch Committed r59165: <http://trac.webkit.org/changeset/59165> I uploded the patch to make cherry-picking easier 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 Revision r59165 cherry-picked into qtwebkit-2.0 with commit 79d424c6674d65561b1d926d04b9c8c52fd51384 |