Bug 74623 – [chromium] Multithreaded compositor unit tests intermittently failing

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kenneth Russell]

See:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/10716
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/10716/steps/webkit_unit_tests/logs/runMultiThread

[Kenneth Russell]

More of the multithreaded compositor tests than just this one are failing, for example CCLayerTreeHostTestScrollMultipleRedraw. See:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28CG%29/builds/3459/steps/webkit_unit_tests

[Kenneth Russell]

Marking CCLayerTreeHostTestScrollSimple and CCLayerTreeHostTestScrollMultipleRedraw disabled until this is diagnosed.

[Kenneth Russell]

Committed r102970: <http://trac.webkit.org/changeset/102970>

[James Robinson]

When did these failures start?

[Kenneth Russell]

Here's the first build where they were seen on this particular bot:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/10706

No suspect changes.

[James Robinson]

I can repro on my mac locally. Investigating.

[James Robinson]

With --gtest_repeat=1000 this fails at 102875 (just before Nat's inhibitDraw patch) and passes at 102863 (just after Sami's CCScopedThreadProxy patch). I don't see any other patches in that interval that have anything to do with the compositor specifically.  Will try bisecting...

[James Robinson]

I take that back, now I'm seeing zero failures just before Nat's patch and a consistent failure with it (with --gtest_repeat=1000).  It hangs even if I neuter most of the patch (make CCThreadProxy::canDraw() unconditionally return true, remove the CCLayerTreeHost changes, etc).

Nat could you take over? I've got a few things to juggle. The command line I'm using is:

./out/Debug/webkit_unit_tests --gtest_filter=CCLayer* --gtest_repeat=1000 --gtest_break_on_failure

[Adrienne Walker]

Committed r103089: <http://trac.webkit.org/changeset/103089>

[Adrienne Walker]

(In reply to comment #9)
> Committed r103089: <http://trac.webkit.org/changeset/103089>

The setNeedsCommit1 test has also been intermittently timing out on mac, see: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.5%20%28CG%29/builds/1787/steps/webkit_unit_tests/logs/runMultiThread

I am not totally convinced that it is related to these other failures, but it seems possible, so please file it as a separate bug if not.

[Kent Tamura]

CCLayerTreeHostTestAddAnimationWithTimingFunction.runMultiThread is also flaky only on Windows. I'll mark FLAKY_.

Looking into a failure in CCLayerTreeHostTestWriteLayersRedraw.runMultiThread, I noticed something about how the tests shut down.

The test failed because there were more commits than expected. The commit counter is only incremented in a function which calls endTest(). So we're processing more tasks after endTest(). Looking at how endTest is implemented, this seems very possible -- if we're on the impl thread, we post a task to the main thread's message loop to actually get the test ended. Before that task is processed, we could process a bunch of tasks on either the main thread or the impl thread message loop. Also, we shut down the message loop by calling Quit, which IIRC processes any remaining tasks.

It seems like the ideal behaviour is that upon calling endTest(), any existing tasks scheduled for the message loops get dropped on the floor, and then we post any tasks required for test shutdown.

[Nat Duca]

Kk. We'll need to find an owner for this. I'm swampified.

Created attachment 142731 [details]
Patch

The flakiness of the CCLayerTreeHost tests stem from processing tasks after the
tests finish. To fix this, I've disabled the dispatch* methods after endTest is
called, effectively preventing any further tasks scheduled by the tests from
being processed. I have also reworked the checks in
CCLayerTreeHostTestWriteLayersRedraw to make the test more robust.

[Adrienne Walker]

Comment on attachment 142731 [details]
Patch

R=me.  Thanks for looking into this.

[James Robinson]

Comment on attachment 142731 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142731&action=review

Thanks for looking into this!  Patch looks good, but if you can think of a way to fold some of the boilerplate together that'd be even better.  At a bare minimum, I doubt we need completely separate implementations of dispatchSetVisible() and dispatchSetInvisible().

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:381
>      static void dispatchSetNeedsAnimate(void* self)

we have so many nearly-identical copies of this now not-quite-trivial function - is there any way we can fold them together?

[WebKit Review Bot]

Comment on attachment 142731 [details]
Patch

Clearing flags on attachment: 142731

Committed r117608: <http://trac.webkit.org/changeset/117608>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.