[chromium] Race condition in CCLayerTreeHostTest shutdown

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Sami Kyostila]

CCLayerTreeHostTest shutdown races if endTest() is called from within the drawLayersOnCCThread() test hook.

[Sami Kyostila]

Created attachment 118057 [details]
Patch to help reproducing bug

Attached a patch that will make the bug more likely to trigger for debugging.

[Sami Kyostila]

Created attachment 118059 [details]
Patch

[James Robinson]

A quite fun one.  Your condition sounds quite plausible.  I also think it's pretty scary to keep the message loop pumping after we tear down the layer tree host....

I'll have to sit down and think about this one carefully.

[James Robinson]

Comment on attachment 118059 [details]
Patch

Ah, I think the fix is much simpler - check the m_shutdown flag in CCScopedThreadProxy::runTaskIfNotShutdown() before touching the m_targetThread. I had to do this for another patch anyway for a similar issue.  I don't really like the idea of moving around the RunAllPendingMessages() thing here, since you'll have the same potential issue with the m_timeoutTask.

[Sami Kyostila]

Created attachment 119197 [details]
Patch

[Sami Kyostila]

Created attachment 119199 [details]
Patch

Looks like webkit-patch also picked up my debugging patch. Let's try that again.

[James Robinson]

Comment on attachment 119199 [details]
Patch

I'm biased, but I prefer the way I wrote this in https://bugs.webkit.org/attachment.cgi?id=119042&action=review (search for "runTaskIfNotShutdown") for a few minor reasons (we prefer early outs, it's better to take ownership of a PassOwnPtr<> ASAP, this is a bit non-obvious and deserves a comment). This does look correct, however. At the very least please add a comment the rest is up to you.

[Sami Kyostila]

(In reply to comment #7)
> (From update of attachment 119199 [details])
> I'm biased, but I prefer the way I wrote this in https://bugs.webkit.org/attachment.cgi?id=119042&action=review (search for "runTaskIfNotShutdown") for a few minor reasons (we prefer early outs, it's better to take ownership of a PassOwnPtr<> ASAP, this is a bit non-obvious and deserves a comment). This does look correct, however. At the very least please add a comment the rest is up to you.

I agree, your patch looks better for the reasons you cited, so let's go with that one.

Should we merge this bug with the other one to keep tabs on when the fix goes in?

[James Robinson]

I think landing this fix ahead of time makes sense since the other patch is large and has a large dependency, while this fix is isolated and useful on its own.

[Sami Kyostila]

Created attachment 119289 [details]
Patch

Extracted fix from bug 74376

[James Robinson]

Comment on attachment 119289 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 119289 [details]
Patch

Clearing flags on attachment: 119289

Committed r102863: <http://trac.webkit.org/changeset/102863>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.