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

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 1 Balazs Kelemen 2011-07-27 09:34:11 PDT
Created attachment 102150 [details]
Patch
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 5 Balazs Kelemen 2011-07-29 05:34:19 PDT
Created attachment 102351 [details]
new solution
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 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?
Comment 10 Balazs Kelemen 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