Matches the behaviour of real sockets in a real Online server better.
Seems to get rid of the occasional hang problem.
I had already some time ago suspected that such a change would fix
that problem, but unfortunately my first attempt missed one crucial
detail, so it didn't, and I spent days looking for other ways to fix
the problem instead, in vain.
Probably I should add some sanity limit on the number of buffered
messages, though... But if such a sanity limit then would be hit, most
likely much else is totally broken already anyway.
Change-Id: Ice43057814ee5abd85b2935ffaa91765845a515a
In 1c7f94045a that introduced
DelaySocket, its getPollEvents() could indeed return -1, but that was
removed later.
Change-Id: Ie3a7e01b7b9a7517d97f6ed3cc6d96bdb3313969
Reviewed-on: https://gerrit.libreoffice.org/61990
Reviewed-by: Tor Lillqvist <tml@collabora.com>
Tested-by: Tor Lillqvist <tml@collabora.com>
Add plumbing to send messages from the Online code to the JavaScript
code, and vice versa. Similar to what is done for iOS.
Sadly, it crashes. Multi-thread issues. Not surprisingly, it crashes
when I call webkit_web_view_run_javascript() in another thread than
the one where the GTK+ and other Webkit calls were done. I need to
come up with some clever way to do everything from the same thread.
(On iOS, I use dispatch_async(dispatch_get_main_queue(),...) to
schedule a block (i.e., a lambda expression) to be run in the main
thread.)
The wakeup() call in SocketPoll::stop() doesn't always (or ever?)
actually cause the wakeup code to be invoked and callbacks called
right after, and we don't want to risk the leftover callbacks being
invoked when the same SocketPoll object is started again. (This did
actually happen.)
In a normal Online, this is not a problem, as SocketPolls aren't
reused. One document per kit process, a separate kit process for each
document. Not so in a mobile app, there we have just one process that
handles document after document as the user closes one, opens another
(or the same anew), etc.
Previously SocketPoll expected to be
running its own thread for polling.
This is unnecessary when we have a
spare thread (e.g. main) that can
(and should, for efficiency) be used
for polling rather than starting
dedicated thread.
Not starting the SocketPoll's thread
and calling SocketPoll::poll() directly
worked, the warning logs on each activity
notwithstanding.
The warnings aren't just noisy, they are
a performance drain as well, and signal
that something is wrong. The new code
now makes the API cleaner and avoids
unnecessary warning logs, while being
faster.
Change-Id: Ibf9a223c59dae6522a5fc2e5d84a8ef191b577b1
The async-signal-safe functions to get thread-id
and thread-name, which cache the results, are
faster, cleaner, and signal-safe. No reason why
we shouldn't always use them.
Especially since it appears the logic was
inverted in Log::prefix, such that the signal
un-safe calls were made during signal-handling,
and the safe ones were called otherwise!
Instead of passing the signal-safe flag to
Log::prefix, we pass the buffer size, for
improved security.
Furthermore, reduce header dependencies
and reduce clutter.
Change-Id: I697689b2f0a290b6d8cce4babc3ac1e576141da6
I am trying to get a SocketPoll object to be "restartable".
Also make the lambda expression in joinThread multiple lines, so that
one can set a breakpoint in it.
Makes things simpler and easier to follow, I hope. I had hoped it
would also make the occasional hang I see when loading the document go
away, but nope. Possibly it isn't caused by FakeSocket after all, even
if FakeSocket is the obvious first suspect.
Re-think the plumbing between the different parts of the C++ Online
code. Do try to have it work more like in real Online on all but the
lowest socket level. Except that we don't have multiple processes, but
threads inside the same process. And instead of using actual system
sockets for WebSocket traffic between the threads, we use our own
FakeSocket things, with no WebSocket framing of messages.
Reduce the amount of #ifdef MOBILEAPP a bit also by compiling in the
UnitFoo things. Hardcode that so that no unit testing is ever
attempted, though. We don't try to dlopen any library.
Corresponding changes in the app Objective-C code. Plus fixes and
functionality improvements.
Now it gets so far that the JavaScript code thinks it has the document
tiles presented, and doesn't crash. But it hangs occasionally. And all
tiles show up blank.
Anyway, progress.
Change-Id: I769497c9a46ddb74984bc7af36d132b7b43895d4
No need for explicit "0x" and std::hex when outputting a pointer. A
pointer will be output as a hex number anyway. We used to have things
like this in the log:
TRC #25 Connected to WS Handler 0x0x3610b60| ./net/WebSocketHandler.hpp:80
At least LOOLWSD.cpp has a couple of global variables of types that in
their constructors invoke FakeSocket APIs. If we turn on FakeSocket
logging (fakeSocketSetLoggingCallback()) only in the app's
initialization code, we will miss logging from those global variable
constructors.
Sure, the clean solution would be to turn those global variables into
members in the LOOLWSD class instead, but this will do for now.
Not sure whether the shutdown() implementation matches real shutdown()
semantics, especially with regards to the behaviour of poll(), read(),
and write() (on both the socket itself and the connectd one)
afterwards. But let's see.
The feed() API turned out to be not needed. (And in any case, it was
suposed to be completely equivalent to writing to the peer socket.)
Won't actually be needed anyway, the way the code is going in my work
tree.
Change-Id: I4480ed59fe96ddcfad8483517f2a23452606f332
Reviewed-on: https://gerrit.libreoffice.org/60576
Reviewed-by: Miklos Vajna <vmiklos@collabora.co.uk>
Tested-by: Miklos Vajna <vmiklos@collabora.co.uk>
It's easier to either call the function to set the logging callback,
or not, in one place in client code, than to comment in and out all
the logging lines in FakeSocket.cpp as needed.
Change-Id: Id17f7e461c7df817440b47cb3124fcece13b189b
As we use #ifdefs at all call sites anyway (because we don't dare use
the generic overrides of close() etc), we can leave out the parameters
related to actual BSD sockets that we ignore anyway.
"Real" Online always uses non-blocking sockets. We don't need to take
flags for that or check it. We can hardcode such behaviour always.
(Assuing that is what we want; let's see once something works.)
Change-Id: I6863907d71c5599b00ce1f8305a44d41bbaf7bee
That is how Online uses the sockets for communication between the
processes anyway: It send and receives complete WebSocket frames.
Sure, in the mobile case there is just one process, but (I think) we
want to keep the same basic structure anyway, even if "wsd" and "kit"
are just threads. (We probably also want to drop the WebSocket framing
of the messages.)
Change-Id: I2397f321029c1cbbbc448a9b2403ad185a51cf14
Intended as a replacement for the real sockets used for WebSocket IPC
in Online on Linux. The idea is that in a mobile app we don't want to
bother with any actual sockets or WebSocket protocol (because we are
running as a single process after all), but keep much of the code that
thinks it is using such mostly unmodified. Just an idea so far, let's
see how this turns out.
Change-Id: I7878b0db99d9cbf70650227186c1fec9677fa74b
Makes it easier to put a breakpoint in it in Xcode...
Not sure if we have any consistent convention around here anyway about
which member functions should be defined inline in the class
definition in the hpp file, and which ones should be in the cpp file.
The app is unimaginatively called "Mobile" for now.
Runs but crashes pretty quickly after loading the document by the LO
core. Will need some heavy changes to get a ClientSession object
created in there, too, to handle the (emulated) WebSocket messages
from the JavaScript. It would then handle some of these messages
itself, and forwards some to the ChildSession, which in this case is
in the same process. Now the messsages from the JavaScript go to a
ChildSession, which is wrong. As the assertion says, "Tile traffic
should go through the DocumentBroker-LoKit WS"
Can't say I understand why, but this turned out to not be a good idea
after all. And no, using constexpr did not work either, so I won't
waste any more time on this triviality, but just revert.
This reverts commit 195b88ac8d.
Change-Id: I49f737dc6a36fa4808841cb8e0335246ad8c6d03
Re-think Linux vs mobile ifdefs a bit. Use #ifdef __linux only to
surround code that actually is Linux-specific. Use #ifdef MOBILEAPP
for code that is for a mobile version (with no separste wsd, forkit,
and kit processes, and with no WebSocket protocol used).
Bypass UnitFoo for mobile. Possibly we do want the UnitFoo stuff after
all on mobile, to run in some special testing mode? Hard to say, let's
skipt it for now.
Again, note that I don't claim this file (or the code-base as such)
would make much sense for iOS as such at the moment. I just want it to
compile for now. Baby steps etc. (And there is no public Xcode project
to compile it even partially.)
Change-Id: I1321d61e9e911c7d97c7309b78aab46d9cecec29
It confused at least me for a while into pondering whether the code
thinks TCP is packet-oriented.
Change-Id: I143fc7821abd6b4023d551cdcb42a00e1613e466
I think the general policy should be to always log errno using both
Util::symbolicErrno() and std::strerror(), never log a naked errno.
But only in cases where we know that it is highly likely that it is
the most recent system call that has failed, so errno makes sense.
Change-Id: I4a1fb31e375ea949e7da17687464361efe7c1761
We had:
auto ipv4 = (struct sockaddr_in *)&clientInfo.sin_addr
even if the clientInfo variable itself was a struct sockaddr_in.
And then we also had:
auto ipv6 = (struct sockaddr_in6 *)&clientInfo.sin_addr
which makes even less sense.
Instead, make clientInfo into a struct sockaddr_in6, which is big
enough to also to be interpreted as a sockaddr_in. Pass the address of
the correct field, either sin_addr or sin6_addr, to inet_ntop(). (Note
that sin_addr in sockaddr_in has a different offset than sin6_addr in
sockaddr_in6.)
At least on my Fedora 28, when I connect using IPv4, accept4() still
returns the client address as an IPv4 mapped IPv6 address, that is
::ffff:192.168.1.113 for 192.168.1.113. So the sample
net.post_allow.host value in loolwsd.xml.in should probably be changed
to have that ::ffff: prefix, too.
Change-Id: I0ad774616b210d94b904982e2f7dc928adc879ed
It was inconsistent that the StreamSocket ctor had a const parameter,
but the SslStreamSocket had a non-const one and used std::move(). Use
the later in the parent class as well for consistency.
Change-Id: I58597b97432a801d164d6ed9d831a0372280e687
moreover:
* noCache is always true in debug mode
* when noCache is true we return an explicit "Cache-Control: no-cache" line
Change-Id: I157a410df0a90f9ab151b899e44566b95cbd9929
Reviewed-on: https://gerrit.libreoffice.org/54517
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Tested-by: Michael Meeks <michael.meeks@collabora.com>
This resolves the erroneous warnings of pinging
on a non-upgraded (i.e. HTTP) socket.
This was due to the fact that we moved the socket
from one SocketHandlerInterface to a WebSocketHandler
after upgrading and since the WSState was a property
of the handler, the WebSocketHandler didn't know
that the socket had already been upgraded.
Also other cosmetics and cleanups.
Change-Id: I1a88edef750117ed551d23245e49380371561422
Reviewed-on: https://gerrit.libreoffice.org/49911
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
And improve the logging support in unit-tests to
help troubleshoot issues faster and more accurately.
Also makes the code more readable (hopefully).
Change-Id: I4f8aafb5245e2f774b03231591a74544f9ec84aa
Reviewed-on: https://gerrit.libreoffice.org/48645
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Using null ssl context to set options is surely not a good idea:
unsigned long SSL_CTX_set_options(SSL_CTX *ctx, unsigned long op)
{
return ctx->options |= op;
}
Change-Id: I2700350e0c3928e372488c81b8111c9ab0b48e06
No derived classes override it, and if they would, that would be a
problem, as setNoDelay() is called from init(), which is called from the
Socket ctor. Calling virtual functions from the base class ctor is not a
good idea, since the object is not fully constructed yet.
Change-Id: I4993e26d09f5d3429c7e2afae7688b84c0061c9d
Two problem types:
- non-final class has virtual functions but no virtual dtor -> mark the
class final
- abstract class has no virtual dtor -> add a virtual dtor
Change-Id: Iae208b65c774e6da7a3dda5e725fe07d4d589e4f
With help from Valgrind to find and verify
these leaks.
Change-Id: I3afeed89dc4bcd714a222f81822144477a346fb0
Reviewed-on: https://gerrit.libreoffice.org/39464
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Thread-affinity checks must be inhibited
not just on Socket, but on the SocketPoll as well,
before destroying DocumentBroker instances.
Also, properly initialize the inhibit statics.
Change-Id: I2ced1554d477f0c3faf09bda74034cbae99e4ce1
Reviewed-on: https://gerrit.libreoffice.org/37608
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Dung out overlapping return enumerations. Move more work into 'move'
callbacks at a safer time, etc.
Change-Id: I62ba5a35f12073b7b9c8de4674be9dae519a8aca
HTTP HEAD verb requires sending back NO content.
Currently we don't support HEAD and that harms
the browser from using its cache where possible.
Change-Id: I3c67dc106df95312c73f6ae786b7b1657a4167fb
Reviewed-on: https://gerrit.libreoffice.org/36871
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This slows things down terribly, particularly the setting on the websocket
made tiles appearing one by one. Let's keep the possibility to zero the buffer
sizes for debugging, but hide that behind an env. variable (and in debug
builds only anyway).
Change-Id: Ie4d2cdb3c0ec3c50f1a2b4f9941a462ac4f2d196
The server correctly saves all documents
and waits to upload them before exiting.
Change-Id: I04dc9ce588bc0fa39a9deb298d0a5efa61a03f1a
Reviewed-on: https://gerrit.libreoffice.org/36654
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Without an explicit WS message, the client
does not get this message and the handler
is not invoked at all.
Change-Id: I71e210a9958965cff35dd4d0f1d99985429b82f4
Reviewed-on: https://gerrit.libreoffice.org/36593
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Apparently pinging was enabled only when
_not_ WebSocket upgraded, which is wrong.
Removed sending ping immediately after
upgrading to WS as it's superfluous.
Change-Id: Ic8103bab063d87f58d371f0eab49f7b7530e2374
Reviewed-on: https://gerrit.libreoffice.org/36322
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Don't think it is necessary/useful to have this header at other places.
This is the most important and perhaps the only where presence of this
header is required and seems sensible to prevent potential attacks.
Change-Id: Iad318e4b83264ac83620b86a40a49e7384e4015e
None of the subclasses override it, and if they would, it would be
problematic, since e.g. the StreamSocket dtor calls it (and virtual
calls during dtors are a problem).
Change-Id: Ie0891349808a81539078fd1f2d95a55a4ce5107a
Plenty of time to do that next time around the cleanup.
We should still, really be doing the majority of the timeout work
inside the DocumentBroker poll itself.
First reason is that compression is very slow, and we re-compress the
files again and again.
Another reason is that IE/Edge doesn't work well with deflate turned on.
Related: https://connect.microsoft.com/IE/feedbackdetail/view/950689
The documents are not loaded at all with current code snapshot modulo
this patch.
Change-Id: I1fdd85856f448dc4ce02e1ab79e9c7474c3bb7f3
Only during shutdown do we expect a callback
to invalidate the pollSockets, but if it happens
we shouldn't invoke the poll handlers.
Change-Id: I2f56da19aec2f04cc871bd4eae1f93da110e0eb9
Reviewed-on: https://gerrit.libreoffice.org/36189
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
When shutting down accept_poll from
main, we can't remove sockets or cleanup.
That work needs to be done fro within accept_poll's
thread. This is different from when DocBroker's
poll needs to cleanup its own sockets before
it exists.
So we split the stop and removeSockets so they
can each be called in the proper way.
For accept_poll and others that joinThread
we queue a callback to cleanup before stopping.
Change-Id: If780d6a97ac0fc6da6897f895d5b4dda443f9e73
Reviewed-on: https://gerrit.libreoffice.org/36186
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
And assume correct thread if poll thread is
not running (i.e. no race).
Change-Id: I17958e682aba434ebb47fe0de199b9f530b54dee
Reviewed-on: https://gerrit.libreoffice.org/36183
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
assert()'s are no-op in the release builds, but we still want to see threading
problems in the log at least.
Change-Id: Idb02bb018e8f2d628a57ab570249613ad00bcff2
The DocBroker might not get a chance to
take ownership of a socket (which is done
via callbacks that are invoked in the poll loop)
if it (or WSD) is flagged for termination.
In that case, DocBroker doesn't take ownership
but ultimately needs to disconnect the socket.
By detaching ownership we signal that any thread
can rightly take ownership and thus avoid spurious
warning or assertions.
Change-Id: Idb192bfaac05c5c86809cb21876f3926a080b775
Reviewed-on: https://gerrit.libreoffice.org/36117
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
And insert sockets after starting the
thread so we poll the socket immediately.
Change-Id: Id336e1838f2f624ebfe59c4c2caf33eaa1a638c9
Reviewed-on: https://gerrit.libreoffice.org/36110
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
...and warn if we are in the wrong thread.
This can happen when the socket is not properly
closed from the poll thread and is being destroyed.
Change-Id: I749c09b15d04b49038f7cee6a7a13e8f0145acff
Reviewed-on: https://gerrit.libreoffice.org/36057
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Valgrind found a number of erroneous data access
during the construction and destruction of SslContext.
Change-Id: Ie5072798a3660ed8acc707ba32ac196fa2d0f8af
Reviewed-on: https://gerrit.libreoffice.org/36055
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This was a workaround to Poco's limitation
of requiring socket receiveFrame be given
preallocated buffer, which couldn't be
exceeded by a larger payload. This meant
the receiver had to know the maximum
payload in advance.
Since only the Kit uses Poco sockets,
and the Kit never receives large payloads,
this preamble is now obsolete.
100% (94/94) of old-style tests PASS.
Change-Id: I76776f89497409e5755e335a3e25553e91cf0876
Reviewed-on: https://gerrit.libreoffice.org/36037
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Callbacks are used to initialize handlers,
as is the case with addSession on DocumentBroker.
If the socket gets data before the callback is
invoked, the handler will fail since the expected
initialization hasn't happened yet.
This race indeed happens (rarely) with addSession.
100% (94/94) of old-style tests PASS.
Change-Id: Id9b4f63b45c5564add252e1671b7b0b08aff8150
Reviewed-on: https://gerrit.libreoffice.org/36035
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
==20033== Invalid read of size 4
==20033== at 0x466504: ChildProcess::close(bool) (DocumentBroker.hpp:111)
==20033== by 0x44EA28: DocumentBroker::terminateChild(std::string const&, bool) (DocumentBroker.cpp:1313)
==20033== by 0x45F70E: DocumentBroker::pollThread() (DocumentBroker.cpp:264)
==20033== by 0x504B2F: SocketPoll::pollingThreadEntry() (Socket.hpp:486)
==20033== by 0x7310E6F: execute_native_thread_routine (thread.cc:84)
==20033== by 0x7AF60A3: start_thread (pthread_create.c:309)
==20033== by 0x7DF002C: clone (clone.S:111)
==20033== Address 0x0 is not stack'd, malloc'd or (recently) free'd
