[webkit-reviews] review denied: [Bug 128741] [curl] Basic SocketStream implementation for libcurl backend : [Attachment 227640] Proposed patch 4

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Mátyás Mustoha
<mmatyas at inf.u-szeged.hu>'s request for review:
Bug 128741: [curl] Basic SocketStream implementation for libcurl backend
https://bugs.webkit.org/show_bug.cgi?id=128741

Attachment 227640: Proposed patch 4
https://bugs.webkit.org/attachment.cgi?id=227640&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=227640&action=review


Thanks for working on this!  I see a few memory leaks in the code, so I think
it needs a bit of work before we proceed.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:65
> +    memset(&hints, 0, sizeof(struct addrinfo));

Can't this just be sizeof(hints)? Less likely to break in the future if the
type of hints is changed.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:75
> +    const char* port = fastStrDup(portString.latin1().data());

I don't understand why we need to copy these strings. Are we concerned
hostnameString and portString won't be around long enough to make the function
call?

This is also leaking memory.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:79
> +	   return nullptr;

We just leaked hostname and port here.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:95
> +static int tryConnection(struct addrinfo* server)

Is it ever appropriate for 'server' to be null? If it is, you should check for
it and handle it.  If not, make it a reference.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:113
> +void SocketStreamHandle::dispatchConnectionOnMainThread(void* context)

Is it ever possible for context to be null? If not, make it a reference. If it
is, handle that case.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:127
> +    for (; server != NULL; server = server->ai_next) {

We don't use NULL in our code. This should be "server != nullptr"

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:128
> +	   if ((socketFD = tryConnection(server)))

server cannot be null, so why not just pass a const reference?

> Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:62
> +    DEFINE_STATIC_LOCAL(Mutex, mutex, ());

This is deprecated nwo (DEFINE_DEPRECATED_STATIC_LOCAL). You might want to use
the "NeverDeleted<>" template.

> Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:73
> +    for (auto entry : m_streamHandleMap) {

You are copying your map entries as you iterate.  You probably want "for (auto&
entry : m_streamHandleMap)

> Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:82
> +void SocketStreamHandleManager::add(SocketStreamHandle* stream)

These methods all take a pointer, which implies you need to check for nullptr.
But I don't think you ever enter this code with a nullptr, so why not just use
references?

> Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:84
> +    Mutex* mutex = SocketStreamHandleManager::sharedMutex();

It seems weird to return a pointer to the mutex. Why not just have an API like
"SocketStreamHandleManager::sharedMutex().lock() ?