[krita] libs: Fix a crash when saving the image when autosave it ready to save
Boudewijn Rempt
boud at valdyas.org
Fri May 6 16:01:58 UTC 2016
Looks like this broke the OSX build:
[ 29%] Building CXX object libs/ui/CMakeFiles/kritaui.dir/KisDocument.cpp.o
In file included from /Users/boud/dev/krita/libs/ui/KisDocument.cpp:115:
In file included from /Users/boud/dev/i/include/boost/thread.hpp:17:
In file included from /Users/boud/dev/i/include/boost/thread/once.hpp:20:
In file included from /Users/boud/dev/i/include/boost/thread/pthread/once_atomic.hpp:20:
In file included from /Users/boud/dev/i/include/boost/atomic.hpp:12:
In file included from /Users/boud/dev/i/include/boost/atomic/atomic.hpp:17:
In file included from /Users/boud/dev/i/include/boost/atomic/detail/platform.hpp:22:
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:961:64: error: no matching constructor
for initialization of 'storage_type' (aka 'boost::atomics::detail::storage128_type')
explicit base_atomic(value_type const& v) BOOST_NOEXCEPT : v_(0)
^ ~
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit copy constructor) not viable: no known conversion from 'int' to 'const
boost::atomics::detail::storage128_type' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit move constructor) not viable: no known conversion from 'int' to
'boost::atomics::detail::storage128_type' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor (the
implicit default constructor) not viable: requires 0 arguments, but 1 was provided
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:968:22: error: no viable conversion from
'int' to 'storage_type' (aka 'boost::atomics::detail::storage128_type')
storage_type tmp = 0;
^ ~
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit copy constructor) not viable: no known conversion from 'int' to 'const
boost::atomics::detail::storage128_type &' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit move constructor) not viable: no known conversion from 'int' to
'boost::atomics::detail::storage128_type &&' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:983:22: error: no viable conversion from
'int' to 'storage_type' (aka 'boost::atomics::detail::storage128_type')
storage_type tmp = 0;
^ ~
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit copy constructor) not viable: no known conversion from 'int' to 'const
boost::atomics::detail::storage128_type &' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit move constructor) not viable: no known conversion from 'int' to
'boost::atomics::detail::storage128_type &&' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:997:22: error: no viable conversion from
'int' to 'storage_type' (aka 'boost::atomics::detail::storage128_type')
storage_type expected_s = 0, desired_s = 0;
^ ~
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit copy constructor) not viable: no known conversion from 'int' to 'const
boost::atomics::detail::storage128_type &' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit move constructor) not viable: no known conversion from 'int' to
'boost::atomics::detail::storage128_type &&' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:997:38: error: no viable conversion from
'int' to 'storage_type' (aka 'boost::atomics::detail::storage128_type')
storage_type expected_s = 0, desired_s = 0;
^ ~
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit copy constructor) not viable: no known conversion from 'int' to 'const
boost::atomics::detail::storage128_type &' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit move constructor) not viable: no known conversion from 'int' to
'boost::atomics::detail::storage128_type &&' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:1013:22: error: no viable conversion
from 'int' to 'storage_type' (aka 'boost::atomics::detail::storage128_type')
storage_type expected_s = 0, desired_s = 0;
^ ~
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit copy constructor) not viable: no known conversion from 'int' to 'const
boost::atomics::detail::storage128_type &' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit move constructor) not viable: no known conversion from 'int' to
'boost::atomics::detail::storage128_type &&' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:1013:38: error: no viable conversion
from 'int' to 'storage_type' (aka 'boost::atomics::detail::storage128_type')
storage_type expected_s = 0, desired_s = 0;
^ ~
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit copy constructor) not viable: no known conversion from 'int' to 'const
boost::atomics::detail::storage128_type &' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
/Users/boud/dev/i/include/boost/atomic/detail/gcc-atomic.hpp:932:28: note: candidate constructor
(the implicit move constructor) not viable: no known conversion from 'int' to
'boost::atomics::detail::storage128_type &&' for 1st argument
struct BOOST_ALIGNMENT(16) storage128_type
^
7 errors generated.
make[2]: *** [libs/ui/CMakeFiles/kritaui.dir/KisDocument.cpp.o] Error 1
make[1]: *** [libs/ui/CMakeFiles/kritaui.dir/all] Error 2
make: *** [all] Error 2
On Fri, 6 May 2016, Dmitry Kazakov wrote:
> Git commit 5ab2ed2f7b19d8be57f9408f88614c282f0abaa0 by Dmitry Kazakov.
> Committed on 06/05/2016 at 08:54.
> Pushed by dkazakov into branch 'master'.
>
> Fix a crash when saving the image when autosave it ready to save
>
> The crash happened because autosave decided to save at
> the moment of time when the user has been saving the
> image itself. And given that we save the image in a
> background thread, autosave could easily do it.
>
> Now SafeSignalLocker blocks not only the image, but also
> a special lock that guard KisDocument from entering the
> saving code twice.
>
> BUG:362675
> Fixes T2430
>
> M +20 -0 libs/global/kis_global.h
> A +63 -0 libs/image/kis_image_barrier_lock_adapter.h [License: GPL (v2+)]
> M +83 -68 libs/ui/KisDocument.cpp
> M +6 -6 libs/ui/KisDocument.h
>
> http://commits.kde.org/krita/5ab2ed2f7b19d8be57f9408f88614c282f0abaa0
>
> diff --git a/libs/global/kis_global.h b/libs/global/kis_global.h
> index bfaabcd..6fef512 100644
> --- a/libs/global/kis_global.h
> +++ b/libs/global/kis_global.h
> @@ -241,5 +241,25 @@ List<QSharedPointer<A>> listToQShared(const List<A*> list) {
> return newList;
> }
>
> +template <class T>
> +struct BoostLockableWrapper {
> + BoostLockableWrapper(T *lock) : m_lock(lock) {}
> +
> + void lock() {
> + m_lock->lock();
> + }
> +
> + bool try_lock() {
> + return m_lock->tryLock();
> + }
> +
> + void unlock() {
> + m_lock->unlock();
> + }
> +
> +private:
> + T *m_lock;
> +};
> +
> #endif // KISGLOBAL_H_
>
> diff --git a/libs/image/kis_image_barrier_lock_adapter.h b/libs/image/kis_image_barrier_lock_adapter.h
> new file mode 100644
> index 0000000..2f949e2
> --- /dev/null
> +++ b/libs/image/kis_image_barrier_lock_adapter.h
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (c) 2015 Dmitry Kazakov <dimula73 at gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + */
> +
> +#ifndef __KIS_IMAGE_BARRIER_LOCK_ADAPTER_H
> +#define __KIS_IMAGE_BARRIER_LOCK_ADAPTER_H
> +
> +
> +template <typename ImagePointer>
> +class KisImageBarrierLockAdapterImpl {
> +public:
> + inline KisImageBarrierLockAdapterImpl(ImagePointer image, bool readOnly = false)
> + : m_image(image),
> + m_readOnly(readOnly)
> + {
> + }
> +
> + inline ~KisImageBarrierLockAdapterImpl() {
> + m_image->unlock();
> + }
> +
> + inline void lock() {
> + m_image->barrierLock(m_readOnly);
> + }
> +
> + // Qt syntax
> + inline bool tryLock() {
> + return m_image->tryBarrierLock(m_readOnly);
> + }
> +
> + // Boost.Thread syntax
> + inline bool try_lock() {
> + return tryLock();
> + }
> +
> + inline void unlock() {
> + m_image->unlock();
> + }
> +
> +private:
> + KisImageBarrierLockAdapterImpl(const KisImageBarrierLockAdapterImpl<ImagePointer> &rhs);
> + ImagePointer m_image;
> + bool m_readOnly;
> +};
> +
> +typedef KisImageBarrierLockAdapterImpl<KisImageSP> KisImageBarrierLockAdapter;
> +typedef KisImageBarrierLockAdapterImpl<KisImage*> KisImageBarrierLockAdapterRaw;
> +
> +#endif /* __KIS_IMAGE_BARRIER_LOCK_ADAPTER_H */
> diff --git a/libs/ui/KisDocument.cpp b/libs/ui/KisDocument.cpp
> index a942911..18feed6 100644
> --- a/libs/ui/KisDocument.cpp
> +++ b/libs/ui/KisDocument.cpp
> @@ -112,6 +112,8 @@
> #include "kis_async_action_feedback.h"
> #include "kis_grid_config.h"
> #include "kis_guides_config.h"
> +#include <boost/thread.hpp>
> +#include "kis_image_barrier_lock_adapter.h"
>
>
> static const char CURRENT_DTD_VERSION[] = "2.0";
> @@ -230,54 +232,6 @@ private:
> KisDocument *m_doc;
> };
>
> -class SafeSavingLocker {
> -public:
> - SafeSavingLocker(KisDocument *doc)
> - : m_doc(doc),
> - m_locked(false)
> - {
> - const int realAutoSaveInterval = KisConfig().autoSaveInterval();
> - const int emergencyAutoSaveInterval = 10; // sec
> -
> - if (!m_doc->image()->tryBarrierLock(true)) {
> - if (m_doc->isAutosaving()) {
> - m_doc->setDisregardAutosaveFailure(true);
> - if (realAutoSaveInterval) {
> - m_doc->setAutoSave(emergencyAutoSaveInterval);
> - }
> - m_locked = false;
> - } else {
> - m_doc->image()->requestStrokeEnd();
> - QApplication::processEvents();
> - m_locked = m_doc->image()->tryBarrierLock(true);
> - }
> - } else {
> - m_locked = true;
> - }
> -
> - if (m_locked) {
> - m_doc->setDisregardAutosaveFailure(false);
> - }
> - }
> -
> - ~SafeSavingLocker() {
> - if (m_locked) {
> - m_doc->image()->unlock();
> -
> - const int realAutoSaveInterval = KisConfig().autoSaveInterval();
> - m_doc->setAutoSave(realAutoSaveInterval);
> - }
> - }
> -
> - bool successfullyLocked() const {
> - return m_locked;
> - }
> -
> -private:
> - KisDocument *m_doc;
> - bool m_locked;
> -};
> -
> class Q_DECL_HIDDEN KisDocument::Private
> {
> public:
> @@ -293,7 +247,7 @@ public:
> isExporting(false),
> password(QString()),
> modifiedAfterAutosave(false),
> - autosaving(false),
> + isAutosaving(false),
> shouldCheckAutoSaveFile(true),
> autoErrorHandlingEnabled(true),
> backupFile(true),
> @@ -354,7 +308,7 @@ public:
> QString lastErrorMessage; // see openFile()
> int autoSaveDelay; // in seconds, 0 to disable.
> bool modifiedAfterAutosave;
> - bool autosaving;
> + bool isAutosaving;
> bool shouldCheckAutoSaveFile; // usually true
> bool autoErrorHandlingEnabled; // usually true
> bool backupFile;
> @@ -381,6 +335,7 @@ public:
> QUrl m_url; // Remote (or local) url - the one displayed to the user.
> QString m_file; // Local file - the only one the part implementation should deal with.
> QEventLoop m_eventLoop;
> + QMutex savingMutex;
>
> bool modified;
> bool readwrite;
> @@ -467,6 +422,72 @@ public:
> image.data(), SLOT(explicitRegenerateLevelOfDetail())));
> }
> }
> +
> + class SafeSavingLocker;
> +};
> +
> +class KisDocument::Private::SafeSavingLocker {
> +public:
> + SafeSavingLocker(KisDocument::Private *_d)
> + : d(_d),
> + m_locked(false),
> + m_imageLock(d->image, true),
> + m_savingLock(&d->savingMutex)
> + {
> + const int realAutoSaveInterval = KisConfig().autoSaveInterval();
> + const int emergencyAutoSaveInterval = 10; // sec
> +
> + /**
> + * Initial try to lock both objects. Locking the image guards
> + * us from any i,age composition threads running in the
> + * background, while savingMutex guards us from entering the
> + * saving code twice by autosave and main threads.
> + *
> + * Since we are trying to lock multiple objects, so we should
> + * do it in a safe manner.
> + */
> + m_locked = boost::try_lock(m_imageLock, m_savingLock) < 0;
> +
> + if (!m_locked) {
> + if (d->isAutosaving) {
> + d->disregardAutosaveFailure = true;
> + if (realAutoSaveInterval) {
> + d->document->setAutoSave(emergencyAutoSaveInterval);
> + }
> + } else {
> + d->image->requestStrokeEnd();
> + QApplication::processEvents();
> +
> + // one more try...
> + m_locked = boost::try_lock(m_imageLock, m_savingLock) < 0;
> + }
> + }
> +
> + if (m_locked) {
> + d->disregardAutosaveFailure = false;
> + }
> + }
> +
> + ~SafeSavingLocker() {
> + if (m_locked) {
> + m_imageLock.unlock();
> + m_savingLock.unlock();
> +
> + const int realAutoSaveInterval = KisConfig().autoSaveInterval();
> + d->document->setAutoSave(realAutoSaveInterval);
> + }
> + }
> +
> + bool successfullyLocked() const {
> + return m_locked;
> + }
> +
> +private:
> + KisDocument::Private *d;
> + bool m_locked;
> +
> + KisImageBarrierLockAdapter m_imageLock;
> + BoostLockableWrapper<QMutex> m_savingLock;
> };
>
> KisDocument::KisDocument()
> @@ -660,7 +681,7 @@ bool KisDocument::saveFile()
> if (!isNativeFormat(outputMimeType)) {
> dbgUI << "Saving to format" << outputMimeType << "in" << localFilePath();
>
> - SafeSavingLocker locker(this);
> + Private::SafeSavingLocker locker(d);
> if (locker.successfullyLocked()) {
> status = d->filterManager->exportDocument(localFilePath(), outputMimeType);
> } else {
> @@ -817,14 +838,14 @@ void KisDocument::slotAutoSave()
> } else {
> connect(this, SIGNAL(sigProgress(int)), KisPart::instance()->currentMainwindow(), SLOT(slotProgress(int)));
> emit statusBarMessage(i18n("Autosaving..."));
> - d->autosaving = true;
> + d->isAutosaving = true;
> bool ret = saveNativeFormat(autoSaveFile(localFilePath()));
> setModified(true);
> if (ret) {
> d->modifiedAfterAutosave = false;
> d->autoSaveTimer.stop(); // until the next change
> }
> - d->autosaving = false;
> + d->isAutosaving = false;
> emit clearStatusBarMessage();
> disconnect(this, SIGNAL(sigProgress(int)), KisPart::instance()->currentMainwindow(), SLOT(slotProgress(int)));
> if (!ret && !d->disregardAutosaveFailure) {
> @@ -866,7 +887,7 @@ bool KisDocument::isModified() const
>
> bool KisDocument::saveNativeFormat(const QString & file)
> {
> - SafeSavingLocker locker(this);
> + Private::SafeSavingLocker locker(d);
> if (!locker.successfullyLocked()) return false;
>
> d->lastErrorMessage.clear();
> @@ -904,7 +925,7 @@ bool KisDocument::saveNativeFormat(const QString & file)
>
> bool result = false;
>
> - if (!isAutosaving()) {
> + if (!d->isAutosaving) {
> KisAsyncActionFeedback f(i18n("Saving document..."), 0);
> result = f.runAction(std::bind(&KisDocument::saveNativeFormatCalligra, this, store));
> } else {
> @@ -941,7 +962,7 @@ bool KisDocument::saveNativeFormatCalligra(KoStore *store)
> (void)store->close();
> }
>
> - if (!isAutosaving()) {
> + if (!d->isAutosaving) {
> if (store->open("preview.png")) {
> // ### TODO: missing error checking (The partition could be full!)
> savePreview(store);
> @@ -1058,11 +1079,6 @@ QString KisDocument::autoSaveFile(const QString & path) const
> return retval;
> }
>
> -void KisDocument::setDisregardAutosaveFailure(bool disregardFailure)
> -{
> - d->disregardAutosaveFailure = disregardFailure;
> -}
> -
> bool KisDocument::importDocument(const QUrl &_url)
> {
> bool ret;
> @@ -1576,7 +1592,7 @@ void KisDocument::setModified(bool mod)
> updateEditingTime(false);
> }
>
> - if (isAutosaving()) // ignore setModified calls due to autosaving
> + if (d->isAutosaving) // ignore setModified calls due to autosaving
> return;
>
> if ( !d->readwrite && d->modified ) {
> @@ -1708,7 +1724,7 @@ bool KisDocument::completeLoading(KoStore* store)
> bool KisDocument::completeSaving(KoStore* store)
> {
> d->kraSaver->saveKeyframes(store, url().url(), isStoredExtern());
> - d->kraSaver->saveBinaryData(store, d->image, url().url(), isStoredExtern(), isAutosaving());
> + d->kraSaver->saveBinaryData(store, d->image, url().url(), isStoredExtern(), d->isAutosaving);
> bool retval = true;
> if (!d->kraSaver->errorMessages().isEmpty()) {
> setErrorMessage(d->kraSaver->errorMessages().join(".\n"));
> @@ -1862,11 +1878,6 @@ void KisDocument::showLoadingErrorDialog()
> }
> }
>
> -bool KisDocument::isAutosaving() const
> -{
> - return d->autosaving;
> -}
> -
> bool KisDocument::isLoading() const
> {
> return d->isLoading;
> @@ -2440,3 +2451,7 @@ KisUndoStore* KisDocument::createUndoStore()
> return new KisDocumentUndoStore(this);
> }
>
> +bool KisDocument::isAutosaving() const
> +{
> + return d->isAutosaving;
> +}
> diff --git a/libs/ui/KisDocument.h b/libs/ui/KisDocument.h
> index 689204c..b56e321 100644
> --- a/libs/ui/KisDocument.h
> +++ b/libs/ui/KisDocument.h
> @@ -365,11 +365,6 @@ public:
> void setAutoSave(int delay);
>
> /**
> - * Checks whether the document is currently in the process of autosaving
> - */
> - bool isAutosaving() const;
> -
> - /**
> * Set whether the next openUrl call should check for an auto-saved file
> * and offer to open it. This is usually true, but can be turned off
> * (e.g. for the preview module). This only checks for names auto-saved
> @@ -625,7 +620,6 @@ private:
> QString newObjectName();
>
> QString autoSaveFile(const QString & path) const;
> - void setDisregardAutosaveFailure(bool disregardFailure);
>
> /**
> * Loads a document
> @@ -686,6 +680,12 @@ private:
> */
> bool isExporting() const;
>
> + /**
> + * Legacy method from KoDocumentBase. Don't use it anywhere
> + * outside KisDocument!
> + */
> + bool isAutosaving() const;
> +
> public:
>
> QString localFilePath() const;
>
--
Boudewijn Rempt | http://www.krita.org, http://www.valdyas.org
More information about the kimageshop
mailing list