Thread related crash on failed dive computer download

[Dirk Hohndel]

On Tue, Oct 31, 2017 at 03:52:28PM +0100, Berthold Stoeger wrote:
> Hi Dirk,
> 
> On Dienstag, 31. Oktober 2017 13:52:45 CET you wrote:
> > > On Oct 31, 2017, at 4:43 AM, Jan Mulder <jlmulder at xs4all.nl> wrote:
> > > 
> > > On 31-10-17 12:05, Jan Mulder wrote:
> > > Sounds like a fundamental problem, with the new way of error reporting ...
> > > but not sure ...
> > Yes, that makes sense. Doing the showNotification there might fail. Instead
> > we should just let the UI thread know that there is a new message to
> > display. This may prevent 4.7.2 today as I won't have time to look at this
> > until late this evening and then we might want to test it :-)
> > 
> > Unless one of you can look at it while I'm on an airplane for the next two
> > hours
> 
> After a quick look it seems that report_error(...) is used quite liberally in 
> core/qt-ble.cpp, core/downloadfromthread.cpp and core/libdivecomputer.c,
> which are all executed in DownloadThread context. Detangling this seems hard 
> as part of it is in C, other parts are callbacks from libdivecomputer, etc.

Yes, the idea was to simply just stage errors (and warnings, frankly) and
have the UI deal with them. Which we did a terrible job of in the past.
My solution made this much more responsive - and broken because of the
thread issue that I missed.

> As a quick stop-gap measure I replaced the report_error(...) calls by 
> report_error_thread(...) calls, which collect the error messages in a thread 
> local storage. The collected error messages are then passed to the main thread 
> via a member.

I only had a minute to glance through the code, but it seems reasonable.

> As a FYI, the patch attached, though without commit-message because it is 
> simply too ugly. It works for my particular test case, but...

Why is it too ugly?

/D

>  void DownloadThread::run()
>  {
> +	errorList.setLocalData(QStringList());

So we initialize to an empty list. I'm unclear if there could be messages
still in that local storage that haven't been delivered, yet.

>  	if (errorText)
>  		error = str_error(errorText, internalData->devname, internalData->vendor, internalData->product);
>  
> +	errors = std::move(errorList.localData());
> +

Because this seems like it would ('move'?) clear out the local storage,
correct?

> +
> +// Dirty hack: report_error(...) crashed when not called from main thread.
> +// Thus, replace report_error(...) calls by report_error_thread(...)
> +// calls, which collect error messages in thread local storage.
> +extern "C" int report_error_thread(const char *fmt, ...);
> +

Again, why is this a dirty hack?

> diff --git a/core/qt-ble.cpp b/core/qt-ble.cpp
> index a72736d4..277e44c6 100644
> --- a/core/qt-ble.cpp
> +++ b/core/qt-ble.cpp
> @@ -18,6 +18,9 @@
>  #include "core/qt-ble.h"
>  #include "core/btdiscovery.h"
>  
> +// Hack! for report_error_thread()
> +#include "core/downloadfromdcthread.h"

Why is that a hack?

> @@ -220,6 +220,9 @@ void DownloadFromDCWidget::updateState(states state)
>  	// got an error
>  	else if (state == ERROR) {
>  		timer->stop();
> +		foreach(const QString &s, thread.errors) {
> +			report_message(s.toUtf8().data());
> +		}

This one I don't understand... why aren't these reported when we do the
std::move() above?

/D