RESOLVED FIXED 34180
[Qt] Enable websockets support in QtWebKit
https://bugs.webkit.org/show_bug.cgi?id=34180
Summary [Qt] Enable websockets support in QtWebKit
Yael
Reported 2010-01-26 13:01:14 PST
A patch is coming soon
Attachments
Patch v1 (11.73 KB, patch)
2010-01-26 17:35 PST, Yael
no flags
Patch v2 (11.72 KB, patch)
2010-01-26 17:50 PST, Yael
hausmann: review-
Patch v3 (12.32 KB, patch)
2010-01-27 10:32 PST, Yael
no flags
Patch v4 (12.72 KB, patch)
2010-01-27 11:06 PST, Yael
no flags
Yael
Comment 1 2010-01-26 14:21:48 PST
(In reply to comment #0) > A patch is coming soon Unfortunately, I am getting a couple of sparious crashes when running websockets tests in DRT. Need more time to investigate the crashes.
Yael
Comment 2 2010-01-26 17:35:55 PST
Created attachment 47474 [details] Patch v1 About 2 out of the 27 websockets tests in DRT are crashing randomly with this patch, but I wanted to start the review process while I am working on the crashes. With this patch - both secure and non-secure websockets work - SSL errors are ignored - proxy authentication is not supported.
Yael
Comment 3 2010-01-26 17:50:20 PST
Created attachment 47476 [details] Patch v2 Got rid of all the tabs in the code. Maybe eclipse is not the best editor after all :-)
Fumitoshi Ukai
Comment 4 2010-01-26 23:36:49 PST
Comment on attachment 47476 [details] Patch v2 > Index: WebCore/WebCore.pro > =================================================================== > --- WebCore/WebCore.pro (revision 53851) > +++ WebCore/WebCore.pro (working copy) > @@ -1530,6 +1530,7 @@ > platform/network/NetworkStateNotifier.h \ > platform/network/ProtectionSpace.h \ > platform/network/qt/QNetworkReplyHandler.h \ > + platform/network/qt/SocketStreamHandlePrivate.h \ > platform/network/ResourceErrorBase.h \ > platform/network/ResourceHandle.h \ > platform/network/ResourceRequestBase.h \ > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 53876) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,34 @@ > +2010-01-26 Yael Aharon <yael.aharon@nokia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + [Qt] Enable websockets support in QtWebKit > + https://bugs.webkit.org/show_bug.cgi?id=34180 > + > + Connected between SocketStreamHandle and QTcpSocket/QSslSocket > + to enable support for websockets. > + > + * WebCore.pro: > + * platform/network/qt/SocketStreamHandle.h: > + * platform/network/qt/SocketStreamHandlePrivate.h: Added. > + * platform/network/qt/SocketStreamHandleQt.cpp: > + (WebCore::SocketStreamHandlePrivate::SocketStreamHandlePrivate): > + (WebCore::SocketStreamHandlePrivate::~SocketStreamHandlePrivate): > + (WebCore::SocketStreamHandlePrivate::socketConnected): > + (WebCore::SocketStreamHandlePrivate::socketReadyRead): > + (WebCore::SocketStreamHandlePrivate::send): > + (WebCore::SocketStreamHandlePrivate::close): > + (WebCore::SocketStreamHandlePrivate::socketSentdata): > + (WebCore::SocketStreamHandlePrivate::socketClosed): > + (WebCore::SocketStreamHandlePrivate::socketError): > + (WebCore::SocketStreamHandlePrivate::socketClosedCallback): > + (WebCore::SocketStreamHandlePrivate::socketErrorCallback): > + (WebCore::SocketStreamHandlePrivate::socketSslErrors): > + (WebCore::SocketStreamHandle::SocketStreamHandle): > + (WebCore::SocketStreamHandle::~SocketStreamHandle): > + (WebCore::SocketStreamHandle::platformSend): > + (WebCore::SocketStreamHandle::platformClose): > + > 2010-01-26 Dmitry Titov <dimich@chromium.org> > > Reviewed by Steve Falkenburg. > Index: WebCore/platform/network/qt/SocketStreamHandlePrivate.h > =================================================================== > --- WebCore/platform/network/qt/SocketStreamHandlePrivate.h (revision 0) > +++ WebCore/platform/network/qt/SocketStreamHandlePrivate.h (revision 0) > @@ -0,0 +1,74 @@ > +/* > + * Copyright (C) 2009 Apple Inc. All rights reserved. > + * Copyright (C) 2009 Google Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions are > + * met: > + * > + * * Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * * Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following disclaimer > + * in the documentation and/or other materials provided with the > + * distribution. > + * * Neither the name of Google Inc. nor the names of its > + * contributors may be used to endorse or promote products derived from > + * this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#ifndef SocketStreamHandlePrivate_h > +#define SocketStreamHandlePrivate_h > + > +#include "SocketStreamHandleBase.h" > + > +#include <QSslSocket> > +#include <QTcpSocket> > + > +class QSslSocket; > + > +namespace WebCore { > + > +class AuthenticationChallenge; > +class Credential; > +class SocketStreamHandleClient; > +class SocketStreamHandlePrivate; > + > +class SocketStreamHandlePrivate : public QObject { > +Q_OBJECT > +public: > + SocketStreamHandlePrivate(SocketStreamHandle* streamHandle, const KURL& url); > + ~SocketStreamHandlePrivate(); > + > +public slots: > + void socketConnected(); > + void socketReadyRead(); > + int send(const char* data, int len); > + void close(); > + void socketSentdata(); > + void socketClosed(); > + void socketError(QAbstractSocket::SocketError); > + void socketClosedCallback(); > + void socketErrorCallback(int); > + void socketSslErrors(const QList<QSslError>&); > +public: > + QTcpSocket* m_socket; > + SocketStreamHandle* m_streamHandle; > + quint64 m_lastSentSize; Do you need this? > +}; > + > +} > + > +#endif > Index: WebCore/platform/network/qt/SocketStreamHandle.h > =================================================================== > --- WebCore/platform/network/qt/SocketStreamHandle.h (revision 53851) > +++ WebCore/platform/network/qt/SocketStreamHandle.h (working copy) > @@ -42,6 +42,7 @@ > class AuthenticationChallenge; > class Credential; > class SocketStreamHandleClient; > + class SocketStreamHandlePrivate; > > class SocketStreamHandle : public RefCounted<SocketStreamHandle>, public SocketStreamHandleBase { > public: > @@ -61,6 +62,8 @@ > void receivedCredential(const AuthenticationChallenge&, const Credential&); > void receivedRequestToContinueWithoutCredential(const AuthenticationChallenge&); > void receivedCancellation(const AuthenticationChallenge&); > + SocketStreamHandlePrivate* m_p; > + friend class SocketStreamHandlePrivate; > }; > > } // namespace WebCore > Index: WebCore/platform/network/qt/SocketStreamHandleQt.cpp > =================================================================== > --- WebCore/platform/network/qt/SocketStreamHandleQt.cpp (revision 53851) > +++ WebCore/platform/network/qt/SocketStreamHandleQt.cpp (working copy) > @@ -35,34 +35,128 @@ > #include "Logging.h" > #include "NotImplemented.h" > #include "SocketStreamHandleClient.h" > +#include "SocketStreamHandlePrivate.h" > > namespace WebCore { > > +SocketStreamHandlePrivate::SocketStreamHandlePrivate(SocketStreamHandle* streamHandle, const KURL& url) : QObject() > +{ > + m_streamHandle = streamHandle; > + m_socket = 0; > + bool isSecure = url.protocol().contains("wss"); url.protocolIs("wss") ? > + if (isSecure) > + m_socket = new QSslSocket(this); > + else > + m_socket = new QTcpSocket(this); > + connect(m_socket, SIGNAL(connected()), this, SLOT(socketConnected())); > + connect(m_socket, SIGNAL(readyRead()), this, SLOT(socketReadyRead())); > + connect(m_socket, SIGNAL(disconnected()), this, SLOT(socketClosed())); > + connect(m_socket, SIGNAL(error(QAbstractSocket::SocketError)), this, SLOT(socketError(QAbstractSocket::SocketError))); > + if (isSecure) > + connect(m_socket, SIGNAL(sslErrors(const QList<QSslError>&)), this, SLOT(socketSslErrors(const QList<QSslError>&))); > + unsigned int port = url.hasPort() ? url.port() : isSecure ? 443 : 80; > + QString host = url.host(); > + if (isSecure) > + qobject_cast<QSslSocket*>(m_socket)->connectToHostEncrypted(host, port); > + else > + m_socket->connectToHost(host, port); > +} > + > +SocketStreamHandlePrivate::~SocketStreamHandlePrivate() > +{ > + Q_ASSERT(!(m_socket && m_socket->state() == QAbstractSocket::ConnectedState)); m_socket is leaked? > +} > + > +void SocketStreamHandlePrivate::socketConnected() > +{ > + if (m_streamHandle && m_streamHandle->client()) { > + m_streamHandle->m_state = SocketStreamHandleBase::Open; > + m_streamHandle->client()->didOpen(m_streamHandle); > + } > +} > + > +void SocketStreamHandlePrivate::socketReadyRead() > +{ > + if (m_streamHandle && m_streamHandle->client()) { > + QByteArray data = m_socket->readAll(); > + m_streamHandle->client()->didReceiveData(m_streamHandle, data.constData(), data.size()); > + } > +} > + > +int SocketStreamHandlePrivate::send(const char* data, int len) > +{ > + m_lastSentSize = m_socket->write(data, len); > + QMetaObject::invokeMethod(this, "socketSentData", Qt::QueuedConnection); > + return m_lastSentSize; > +} > + > +void SocketStreamHandlePrivate::close() > +{ > + if (m_socket && m_socket->state() == QAbstractSocket::ConnectedState) > + m_socket->close(); > +} > + > +void SocketStreamHandlePrivate::socketSentdata() > +{ > + if (m_streamHandle) > + m_streamHandle->sendPendingData(); > +} > + > +void SocketStreamHandlePrivate::socketClosed() > +{ > + QMetaObject::invokeMethod(this, "socketClosedCallback", Qt::QueuedConnection); > +} > + > +void SocketStreamHandlePrivate::socketError(QAbstractSocket::SocketError error) > +{ > + QMetaObject::invokeMethod(this, "socketError", Qt::QueuedConnection, Q_ARG(int, error)); > +} > + > +void SocketStreamHandlePrivate::socketClosedCallback() > +{ > + if (m_streamHandle && m_streamHandle->client()) { > + m_streamHandle->client()->didClose(m_streamHandle); > + m_streamHandle = 0; > + } > +} > + > +void SocketStreamHandlePrivate::socketErrorCallback(int error) > +{ > + // FIXME - in the future, we might not want to treat all errors as fatal. > + if (m_streamHandle && m_streamHandle->client()) { > + m_streamHandle->client()->didClose(m_streamHandle); > + m_streamHandle = 0; > + } > +} > + > +void SocketStreamHandlePrivate::socketSslErrors(const QList<QSslError>&) > +{ > + qobject_cast<QSslSocket*>(m_socket)->ignoreSslErrors(); > +} > SocketStreamHandle::SocketStreamHandle(const KURL& url, SocketStreamHandleClient* client) > : SocketStreamHandleBase(url, client) > { > LOG(Network, "SocketStreamHandle %p new client %p", this, m_client); > - notImplemented(); > + m_p = new SocketStreamHandlePrivate(this, url); > } > > SocketStreamHandle::~SocketStreamHandle() > { > LOG(Network, "SocketStreamHandle %p delete", this); > setClient(0); > - notImplemented(); > + delete m_p; > } > > -int SocketStreamHandle::platformSend(const char*, int) > +int SocketStreamHandle::platformSend(const char* data, int len) > { > LOG(Network, "SocketStreamHandle %p platformSend", this); > - notImplemented(); > - return 0; > + return m_p->send(data, len); > } > > void SocketStreamHandle::platformClose() > { > LOG(Network, "SocketStreamHandle %p platformClose", this); > - notImplemented(); > + m_p->close(); > } > > void SocketStreamHandle::didReceiveAuthenticationChallenge(const AuthenticationChallenge&) > @@ -86,3 +180,5 @@ > } > > } // namespace WebCore > + > +#include "moc_SocketStreamHandlePrivate.cpp" > Index: LayoutTests/platform/qt/Skipped > =================================================================== > --- LayoutTests/platform/qt/Skipped (revision 53851) > +++ LayoutTests/platform/qt/Skipped (working copy) > @@ -4965,7 +4965,6 @@ > http/tests/xmlhttprequest/workers/shared-worker-methods-async.html > > # Missing SocketStreamHandle implementation > -websocket/tests > > # Need rebaseline: https://bugs.webkit.org/show_bug.cgi?id=26830 > fast/multicol/single-line.html > Index: LayoutTests/ChangeLog > =================================================================== > --- LayoutTests/ChangeLog (revision 53876) > +++ LayoutTests/ChangeLog (working copy) > @@ -1,3 +1,14 @@ > +2010-01-26 Yael Aharon <yael.aharon@nokia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + [Qt] Enable websockets support in QtWebKit > + https://bugs.webkit.org/show_bug.cgi?id=34180 > + > + Remove websockets tests from skiplist. > + > + * platform/qt/Skipped: > + > 2010-01-26 Fumitoshi Ukai <ukai@chromium.org> > > Reviewed by Alexey Proskuryakov.
Simon Hausmann
Comment 5 2010-01-27 01:04:10 PST
Comment on attachment 47476 [details] Patch v2 Great that you're starting with this, Yael! > +#include <QSslSocket> > +#include <QTcpSocket> > + > +class QSslSocket; Why include QSslSocket and also forward declare it? :) > +class SocketStreamHandlePrivate : public QObject { > +Q_OBJECT The Q_OBJECT macro us usually indented. > +public: > + SocketStreamHandlePrivate(SocketStreamHandle* streamHandle, const KURL& url); WebKit style suggests to remove the names of the arguments here, as they're redundant (The type is the same as the variable name). > + connect(m_socket, SIGNAL(connected()), this, SLOT(socketConnected())); > + connect(m_socket, SIGNAL(readyRead()), this, SLOT(socketReadyRead())); > + connect(m_socket, SIGNAL(disconnected()), this, SLOT(socketClosed())); > + connect(m_socket, SIGNAL(error(QAbstractSocket::SocketError)), this, SLOT(socketError(QAbstractSocket::SocketError))); > + if (isSecure) > + connect(m_socket, SIGNAL(sslErrors(const QList<QSslError>&)), this, SLOT(socketSslErrors(const QList<QSslError>&))); > + unsigned int port = url.hasPort() ? url.port() : isSecure ? 443 : 80; This looks rather convoluted :). Do you mind splitting this up a little bit or adding parentheses? > + QString host = url.host(); > + if (isSecure) > + qobject_cast<QSslSocket*>(m_socket)->connectToHostEncrypted(host, port); I suggest to do a static_cast here, it's faster and if something is wrong it'll crash either way ;-) > +void SocketStreamHandlePrivate::socketReadyRead() > +{ > + if (m_streamHandle && m_streamHandle->client()) { > + QByteArray data = m_socket->readAll(); > + m_streamHandle->client()->didReceiveData(m_streamHandle, data.constData(), data.size()); > + } > +} Is it intentional to read _all_ data here and block until all is available? I wonder if it should be done like in QNetworkReplyHandler instead: QByteArray data = m_socket->read(m_socket->bytesAvailable()); Performance wise it's sad that we have to copy the data here, but I don't see any obvious solution here (as well as in QNetworkReplyHandler). > +int SocketStreamHandlePrivate::send(const char* data, int len) > +{ > + m_lastSentSize = m_socket->write(data, len); Looks like one whitespace too much here :) > + QMetaObject::invokeMethod(this, "socketSentData", Qt::QueuedConnection); What's the reason for this queued connection? Is the goal to call sendPendingData() only once the previous data has been written? (That itself may not be guaranteed with a queued connection that just goes through the event loop once) > +void SocketStreamHandlePrivate::socketClosed() > +{ > + QMetaObject::invokeMethod(this, "socketClosedCallback", Qt::QueuedConnection); > +} > + > +void SocketStreamHandlePrivate::socketError(QAbstractSocket::SocketError error) > +{ > + QMetaObject::invokeMethod(this, "socketError", Qt::QueuedConnection, Q_ARG(int, error)); > +} Out of curiousity again, why the queued connections? > +void SocketStreamHandlePrivate::socketErrorCallback(int error) > +{ > + // FIXME - in the future, we might not want to treat all errors as fatal. > + if (m_streamHandle && m_streamHandle->client()) { > + m_streamHandle->client()->didClose(m_streamHandle); Since this is called in case of socket errors, shouldn't we call didFail() on the stream handle client instead of just didClose()? > +void SocketStreamHandlePrivate::socketSslErrors(const QList<QSslError>&) > +{ > + qobject_cast<QSslSocket*>(m_socket)->ignoreSslErrors(); > +} Hmmm, doesn't that effectively remove any security from encrypted connections? If someone spoofs a certificate we ignore it and still happily connect, allowing a man-in-the-middle to read all the traffic. > Index: LayoutTests/platform/qt/Skipped > =================================================================== > --- LayoutTests/platform/qt/Skipped (revision 53851) > +++ LayoutTests/platform/qt/Skipped (working copy) > @@ -4965,7 +4965,6 @@ > http/tests/xmlhttprequest/workers/shared-worker-methods-async.html > > # Missing SocketStreamHandle implementation > -websocket/tests I like this part! Yay for layout testing :) But you should also remove the comment then that says the implementation is missing ;-)
Yael
Comment 6 2010-01-27 06:13:01 PST
Thank you Ukai & Simon for reviewing this patch! (In reply to comment #4) >Do you need this? I thought I would need it, but it turns out that I don't. Wiill take it out >url.protocolIs("wss") ? will do >m_socket is leaked? The QObject system will delete it when its parent is deleted (SocketStreamHandlePrivate) (In reply to comment #5) >Why include QSslSocket and also forward declare it? :) I should take it out! >The Q_OBJECT macro us usually indented. will do >WebKit style suggests to remove the names of the arguments here, as they're >redundant (The type is the same as the variable name). will do >This looks rather convoluted :). Do you mind splitting this up a little bit or >adding parentheses? will do >> + QString host = url.host(); >> + if (isSecure) >> + qobject_cast<QSslSocket*>(m_socket)->connectToHostEncrypted(host, port); >I suggest to do a static_cast here, it's faster and if something is wrong it'll >crash either way ;-) will do >Is it intentional to read _all_ data here and block until all is available? > >I wonder if it should be done like in QNetworkReplyHandler instead: > > QByteArray data = m_socket->read(m_socket->bytesAvailable()); > >Performance wise it's sad that we have to copy the data here, but I don't see >any obvious solution here (as well as in QNetworkReplyHandler). I thought that the result of readAll() and bytesAvailable(0 would be the same, but I will change that. >> + m_lastSentSize = m_socket->write(data, len); > >Looks like one whitespace too much here :) Should be added to chek-webkit-style :-) >> + QMetaObject::invokeMethod(this, "socketSentData", Qt::QueuedConnection); > >What's the reason for this queued connection? Is the goal to call >sendPendingData() only once the previous data has been written? (That itself >may not be guaranteed with a queued connection that just goes through the event >loop once) I wanted to match chromium port that calls this asynchronously. QTcpSocket does not provide a callback when it is really done sending. >> +void SocketStreamHandlePrivate::socketClosed() >> +{ > + QMetaObject::invokeMethod(this, "socketClosedCallback", Qt::QueuedConnection); >> +} >> + >> +void SocketStreamHandlePrivate::socketError(QAbstractSocket::SocketError error) >> +{ >> + QMetaObject::invokeMethod(this, "socketError", Qt::QueuedConnection, Q_ARG(int, error)); >> +} > >Out of curiousity again, why the queued connections? These calls would result in cleanup, and I thought it was safer to call them asynchronously. Do you think this will cause performance issues? >> +void SocketStreamHandlePrivate::socketErrorCallback(int error) >> +{ >> + // FIXME - in the future, we might not want to treat all errors as fatal. >> + if (m_streamHandle && m_streamHandle->client()) { >> + m_streamHandle->client()->didClose(m_streamHandle); > >Since this is called in case of socket errors, shouldn't we call didFail() on >the stream handle client instead of just didClose()? Chromium port is calling didClose(), so I followed it, but I will look into this again >> +void SocketStreamHandlePrivate::socketSslErrors(const QList<QSslError>&) >> +{ >> + qobject_cast<QSslSocket*>(m_socket)->ignoreSslErrors(); >> +} > >Hmmm, doesn't that effectively remove any security from encrypted connections? >If someone spoofs a certificate we ignore it and still happily connect, >allowing a man-in-the-middle to read all the traffic. Yes, I acknowledged this in comment #2, as a missing feature. >> # Missing SocketStreamHandle implementation >> -websocket/tests> > >I like this part! Yay for layout testing :) > >But you should also remove the comment then that says the implementation is >missing ;-) oops!
Yael
Comment 7 2010-01-27 06:47:42 PST
Changing didClose() to didFail() caused 7 layout tests to fail. Since the error code is not used for anything, I would like to keep the didClose() call.
Yael
Comment 8 2010-01-27 10:32:37 PST
Created attachment 47547 [details] Patch v3 Thanks to valgrind I found and fixed the random crashes. With this patch proxy authentication still does not work, and for the purpose of testing, I allow wss connections with bad certificates. This should be disabled later on.
Laszlo Gombos
Comment 9 2010-01-27 11:04:15 PST
v3 looks good to me - with the minor comment to include the status of the implementation in the ChangeLog as well.
Yael
Comment 10 2010-01-27 11:06:01 PST
Created attachment 47551 [details] Patch v4 Added more comments at Laszlo's request.
Laszlo Gombos
Comment 11 2010-01-27 11:08:11 PST
Comment on attachment 47551 [details] Patch v4 Looks good to me; all the previous review comments have been addressed.
WebKit Commit Bot
Comment 12 2010-01-27 12:16:59 PST
Comment on attachment 47551 [details] Patch v4 Clearing flags on attachment: 47551 Committed r53947: <http://trac.webkit.org/changeset/53947>
WebKit Commit Bot
Comment 13 2010-01-27 12:17:06 PST
All reviewed patches have been landed. Closing bug.
Markus Goetz
Comment 14 2010-02-01 01:32:26 PST
Small note: Theoretically, the SSL channel can only be seen as open after encrypted() has been emitted. The connected() signal is for the underlying TCP connection. I don't know if it is worth to fix this or not.
Markus Goetz
Comment 15 2010-02-01 01:39:07 PST
The check "m_socket->state() == QAbstractSocket::ConnectedState" should maybe be removed since we also want to close a socket that is e.g. still in HostLookupState.
Yael
Comment 16 2010-02-01 11:44:46 PST
Created https://bugs.webkit.org/show_bug.cgi?id=34425 to address comments #14 and #15.
Note You need to log in before you can comment on or make changes to this bug.