From 8a59d78faa4792934a11a774c9dffcd85f0c3340 Mon Sep 17 00:00:00 2001 From: "Robert C. Helling" Date: Tue, 15 Mar 2016 21:31:59 +0100 Subject: When handing off a picture to a worker thread, copy it first as otherwise we crash when the picture is freed before the worker thread (to load from the net or to compute hashes) is finished Signed-off-by: Robert C. Helling Signed-off-by: Dirk Hohndel --- subsurface-core/dive.c | 14 +++++++++++++- subsurface-core/dive.h | 2 ++ subsurface-core/imagedownloader.cpp | 13 +++++++++---- subsurface-core/imagedownloader.h | 1 + subsurface-core/qthelper.cpp | 6 +++++- 5 files changed, 30 insertions(+), 6 deletions(-) diff --git a/subsurface-core/dive.c b/subsurface-core/dive.c index 47b3e1f06..cad2a85cc 100644 --- a/subsurface-core/dive.c +++ b/subsurface-core/dive.c @@ -3374,7 +3374,7 @@ void dive_set_geodata_from_picture(struct dive *dive, struct picture *picture) } } -static void picture_free(struct picture *picture) +void picture_free(struct picture *picture) { if (!picture) return; @@ -3383,6 +3383,18 @@ static void picture_free(struct picture *picture) free(picture); } +// When handling pictures in different threads, we need to copy them so we don't +// run into problems when the main thread frees the picture. + +struct picture *clone_picture(struct picture *src) +{ + struct picture *dst; + + dst = alloc_picture(); + copy_pl(src, dst); + return dst; +} + void dive_remove_picture(char *filename) { struct picture **picture = ¤t_dive->picture_list; diff --git a/subsurface-core/dive.h b/subsurface-core/dive.h index 74f55ee81..484a67f8a 100644 --- a/subsurface-core/dive.h +++ b/subsurface-core/dive.h @@ -376,6 +376,7 @@ struct picture { for (struct picture *picture = (_divestruct).picture_list; picture; picture = picture->next) extern struct picture *alloc_picture(); +extern struct picture *clone_picture(struct picture *src); extern bool dive_check_picture_time(struct dive *d, int shift_time, timestamp_t timestamp); extern void dive_create_picture(struct dive *d, char *filename, int shift_time, bool match_all); extern void dive_add_picture(struct dive *d, struct picture *newpic); @@ -385,6 +386,7 @@ extern bool picture_check_valid(char *filename, int shift_time); extern void picture_load_exif_data(struct picture *p); extern timestamp_t picture_get_timestamp(char *filename); extern void dive_set_geodata_from_picture(struct dive *d, struct picture *pic); +extern void picture_free(struct picture *picture); extern int explicit_first_cylinder(struct dive *dive, struct divecomputer *dc); extern int get_depth_at_time(struct divecomputer *dc, unsigned int time); diff --git a/subsurface-core/imagedownloader.cpp b/subsurface-core/imagedownloader.cpp index 9451f8a1b..d28ec9503 100644 --- a/subsurface-core/imagedownloader.cpp +++ b/subsurface-core/imagedownloader.cpp @@ -17,6 +17,11 @@ ImageDownloader::ImageDownloader(struct picture *pic) picture = pic; } +ImageDownloader::~ImageDownloader() +{ + picture_free(picture); +} + void ImageDownloader::load(bool fromHash){ QUrl url; if(fromHash) @@ -79,21 +84,21 @@ SHashedImage::SHashedImage(struct picture *picture) : QImage() if (filename.isNull()) { // That didn't produce a local filename. // Try the cloud server - QtConcurrent::run(loadPicture, picture, true); + QtConcurrent::run(loadPicture, clone_picture(picture), true); } else { // Load locally from translated file name load(filename); if (!isNull()) { // Make sure the hash still matches the image file - QtConcurrent::run(updateHash, picture); + QtConcurrent::run(updateHash, clone_picture(picture)); } else { // Interpret filename as URL - QtConcurrent::run(loadPicture, picture, false); + QtConcurrent::run(loadPicture, clone_picture(picture), false); } } } else { // We loaded successfully. Now, make sure hash is up to date. - QtConcurrent::run(hashPicture, picture); + QtConcurrent::run(hashPicture, clone_picture(picture)); } } diff --git a/subsurface-core/imagedownloader.h b/subsurface-core/imagedownloader.h index cd85c9509..2ff68342c 100644 --- a/subsurface-core/imagedownloader.h +++ b/subsurface-core/imagedownloader.h @@ -14,6 +14,7 @@ class ImageDownloader : public QObject { Q_OBJECT; public: ImageDownloader(struct picture *picture); + ~ImageDownloader(); void load(bool fromHash); private: diff --git a/subsurface-core/qthelper.cpp b/subsurface-core/qthelper.cpp index 7c67e0993..b3891298a 100644 --- a/subsurface-core/qthelper.cpp +++ b/subsurface-core/qthelper.cpp @@ -1108,11 +1108,14 @@ QString fileFromHash(char *hash) return localFilenameOf[QByteArray::fromHex(hash)]; } +// This needs to operate on a copy of picture as it frees it after finishing! void updateHash(struct picture *picture) { QByteArray hash = hashFile(fileFromHash(picture->hash)); learnHash(picture, hash); + picture_free(picture); } +// This needs to operate on a copy of picture as it frees it after finishing! void hashPicture(struct picture *picture) { char *oldHash = copy_string(picture->hash); @@ -1120,13 +1123,14 @@ void hashPicture(struct picture *picture) if (!same_string(picture->hash, "") && !same_string(picture->hash, oldHash)) mark_divelist_changed((true)); free(oldHash); + picture_free(picture); } extern "C" void cache_picture(struct picture *picture) { QString filename = picture->filename; if (!hashOf.contains(filename)) - QtConcurrent::run(hashPicture, picture); + QtConcurrent::run(hashPicture, clone_picture(picture)); } void learnImages(const QDir dir, int max_recursions) -- cgit v1.2.3-70-g09d2