WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
65263
MessageQueue::waitForMessageFilteredWithTimeout can triggers an assertion
https://bugs.webkit.org/show_bug.cgi?id=65263
Summary
MessageQueue::waitForMessageFilteredWithTimeout can triggers an assertion
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+
Details
Formatted Diff
Diff
new solution
(2.42 KB, patch)
2011-07-29 05:34 PDT
,
Balazs Kelemen
no flags
Details
Formatted Diff
Diff
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.
Top of Page
Format For Printing
XML
Clone This Bug