[Webkit-unassigned] [Bug 128741] [curl] Basic SocketStream implementation for libcurl backend

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=128741


Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #227640|review?                     |review-
               Flag|                            |




--- Comment #18 from Brent Fulgham <bfulgham at webkit.org>  2014-04-16 12:10:37 PST ---
(From update of attachment 227640)
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() ?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.