Crash upon closing, after rapidly scrolling through many large photos

[mmadia]

hrev44550-gcc2h

To reproduce:

• Use ShowImage on an image within a folder or tracker query with many photos with a large file size. e.g., several hundred KiB or larger
• hold the up/down arrow key to rapidly cycle through the images
• once the entire cpu becomes pegged, CMD+W to close ShowImage.

Note: Use Pulse/ProcessController to disable extra cores, as this will make it easier to reproduce.

It was reproduced with both WonderBrush images and JPEG's.

It may take a few attempts, but the following crash will occur:

GNU gdb 6.3
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i586-pc-haiku"...(no debugging symbols found)

[tcsetpgrp failed in terminal_inferior: Invalid Argument]
Thread 23952 caused an exception: Segment violation
Reading symbols from /boot/system/runtime_loader...done.
Loaded symbols for /boot/system/runtime_loader
Reading symbols from /boot/system/lib/libbe.so...done.
Loaded symbols for /boot/system/lib/libbe.so
Reading symbols from /boot/system/lib/libtracker.so...done.
Loaded symbols for /boot/system/lib/libtracker.so
Reading symbols from /boot/system/lib/libtranslation.so...done.
Loaded symbols for /boot/system/lib/libtranslation.so
Reading symbols from /boot/system/lib/libstdc++.r4.so...done.
Loaded symbols for /boot/system/lib/libstdc++.r4.so
Reading symbols from /boot/system/lib/libroot.so...done.
Loaded symbols for /boot/system/lib/libroot.so
Reading symbols from /boot/system/lib/libicudata.so.48.1.1...done.
Loaded symbols for /boot/system/lib/libicudata.so.48.1.1
Reading symbols from /boot/system/lib/libicui18n.so.48.1.1...done.
Loaded symbols for /boot/system/lib/libicui18n.so.48.1.1
Reading symbols from /boot/system/lib/libicuio.so.48.1.1...done.
Loaded symbols for /boot/system/lib/libicuio.so.48.1.1
Reading symbols from /boot/system/lib/libicule.so.48.1.1...done.
Loaded symbols for /boot/system/lib/libicule.so.48.1.1
Reading symbols from /boot/system/lib/libiculx.so.48.1.1...done.
Loaded symbols for /boot/system/lib/libiculx.so.48.1.1
Reading symbols from /boot/system/lib/libicutu.so.48.1.1...done.
Loaded symbols for /boot/system/lib/libicutu.so.48.1.1
Reading symbols from /boot/system/lib/libicuuc.so.48.1.1...done.
Loaded symbols for /boot/system/lib/libicuuc.so.48.1.1
Reading symbols from /boot/system/lib/libtextencoding.so...done.
Loaded symbols for /boot/system/lib/libtextencoding.so
Reading symbols from /boot/system/lib/libroot-addon-icu.so...done.
Loaded symbols for /boot/system/lib/libroot-addon-icu.so
Reading symbols from /boot/system/add-ons/Translators/BMPTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/BMPTranslator
Reading symbols from /boot/system/add-ons/Translators/EXRTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/EXRTranslator
Reading symbols from /boot/system/lib/libilmimf.so...done.
Loaded symbols for /boot/system/lib/libilmimf.so
Reading symbols from /boot/system/lib/libz.so.1...done.
Loaded symbols for /boot/system/lib/libz.so.1
Reading symbols from /boot/system/add-ons/Translators/GIFTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/GIFTranslator
Reading symbols from /boot/system/add-ons/Translators/HVIFTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/HVIFTranslator
Reading symbols from /boot/system/add-ons/Translators/ICOTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/ICOTranslator
Reading symbols from /boot/system/add-ons/Translators/JPEG2000Translator...done.
Loaded symbols for /boot/system/add-ons/Translators/JPEG2000Translator
Reading symbols from /boot/system/add-ons/Translators/JPEGTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/JPEGTranslator
Reading symbols from /boot/system/lib/libjpeg.so.8.0...done.
Loaded symbols for /boot/system/lib/libjpeg.so.8.0
Reading symbols from /boot/system/add-ons/Translators/PCXTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/PCXTranslator
Reading symbols from /boot/system/add-ons/Translators/PNGTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/PNGTranslator
Reading symbols from /boot/system/lib/libpng.so.1.4...done.
Loaded symbols for /boot/system/lib/libpng.so.1.4
Reading symbols from /boot/system/add-ons/Translators/PPMTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/PPMTranslator
Reading symbols from /boot/system/add-ons/Translators/RAWTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/RAWTranslator
Reading symbols from /boot/system/add-ons/Translators/RTFTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/RTFTranslator
Reading symbols from /boot/system/add-ons/Translators/SGITranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/SGITranslator
Reading symbols from /boot/system/add-ons/Translators/STXTTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/STXTTranslator
Reading symbols from /boot/system/add-ons/Translators/TGATranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/TGATranslator
Reading symbols from /boot/system/add-ons/Translators/TIFFTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/TIFFTranslator
Reading symbols from /boot/system/lib/libtiff.so.3.8...done.
Loaded symbols for /boot/system/lib/libtiff.so.3.8
Reading symbols from /boot/system/add-ons/Translators/WebPTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/WebPTranslator
Reading symbols from /boot/system/add-ons/Translators/WonderBrushTranslator...done.
Loaded symbols for /boot/system/add-ons/Translators/WonderBrushTranslator
[tcsetpgrp failed in terminal_inferior: Invalid Argument]
[Switching to team /boot/system/apps/ShowImage (23936) thread image loader (23952)]
0xffff01e4 in ?? ()
(gdb) bt 
#0  0xffff01e4 in ?? ()
#1  0x78137c18 in ?? ()
#2  0x00001900 in ?? ()
#3  0x0074106d in BBitmapStream::WriteAt () from /boot/system/lib/libtranslation.so
#4  0x00484fae in BPositionIO::Write () from /boot/system/lib/libbe.so
#5  0x021d06fa in JPEGTranslator::Decompress ()
   from /boot/system/add-ons/Translators/JPEGTranslator
#6  0x021cf744 in JPEGTranslator::DerivedTranslate ()
   from /boot/system/add-ons/Translators/JPEGTranslator
#7  0x021d1f40 in BaseTranslator::BitsTranslate ()
   from /boot/system/add-ons/Translators/JPEGTranslator
#8  0x021d1fb4 in BaseTranslator::Translate ()
   from /boot/system/add-ons/Translators/JPEGTranslator
#9  0x007489eb in BTranslatorRoster::Translate ()
   from /boot/system/lib/libtranslation.so
#10 0x0021dc57 in ImageCache::_RetrieveImage ()
#11 0x0021d6e8 in ImageCache::_QueueWorkerThread ()
#12 0x007b66e3 in thread_entry () from /boot/system/lib/libroot.so
#13 0x78137fec in ?? ()
(gdb) 

[mmadia]

Debugger report with debug builds of ShowImage, all Translators, libbe, libtranslation.

[anevilyak]

After some quick digging, the problem here appears to be in the ImageCache. It spawns worker threads to handle image loading/translation, but doesn't actually wait for them to complete/exit when it's shut down (there's furthermore a TODO note in the ImageCache destructor to free various resources so it clearly needs a bit more TLC there anyways).

Reassigning to Axel since that code was his.

[axeld]

Thanks for your contribution! The patch looks good, and seems to hit the problem on the nail. I have some minor complaints, however:

• Why not use fQueue.front() instead of *begin()?
• I would never ask for a specific error code when trying to bail out of an otherwise endless loop. Just imagine you don't have the privileges to use find_thread() in a future Haiku, and get \ B_PERMISSION_DENIED back?
• Is there maybe another way than to identify the threads by name?
• Is the window already closed at that time? Otherwise it might not be a nice user experience.

[axeld]

Oh BTW, you might want to call Stop() in ~ImageCache() as well, and remove that TODO comment.

[Janus]

Call Stop() in ~ImageCache() is too late, the app is already crashed. ImageCache is static!

[Janus]

Replying to axeld:

Thanks for your contribution! The patch looks good, and seems to hit the problem on the nail. I have some minor complaints, however:

• Why not use fQueue.front() instead of *begin()?

I have copied the code already present in the _QueueWorkerThread :-)

• I would never ask for a specific error code when trying to bail out of an otherwise endless loop. Just imagine you don't have the privileges to use find_thread() in a future Haiku, and get \ B_PERMISSION_DENIED back?

B_NAME_NOT_FOUND is the only value returned in the bebook (see next answer)

• Is there maybe another way than to identify the threads by name?

I use this method because is very simple. I can't think anything without saving the threadId. Do you have any suggestion?

• Is the window already closed at that time? Otherwise it might not be a nice user experience.

Yes the windows are already closed, have you experience something different?

[Janus]

I have given some thought to B_PERMISSION_DENIED. In this case even without a infinite loop we have a problem, because the app will crash like now. I think at the end all the code doesn't behave properly if a function doesn't work as expected.

[axeld]

I haven't had the time to actually test your changes, I just looked at the patch.

Anyway, the point I was trying to make is that it is never a good idea to assume a certain error code in such a way. The error code was just an example. Another example would be that we'd memorize thread IDs in the kernel, and find_thread() would then return B_THREAD_EXITED if the thread is no longer with us -- I hope you get the idea now :-)

The BeBook is not a definitive reference to the Haiku API, and is in itself not complete either. Fact is, error codes may change.

[Janus]

Replying to axeld:

I haven't had the time to actually test your changes, I just looked at the patch.

Anyway, the point I was trying to make is that it is never a good idea to assume a certain error code in such a way. The error code was just an example. Another example would be that we'd memorize thread IDs in the kernel, and find_thread() would then return B_THREAD_EXITED if the thread is no longer with us -- I hope you get the idea now :-)

Yes, I have understood your concern and I agree with you 100%. I know that it isn't a good practice to test for a single error, but in this particular case I don't have a better solution. I also prefer not penalize the normal flow for the exit condition. I don't like this solution myself.

[Janus]

I know it is an example ;-), but B_THREAD_EXITED would be very difficult to implement in find_thread. You can have multiple threads with the same name. Which one is the B_THREAD_EXITED thread?

[axeld]

Damn, and I thought I'd come up with a more probable example :-)

Anyway, when Stop() in ~ImageCache() is too late, it probably shouldn't be a static object, but part of the BApplication.

[Janus]

Replying to axeld:

Anyway, when Stop() in ~ImageCache() is too late, it probably shouldn't be a static object, but part of the BApplication.

Absolutely.

[waddlesplash]

Janus fixed this in hrev49006 but he forgot to close the ticket.