Bug 31657 – MessageQueue::removeIf() has a bug that leads to failed assertions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

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.

[Dumitru Daniliuc]

Created attachment 43477 [details]
patch

[Dmitry Titov]

Comment on attachment 43477 [details]
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;
}

[Dumitru Daniliuc]

(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.

[Dumitru Daniliuc]

Landed as r51198.

[Balazs Kelemen]

I have found a similar bug: https://bugs.webkit.org/show_bug.cgi?id=65263