|Summary:||[chromium] Race condition in CCLayerTreeHostTest shutdown|
|Product:||WebKit||Reporter:||Sami Kyostila <skyostil>|
|Component:||Tools / Tests||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||cc-bugs, jamesr, nduca, webkit.review.bot|
|Version:||528+ (Nightly build)|
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 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 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 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.