From ec4e82bdc01e8bcfb56fcf64cbd0ec1ad7e6394a Mon Sep 17 00:00:00 2001 From: David Capello Date: Mon, 25 Mar 2024 15:37:11 -0300 Subject: [PATCH] Fix crashes with SpriteJob(s) that weren't locking the doc correctly (fix #4315) This was mainly found in SpriteSizeJob crash reports. In these reports deleted image buffers were still used to paint the Editor canvas because the doc was write-locked in the main thread (same thread where the canvas is painted). This produced a re-entrant lock in the Editor::onPaint() as we can still read-lock from the same thread where we write-locked the doc. With this change we write-lock the doc from the SpriteJob background thread (not the main thread) only if it's necessary (i.e. when the doc is not already locked in the main thread, e.g. when running a script). This makes that the main thread (Editor::onPaint) cannot read the doc until we finish the whole SpriteJob transaction/Tx. --- src/app/commands/cmd_change_pixel_format.cpp | 15 ++++--- src/app/commands/cmd_color_quantization.cpp | 47 ++++++++++---------- src/app/commands/cmd_rotate.cpp | 21 +++++---- src/app/commands/cmd_sprite_size.cpp | 30 +++++++------ src/app/job.cpp | 4 +- src/app/job.h | 5 ++- src/app/sprite_job.cpp | 40 +++++++++++++---- src/app/sprite_job.h | 36 ++++++++++++--- 8 files changed, 125 insertions(+), 73 deletions(-) diff --git a/src/app/commands/cmd_change_pixel_format.cpp b/src/app/commands/cmd_change_pixel_format.cpp index 73fb002a8..6f6f4fc1b 100644 --- a/src/app/commands/cmd_change_pixel_format.cpp +++ b/src/app/commands/cmd_change_pixel_format.cpp @@ -1,5 +1,5 @@ // Aseprite -// Copyright (C) 2019-2023 Igara Studio S.A. +// Copyright (C) 2019-2024 Igara Studio S.A. // Copyright (C) 2001-2018 David Capello // // This program is distributed under the terms of @@ -646,12 +646,12 @@ void ChangePixelFormatCommand::onExecute(Context* context) #endif // ENABLE_UI // No conversion needed - if (context->activeDocument()->sprite()->pixelFormat() == m_format) + Doc* doc = context->activeDocument(); + if (doc->sprite()->pixelFormat() == m_format) return; { - const ContextReader reader(context); - SpriteJob job(reader, Strings::color_mode_title().c_str()); + SpriteJob job(context, doc, Strings::color_mode_title()); Sprite* sprite(job.sprite()); // TODO this was moved in the main UI thread because @@ -662,16 +662,17 @@ void ChangePixelFormatCommand::onExecute(Context* context) // https://github.com/aseprite/aseprite/issues/509 // https://github.com/aseprite/aseprite/issues/378 if (flatten) { + Tx tx(Tx::LockDoc, context, doc); const bool newBlend = Preferences::instance().experimental.newBlend(); SelectedLayers selLayers; for (auto layer : sprite->root()->layers()) selLayers.insert(layer); - job.tx()(new cmd::FlattenLayers(sprite, selLayers, newBlend)); + tx(new cmd::FlattenLayers(sprite, selLayers, newBlend)); } job.startJobWithCallback( - [this, &job, sprite] { - job.tx()( + [this, &job, sprite](Tx& tx) { + tx( new cmd::SetPixelFormat( sprite, m_format, m_dithering, diff --git a/src/app/commands/cmd_color_quantization.cpp b/src/app/commands/cmd_color_quantization.cpp index 1fcf01d5d..dcf339412 100644 --- a/src/app/commands/cmd_color_quantization.cpp +++ b/src/app/commands/cmd_color_quantization.cpp @@ -1,5 +1,5 @@ // Aseprite -// Copyright (C) 2019-2023 Igara Studio S.A. +// Copyright (C) 2019-2024 Igara Studio S.A. // Copyright (C) 2001-2018 David Capello // // This program is distributed under the terms of @@ -177,45 +177,46 @@ void ColorQuantizationCommand::onExecute(Context* ctx) return; try { - ContextReader reader(ctx); + Doc* doc = site.document(); Sprite* sprite = site.sprite(); frame_t frame = site.frame(); const Palette* curPalette = site.sprite()->palette(frame); Palette tmpPalette(frame, entries.picks()); - SpriteJob job(reader, "Color Quantization"); + SpriteJob job(ctx, doc, "Color Quantization"); const bool newBlend = pref.experimental.newBlend(); job.startJobWithCallback( - [sprite, withAlpha, &tmpPalette, &job, newBlend, algorithm]{ + [sprite, withAlpha, curPalette, &tmpPalette, &job, &entries, + newBlend, algorithm, createPal, site, frame](Tx& tx) { render::create_palette_from_sprite( sprite, 0, sprite->lastFrame(), withAlpha, &tmpPalette, &job, // SpriteJob is a render::TaskDelegate newBlend, algorithm); + + std::unique_ptr newPalette( + new Palette(createPal ? tmpPalette: + *site.palette())); + + if (createPal) { + entries = PalettePicks(newPalette->size()); + entries.all(); + } + + int i = 0, j = 0; + for (bool state : entries) { + if (state) + newPalette->setEntry(i, tmpPalette.getEntry(j++)); + ++i; + } + + if (*curPalette != *newPalette) + tx(new cmd::SetPalette(sprite, frame, newPalette.get())); }); job.waitJob(); if (job.isCanceled()) return; - - std::unique_ptr newPalette( - new Palette(createPal ? tmpPalette: - *site.palette())); - - if (createPal) { - entries = PalettePicks(newPalette->size()); - entries.all(); - } - - int i = 0, j = 0; - for (bool state : entries) { - if (state) - newPalette->setEntry(i, tmpPalette.getEntry(j++)); - ++i; - } - - if (*curPalette != *newPalette) - job.tx()(new cmd::SetPalette(sprite, frame, newPalette.get())); } catch (const base::Exception& e) { Console::showException(e); diff --git a/src/app/commands/cmd_rotate.cpp b/src/app/commands/cmd_rotate.cpp index 3419a672f..49ef0826f 100644 --- a/src/app/commands/cmd_rotate.cpp +++ b/src/app/commands/cmd_rotate.cpp @@ -1,5 +1,5 @@ // Aseprite -// Copyright (C) 2019-2022 Igara Studio S.A. +// Copyright (C) 2019-2024 Igara Studio S.A. // Copyright (C) 2001-2018 David Capello // // This program is distributed under the terms of @@ -13,7 +13,6 @@ #include "app/cmd/set_cel_bounds.h" #include "app/commands/cmd_rotate.h" #include "app/commands/params.h" -#include "app/context_access.h" #include "app/doc_api.h" #include "app/doc_range.h" #include "app/i18n/strings.h" @@ -45,10 +44,10 @@ class RotateJob : public SpriteJob { public: - RotateJob(const ContextReader& reader, + RotateJob(Context* ctx, Doc* doc, const std::string& jobName, int angle, const CelList& cels, bool rotateSprite) - : SpriteJob(reader, jobName.c_str()) + : SpriteJob(ctx, doc, jobName) , m_cels(cels) , m_rotateSprite(rotateSprite) { m_angle = angle; @@ -80,8 +79,8 @@ protected: } // [working thread] - void onJob() override { - DocApi api = document()->getApi(tx()); + void onSpriteJob(Tx& tx) override { + DocApi api = document()->getApi(tx); // 1) Rotate cel positions for (Cel* cel : m_cels) { @@ -93,7 +92,7 @@ protected: gfx::RectF bounds = cel->boundsF(); rotate_rect(bounds); if (cel->boundsF() != bounds) - tx()(new cmd::SetCelBoundsF(cel, bounds)); + tx(new cmd::SetCelBoundsF(cel, bounds)); } else { gfx::Rect bounds = cel->bounds(); @@ -192,6 +191,7 @@ void RotateCommand::onExecute(Context* context) { { Site site = context->activeSite(); + Doc* doc = site.document(); CelList cels; bool rotateSprite = false; @@ -203,7 +203,7 @@ void RotateCommand::onExecute(Context* context) // If we want to rotate the visible mask, we can go to // MovingPixelsState (even when the range is enabled, because // now PixelsMovement support ranges). - if (site.document()->isMaskVisible()) { + if (doc->isMaskVisible()) { // Select marquee tool if (tools::Tool* tool = App::instance()->toolBox() ->getToolById(tools::WellKnownTools::RectangularMarquee)) { @@ -237,13 +237,12 @@ void RotateCommand::onExecute(Context* context) rotateSprite = true; } - ContextReader reader(context); { - RotateJob job(reader, friendlyName(), m_angle, cels, rotateSprite); + RotateJob job(context, doc, friendlyName(), m_angle, cels, rotateSprite); job.startJob(); job.waitJob(); } - update_screen_for_document(reader.document()); + update_screen_for_document(doc); } } diff --git a/src/app/commands/cmd_sprite_size.cpp b/src/app/commands/cmd_sprite_size.cpp index 3327fc30f..98383def9 100644 --- a/src/app/commands/cmd_sprite_size.cpp +++ b/src/app/commands/cmd_sprite_size.cpp @@ -1,5 +1,5 @@ // Aseprite -// Copyright (C) 2019-2023 Igara Studio S.A. +// Copyright (C) 2019-2024 Igara Studio S.A. // Copyright (C) 2001-2018 David Capello // // This program is distributed under the terms of @@ -80,8 +80,11 @@ class SpriteSizeJob : public SpriteJob { public: - SpriteSizeJob(const ContextReader& reader, int new_width, int new_height, ResizeMethod resize_method) - : SpriteJob(reader, Strings::sprite_size_title().c_str()) { + SpriteSizeJob(Context* ctx, Doc* doc, + const int new_width, + const int new_height, + const ResizeMethod resize_method) + : SpriteJob(ctx, doc, Strings::sprite_size_title()) { m_new_width = new_width; m_new_height = new_height; m_resize_method = resize_method; @@ -90,8 +93,8 @@ public: protected: // [working thread] - void onJob() override { - DocApi api = writer().document()->getApi(tx()); + void onSpriteJob(Tx& tx) override { + DocApi api = document()->getApi(tx); Tilesets* tilesets = sprite()->tilesets(); int img_count = 0; @@ -147,7 +150,7 @@ protected: ++progress; ++idx; } - tx()(new cmd::ReplaceTileset(sprite(), tsi, newTileset)); + tx(new cmd::ReplaceTileset(sprite(), tsi, newTileset)); // Cancel all the operation? if (isCanceled()) @@ -170,11 +173,11 @@ protected: cel->y()*scale.h, canvasSize.w, canvasSize.h); - tx()(new cmd::SetCelBoundsF(cel, newBounds)); + tx(new cmd::SetCelBoundsF(cel, newBounds)); } else { resize_cel_image( - tx(), cel, scale, + tx, cel, scale, m_resize_method, cel->layer()->isReference() ? -cel->boundsF().origin(): @@ -239,7 +242,7 @@ protected: newKey.setPivot(gfx::Point(scale_x(newKey.pivot().x), scale_y(newKey.pivot().y))); - tx()(new cmd::SetSliceKey(slice, k.frame(), newKey)); + tx(new cmd::SetSliceKey(slice, k.frame(), newKey)); } } @@ -373,8 +376,9 @@ void SpriteSizeCommand::onExecute(Context* context) #ifdef ENABLE_UI const bool ui = (params().ui() && context->isUIAvailable()); #endif - const ContextReader reader(context); - const Sprite* sprite(reader.sprite()); + const Site site = context->activeSite(); + Doc* doc = site.document(); + Sprite* sprite = site.sprite(); auto& params = this->params(); double ratio = sprite->width() / double(sprite->height()); @@ -461,13 +465,13 @@ void SpriteSizeCommand::onExecute(Context* context) new_height = std::clamp(new_height, 1, DOC_SPRITE_MAX_HEIGHT); { - SpriteSizeJob job(reader, new_width, new_height, resize_method); + SpriteSizeJob job(context, doc, new_width, new_height, resize_method); job.startJob(); job.waitJob(); } #ifdef ENABLE_UI - update_screen_for_document(reader.document()); + update_screen_for_document(doc); #endif } diff --git a/src/app/job.cpp b/src/app/job.cpp index 601709c32..592bbd7ea 100644 --- a/src/app/job.cpp +++ b/src/app/job.cpp @@ -1,5 +1,5 @@ // Aseprite -// Copyright (C) 2021 Igara Studio S.A. +// Copyright (C) 2021-2024 Igara Studio S.A. // Copyright (C) 2001-2018 David Capello // // This program is distributed under the terms of @@ -33,7 +33,7 @@ int Job::runningJobs() return g_runningJobs; } -Job::Job(const char* jobName) +Job::Job(const std::string& jobName) { m_last_progress = 0.0; m_done_flag = false; diff --git a/src/app/job.h b/src/app/job.h index 0adc9a169..6c21bdd22 100644 --- a/src/app/job.h +++ b/src/app/job.h @@ -1,5 +1,5 @@ // Aseprite -// Copyright (C) 2021 Igara Studio S.A. +// Copyright (C) 2021-2024 Igara Studio S.A. // Copyright (C) 2001-2018 David Capello // // This program is distributed under the terms of @@ -15,6 +15,7 @@ #include #include #include +#include #include namespace app { @@ -23,7 +24,7 @@ namespace app { public: static int runningJobs(); - Job(const char* jobName); + Job(const std::string& jobName); virtual ~Job(); // Starts the job calling onJob() event in another thread and diff --git a/src/app/sprite_job.cpp b/src/app/sprite_job.cpp index 8d8792ccd..dbbac0615 100644 --- a/src/app/sprite_job.cpp +++ b/src/app/sprite_job.cpp @@ -1,4 +1,5 @@ // Aseprite +// Copyright (C) 2024 Igara Studio S.A. // Copyright (C) 2017 David Capello // // This program is distributed under the terms of @@ -10,26 +11,49 @@ #include "app/sprite_job.h" +#include "base/log.h" + namespace app { -SpriteJob::SpriteJob(const ContextReader& reader, const char* jobName) +SpriteJob::SpriteJob(Context* ctx, Doc* doc, + const std::string& jobName) : Job(jobName) - , m_writer(reader, 500) - , m_document(m_writer.document()) - , m_sprite(m_writer.sprite()) - , m_tx(m_writer, jobName, ModifyDocument) + , m_doc(doc) + , m_sprite(doc->sprite()) + , m_tx(Tx::DontLockDoc, ctx, doc, jobName, ModifyDocument) + , m_lockAction(Tx::LockDoc) { + // Try to write-lock the document to see if we have to lock the + // document in the background thread. + auto lockResult = m_doc->writeLock(500); + if (lockResult != Doc::LockResult::Fail) { + if (lockResult == Doc::LockResult::Reentrant) + m_lockAction = Tx::DontLockDoc; + m_doc->unlock(lockResult); + } } SpriteJob::~SpriteJob() { - if (!isCanceled()) - m_tx.commit(); + try { + if (!isCanceled()) + m_tx.commit(); + } + catch (const std::exception& ex) { + LOG(ERROR, "Error committing changes: %s\n", ex.what()); + } +} + +void SpriteJob::onSpriteJob(Tx& tx) +{ + if (m_callback) + m_callback(tx); } void SpriteJob::onJob() { - m_callback(); + Tx subtx(m_lockAction, m_ctx, m_doc); + onSpriteJob(subtx); } bool SpriteJob::continueTask() diff --git a/src/app/sprite_job.h b/src/app/sprite_job.h index d6cbf61b4..d0fe180df 100644 --- a/src/app/sprite_job.h +++ b/src/app/sprite_job.h @@ -1,4 +1,5 @@ // Aseprite +// Copyright (C) 2024 Igara Studio S.A. // Copyright (C) 2017-2018 David Capello // // This program is distributed under the terms of @@ -15,19 +16,32 @@ #include "render/task_delegate.h" #include +#include +#include namespace app { +class Context; + +// Creates a Job to run a task in a background thread. At the same +// time it creates a new Tx in the main thread (to group all sub-Txs) +// without locking the sprite. You have to lock the sprite with a sub +// Tx (the onSpriteJob(Tx) already has the sprite locked for write +// access). +// +// This class takes care to lock the sprite in the background thread +// for write access, or to avoid re-locking the sprite in case it's +// already locked from the main thread (where SpriteJob was created, +// generally true when we're running a script). class SpriteJob : public Job, public render::TaskDelegate { public: - SpriteJob(const ContextReader& reader, const char* jobName); + SpriteJob(Context* ctx, Doc* doc, + const std::string& jobName); ~SpriteJob(); - ContextWriter& writer() { return m_writer; } - Doc* document() const { return m_document; } + Doc* document() const { return m_doc; } Sprite* sprite() const { return m_sprite; } - Tx& tx() { return m_tx; } template void startJobWithCallback(T&& callback) { @@ -36,6 +50,8 @@ public: } private: + virtual void onSpriteJob(Tx& tx); + // Job impl void onJob() override; @@ -44,15 +60,21 @@ private: bool continueTask() override; void notifyTaskProgress(double progress) override; - ContextWriter m_writer; - Doc* m_document; + Context* m_ctx; + Doc* m_doc; Sprite* m_sprite; Tx m_tx; + // What action to do with the sub-Tx inside the background thread. + // This is required to check if the sprite is already locked for + // write access in the main thread, in that case we don't need to + // lock it again from the background thread. + Tx::LockAction m_lockAction; + // Default implementation calls the given function in // startJob(). Anyway you can just extended the SpriteJob and // override onJob(). - std::function m_callback; + std::function m_callback; }; } // namespace app