54408 2011-02-14 12:31:43 -0800 Fix Mutex::tryLock() on Windows to work properly with PlatformCondition::timedWait() 2011-02-15 11:53:16 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 0 crogers webkit-unassigned ap aroben kbr sfalken oldest_to_newest 350822 0 crogers 2011-02-14 12:31:43 -0800 Fix Mutex::tryLock() on Windows to work properly with PlatformCondition::timedWait() 350823 1 82352 crogers 2011-02-14 12:33:20 -0800 Created attachment 82352 Patch 350825 2 crogers 2011-02-14 12:39:34 -0800 The bug I'm seeing without this fix is that tryLock() will always fail on Windows when another thread is waiting on a ThreadCondition. Here's the pattern: // Worker thread waiting to be signalled { MutexLocker locker(m_backgroundThreadLock); while (!m_moreInputBuffered && !m_wantsToExit) m_backgroundThreadCondition.wait(m_backgroundThreadLock); } . . . // Another thread has some work and tries to signal (using a tryLock()) // Not using a MutexLocker looks strange, but we use a tryLock() instead because this is run on the real-time // thread where it is a disaster for the lock to be contended (causes audio glitching). It's OK if we fail to // signal from time to time, since we'll get to it the next time we're called. We're called repeatedly // and frequently (around every 3ms). The background thread is processing well into the future and has a considerable amount of // leeway here... if (m_backgroundThreadLock.tryLock()) { // <----------------- this will alway FAIL currently on Windows m_moreInputBuffered = true; m_backgroundThreadCondition.signal(); m_backgroundThreadLock.unlock(); } 350833 3 82352 ap 2011-02-14 12:59:45 -0800 Comment on attachment 82352 Patch Seems fine to make this change, although the design seems surprising. Why is the boolean variable shared between threads needed at all? Can you just use wait/signal without a separate boolean variable? 350841 4 ap 2011-02-14 13:13:08 -0800 I'm wondering if we can use "fast mutexes" on windows instead: <http://msdn.microsoft.com/en-us/library/ms810047.aspx#locks__topic7>. 350842 5 crogers 2011-02-14 13:16:31 -0800 Thanks Alexey. I think you may be right about my use of the bool variable being redundant. I'll have to look closer at what I was thinking there and may be able to simplify that. But I'll land this fix first. I'm by no means a Windows programming expert so I'm not sure about the "fast mutexes"... 350885 6 kbr 2011-02-14 14:26:00 -0800 (In reply to comment #4) > I'm wondering if we can use "fast mutexes" on windows instead: <http://msdn.microsoft.com/en-us/library/ms810047.aspx#locks__topic7>. Those look like they're for implementing device drivers... 351439 7 crogers 2011-02-15 11:53:16 -0800 Committed r78597: <http://trac.webkit.org/changeset/78597> 82352 2011-02-14 12:33:20 -0800 2011-02-14 12:59:45 -0800 Patch bug-54408-20110214123319.patch text/plain 1316 crogers SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gNzg0OTYpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAK KzIwMTEtMDItMTQgIENocmlzIFJvZ2VycyAgPGNyb2dlcnNAZ29vZ2xlLmNvbT4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggTXV0ZXg6OnRyeUxv Y2soKSBvbiBXaW5kb3dzIHRvIHdvcmsgcHJvcGVybHkgd2l0aCBQbGF0Zm9ybUNvbmRpdGlvbjo6 dGltZWRXYWl0KCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTU0NDA4CisKKyAgICAgICAgKiB3dGYvVGhyZWFkaW5nV2luLmNwcDoKKyAgICAgICAgKFdU Rjo6UGxhdGZvcm1Db25kaXRpb246OnRpbWVkV2FpdCk6CisKIDIwMTEtMDItMTQgIFBhdmVsIFBv ZGl2aWxvdiAgPHBvZGl2aWxvdkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkg WXVyeSBTZW1pa2hhdHNreS4KSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvVGhyZWFk aW5nV2luLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL1RocmVh ZGluZ1dpbi5jcHAJKHJldmlzaW9uIDc4MzU5KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3d0 Zi9UaHJlYWRpbmdXaW4uY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zMzIsNiArMzMyLDcgQEAgYm9v bCBQbGF0Zm9ybUNvbmRpdGlvbjo6dGltZWRXYWl0KFBsYXRmbwogICAgIHJlcyA9IFJlbGVhc2VT ZW1hcGhvcmUobV9ibG9ja0xvY2ssIDEsIDApOwogICAgIEFTU0VSVChyZXMpOwogCisgICAgLS1t dXRleC5tX3JlY3Vyc2lvbkNvdW50OwogICAgIExlYXZlQ3JpdGljYWxTZWN0aW9uKCZtdXRleC5t X2ludGVybmFsTXV0ZXgpOwogCiAgICAgLy8gTWFpbiB3YWl0IC0gdXNlIHRpbWVvdXQuCkBAIC0z NjUsNiArMzY2LDcgQEAgYm9vbCBQbGF0Zm9ybUNvbmRpdGlvbjo6dGltZWRXYWl0KFBsYXRmbwog ICAgIH0KIAogICAgIEVudGVyQ3JpdGljYWxTZWN0aW9uICgmbXV0ZXgubV9pbnRlcm5hbE11dGV4 KTsKKyAgICArK211dGV4Lm1fcmVjdXJzaW9uQ291bnQ7CiAKICAgICByZXR1cm4gIXRpbWVkT3V0 OwogfQo=