31657 2009-11-18 18:43:07 -0800 MessageQueue::removeIf() has a bug that leads to failed assertions 2011-07-27 09:18:50 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 dumi dumi dglazkov kbalazs michaeln oldest_to_newest 164872 0 dumi 2009-11-18 18:43:07 -0800 MessageQueue::removeIf() has a bug: DequeConstIterator<DataType*> found = m_queue.end(); // Adds 'found' to the list of iterators of 'm_queue' while ((found = m_queue.findIf(predicate) != m_queue.end()) { // operator=() checks that found.m_queue is not NULL DataType* message = *found; m_queue.remove(found); // triggers m_queue.invalidateIteratorators() which sets it.m_queue = NULL for all current iterators of 'm_queue', including 'found' delete message; } So the first time the loop is run, everything is fine. But then the element to which 'found' points is removed from the list. This triggers Deque::invalidateIterators(), which sets found.m_queue = NULL. The second time 'found = m_queue.findIf(predicate)' is executed, DequeConstIterator::operator=() is called, which eventually calls DequeIteratorBase::operator=(), which ASSERTs that 'found.m_queue' is not NULL. This ASSERT fails. So any time this method is called with a predicate that matches at least one element in the queue, we get an assertion failure. Solution: Do not carry over the same DequeConstIterator variable from one run of the loop to the other, i.e. declare 'found' inside the loop. 164875 1 43477 dumi 2009-11-18 18:52:31 -0800 Created attachment 43477 patch 164893 2 43477 dimich 2009-11-18 21:23:06 -0800 Comment on attachment 43477 patch Cool catch! r=me Please consider also this shape of the loop, it seems a bit simpler (if you agree, it can be changed on landing): while (true) { DequeConstIterator<DataType*> found = m_queue.findIf(predicate); if (found == m_queue.end()) break; DataType* message = *found; m_queue.remove(found); delete message; } 165039 3 dumi 2009-11-19 11:51:35 -0800 (In reply to comment #2) > (From update of attachment 43477 [details]) > Cool catch! r=me > > Please consider also this shape of the loop, it seems a bit simpler (if you > agree, it can be changed on landing): > > while (true) { > DequeConstIterator<DataType*> found = m_queue.findIf(predicate); > if (found == m_queue.end()) > break; > > DataType* message = *found; > m_queue.remove(found); > delete message; > } Changing the loop as you suggested. Wasn't sure if 'while (true)' was acceptable in WebKit code. 165047 4 dumi 2009-11-19 12:00:24 -0800 Landed as r51198. 442966 5 kbalazs 2011-07-27 09:18:50 -0700 I have found a similar bug: https://bugs.webkit.org/show_bug.cgi?id=65263 43477 2009-11-18 18:52:31 -0800 2009-11-18 21:23:05 -0800 patch patch text/plain 1663 dumi SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDUxMTY2KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMDktMTEtMTggIER1bWl0cnUg RGFuaWxpdWMgIDxkdW1pQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBGaXhpbmcgYSBidWcgaW4gTWVzc2FnZVF1ZXVlOjpyZW1v dmVJZigpIHRoYXQgbGVhZHMgdG8gYW4KKyAgICAgICAgYXNzZXJ0aW9uIGZhaWx1cmUuCisKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTMxNjU3CisKKyAg ICAgICAgKiB3dGYvTWVzc2FnZVF1ZXVlLmg6CisgICAgICAgIChXVEY6Ok1lc3NhZ2VRdWV1ZTo6 cmVtb3ZlSWYpOgorCiAyMDA5LTExLTE4ICBMYXN6bG8gR29tYm9zICA8bGFzemxvLjEuZ29tYm9z QG5va2lhLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBLZW5uZXRoIFJvaGRlIENocmlzdGlh bnNlbi4KSW5kZXg6IEphdmFTY3JpcHRDb3JlL3d0Zi9NZXNzYWdlUXVldWUuaAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBKYXZhU2NyaXB0Q29yZS93dGYvTWVzc2FnZVF1ZXVlLmgJKHJldmlzaW9uIDUxMTYzKQor KysgSmF2YVNjcmlwdENvcmUvd3RmL01lc3NhZ2VRdWV1ZS5oCSh3b3JraW5nIGNvcHkpCkBAIC0x NzMsMTEgKzE3MywxNiBAQCBuYW1lc3BhY2UgV1RGIHsKICAgICBpbmxpbmUgdm9pZCBNZXNzYWdl UXVldWU8RGF0YVR5cGU+OjpyZW1vdmVJZihQcmVkaWNhdGUmIHByZWRpY2F0ZSkKICAgICB7CiAg ICAgICAgIE11dGV4TG9ja2VyIGxvY2sobV9tdXRleCk7Ci0gICAgICAgIERlcXVlQ29uc3RJdGVy YXRvcjxEYXRhVHlwZSo+IGZvdW5kID0gbV9xdWV1ZS5lbmQoKTsKLSAgICAgICAgd2hpbGUgKChm b3VuZCA9IG1fcXVldWUuZmluZElmKHByZWRpY2F0ZSkpICE9IG1fcXVldWUuZW5kKCkpIHsKLSAg ICAgICAgICAgIERhdGFUeXBlKiBtZXNzYWdlID0gKmZvdW5kOwotICAgICAgICAgICAgbV9xdWV1 ZS5yZW1vdmUoZm91bmQpOwotICAgICAgICAgICAgZGVsZXRlIG1lc3NhZ2U7CisgICAgICAgIC8v IFNlZSBidWcgMzE2NTcgZm9yIHdoeSB0aGlzIGxvb3AgbG9va3Mgc28gd2VpcmQKKyAgICAgICAg Ym9vbCBkb25lID0gZmFsc2U7CisgICAgICAgIHdoaWxlICghZG9uZSkgeworICAgICAgICAgICAg RGVxdWVDb25zdEl0ZXJhdG9yPERhdGFUeXBlKj4gZm91bmQgPSBtX3F1ZXVlLmZpbmRJZihwcmVk aWNhdGUpOworICAgICAgICAgICAgaWYgKGZvdW5kICE9IG1fcXVldWUuZW5kKCkpIHsKKyAgICAg ICAgICAgICAgICBEYXRhVHlwZSogbWVzc2FnZSA9ICpmb3VuZDsKKyAgICAgICAgICAgICAgICBt X3F1ZXVlLnJlbW92ZShmb3VuZCk7CisgICAgICAgICAgICAgICAgZGVsZXRlIG1lc3NhZ2U7Cisg ICAgICAgICAgICB9IGVsc2UKKyAgICAgICAgICAgICAgICBkb25lID0gdHJ1ZTsKICAgICAgICB9 CiAgICAgfQogCg==