Bug 73926 - [chromium] Race condition in CCLayerTreeHostTest shutdown
Summary: [chromium] Race condition in CCLayerTreeHostTest shutdown
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-06 08:59 PST by Sami Kyostila
Modified: 2011-12-14 18:03 PST (History)
4 users (show)

See Also:


Attachments
Patch to help reproducing bug (1020 bytes, patch)
2011-12-06 09:03 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (4.57 KB, patch)
2011-12-06 09:05 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (3.68 KB, patch)
2011-12-14 04:09 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (2.88 KB, patch)
2011-12-14 04:14 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2011-12-14 13:48 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sami Kyostila 2011-12-06 08:59:24 PST
CCLayerTreeHostTest shutdown races if endTest() is called from within the drawLayersOnCCThread() test hook.
Comment 1 Sami Kyostila 2011-12-06 09:03:57 PST
Created attachment 118057 [details]
Patch to help reproducing bug

Attached a patch that will make the bug more likely to trigger for debugging.
Comment 2 Sami Kyostila 2011-12-06 09:05:46 PST
Created attachment 118059 [details]
Patch
Comment 3 James Robinson 2011-12-08 19:53:13 PST
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.
Comment 4 James Robinson 2011-12-13 21:12:27 PST
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.
Comment 5 Sami Kyostila 2011-12-14 04:09:17 PST
Created attachment 119197 [details]
Patch
Comment 6 Sami Kyostila 2011-12-14 04:14:21 PST
Created attachment 119199 [details]
Patch

Looks like webkit-patch also picked up my debugging patch. Let's try that again.
Comment 7 James Robinson 2011-12-14 11:58:43 PST
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.
Comment 8 Sami Kyostila 2011-12-14 12:29:46 PST
(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?
Comment 9 James Robinson 2011-12-14 12:38:11 PST
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.
Comment 10 Sami Kyostila 2011-12-14 13:48:48 PST
Created attachment 119289 [details]
Patch

Extracted fix from bug 74376
Comment 11 James Robinson 2011-12-14 14:44:13 PST
Comment on attachment 119289 [details]
Patch

R=me
Comment 12 WebKit Review Bot 2011-12-14 18:03:14 PST
Comment on attachment 119289 [details]
Patch

Clearing flags on attachment: 119289

Committed r102863: <http://trac.webkit.org/changeset/102863>
Comment 13 WebKit Review Bot 2011-12-14 18:03:18 PST
All reviewed patches have been landed.  Closing bug.