[Libreoffice-commits] online.git: loolwsd/Admin.cpp loolwsd/IoUtil.cpp loolwsd/IoUtil.hpp loolwsd/protocol.txt loolwsd/test
Tor Lillqvist
tml at collabora.com
Tue May 3 06:51:53 UTC 2016
loolwsd/Admin.cpp | 17 +--------------
loolwsd/IoUtil.cpp | 47 +++++++++++++++++++++++++++----------------
loolwsd/IoUtil.hpp | 6 +++++
loolwsd/protocol.txt | 11 ++++++++++
loolwsd/test/UnitPrefork.cpp | 7 +++---
5 files changed, 53 insertions(+), 35 deletions(-)
New commits:
commit bfcf9756f5d9cb2c4fc357e3e6535522fc96f65f
Author: Tor Lillqvist <tml at collabora.com>
Date: Tue May 3 09:28:57 2016 +0300
Don't assert on PING frames that we send
UnitPrefork got what I assume is one of those PING frames that
ChildProcess::isAlive() sends before the actual reply when it sent the
"unit-memdump" message, and did not like it. Uncommenting the line
that outputs the "memory stats" message it expects showed:
Got memory stats 'PING'
Followed by:
Assertion `tokens.count() == 2' failed.
Fix by factoring out the handling of PING frames, PONG frames, and the
pseudo-PONG frames that we send ourselves in reply to PING frames into
a new function IoUtil::receiveFrame(). Use that in a couple of
places. (Probably should use in many more places.)
Getting past this then leads to later cppunit tests again being run,
and their failures then again showing up...
diff --git a/loolwsd/Admin.cpp b/loolwsd/Admin.cpp
index 1441811..38cae5b 100644
--- a/loolwsd/Admin.cpp
+++ b/loolwsd/Admin.cpp
@@ -81,22 +81,9 @@ void AdminRequestHandler::handleWSRequests(HTTPServerRequest& request, HTTPServe
if (ws->poll(waitTime, Socket::SELECT_READ))
{
- n = ws->receiveFrame(buffer, sizeof(buffer), flags);
+ n = IoUtil::receiveFrame(*ws, buffer, sizeof(buffer), flags);
- if ((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_PING)
- {
- // Echo back the ping payload as pong.
- // Technically, we should send back a PONG control frame.
- // However Firefox (probably) or Node.js (possibly) doesn't
- // like that and closes the socket when we do.
- // Echoing the payload as a normal frame works with Firefox.
- ws->sendFrame(buffer, n /*, WebSocket::FRAME_OP_PONG*/);
- }
- else if ((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_PONG)
- {
- // In case we do send pings in the future.
- }
- else if (n <= 0)
+ if (n <= 0)
{
// Connection closed.
Log::warn() << "Received " << n
diff --git a/loolwsd/IoUtil.cpp b/loolwsd/IoUtil.cpp
index c7abe51..d0c61d6 100644
--- a/loolwsd/IoUtil.cpp
+++ b/loolwsd/IoUtil.cpp
@@ -37,6 +37,34 @@ using Poco::Net::WebSocketException;
namespace IoUtil
{
+int receiveFrame(WebSocket& socket, void* buffer, int length, int& flags)
+{
+ while (true)
+ {
+ int n = socket.receiveFrame(buffer, length, flags);
+ if ((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_PING)
+ {
+ // Technically, we should send back a PONG control frame. However Firefox (probably) or
+ // Node.js (possibly) doesn't like that and closes the socket when we do.
+ socket.sendFrame("pong", strlen("pong"));
+ }
+ else if ((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_PONG)
+ {
+ // In case we do send pings in the future.
+ }
+ else if (((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_TEXT ||
+ (flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_BINARY) &&
+ n == 4 && memcmp((char*)buffer, "pong", 4) == 0)
+ {
+ // Ignore what we send above. Be lenient, also ignore binary "pong" frames.
+ }
+ else
+ {
+ return n;
+ }
+ }
+}
+
// Synchronously process WebSocket requests and dispatch to handler.
// Handler returns false to end.
void SocketProcessor(const std::shared_ptr<WebSocket>& ws,
@@ -75,25 +103,10 @@ void SocketProcessor(const std::shared_ptr<WebSocket>& ws,
}
payload.resize(payload.capacity());
- n = ws->receiveFrame(payload.data(), payload.capacity(), flags);
+ n = receiveFrame(*ws, payload.data(), payload.capacity(), flags);
payload.resize(n > 0 ? n : 0);
- if ((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_PING)
- {
- // Echo back the ping payload as pong.
- // Technically, we should send back a PONG control frame.
- // However Firefox (probably) or Node.js (possibly) doesn't
- // like that and closes the socket when we do.
- // Echoing the payload as a normal frame works with Firefox.
- ws->sendFrame(payload.data(), n /*, WebSocket::FRAME_OP_PONG*/);
- continue;
- }
- else if ((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_PONG)
- {
- // In case we do send pings in the future.
- continue;
- }
- else if (n <= 0 || ((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_CLOSE))
+ if (n <= 0 || ((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_CLOSE))
{
closeFrame();
Log::warn("Connection closed.");
diff --git a/loolwsd/IoUtil.hpp b/loolwsd/IoUtil.hpp
index 6f98136..780754c 100644
--- a/loolwsd/IoUtil.hpp
+++ b/loolwsd/IoUtil.hpp
@@ -21,6 +21,12 @@
namespace IoUtil
{
+ // Wrapper for WebSocket::receiveFrame() that handles PING frames (by replying with a
+ // "pseudo-PONG" frame, see protocol.txt) and PONG frames. Also our "pseudo-PONG" frames are
+ // ignored.
+ // Should we also factor out the handling of non-final and continuation frames into this?
+ int receiveFrame(Poco::Net::WebSocket& socket, void* buffer, int length, int& flags);
+
/// Synchronously process WebSocket requests and dispatch to handler.
//. Handler returns false to end.
void SocketProcessor(const std::shared_ptr<Poco::Net::WebSocket>& ws,
diff --git a/loolwsd/protocol.txt b/loolwsd/protocol.txt
index b6afb37..11b6677 100644
--- a/loolwsd/protocol.txt
+++ b/loolwsd/protocol.txt
@@ -156,6 +156,13 @@ userinactive
See 'useractive'.
+pong
+
+ Sent instead of a PONG frame as reply to a PING frame. A comment
+ in our code says "Technically, we should send back a PONG control
+ frame. However Firefox (probably) or Node.js (possibly) doesn't
+ like that and closes the socket when we do."
+
server -> client
================
@@ -284,6 +291,10 @@ unocommandresult: <payload>
Callback that an UNO command has finished.
See LOK_CALLBACK_UNO_COMMAND_RESULT for details.
+pong
+
+ See above.
+
child -> parent
===============
diff --git a/loolwsd/test/UnitPrefork.cpp b/loolwsd/test/UnitPrefork.cpp
index ce67dd6..e90ba79 100644
--- a/loolwsd/test/UnitPrefork.cpp
+++ b/loolwsd/test/UnitPrefork.cpp
@@ -14,9 +14,10 @@
#include <sys/types.h>
#include <dirent.h>
-#include "Util.hpp"
-#include "Unit.hpp"
+#include "IoUtil.hpp"
#include "LOOLProtocol.hpp"
+#include "Unit.hpp"
+#include "Util.hpp"
#include <Poco/Timestamp.h>
#include <Poco/StringTokenizer.h>
@@ -51,7 +52,7 @@ public:
int flags;
char buffer[4096];
- int length = socket->receiveFrame(buffer, sizeof (buffer), flags);
+ int length = IoUtil::receiveFrame(*socket, buffer, sizeof (buffer), flags);
std::string memory = LOOLProtocol::getFirstLine(buffer, length);
if (!memory.compare(0,6,"Error:"))
More information about the Libreoffice-commits
mailing list