Bug 65263 - MessageQueue::waitForMessageFilteredWithTimeout can triggers an assertion
Summary: MessageQueue::waitForMessageFilteredWithTimeout can triggers an assertion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords:
Depends on:
Blocks: 63531
  Show dependency treegraph
 
Reported: 2011-07-27 09:18 PDT by Balazs Kelemen
Modified: 2011-07-30 12:28 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.33 KB, patch)
2011-07-27 09:34 PDT, Balazs Kelemen
dimich: review+
Details | Formatted Diff | Diff
new solution (2.42 KB, patch)
2011-07-29 05:34 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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