Bug 65414 – DequeIterator lacks default constructor

• Component: Web Template Framework
• Status: RESOLVED LATER

[Balazs Kelemen]

Pointed out in https://bugs.webkit.org/show_bug.cgi?id=65263#c9.

[Balazs Kelemen]

Created attachment 106405 [details]
proposed fix

[WebKit Review Bot]

Comment on attachment 106405 [details]
proposed fix

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

[WebKit Review Bot]

Comment on attachment 106405 [details]
proposed fix

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

[Darin Adler]

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.

[Balazs Kelemen]

(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.

[Balazs Kelemen]

> 
> 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.

[Balazs Kelemen]

(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.

[WebKit Review Bot]

Comment on attachment 106405 [details]
proposed fix

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

[WebKit Review Bot]

Comment on attachment 106405 [details]
proposed fix

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

[Balazs Kelemen]

(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?

[Darin Adler]

Probably a compiler bug.

[Hajime Morrita]

> 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.

[Balazs Kelemen]

(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.

[Balazs Kelemen]

Let it rot for a while.