|Summary:||MessageQueue::waitForMessageFilteredWithTimeout can triggers an assertion|
|Product:||WebKit||Reporter:||Balazs Kelemen <kbalazs>|
|Component:||Web Template Framework||Assignee:||Balazs Kelemen <kbalazs>|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description Balazs Kelemen 2011-07-27 09:18:08 PDT
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.
Comment 2 Dmitry Titov 2011-07-27 17:28:46 PDT
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?
Comment 3 Balazs Kelemen 2011-07-28 01:21:31 PDT
(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.
Comment 4 Balazs Kelemen 2011-07-28 02:13:02 PDT
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?
Comment 6 Dmitry Titov 2011-07-29 13:13:32 PDT
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 7 Balazs Kelemen 2011-07-30 04:43:17 PDT
Comment on attachment 102351 [details] new solution Clearing flags on attachment: 102351 Committed r92050: <http://trac.webkit.org/changeset/92050>
Comment 8 Balazs Kelemen 2011-07-30 04:43:26 PDT
All reviewed patches have been landed. Closing bug.
Comment 9 Darin Adler 2011-07-30 10:55:41 PDT
Comment 10 Balazs Kelemen 2011-07-30 12:28:16 PDT