| Summary: | REGRESSION (r181720): Unnecessary layout triggered any time animated GIF advances to a new frame | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> |
| Component: | Layout and Rendering | Assignee: | Darin Adler <darin> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | ap, buildbot, commit-queue, ddkilzer, esprehn+autocc, glenn, kling, kondapallykalyan, rniwa |
| Priority: | P2 | Keywords: | InRadar |
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: | |||
|
Description
Darin Adler
2015-06-06 22:01:40 PDT
Created attachment 254425 [details]
Patch
I had a *much* bigger patch, but then when it failed some tests I decided to scale back and concentrate on testing and fixing just this regression. Comment on attachment 254425 [details] Patch Attachment 254425 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5420209930240000 New failing tests: fast/images/animated-gif-no-layout.html Created attachment 254426 [details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Damn, looks like the test failed on the WebKit1 bot but succeeded on the WebKit2 bot. I predict I could make it pass by making the delay constant in the test longer, but of course that would make Alexey unhappy. I wonder what we should do with this. Yes, looking at the result I can see that the timer for the second stage of the test fired before the GIF advanced to a new frame, and that’s why the test failed. So a longer timer or faster GIF animation would be the pragmatic solution; not sure what the real long term solution would be. Created attachment 254428 [details]
Patch
Comment on attachment 254428 [details] Patch Attachment 254428 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5374759915225088 New failing tests: fast/images/animated-gif-no-layout.html Created attachment 254429 [details]
Archive of layout-test-results from ews103 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 254428 [details] Patch Attachment 254428 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5284660091289600 New failing tests: fast/images/animated-gif-no-layout.html Created attachment 254431 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
(In reply to comment #8) > Created attachment 254428 [details] > Patch There are no code changes with this patch, only a layout test. With the number of timers involved in this test, it seems nearly impossible to reason about its behavior. I would suggest some extra heavy local testing: run-webkit-tests the-test.html --guard-malloc --repeat 10000 -f --child-processes 15 (the number should be well above the number of cores including hyperthreading, so something like 40 for a 12-core Mac Pro). Plus a few variations - same but WK1, without GuardMalloc, maybe with --leaks. If the test can't survive that, chances are that it won't do good in regular testing. We should make EWS do that for all new tests. Long running tests that are mostly idle waiting for a timer are not a huge problem. With enough RAM, we run as many test processes as there are virtual cores, which is somewhat excessive, given that there are other processes involved, and that hyperthreading only kicks in when the main thread of execution stalls. So a test that takes 500ms shouldn't make the whole test suite 500ms slower, I think. I did a lot of local testing but I realize now it was all WebKit 2. (In reply to comment #13) > There are no code changes with this patch, only a layout test. Oops! That’s the real reason the test is failing. I forgot the bug fix in this second patch! Created attachment 254451 [details]
Patch
If we still experience any test flakiness, I suspect a longer delay will do the trick. I’m going to try Alexey’s recipe for stress testing the test. There really aren’t as many timers involved as you might think. The “force layout” stuff takes the timing of the layout timer out of the equation so it’s possibly just the GIF animation frame timer and the timeout in the test itself that are racing. Comment on attachment 254451 [details] Patch Attachment 254451 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5710859359748096 New failing tests: fast/images/animated-gif-no-layout.html Created attachment 254453 [details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
So the test works with WebKit 2 and not WebKit 1. It’s not a raciness issue necessarily. Here’s what I’ve learned: The test I based this new test on, gif-loop-count.html, doesn’t actually test anything unless it’s run as a pixel test. It passes when run as a pixel test under WebKit 2, but fails when run as a pixel test under WebKit 1. So it seems that animated GIFs don’t animate inside DumpRenderTree. That’s not new to this test, just wasn’t noticed before. Created attachment 254464 [details]
Patch
Comment on attachment 254464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254464&action=review r=me > Source/WebCore/testing/Internals.idl:301 > + readonly attribute unsigned long layoutCount; Shouldn't this be "unsigned int" instead of "unsigned long"? This is a nice addition btw, we should make more tests using it :) Comment on attachment 254464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254464&action=review >> Source/WebCore/testing/Internals.idl:301 >> + readonly attribute unsigned long layoutCount; > > Shouldn't this be "unsigned int" instead of "unsigned long"? > This is a nice addition btw, we should make more tests using it :) No, it's a quirk of IDL that unsigned long in the IDL file is how you write bindings for something that is unsigned int in our C++. Would be nice to fix that some day. I think it's because IDL was developed a long time ago and used short to mean 16 bit and long to mean 32 bit. Comment on attachment 254464 [details] Patch Clearing flags on attachment: 254464 Committed r185310: <http://trac.webkit.org/changeset/185310> All reviewed patches have been landed. Closing bug. |