Bug 65414

Created attachment 106405 [details] proposed fix

Comment on attachment 106405 [details] proposed fix Attachment 106405 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9594610

Comment on attachment 106405 [details] proposed fix Attachment 106405 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9592827

Comment on attachment 106405 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=106405&action=review > Source/JavaScriptCore/wtf/Deque.h:598 > + , m_next(0) > + , m_previous(0) I’m not sure I understand the full rationale for setting these in a debug build but not a release build. Will this help us catch mistakes we would otherwise miss? Or perhaps it will hide mistakes we might otherwise catch. > Source/JavaScriptCore/wtf/MainThread.cpp:-213 > - // We must redefine 'i' each pass, because the itererator's operator= > - // requires 'this' to be valid, and remove() invalidates all iterators The substance of this comment still seems correct, even if the word redefine is no longer accurate, so it’s not clear to me that it’s good to remove it.

(In reply to comment #4) > (From update of attachment 106405 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106405&action=review > > > Source/JavaScriptCore/wtf/Deque.h:598 > > + , m_next(0) > > + , m_previous(0) > > I’m not sure I understand the full rationale for setting these in a debug build but not a release build. Will this help us catch mistakes we would otherwise miss? Or perhaps it will hide mistakes we might otherwise catch. This is just to pass the assertions in removeFromIteratorList with an empty iterator: if (!m_deque) { ASSERT(!m_next); ASSERT(!m_previous); } Maybe ASSERT_DISABLED would be the appropriate condition. > > > Source/JavaScriptCore/wtf/MainThread.cpp:-213 > > - // We must redefine 'i' each pass, because the itererator's operator= > > - // requires 'this' to be valid, and remove() invalidates all iterators > > The substance of this comment still seems correct, even if the word redefine is no longer accurate, so it’s not clear to me that it’s good to remove it. I think this comment is not valid since http://trac.webkit.org/changeset/92050 - my bad to not updated this call site - so we can remove it.

> > Maybe ASSERT_DISABLED would be the appropriate condition. > On the other hand removeFromIteratorList is in #ifndef NDEBUG so I won't change this in the patch.

(In reply to comment #3) > (From update of attachment 106405 [details]) > Attachment 106405 [details] did not pass cr-mac-ews (chromium): > Output: http://queues.webkit.org/results/9592827 From the output it does not seems to be my fault.

Comment on attachment 106405 [details] proposed fix Attachment 106405 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9599942

Comment on attachment 106405 [details] proposed fix Attachment 106405 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9615060

(In reply to comment #9) > (From update of attachment 106405 [details]) > Attachment 106405 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/9615060 This is the build error: cc1plus: warnings being treated as errors /Users/balazs/WebKitGit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/MessageQueue.h: In member function 'void* WebCore::DatabaseThread::databaseThread()': /Users/balazs/WebKitGit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/MessageQueue.h:135: warning: 'found.WTF::DequeConstIterator<WebCore::DatabaseTask*, 0ul>::<anonymous>.WTF::DequeIteratorBase<WebCore::DatabaseTask*, 0ul>::m_index' may be used uninitialized in this function /Users/balazs/WebKitGit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/MessageQueue.h:135: note: 'found.WTF::DequeConstIterator<WebCore::DatabaseTask*, 0ul>::<anonymous>.WTF::DequeIteratorBase<WebCore::DatabaseTask*, 0ul>::m_index' was declared here Honestly I don't get it. It is coming from here: DequeConstIterator<DataType*> found; while (!m_killed && !timedOut && (found = m_queue.findIf(predicate)) == m_queue.end()) timedOut = !m_condition.timedWait(m_mutex, absoluteTime); found.m_index is uninitialized first bug the iterator is assigned before used and operator= sets the value. Maybe a compiler error?

Probably a compiler bug.

> Honestly I don't get it. It is coming from here: > > DequeConstIterator<DataType*> found; > while (!m_killed && !timedOut && (found = m_queue.findIf(predicate)) == m_queue.end()) > timedOut = !m_condition.timedWait(m_mutex, absoluteTime); > > found.m_index is uninitialized first bug the iterator is assigned before used and operator= sets the value. Maybe a compiler error? The default constructor of DequeConstIteratorBase doesn't initialized m_index, and this patch instantiates the constructor code, that causes the warning. You can add "m_index(0)" to the constructor definition.

(In reply to comment #12) > > Honestly I don't get it. It is coming from here: > > > > DequeConstIterator<DataType*> found; > > while (!m_killed && !timedOut && (found = m_queue.findIf(predicate)) == m_queue.end()) > > timedOut = !m_condition.timedWait(m_mutex, absoluteTime); > > > > found.m_index is uninitialized first bug the iterator is assigned before used and operator= sets the value. Maybe a compiler error? > > The default constructor of DequeConstIteratorBase doesn't initialized m_index, > and this patch instantiates the constructor code, that causes the warning. > You can add "m_index(0)" to the constructor definition. It does not change the fact that it is not used (read) before it has been initialized. The whole point is to avoid unnecessary initializations so adding "m_index(0)" is not a very good solution. I would rather keep the current code - where callers initialize iterators with the end value - if nobody has an idea how to trick over the warning. Maybe the current patch will work when Mac will have a newer gcc.

Let it rot for a while.