Bug 74623 - [chromium] Multithreaded compositor unit tests intermittently failing
Summary: [chromium] Multithreaded compositor unit tests intermittently failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-15 10:26 PST by Kenneth Russell
Modified: 2012-05-18 11:10 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.33 KB, patch)
2012-05-18 10:13 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Kenneth Russell 2011-12-15 11:17:38 PST
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
Comment 2 Kenneth Russell 2011-12-15 11:26:13 PST
Marking CCLayerTreeHostTestScrollSimple and CCLayerTreeHostTestScrollMultipleRedraw disabled until this is diagnosed.
Comment 3 Kenneth Russell 2011-12-15 11:34:31 PST
Committed r102970: <http://trac.webkit.org/changeset/102970>
Comment 4 James Robinson 2011-12-15 13:15:37 PST
When did these failures start?
Comment 5 Kenneth Russell 2011-12-15 13:23:49 PST
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.
Comment 6 James Robinson 2011-12-15 16:48:34 PST
I can repro on my mac locally. Investigating.
Comment 7 James Robinson 2011-12-15 18:08:18 PST
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...
Comment 8 James Robinson 2011-12-15 18:34:59 PST
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
Comment 9 Adrienne Walker 2011-12-16 12:04:16 PST
Committed r103089: <http://trac.webkit.org/changeset/103089>
Comment 10 Adrienne Walker 2011-12-16 12:07:17 PST
(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.
Comment 11 Kent Tamura 2012-05-09 00:36:29 PDT
CCLayerTreeHostTestAddAnimationWithTimingFunction.runMultiThread is also flaky only on Windows. I'll mark FLAKY_.
Comment 12 vollick 2012-05-09 08:56:56 PDT
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.
Comment 13 Nat Duca 2012-05-09 09:14:43 PDT
Kk. We'll need to find an owner for this. I'm swampified.
Comment 14 vollick 2012-05-18 10:13:07 PDT
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.
Comment 15 Adrienne Walker 2012-05-18 10:59:11 PDT
Comment on attachment 142731 [details]
Patch

R=me.  Thanks for looking into this.
Comment 16 James Robinson 2012-05-18 11:08:30 PDT
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?
Comment 17 WebKit Review Bot 2012-05-18 11:10:27 PDT
Comment on attachment 142731 [details]
Patch

Clearing flags on attachment: 142731

Committed r117608: <http://trac.webkit.org/changeset/117608>
Comment 18 WebKit Review Bot 2012-05-18 11:10:34 PDT
All reviewed patches have been landed.  Closing bug.