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.
Created attachment 102150 [details] Patch
Comment on attachment 102150 [details] 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?
(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.
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?
Created attachment 102351 [details] new solution
Comment on attachment 102351 [details] 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!
Comment on attachment 102351 [details] new solution Clearing flags on attachment: 102351 Committed r92050: <http://trac.webkit.org/changeset/92050>
All reviewed patches have been landed. Closing bug.
Comment on attachment 102351 [details] 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?
(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