Bug 65414 - DequeIterator lacks default constructor
Summary: DequeIterator lacks default constructor
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-30 12:25 PDT by Balazs Kelemen
Modified: 2011-10-03 03:57 PDT (History)
4 users (show)

See Also:


Attachments
proposed fix (4.74 KB, patch)
2011-09-06 04:30 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-30 12:25:56 PDT
Pointed out in https://bugs.webkit.org/show_bug.cgi?id=65263#c9.
Comment 1 Balazs Kelemen 2011-09-06 04:30:06 PDT
Created attachment 106405 [details]
proposed fix
Comment 2 WebKit Review Bot 2011-09-06 05:47:19 PDT
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 3 WebKit Review Bot 2011-09-06 06:48:32 PDT
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 4 Darin Adler 2011-09-06 08:47:07 PDT
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.
Comment 5 Balazs Kelemen 2011-09-07 00:57:59 PDT
(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.
Comment 6 Balazs Kelemen 2011-09-07 01:09:13 PDT
> 
> 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.
Comment 7 Balazs Kelemen 2011-09-07 01:09:50 PDT
(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 8 WebKit Review Bot 2011-09-07 19:51:27 PDT
Comment on attachment 106405 [details]
proposed fix

Attachment 106405 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9599942
Comment 9 WebKit Review Bot 2011-09-07 21:18:38 PDT
Comment on attachment 106405 [details]
proposed fix

Attachment 106405 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9615060
Comment 10 Balazs Kelemen 2011-09-08 03:44:19 PDT
(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?
Comment 11 Darin Adler 2011-09-09 18:27:25 PDT
Probably a compiler bug.
Comment 12 Hajime Morrita 2011-09-29 00:50:03 PDT
> 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.
Comment 13 Balazs Kelemen 2011-10-03 02:05:35 PDT
(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.
Comment 14 Balazs Kelemen 2011-10-03 03:57:24 PDT
Let it rot for a while.