Bug 65263

Summary: MessageQueue::waitForMessageFilteredWithTimeout can triggers an assertion
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: Web Template FrameworkAssignee: Balazs Kelemen <kbalazs>
Status: RESOLVED FIXED    
Severity: Normal CC: dimich, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 63531    
Attachments:
Description Flags
Patch
dimich: review+
new solution none

Balazs Kelemen
Reported 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.
Attachments
Patch (2.33 KB, patch)
2011-07-27 09:34 PDT, Balazs Kelemen 		dimich: review+
DetailsFormatted DiffDiff
new solution (2.42 KB, patch)
2011-07-29 05:34 PDT, Balazs Kelemen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Balazs Kelemen
Comment 1 2011-07-27 09:34:11 PDT
Created attachment 102150 [details] Patch
Dmitry Titov
Comment 2 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?
Balazs Kelemen
Comment 3 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.
Balazs Kelemen
Comment 4 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?
Balazs Kelemen
Comment 5 2011-07-29 05:34:19 PDT
Created attachment 102351 [details] new solution
Dmitry Titov
Comment 6 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!
Balazs Kelemen
Comment 7 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>
Balazs Kelemen
Comment 8 2011-07-30 04:43:26 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 9 2011-07-30 10:55:41 PDT
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?
Balazs Kelemen
Comment 10 2011-07-30 12:28:16 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.