Bug 120718 – Vector::releaseBuffer should return an OwnPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

Vector::releaseBuffer should return an OwnPtr

[Anders Carlsson]

Created attachment 210538 [details]
Patch

[WebKit Commit Bot]

Attachment 210538 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Vector.h', u'Source/WTF/wtf/text/StringBuffer.h', u'Source/WTF/wtf/text/StringImpl.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp']" exit_code: 1
Source/WTF/wtf/Vector.h:311:  The return type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WTF/wtf/Vector.h:491:  The return type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WTF/wtf/Vector.h:669:  The return type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 210538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210538&action=review

r=me

> Source/WTF/ChangeLog:9
> +        Change Vector::releaseBuffer() to return an OwnPtr. I intentionally chose
> +        to use an OwnPtr over a PassOwnPtr since we're trying to move away from PassOwnPtr objects.

The future is now!

[Anders Carlsson]

Committed r155121: <http://trac.webkit.org/changeset/155121>

[Csaba Osztrogonác]

FYI, It broke everything everywhere:
- Apple Windows: http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/38030
- GTK: http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/40434
- Qt: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/62669
- Maybe EFL too, but EFL bots haven't picked this revision up.

[Darin Adler]

Comment on attachment 210538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210538&action=review

> Source/WTF/wtf/Vector.h:316
> +        return adoptPtr(buffer);

This doesn’t seem write. If this was allocated with new, then adoptPtr makes sense, or if it was array new, then adoptArrayPtr makes sense. But I don’t think we have an adopt function that is designed to work with fastMalloc’d memory.

> Source/WTF/wtf/Vector.h:1164
> +        buffer = adoptPtr(static_cast<T*>(fastMalloc(bytes)));

This doesn’t look safe. Can we allocate something with fastMalloc and then delete it with delete and expect it to work?

> Source/WTF/wtf/text/StringBuffer.h:47
> +        m_data = adoptPtr(static_cast<CharType*>(fastMalloc(m_length * sizeof(CharType))));

Ditto.

> Source/WTF/wtf/text/StringBuffer.h:65
> +            m_data = adoptPtr(static_cast<UChar*>(fastRealloc(m_data.release().leakPtr(), newLength * sizeof(UChar))));

Ditto.

> Source/WTF/wtf/text/StringBuffer.h:82
> +        OwnPtr<CharType> data = m_data.release();
> +        return data.release();

Why the local variable?

[Darin Adler]

Comment on attachment 210538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210538&action=review

>> Source/WTF/wtf/Vector.h:316
>> +        return adoptPtr(buffer);
> 
> This doesn’t seem write. If this was allocated with new, then adoptPtr makes sense, or if it was array new, then adoptArrayPtr makes sense. But I don’t think we have an adopt function that is designed to work with fastMalloc’d memory.

s/write/right/

[Anders Carlsson]

(In reply to comment #6)
> (From update of attachment 210538 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210538&action=review
> 
> > Source/WTF/wtf/Vector.h:316
> > +        return adoptPtr(buffer);
> 
> This doesn’t seem write. If this was allocated with new, then adoptPtr makes sense, or if it was array new, then adoptArrayPtr makes sense. But I don’t think we have an adopt function that is designed to work with fastMalloc’d memory.

Yeah this is completely wrong. What we need is a smart pointer for fastMalloced memory. (Or we could just use new/delete).