65263 2011-07-27 09:18:08 -0700 MessageQueue::waitForMessageFilteredWithTimeout can triggers an assertion 2011-07-30 12:28:16 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 63531 1 kbalazs kbalazs dimich levin oldest_to_newest 442964 0 kbalazs 2011-07-27 09:18:08 -0700 Actually anything that assigns a value to an already initialized iterator of m_queue can trigger an invalid assertion. This has also spotted in https://bugs.webkit.org/show_bug.cgi?id=31657. waitForMessageFilteredWithTimeout has the following loop: while (!m_killed && !timedOut && (found = m_queue.findIf(predicate)) == m_queue.end()) timedOut = !m_condition.timedWait(m_mutex, absoluteTime); The situation that leads to an invalid assertion is the following: - this loop is waiting on m_condition - during that another thread calls something that modify m_queue that invalidate's it's iterators (i.e. Deque::invalidateIterators will be called on m_queue) - when the loop is awakening it will reassign the iterator 'found' that is in an invalidated state - DequeIteratorBase::operator= -> checkValidity -> ASSERT(m_queue) will fail The solution can be the same as in #31657 i.e. make the iterator local to the loop. There is no code in WebKit that currently triggers this. I triggered it with my patch that had been uploaded as https://bug-63531-attachments.webkit.org/attachment.cgi?id=101999. 442972 1 102150 kbalazs 2011-07-27 09:34:11 -0700 Created attachment 102150 Patch 443235 2 102150 dimich 2011-07-27 17:28:46 -0700 Comment on attachment 102150 Patch Looks good. Due to quite subtle effect here, could you put a small comment to the code with at least a ref to the bug # before landing? 443404 3 kbalazs 2011-07-28 01:21:31 -0700 (In reply to comment #2) > (From update of attachment 102150 [details]) > Looks good. Due to quite subtle effect here, could you put a small comment to the code with at least a ref to the bug # before landing? Sure. 443414 4 kbalazs 2011-07-28 02:13:02 -0700 By the way another idea has came to my mind. Maybe the more general solution would be to simply remove the checkValidity call from DequeIteratorBase::operator=. It's not an error if an iterator has an invalid value if it will be reassigned immediately. What do you think? 443899 5 102351 kbalazs 2011-07-29 05:34:19 -0700 Created attachment 102351 new solution 444059 6 102351 dimich 2011-07-29 13:13:32 -0700 Comment on attachment 102351 new solution Agreed. The check was added in http://trac.webkit.org/changeset/41068/. I've talked with David Levin and we both don't see why this check has to be there. Removal of it provides for cleaner code indeed. Thanks for finding a better solution! 444273 7 102351 kbalazs 2011-07-30 04:43:17 -0700 Comment on attachment 102351 new solution Clearing flags on attachment: 102351 Committed r92050: <http://trac.webkit.org/changeset/92050> 444274 8 kbalazs 2011-07-30 04:43:26 -0700 All reviewed patches have been landed. Closing bug. 444300 9 102351 darin 2011-07-30 10:55:41 -0700 Comment on attachment 102351 new solution View in context: https://bugs.webkit.org/attachment.cgi?id=102351&action=review > Source/JavaScriptCore/wtf/MessageQueue.h:175 > + DequeConstIterator<DataType*> found = m_queue.end(); Why are we initializing the iterator here? It's immediately overwritten on the next line of code? 444317 10 kbalazs 2011-07-30 12:28:16 -0700 (In reply to comment #9) > (From update of attachment 102351 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102351&action=review > > > Source/JavaScriptCore/wtf/MessageQueue.h:175 > > + DequeConstIterator<DataType*> found = m_queue.end(); > > Why are we initializing the iterator here? It's immediately overwritten on the next line of code? Good point. We need to add default constructor to DequeIterator to avoid the unecessary initialization. Filed a bug for this: https://bugs.webkit.org/show_bug.cgi?id=65414 102150 2011-07-27 09:34:11 -0700 2011-07-29 05:33:17 -0700 Patch bug-65263-20110727183409.patch text/plain 2383 kbalazs U3VidmVyc2lvbiBSZXZpc2lvbjogOTE4MzQKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDQy OWY4Y2RiZGE1NTk1NjJkYmE4ODI1OTQ3NWNiMTUyNWQ1ZDU3N2IuLjcwMjE4YTkxMTJhYjUwNDcz M2VlNzYyNmRjMmRjOTBkMzAwOWUyNjUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwpAQCAtMSwz ICsxLDEzIEBACisyMDExLTA3LTI3ICBCYWxhenMgS2VsZW1lbiAgPGtiYWxhenNAd2Via2l0Lm9y Zz4KKworICAgICAgICBNZXNzYWdlUXVldWU6OndhaXRGb3JNZXNzYWdlRmlsdGVyZWRXaXRoVGlt ZW91dCBjYW4gdHJpZ2dlcnMgYW4gYXNzZXJ0aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD02NTI2MworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgICogd3RmL01lc3NhZ2VRdWV1ZS5oOgorICAgICAgICAoV1RG Ojo6OndhaXRGb3JNZXNzYWdlRmlsdGVyZWRXaXRoVGltZW91dCk6IE1ha2UgdGhlIGl0ZXJhdG9y IGxvY2FsIHRvIHRoZSBsb29wLgorCiAyMDExLTA3LTI3ICBGaWxpcCBQaXpsbyAgPGZwaXpsb0Bh cHBsZS5jb20+CiAKICAgICAgICAgREZHIEpJVCBzcGVjdWxhdGlvbiBmYWlsdXJlIGNvZGUgcGVy Zm9ybXMgaW5jb3JyZWN0IGNvbnZlcnNpb25zIGluCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvd3RmL01lc3NhZ2VRdWV1ZS5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi9N ZXNzYWdlUXVldWUuaAppbmRleCAzZTQ4MTU0Nzc5NGU1Zjc4NGQ2YTdjMjhkMjM0MzBiYTI4ZjIy NTE5Li40N2VjZTQyODNjNDRhNWRlMGQ5M2E4ZDg5YjQ1ZGU0NjE2ZDgzZmFjIDEwMDY0NAotLS0g YS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL01lc3NhZ2VRdWV1ZS5oCisrKyBiL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS93dGYvTWVzc2FnZVF1ZXVlLmgKQEAgLTEzMiw5ICsxMzIsMTcgQEAgbmFt ZXNwYWNlIFdURiB7CiAgICAgICAgIE11dGV4TG9ja2VyIGxvY2sobV9tdXRleCk7CiAgICAgICAg IGJvb2wgdGltZWRPdXQgPSBmYWxzZTsKIAotICAgICAgICBEZXF1ZUNvbnN0SXRlcmF0b3I8RGF0 YVR5cGUqPiBmb3VuZCA9IG1fcXVldWUuZW5kKCk7Ci0gICAgICAgIHdoaWxlICghbV9raWxsZWQg JiYgIXRpbWVkT3V0ICYmIChmb3VuZCA9IG1fcXVldWUuZmluZElmKHByZWRpY2F0ZSkpID09IG1f cXVldWUuZW5kKCkpCisgICAgICAgIHdoaWxlICghbV9raWxsZWQgJiYgIXRpbWVkT3V0KSB7Cisg ICAgICAgICAgICBEZXF1ZUNvbnN0SXRlcmF0b3I8RGF0YVR5cGUqPiBmb3VuZCA9IG1fcXVldWUu ZmluZElmKHByZWRpY2F0ZSk7CisgICAgICAgICAgICBpZiAoZm91bmQgIT0gbV9xdWV1ZS5lbmQo KSkgeworICAgICAgICAgICAgICAgIE93blB0cjxEYXRhVHlwZT4gbWVzc2FnZSA9IGFkb3B0UHRy KCpmb3VuZCk7CisgICAgICAgICAgICAgICAgbV9xdWV1ZS5yZW1vdmUoZm91bmQpOworICAgICAg ICAgICAgICAgIHJlc3VsdCA9IE1lc3NhZ2VRdWV1ZU1lc3NhZ2VSZWNlaXZlZDsKKyAgICAgICAg ICAgICAgICByZXR1cm4gbWVzc2FnZS5yZWxlYXNlKCk7CisgICAgICAgICAgICB9CisKICAgICAg ICAgICAgIHRpbWVkT3V0ID0gIW1fY29uZGl0aW9uLnRpbWVkV2FpdChtX211dGV4LCBhYnNvbHV0 ZVRpbWUpOworICAgICAgICB9CiAKICAgICAgICAgQVNTRVJUKCF0aW1lZE91dCB8fCBhYnNvbHV0 ZVRpbWUgIT0gaW5maW5pdGVUaW1lKCkpOwogCkBAIC0xNDMsMTYgKzE1MSw5IEBAIG5hbWVzcGFj ZSBXVEYgewogICAgICAgICAgICAgcmV0dXJuIG51bGxwdHI7CiAgICAgICAgIH0KIAotICAgICAg ICBpZiAodGltZWRPdXQpIHsKLSAgICAgICAgICAgIHJlc3VsdCA9IE1lc3NhZ2VRdWV1ZVRpbWVv dXQ7Ci0gICAgICAgICAgICByZXR1cm4gbnVsbHB0cjsKLSAgICAgICAgfQotCi0gICAgICAgIEFT U0VSVChmb3VuZCAhPSBtX3F1ZXVlLmVuZCgpKTsKLSAgICAgICAgT3duUHRyPERhdGFUeXBlPiBt ZXNzYWdlID0gYWRvcHRQdHIoKmZvdW5kKTsKLSAgICAgICAgbV9xdWV1ZS5yZW1vdmUoZm91bmQp OwotICAgICAgICByZXN1bHQgPSBNZXNzYWdlUXVldWVNZXNzYWdlUmVjZWl2ZWQ7Ci0gICAgICAg IHJldHVybiBtZXNzYWdlLnJlbGVhc2UoKTsKKyAgICAgICAgQVNTRVJUKHRpbWVkT3V0KTsKKyAg ICAgICAgcmVzdWx0ID0gTWVzc2FnZVF1ZXVlVGltZW91dDsKKyAgICAgICAgcmV0dXJuIG51bGxw dHI7CiAgICAgfQogCiAgICAgdGVtcGxhdGU8dHlwZW5hbWUgRGF0YVR5cGU+Cg== 102351 2011-07-29 05:34:19 -0700 2011-07-30 10:55:41 -0700 new solution bug-65263-20110729143418.patch text/plain 2477 kbalazs U3VidmVyc2lvbiBSZXZpc2lvbjogOTE3NTAKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IGJj NTBkMjA1NTU0YTBiOTIyZjFhNGRhYWI2ZGVjODdiNzZiMzQzNWQuLmRlMzk5NDgyY2U4ZTMxNDFj YTYzZDJhNzVjNThjMTM4NjU5ZDJmN2UgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwpAQCAtMSwz ICsxLDE3IEBACisyMDExLTA3LTI5ICBCYWxhenMgS2VsZW1lbiAgPGtiYWxhenNAd2Via2l0Lm9y Zz4KKworICAgICAgICBNZXNzYWdlUXVldWU6OndhaXRGb3JNZXNzYWdlRmlsdGVyZWRXaXRoVGlt ZW91dCBjYW4gdHJpZ2dlcnMgYW4gYXNzZXJ0aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD02NTI2MworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgICogd3RmL0RlcXVlLmg6CisgICAgICAgIChXVEY6Ojo6b3Bl cmF0b3IpOiBEb24ndCBjaGVjayB0aGUgdmFsaWRpdHkgb2YgYW4gaXRlcmF0b3IKKyAgICAgICAg dGhhdCB3aWxsIGJlIHJlYXNzaWduZWQgcmlnaHQgbm93LgorICAgICAgICAqIHd0Zi9NZXNzYWdl UXVldWUuaDoKKyAgICAgICAgKFdURjo6OjpyZW1vdmVJZik6IFJldmVydCByNTExOTggYXMgSSBi ZWxlYXZlIHRoaXMgaXMgdGhlIGJldHRlcgorICAgICAgICBzb2x1dGlvbiBmb3IgdGhlIHByb2Js ZW0gdGhhdCB3YXMgc29sdmVkIGJ5IHRoYXQuCisKIDIwMTEtMDctMjYgIFNoaW55YSBLYXdhbmFr YSAgPHNoaW55YWtAZ29vZ2xlLmNvbT4KIAogICAgICAgICBBZGRlZCBhbiBpbnRlcmZhY2UgdG8g dGFrZSBJc1doaXRlU3BhY2VGdW5jdGlvblB0ci4KZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS93dGYvRGVxdWUuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvRGVxdWUuaApp bmRleCA3ZGM0NGRkNGZjMDZlZmVhMmZhZTFjMzY2NTU4YzU1YWVmYzFhMTdhLi4xZWQ0N2E4ODg0 YmE4NzM2YTNhZTc0NWMxOTgwZWE3YTdjM2FkNmIyIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvd3RmL0RlcXVlLmgKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi9EZXF1 ZS5oCkBAIC02MTMsNyArNjEzLDYgQEAgbmFtZXNwYWNlIFdURiB7CiAgICAgdGVtcGxhdGU8dHlw ZW5hbWUgVCwgc2l6ZV90IGlubGluZUNhcGFjaXR5PgogICAgIGlubGluZSBEZXF1ZUl0ZXJhdG9y QmFzZTxULCBpbmxpbmVDYXBhY2l0eT4mIERlcXVlSXRlcmF0b3JCYXNlPFQsIGlubGluZUNhcGFj aXR5Pjo6b3BlcmF0b3I9KGNvbnN0IEJhc2UmIG90aGVyKQogICAgIHsKLSAgICAgICAgY2hlY2tW YWxpZGl0eSgpOwogICAgICAgICBvdGhlci5jaGVja1ZhbGlkaXR5KCk7CiAgICAgICAgIHJlbW92 ZUZyb21JdGVyYXRvcnNMaXN0KCk7CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS93dGYvTWVzc2FnZVF1ZXVlLmggYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL01lc3NhZ2VR dWV1ZS5oCmluZGV4IDNlNDgxNTQ3Nzk0ZTVmNzg0ZDZhN2MyOGQyMzQzMGJhMjhmMjI1MTkuLmZl MDQ3MjJmMTBlMGM0OGRjODllNGQxNzM0MTFlZWJiODE0MGQ3MmMgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9KYXZhU2NyaXB0Q29yZS93dGYvTWVzc2FnZVF1ZXVlLmgKKysrIGIvU291cmNlL0phdmFTY3Jp cHRDb3JlL3d0Zi9NZXNzYWdlUXVldWUuaApAQCAtMTcyLDE2ICsxNzIsMTIgQEAgbmFtZXNwYWNl IFdURiB7CiAgICAgaW5saW5lIHZvaWQgTWVzc2FnZVF1ZXVlPERhdGFUeXBlPjo6cmVtb3ZlSWYo UHJlZGljYXRlJiBwcmVkaWNhdGUpCiAgICAgewogICAgICAgICBNdXRleExvY2tlciBsb2NrKG1f bXV0ZXgpOwotICAgICAgICAvLyBTZWUgYnVnIDMxNjU3IGZvciB3aHkgdGhpcyBsb29wIGxvb2tz IHNvIHdlaXJkCi0gICAgICAgIHdoaWxlICh0cnVlKSB7Ci0gICAgICAgICAgICBEZXF1ZUNvbnN0 SXRlcmF0b3I8RGF0YVR5cGUqPiBmb3VuZCA9IG1fcXVldWUuZmluZElmKHByZWRpY2F0ZSk7Ci0g ICAgICAgICAgICBpZiAoZm91bmQgPT0gbV9xdWV1ZS5lbmQoKSkKLSAgICAgICAgICAgICAgICBi cmVhazsKLQorICAgICAgICBEZXF1ZUNvbnN0SXRlcmF0b3I8RGF0YVR5cGUqPiBmb3VuZCA9IG1f cXVldWUuZW5kKCk7CisgICAgICAgIHdoaWxlICgoZm91bmQgPSBtX3F1ZXVlLmZpbmRJZihwcmVk aWNhdGUpKSAhPSBtX3F1ZXVlLmVuZCgpKSB7CiAgICAgICAgICAgICBEYXRhVHlwZSogbWVzc2Fn ZSA9ICpmb3VuZDsKICAgICAgICAgICAgIG1fcXVldWUucmVtb3ZlKGZvdW5kKTsKICAgICAgICAg ICAgIGRlbGV0ZSBtZXNzYWdlOwotICAgICAgIH0KKyAgICAgICAgfQogICAgIH0KIAogICAgIHRl bXBsYXRlPHR5cGVuYW1lIERhdGFUeXBlPgo=