Tests should read errors and parse them if they are
expected. Otherwise, we should not ignore them and move
on, because they are (by the above definition) unexpected.
Change-Id: I9d7a9fb23879044ac4f11461e92c5f6fd6b03fa1
Reviewed-on: https://gerrit.libreoffice.org/30194
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Proper parsing of incoming messages and better
prefix matching is done, as well as slightly
better logging when we timeout or hit exceptions.
Change-Id: I11adbf24a5505f6cda4a18ba855898dc5f82e238
Reviewed-on: https://gerrit.libreoffice.org/30190
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
The last key=value entry of a message with binary payload
will not be parsed with space alone, since it is followed
immediately by a new line.
To parse these cases correctly we need to break at newline
as well.
Change-Id: I43fff48362a69dc91f36860014d49d128c0edb68
Reviewed-on: https://gerrit.libreoffice.org/30187
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Connect sockets to the local process using the details we can introspect.
Implement unit test for storage / load failure.
Cleanup tile-cache test a little.
- show outgoing and incoming network messages with some
highlighting in Javascript console (default in tile
debug mode)
- add optional automatic typing
- add layer controls for overlays and automatic typing
- differentiate rectangle borders (newer is more opaque)
- fixes:
- fix function call at cancelled tiles
- hide all attribution control when disabling debug mode
permission= parameter in URL is still supported, but overridden
by UserCanWrite parameter.
Also, introduce a new protocol message, perm: which dictates
loleaflet about the permission rather than the other way around
(only in case of WOPI)
It is to be noted that by default loolwsd assumes very
restrictive permissions, so not providing UserCanWrite in WOPI
implementation by a WOPI host would lead to opening of only
readonly session.
Change-Id: I2013c1661fd491c79bb367a41e1a7036fa03f984
Throw if document load fails. Ignoring such a scenario would lead
to crash later in the document load process.
Change-Id: Ie80fc4f26e08e4920d4c04726f947edd2837a0cf
Produces a human-readable logging string describing a LOOL protocol
frame. To be used for re-factoring snippets of similar code that
sometimes print also the flags, sometimes pointlessly print the
(binary) contents of a CLOSE frame (as if it was text), etc.
It has always been the intent that getAbbreviatedMessage() is for
producing human-readbale nice output for logging purposes only. But
this has not been mentioned, so (naturally) the function has been
misused. Sigh.
These Log::foo() calls are in ClientRequestHandler::handleClientRequest(),
not in ClientRequestHandler::handleRequest().
Actually I wonder why we show the name of the function in this handful of
places. We don't do it in general. Consistency, bah.
I fear they get can out of sync when HTTPCrashTest::killLoKitProcesses()
kills a loolkit process, and this then causes HTTPWSError::testMaxDocuments()
to fail. This is to help debugging that.
Haven't fully understood what is going on yet. But one thing is sure:
It is a bad idea to duplicate the same state information in two
places, we shouldn't really use that separate NumDocBrokers
variable.
Probably also NumConnections tracks state that can easily be
calculated from the data structures, but maybe NumConnections does not
that easily get out of sync.
The waiting for the expected number of loolkit processes after that
test never succeeded. That apparently was caused by the fact that the
loolwsd process is waiting for up to COMMAND_TIMEOUT_MS (five seconds
currently) for an auto-save of the document to succeed, and that never
happens in this case.
countLoolKitProcesses() on the other hand was waiting only a bit over
three seconds (calculated in a complicated way from POLL_TIMEOUT_MS
and a few magic numbers) for the number of loolkit processes to reach
the expected number. Or something like that.
COMMAND_TIMEOUT_MS=5000 and POLL_TIMEOUT_MS=COMMAND_TIMEOUT_MS/10 =>
500. We used to have in countLoolKitProcesses():
sleepMs=POLL_TIMEOUT_MS/3 => 166 and repeat=(3000/sleepMs)+1 =>
19. 19*166 => 3154.
Fix by calculating the max time to wait for the expected number of
loolkit processes in countLoolKitProcesses() in a different way, so
that it is always longer than COMMAND_TIMEOUT_MS.
Use the previously unused UnauthorizedRequestException for this, and
throw a such in StorageBase::create() when the WOPI host doesn't match
any of those configured.
In a developer debug build, without access to any real WOPI
functionality, you can test by setting the FAKE_UNAUTHORIZED
environment variable and attempting to edit a plain local file:
URI. That will cause such an exception to be thrown in that function.
Catch that UnauthorizedRequestException in
ClientRequestHandler::handleGetRequest(), and send an 'error:
cmd=internal kind=unauthorized' message to the client. Handle that in
loleaflet in the same place where the 'error: cmd=internal
kild=diskfull' message is handled, and in the same fashion, giving up
on the document.
Actually, using exceptions for relatively non-exceptional situations
like this is lame and makes understanding the code harder, but that is
just my personal preference...
FIXME: By the time StorageBase::create() gets called we have already
sent three 'statusindicator:' messages ('find', 'connect', and
'ready') to the client. We should ideally do the checks we do in
StorageBase::create() much earlier.
Also consider that ClientRequestHandler::handleClientRequest() has
code that catches UnauthorizedRequestException and
BadRequestException, and tries to set the HTTP response in those
cases. I am not sure if that functionality has ever been exercised,
though. Currently, we upgrade the HTTP connection to WebSocket early,
and only after that we check whether the WOPI host is authorized
etc. By that time it is too late to return an HTTP response to the
user. If that even is what we ideally should do? If not, then we
probably should drop the code that constructs HTTP responses and
attempts to send them.
Also, if I, as a test, force an HTTPResponse::HTTP_BAD_REQUEST to be
sent before the HTTP connection is upgraded to WebSocket, loleaflet
throws up the generic "Well, this is embarrassing" dialog anyway. At
least in Firefox on Linux. (Instead of the browser showing some own
dialog, which I was half-expecting to happen.)
Doing a plain CPPUNIT_TEST_NAME='somettest' make check will
result in executing all the tests in external test suite. This is
a problem when one wants to execute only internal tests (unit-*
ones) as test harness first executes those followed by all of the
tests in external test-suites.
Lets execute all the tests only when no CPPUNIT_TEST_NAME is
provided, and ignore when it is provided but no match is found.
Change-Id: I7e40b6f3124e6965a86cfb6395d246df3b5c17ba
There doesn't seem to be any failure on this test anymore.
Consecutive 20 runs of this test gives no failure, so lets enable
it again.
Change-Id: I77ddd1c36d18162bdc75fd24d51c1a2df22f749d
