Bug 144628 – [Curl] WebSocket platform part is not implemented.

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

The Curl platform code for WebSockets has not been implemented yet.

Created attachment 252386 [details]
Patch

The patch passes all http websocket tests :)

[Darin Adler]

Comment on attachment 252386 [details]
Patch

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

Code looks OK, but lots of room for improvement.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:57
> +class SocketStreamHandle : public RefCounted<SocketStreamHandle>, public SocketStreamHandleBase {

Are we guaranteed that all the ref/deref are on the same thread? I am concerned that is not so.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:59
> +    static PassRefPtr<SocketStreamHandle> create(const URL& url, SocketStreamHandleClient* client) { return adoptRef(new SocketStreamHandle(url, client)); }

Should return Ref, not PassRefPtr.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:63
> +protected:

A class that is never used as a base class should not have any protected members at all. These should all be private, not protected.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:69
> +    bool readData(CURL* curlHandle);
> +    bool sendData(CURL* curlHandle);
> +    bool waitForAvailableData(CURL* curlHandle, int selectTimeoutMS);

These CURL* don’t need argument names in the header, and also, should be CURL&.

The selectTimeoutMS is an old fashioned choice and not what we should do in new code. A better type for the argument would be std::chrono::microseconds or perhaps std::chrono::milliseconds. Either way we would avoid all the comments about units and actually get compile time checking for it.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:72
> +    static void worker(void*);

This should be a lambda inside the member function that needs it, not a static member function.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:90
> +    class SocketData {

This seems like a peculiar class. Maybe it should just be a struct instead of a class.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:106
> +        const char* m_data;

This needs to be a std::unique_ptr<const char[]>; doing all the new and delete by hand is not a good idea.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:57
>      setClient(0);

nullptr

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:100
> +    char* data = new char[bufferSize];

Should use unique_ptr, not manual new/delete.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:108
> +        callOnMainThread(didReceiveData, this);

It would be better to use a lambda here rather than the mess with a static data member and a case of the data pointer inside it. If we had a thread safe reference count then we could use have the lambda capture a reference count to the handle, and then we would not need to use cancelCallOnMainThread in the destructor.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:222
> +    SocketStreamHandle* handle = reinterpret_cast<SocketStreamHandle*>(context);

This should be static_cast, not reinterpret_cast.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:233
> +    auto receiveData = m_receiveData;
> +    m_receiveData.clear();

This should use WTF::move rather than copy and clear. Much more efficient and required if we change SocketData to read-only, which we should do.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:237
> +    for (auto socketData : receiveData) {

This needs to be auto& to avoid copying the socket data.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:252
> +    SocketStreamHandle* handle = reinterpret_cast<SocketStreamHandle*>(context);

This should be static_cast instead.

Thanks for reviewing :) Will upload another patch soon.

Created attachment 252612 [details]
Patch

(In reply to comment #3)
> 
> > Source/WebCore/platform/network/curl/SocketStreamHandle.h:69
> > +    bool readData(CURL* curlHandle);
> > +    bool sendData(CURL* curlHandle);
> > +    bool waitForAvailableData(CURL* curlHandle, int selectTimeoutMS);
> 
> These CURL* don’t need argument names in the header, and also, should be
> CURL&.
> 

I didn't change the parameter type, as CURL& didn't compile, since CURL is of type void.

[Darin Adler]

Comment on attachment 252612 [details]
Patch

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

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:49
> +#include <wtf/threading.h>

This should have a capital "t".

The headers here aren’t sorted alphabetically the way the WebKit style guide says they should be.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:87
> +    class SocketData {

This should just be a struct, not a class. I don’t see us getting any benefit from making it a class.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:89
> +        SocketData(const char* data, int size)

I think it’s too subtle that this takes ownership of the data. Making this a struct might help.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:100
> +        SocketData(SocketData&& other)
> +        {
> +            m_data = WTF::move(other.m_data);
> +            m_size = other.m_size;
> +            other.m_size = 0;
> +        }

Is it important that the other’s size gets set to 0 when moving? If not, then I suggest we just let the compiler generate this.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:104
> +        ~SocketData()
> +        {
> +        }

No need to define this.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:69
> +    char* copy = new char[length];

This should go right into a std::unique_ptr when it’s allocated.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:101
> +        m_receiveData.append(SocketData(data.release(), bytesRead));

This shows us we have a poor design for SocketData. It’s not good to call unique_ptr.release just to put something into another unique_ptr. If SocketData was a struct we could just write:

    m_receiveData.append(SocketData { WTF::move(data), bytesRead });

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:127
> +        auto sendData = WTF::move(m_sendData.at(0));
> +        m_sendData.remove(0);

Should use takeFirst, not move/at/remove. And the fact that a Vector doesn’t have a takeFirst should make you realize that this should be a Duque, not a Vector.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:143
> +            char* copy = new char[restLength];

Should go right into a std::unique_ptr.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:173
> +    if (!m_workerThread) {

Early return would be better than nesting the entire function inside an if.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:174
> +        ref();

Need a comment explaining where the deref that matches this is.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:184
> +            CString host = m_url.host().ascii();

The String::ascii() function should not be used unless there is a guarantee that all characters are ASCII. I don’t think there is a guarantee that all characters from URL::host will be ASCII. Maybe you want latin1() or utf8()?

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:217
> +    if (m_workerThread) {

Early return would be better than nesting the entire function inside an if.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:218
> +        m_stopThread = true;

Probably needs to be an atomic bool, not just a normal bool.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:220
> +        m_workerThread = 0;

nullptr

Created attachment 252729 [details]
Patch

(In reply to comment #7)

Thanks again for helpful review :)

> 
> > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:220
> > +        m_workerThread = 0;
> 
> nullptr

I didn't change this, as m_workerThread does not seem to be of pointer type.

[Darin Adler]

Comment on attachment 252729 [details]
Patch

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

lets not use this unnecessary std::unique_ptr

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:48
> +#if PLATFORM(WIN)
> +#include <windows.h>
> +#include <winsock2.h>
> +#endif
> +
> +#include <curl/curl.h>
> +
> +#include <mutex>
> +
> +#include <wtf/Deque.h>
>  #include <wtf/RefCounted.h>
> +#include <wtf/Threading.h>

These aren’t sorted in the normal WebKit project way.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:88
> +        const char* data() const { return m_data.get(); }
> +        int size() const { return m_size; }

No need for these.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:90
> +        std::unique_ptr<const char[]> m_data;

Should just name this data.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:91
> +        int m_size;

Should just name this size. I also suggest initializing it to zero instead of leaving it uninitialized.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:99
> +    Deque<std::unique_ptr<SocketData>> m_sendData;
> +    Deque<std::unique_ptr<SocketData>> m_receiveData;

These should be Deque<SocketData> instead of Deque<std::unique_ptr<SocketData>). There’s no need for all the extra heap allocation.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:51
> +    startThread();

Should probably ASSERT(isMainThread()) here.

But also, why do we need to start the thread? I don’t think we do.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:57
> +    stopThread();

Should just be ASSERT(!m_workerThread). The handle is ref'd as long as the thread is going.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:58
> +    setClient(nullptr);

I don’t understand the value of doing this. The setClient function has no side effects, and once the handle is deleted the null pointer can be overwritten by other random values. I don’t think this class needs a destructor at all, although perhaps the logging and assertions would be OK.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:70
> +    std::unique_ptr<const char[]> copy(new const char[length]);
> +    memcpy(const_cast<char*>(copy.get()), data, length);

Should be able to allocate a std::unique_ptr<char[]> and then pass it to a function expecting a std::unique_ptr<const char[]> and have it be converted. We’d avoid a const_cast that way.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:124
> +    std::lock_guard<std::mutex> lock(m_mutexSend);

Do we really need to lock during the actual send? This seems like a really long time to hold a lock.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:144
> +            std::unique_ptr<const char[]> copy(new const char[restLength]);
> +            memcpy(const_cast<char*>(copy.get()), sendData->data() + totalBytesSent, restLength);
> +            m_sendData.prepend(std::unique_ptr<SocketData>(new SocketData { WTF::move(copy), restLength } ));

Would be nice to share this code with platformSend.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:158
> +    timeout.tv_sec = 0;
> +    timeout.tv_usec = std::chrono::duration_cast<std::chrono::microseconds>(selectTimeout).count();

This can do something crazy thing if selectTimeout is 1 second or more or if it’s negative.

http://stackoverflow.com/questions/17402657/how-to-convert-stdchronosystem-clockduration-into-struct-timeval

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:188
> +        CString host = m_url.host().utf8();
> +        curl_easy_setopt(curlHandle, CURLOPT_URL, host.data());

No need for the local variable here.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:200
> +            if (refCount() > 1)
> +                didOpenSocket();

This check of refCount is peculiar. Is this an important optimization?

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:217
> +void SocketStreamHandle::stopThread()

This function needs an ASSERT(isMainThread())

Created attachment 252878 [details]
Patch

(In reply to comment #10)
> Comment on attachment 252729 [details]
> Patch
> 

Thank you for your patience :)

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252729&action=review
> 
> lets not use this unnecessary std::unique_ptr
> 
> > Source/WebCore/platform/network/curl/SocketStreamHandle.h:48
> > +#if PLATFORM(WIN)
> > +#include <windows.h>
> > +#include <winsock2.h>
> > +#endif
> > +
> > +#include <curl/curl.h>
> > +
> > +#include <mutex>
> > +
> > +#include <wtf/Deque.h>
> >  #include <wtf/RefCounted.h>
> > +#include <wtf/Threading.h>
> 
> These aren’t sorted in the normal WebKit project way.
> 

winsock2.h is needed before curl.h is included, so I kept this.

> 
> > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:51
> > +    startThread();
> 
> Should probably ASSERT(isMainThread()) here.
> 
> But also, why do we need to start the thread? I don’t think we do.
> 

startThread() seems to be needed here, many tests timed out without it.

> 
> > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:70
> > +    std::unique_ptr<const char[]> copy(new const char[length]);
> > +    memcpy(const_cast<char*>(copy.get()), data, length);
> 
> Should be able to allocate a std::unique_ptr<char[]> and then pass it to a
> function expecting a std::unique_ptr<const char[]> and have it be converted.
> We’d avoid a const_cast that way.
>

MSVC didn't want to convert this, I got the error:

error C2664: 'std::unique_ptr<const char [],std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : 
cannot convert argument 1 from 'std::unique_ptr<char [],std::default_delete<_Ty>>' to 'std::unique_ptr<const char [],std::default_delete<_Ty>> &&'

 
> 
> > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:200
> > +            if (refCount() > 1)
> > +                didOpenSocket();
> 
> This check of refCount is peculiar. Is this an important optimization?
> 

I'm also not too happy with this :) I added it to fix a crash where the call was invoked on the main thread after all other references were released, and the SocketStreamClient was deleted. Accessing the SocketStreamClient in didOpenSocket() then caused a crash.

I had to add a SocketData constructor since m_sendData.append(SocketData { WTF::move(copy), length }) didn't compile with MSVC. The error I got was:

error C2440 : '<function-style-cast>' : cannot convert from 'initializer-list' to 'WebCore::SocketStreamHandle::SocketData'
1>          No constructor could take the source type, or constructor overload resolution was ambiguous

I also had to add a move constructor, otherwise I got an error stating that 
Deque.h (line 422) was trying to reference the deleted unique_ptr copy constructor.

[Alex Christensen]

> MSVC didn't want to convert this, I got the error:
> 
> error C2664: 'std::unique_ptr<const char
> [],std::default_delete<_Ty>>::unique_ptr(const
> std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : 
> cannot convert argument 1 from 'std::unique_ptr<char
> [],std::default_delete<_Ty>>' to 'std::unique_ptr<const char
> [],std::default_delete<_Ty>> &&'
If this really is a bug in MSVC, could you use const_cast to add const instead of taking it away?  That would be nicer.
> I had to add a SocketData constructor since m_sendData.append(SocketData {
> WTF::move(copy), length }) didn't compile with MSVC. The error I got was:
> 
> error C2440 : '<function-style-cast>' : cannot convert from
> 'initializer-list' to 'WebCore::SocketStreamHandle::SocketData'
> 1>          No constructor could take the source type, or constructor
> overload resolution was ambiguous
Does it work to call the constructor without any initializer-list? (no {})

[Darin Adler]

Comment on attachment 252878 [details]
Patch

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

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:77
> +    std::unique_ptr<const char[]> createCopy(const char* data, int length);

Should be static.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:99
> +        SocketData(std::unique_ptr<const char[]>&& source, int length)
> +        {
> +            data = WTF::move(source);
> +            size = length;
> +        }
> +
> +        SocketData(SocketData&& other)
> +        {
> +            data = WTF::move(other.data);
> +            size = other.size;
> +            other.size = 0;
> +        }

I don’t understand why you had to define these.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:71
> +    m_sendData.append(SocketData(WTF::move(copy), length));

Should just be SocketData { WTF::move(copy), length } and then it should work without a constructor.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:215
> +            if (refCount() > 1)
> +                didOpenSocket();

Needs a comment explaining why it’s here. Something like how you replied to my query about this.

Created attachment 253030 [details]
Patch

(In reply to comment #14)
> Comment on attachment 252878 [details]
> Patch
>

Thanks again :)
 
> 
> > Source/WebCore/platform/network/curl/SocketStreamHandle.h:99
> > +        SocketData(std::unique_ptr<const char[]>&& source, int length)
> > +        {
> > +            data = WTF::move(source);
> > +            size = length;
> > +        }
> > +
> > +        SocketData(SocketData&& other)
> > +        {
> > +            data = WTF::move(other.data);
> > +            size = other.size;
> > +            other.size = 0;
> > +        }
> 
> I don’t understand why you had to define these.
> 
> > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:71
> > +    m_sendData.append(SocketData(WTF::move(copy), length));
> 
> Should just be SocketData { WTF::move(copy), length } and then it should
> work without a constructor.
> 

I'm sorry, but I don't seem to be able to get rid of these constructors.
When I remove them, I get the following error:

1>C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\xmemory0(611): error C2280: 'std::unique_ptr<char [],std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function
1>          with
1>          [
1>              _Ty=char []
1>          ]
1>          C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\memory(1618) : see declaration of 'std::unique_ptr<char [],std::default_delete<_Ty>>::unique_ptr'
1>          with
1>          [
1>              _Ty=char []
1>          ]
1>          This diagnostic occurred in the compiler generated function 'WebCore::SocketStreamHandle::SocketData::SocketData(const WebCore::SocketStreamHandle::SocketData &)'
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

I believe this happens because Deque::append and Deque::prepend makes MSVC generate a SocketData copy constructor, which is not possible since the copy constructor in unique_ptr is deleted.

There seems to be reports of similar problems:

https://social.msdn.microsoft.com/Forums/vstudio/en-US/ace575e6-3969-4672-9db1-85c9d6588e4a/cant-put-class-with-uniqueptr-member-in-stl-container?forum=vclanguage

http://stackoverflow.com/questions/10133591/using-smart-pointers-in-a-struct-or-class

Is this a MSVC problem? I assume gcc doesn't have these problems.

(In reply to comment #13)
> > MSVC didn't want to convert this, I got the error:
> > 
> > error C2664: 'std::unique_ptr<const char
> > [],std::default_delete<_Ty>>::unique_ptr(const
> > std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : 
> > cannot convert argument 1 from 'std::unique_ptr<char
> > [],std::default_delete<_Ty>>' to 'std::unique_ptr<const char
> > [],std::default_delete<_Ty>> &&'
> If this really is a bug in MSVC, could you use const_cast to add const
> instead of taking it away?  That would be nicer.
> > I had to add a SocketData constructor since m_sendData.append(SocketData {
> > WTF::move(copy), length }) didn't compile with MSVC. The error I got was:
> > 
> > error C2440 : '<function-style-cast>' : cannot convert from
> > 'initializer-list' to 'WebCore::SocketStreamHandle::SocketData'
> > 1>          No constructor could take the source type, or constructor
> > overload resolution was ambiguous
> Does it work to call the constructor without any initializer-list? (no {})

Thanks for looking into this :)

I removed the const modifier from the unique_ptr member in the last patch to avoid all the casting. I hope this is acceptable :)

[Darin Adler]

(In reply to comment #16)
> I believe this happens because Deque::append and Deque::prepend makes MSVC
> generate a SocketData copy constructor, which is not possible since the copy
> constructor in unique_ptr is deleted.

As you say, I think this is indeed an MSVC bug.

[Darin Adler]

Comment on attachment 253030 [details]
Patch

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

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:92
> +        SocketData(std::unique_ptr<char[]>&& source, int length)
> +        {
> +            data = WTF::move(source);
> +            size = length;
> +        }

Really irritating that we have to do this to work around a bug in MSVC, but I’m pretty sure that is indeed what we are doing, working around a bug. Two separate related bugs, I guess.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:99
> +        SocketData(SocketData&& other)
> +        {
> +            data = WTF::move(other.data);
> +            size = other.size;
> +            other.size = 0;
> +        }

Really irritating that we have to do this to work around a bug in MSVC, but I’m pretty sure that is indeed what we are doing, working around a bug. Two separate related bugs, I guess.

(In reply to comment #19)
> Comment on attachment 253030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253030&action=review
> 
> > Source/WebCore/platform/network/curl/SocketStreamHandle.h:92
> > +        SocketData(std::unique_ptr<char[]>&& source, int length)
> > +        {
> > +            data = WTF::move(source);
> > +            size = length;
> > +        }
> 
> Really irritating that we have to do this to work around a bug in MSVC, but
> I’m pretty sure that is indeed what we are doing, working around a bug. Two
> separate related bugs, I guess.
> 

Thanks for reviewing :) I can try to remember to revisit this code when we move to the next MSVC version.

Committed r184389: <http://trac.webkit.org/changeset/184389>