Bug 145733 – REGRESSION (r181720): Unnecessary layout triggered any time animated GIF advances to a new frame

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

[Darin Adler]

REGRESSION (r181720): Unnecessary layout triggered any time animated GIF advances to a new frame

[Darin Adler]

Created attachment 254425 [details]
Patch

[Darin Adler]

<rdar://problem/21177253>

[Darin Adler]

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.

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

Created attachment 254428 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> Created attachment 254428 [details]
> Patch

There are no code changes with this patch, only a layout test.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Darin Adler]

I did a lot of local testing but I realize now it was all WebKit 2.

[Darin Adler]

(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!

[Darin Adler]

Created attachment 254451 [details]
Patch

[Darin Adler]

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.

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

So the test works with WebKit 2 and not WebKit 1. It’s not a raciness issue necessarily.

[Darin Adler]

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.

[Darin Adler]

Created attachment 254464 [details]
Patch

[Andreas Kling]

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 :)

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 254464 [details]
Patch

Clearing flags on attachment: 254464

Committed r185310: <http://trac.webkit.org/changeset/185310>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.