Bug 128741 – [curl] Basic SocketStream implementation for libcurl backend

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mátyás Mustoha]

Add SocketStream implementation for libcurl backend, using POSIX function calls.

[Mátyás Mustoha]

This patch is working on the Nix port, but

[Mátyás Mustoha]

This patch works fine on Nix, but I couldn't try it out on EFL.
Could you test it, peavo?

For example, on http://websocketstest.com/ or on http://www.websocket.org/echo.html it should work fine, except the SSL/TSL support, which might not behave correctly yet.

(In reply to comment #2)
> This patch works fine on Nix, but I couldn't try it out on EFL.
> Could you test it, peavo?
> 
> For example, on http://websocketstest.com/ or on http://www.websocket.org/echo.html it should work fine, except the SSL/TSL support, which might not behave correctly yet.

I will test it, but I cannot find the patch.

[Mátyás Mustoha]

Created attachment 224191 [details]
Proposed patch

Sorry, somehow I forgot to upload it.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:86:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:88:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:92:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:132:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:76:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:79:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:98:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:105:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:119:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:119:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 11 in 8 files


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

There seems to be some include errors compiling WinCairo:

1>..\platform\network\curl\SocketStreamHandleCurl.cpp(43): fatal error C1083: Cannot open include file: 'arpa/inet.h': No such file or directory

The same goes for <netdb.h>, and <sys/socket.h>

1>..\platform\network\curl\SocketStreamHandleManager.cpp(34): fatal error C1083: Cannot open include file: 'sys/socket.h': No such file or directory

[Peter Gal]

(In reply to comment #6)
> There seems to be some include errors compiling WinCairo:
> 
> 1>..\platform\network\curl\SocketStreamHandleCurl.cpp(43): fatal error C1083: Cannot open include file: 'arpa/inet.h': No such file or directory
> 
> The same goes for <netdb.h>, and <sys/socket.h>
> 
> 1>..\platform\network\curl\SocketStreamHandleManager.cpp(34): fatal error C1083: Cannot open include file: 'sys/socket.h': No such file or directory

maybe we need the windows.h and winsock2.h includes on windows.

[Mátyás Mustoha]

Created attachment 225069 [details]
Proposed patch 2

I've added the Windows headers, this might fix the problem.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:91:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:93:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:97:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:137:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:82:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:85:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:104:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:111:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:125:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:125:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 11 in 8 files


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

[Csaba Osztrogonác]

Comment on attachment 225069 [details]
Proposed patch 2

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

> Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:3
> + * Copyright (C) 2014 University of Szeged
> + * All rights reserved.

Just a nit: Please use same copyright in all files - Copyright (C) 2014 University of Szeged. All rights reserved.

[Mátyás Mustoha]

Created attachment 225767 [details]
Proposed patch 3

I've updated the ChangeLog and fixed the license problem.
Peavo, did the Windows includes fix the build errors?

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:136:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:82:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:85:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:125:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:125:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 8 files


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

(In reply to comment #11)
> Created an attachment (id=225767) [details]
> Proposed patch 3
> 
> I've updated the ChangeLog and fixed the license problem.
> Peavo, did the Windows includes fix the build errors?

Maybe include <Ws2tcpip.h> ?

1>  SocketStreamHandleCurl.cpp
1>..\platform\network\curl\SocketStreamHandleCurl.cpp(75): error C2039: 'getaddrinfo' : is not a member of '`global namespace''
1>..\platform\network\curl\SocketStreamHandleCurl.cpp(75): error C3861: 'getaddrinfo': identifier not found
1>..\platform\network\curl\SocketStreamHandleCurl.cpp(85): error C2039: 'getnameinfo' : is not a member of '`global namespace''
1>..\platform\network\curl\SocketStreamHandleCurl.cpp(85): error C3861: 'getnameinfo': identifier not found
1>..\platform\network\curl\SocketStreamHandleCurl.cpp(104): error C2039: 'close' : is not a member of '`global namespace''
1>..\platform\network\curl\SocketStreamHandleCurl.cpp(104): error C3861: 'close': identifier not found
1>..\platform\network\curl\SocketStreamHandleCurl.cpp(130): error C3861: 'freeaddrinfo': identifier not found

1>  SocketStreamHandleManager.cpp
1>..\platform\network\curl\SocketStreamHandleManager.cpp(34): fatal error C1083: Cannot open include file: 'sys/socket.h': No such file or directory

[Mátyás Mustoha]

That's odd, 'sys/socket.h' shouldn't be included on Windows.
There are guards in order to use its Windows equivalents.

(In reply to comment #14)
> That's odd, 'sys/socket.h' shouldn't be included on Windows.
> There are guards in order to use its Windows equivalents.

My mistake, somehow I didn't manage to completely apply the patch :)

1>..\platform\network\curl\SocketStreamHandleManager.cpp(90): error C2039: 'close' : is not a member of '`global namespace''
1>..\platform\network\curl\SocketStreamHandleManager.cpp(90): error C3861: 'close': identifier not found
1>..\platform\network\curl\SocketStreamHandleManager.cpp(102): error C2065: 'ssize_t' : undeclared identifier
1>..\platform\network\curl\SocketStreamHandleManager.cpp(102): error C2146: syntax error : missing ';' before identifier 'readCount'
1>..\platform\network\curl\SocketStreamHandleManager.cpp(102): error C2065: 'readCount' : undeclared identifier
1>..\platform\network\curl\SocketStreamHandleManager.cpp(102): error C2039: 'read' : is not a member of '`global namespace''
1>..\platform\network\curl\SocketStreamHandleManager.cpp(102): error C3861: 'read': identifier not found
1>..\platform\network\curl\SocketStreamHandleManager.cpp(104): error C2065: 'readCount' : undeclared identifier
1>..\platform\network\curl\SocketStreamHandleManager.cpp(110): error C2065: 'readCount' : undeclared identifier
1>..\platform\network\curl\SocketStreamHandleManager.cpp(111): error C2065: 'readCount' : undeclared identifier
1>..\platform\network\curl\SocketStreamHandleManager.cpp(114): error C2065: 'readCount' : undeclared identifier
1>..\platform\network\curl\SocketStreamHandleManager.cpp(126): error C2065: 'ssize_t' : undeclared identifier
1>..\platform\network\curl\SocketStreamHandleManager.cpp(126): error C2146: syntax error : missing ';' before identifier 'sent'
1>..\platform\network\curl\SocketStreamHandleManager.cpp(126): error C2065: 'sent' : undeclared identifier
1>..\platform\network\curl\SocketStreamHandleManager.cpp(126): error C2039: 'write' : is not a member of '`global namespace''
1>..\platform\network\curl\SocketStreamHandleManager.cpp(126): error C3861: 'write': identifier not found
1>..\platform\network\curl\SocketStreamHandleManager.cpp(128): error C2065: 'sent' : undeclared identifier
1>..\platform\network\curl\SocketStreamHandleManager.cpp(134): error C2065: 'sent' : undeclared identifier
1>..\platform\network\curl\SocketStreamHandleManager.cpp(135): error C2065: 'sent' : undeclared identifier

[Mátyás Mustoha]

Created attachment 227640 [details]
Proposed patch 4

Sorry for the late reply, this patch should solve the problems.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:145:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:84:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:87:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:127:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:127:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 8 files


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

[Brent Fulgham]

Comment on attachment 227640 [details]
Proposed patch 4

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

[Basuke Suzuki]

This was implemented on WinCairo curl backend.