Rawtherapee: Crash when stopping output queue

[thirtythreeforty]

I get a crash (SIGABRT) when I have a series of images in the output queue, and flip the big switch in the top left to OFF. It seems RT finishes its current image, hangs for a few seconds, then crashes.

Steps:

1. Put several images to the output queue. I am using raws from a Fuji X-T10. The profile does not seem to make much difference.
2. Go to the Queue tab and flip the switch to ON.
3. Wait for at least one image to finish.
4. Flip the switch to OFF.
5. Wait a few seconds for RT to crash.

Backtrace info is attached as log.txt.

I can provide a GDB core file if needed.

Version info:

Version: 5.4-964-g18f812d96
Branch: dev
Commit: 18f812d96
Commit date: 2018-10-19
Compiler: cc 8.2.1
Processor: x86_64
System: Linux
Bit depth: 64 bits
Gtkmm: V3.22.2
Lensfun: V0.3.2.0
Build type: Debug
Build flags:  -std=c++11 -march=native -Werror=unused-label -flto -fopenmp -Werror=unknown-pragmas -Wall -Wno-unused-result -Wno-deprecated-declarations -g -ftree-vectorize
Link flags:  -march=native -flto
OpenMP support: ON
MMAP support: ON

...hot off my compiler on Arch. Same behavior with 5.4 release.

[thirtythreeforty]

It looks like GTK code is being called from a separate thread. I am not all that familiar with GTK, but with most GUI toolkits I am familiar with, that is usually disallowed.

[thirtythreeforty]

Simple patch which seems to fix the issue is as follows. This is my first time touching RT code so it may not be the best place to stick the lock. But it works for me:

diff --git a/rtgui/batchqueuepanel.cc b/rtgui/batchqueuepanel.cc
index b698ee9d6..bc03c2386 100644
--- a/rtgui/batchqueuepanel.cc
+++ b/rtgui/batchqueuepanel.cc
@@ -322,7 +322,7 @@ void BatchQueuePanel::addBatchQueueJobs(const std::vector<BatchQueueEntry*>& ent

 bool BatchQueuePanel::canStartNext ()
 {
-
+    GThreadLock lock;
     if (qStartStop->get_active()) {
         return true;
     } else {

[heckflosse]

I confirm the crash. Didn't test your patch, but will do that tomorrow. Thanks for reporting and providing a patch. Very much appreciated :+1:

[heckflosse]

@thirtythreeforty Tested. Works fine. Shall I commit your patch or do you want to open a pull request?

[Floessie]

While this is a good catch and a quick fix, isn't the mechanism behind GThreadLock deprecated?

[thirtythreeforty]

Feel free to commit it. I'm just glad to have it fixed, I don't need my name in the commit history for such an easy patch. Thanks!

[heckflosse]

@Floessie Afaik you're right. But shouldn't we apply this quick fix nevertheless?

[thirtythreeforty]

I'd agree the quick fix should be applied to not block 5.5. But I'd also be interested in helping eliminate GThreadLock throughout the codebase, since it is in fact deprecated with no real replacement.

[heckflosse]

I had to remove the lock because queue processing now deadlocks sometimes.

[thirtythreeforty]

That's unfortunate. Which threads deadlock?

[heckflosse]

Didn't test yet. Debug build is still compiling...

[heckflosse]

Hmm, how can I check which threads deadlock? I'm on windows. ctrl-c in gdb does terminate gdb here :(

[thirtythreeforty]

Can you provide steps to trigger the deadlock? I can look. Not sure about Windows debug, that sounds pretty dumb...

[heckflosse]

Simply put the same image (using neutral profile) 7 times into the queue, close rt, start rt, start the queue. Sometimes it freezes after the first image, sometimes later but I never managed to process all 7 images without a freeze.

[thirtythreeforty]

Zero issues running through the queue like that on eee683738 for me. Maybe it's a Windows-specific issue? Perhaps you could try these suggestions, those people seem to have your same problem. Or do a Visual Studio build, they have a world-class debugger. (And I say that as a staunch Linux user :)

[Floessie]

@thirtythreeforty @heckflosse George, Ingo, as this must be solved differently in the long run because of the deprecation, how about this approach: Instead of querying qStartStop directly, connect a signal which feeds a function that stores the "active" state in an atomic bool. Then in BatchQueuePanel::canStartNext() use that bool to make a decision and handle the false case via IdleRegister.

HTH,
Flössie

[thirtythreeforty]

@Floessie that's a more forward-facing approach. I will see if I can hack something up.