Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd a (broken) Acid2 reftest. Fix #1993 #2741
Conversation
hoppipolla-critic-bot
commented
Jul 1, 2014
|
Critic review: https://critic.hoppipolla.co.uk/r/1941 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
|
Symptoms of the "infinite loop": Servo is unresponsive to input and uses ~200% CPU (i.e. two CPU-bound threads). Output with |
|
@SimonSapin, are you planning to revive this PR? |
|
So, Acid2 is still broken (although in a different way) but it seems to render reliably now (instead of intermittently going into an infinite loop or rendering a white page.) I’ve rebased this and updated the expected rendering. This should be ready to merge now. r? @Ms2ger |
|
|
|
Can we check this again with a rebase? There are some changes (e.g., #3102 ) that fix some memory corruption issues that we causing us major graphics painting issues on some platforms. |
|
This is what I’m getting on Linux: Note the 1px horizontal white line near the chin. That line is not white on Travis OS X. Travis Linux is different, but I don’t know how. |
|
Rebased: https://travis-ci.org/servo/servo/builds/32797282 Doesn’t seem to change much compared to last week. |
The rendering is incorrect
and the test is marked as flaky on Linux or on GPU rendering,
but this should help catch further regressions.
The test is modified to add the following CSS:
.intro { display: none }
html #top { margin-top: 0 }
To disable the "scrolling" part of the test.
The reference is modified to match the current rendering of the test:
* Rows 4 and 5 (eyes) have a red background.
* The nose is not quite where it should be.
* Row 12’s height is 3px more than it should be.
(See http://www.webstandards.org/action/acid2/guide/ for row numbers.)
The nose issue seems be related to rounding vs truncating
when pixel-snapping borders, but this is only a guess.
|
Another attempt: this time the test is marked with |
|
I bisected this yesterday and the white line started appearing on my linux box with this commit: 81f5da9 It's a big diff, but I will try to look into it over the next day or so to narrow down the exact cause. |
|
r+ |
SimonSapin commentedJul 1, 2014
Do not merge yet. This new test is very much intermittent, at least on my machine. Just as frequently as it passes, it test sends Servo in an infinite loop or just gives an empty (white) output (and obviously fails.)
@pcwalton, any idea what we could do about this?
The rendering is incorrect, but this should help catch further regressions.
The test is modified to add the following CSS:
This works around the lack of layer pixel snapping. (See #2035)
The reference is modified to match the current rendering of the test:
(See http://www.webstandards.org/action/acid2/guide/ for row numbers.)
The nose issue seems be related to rounding vs truncating when pixel-snapping borders, but this is only a guess.
Adding
.chin div { font-size: 3px }works around the chin issue (rows 12 to 14) for reasons that escape me yet.