Fix issue #394 - crash when we cancel the file save operation

There were problems calling a pure virtual function (IFileOpProgress
implemented by OpenFileJob) when we are already in ~Job() dtor. So we've
to wait the background thread (added Job::waitJob() function) to join
the thread so it can use IFileOpProgress safely.

Also the save process of .ase files now can be cancelled (it wasn't
possible before).

src/app/commands/cmd_open_file.cpp

@@ -69,7 +69,11 @@ public:
 
   void showProgressWindow() {
     startJob();
+
+    if (isCanceled())
       fop_stop(m_fop);
+
+    waitJob();
   }
 
 private:

src/app/commands/cmd_rotate_canvas.cpp

@@ -204,6 +204,7 @@ void RotateCanvasCommand::onExecute(Context* context)
   {
     RotateCanvasJob job(reader, m_angle);
     job.startJob();
+    job.waitJob();
   }
   reader.document()->generateMaskBoundaries();
   update_screen_for_document(reader.document());

src/app/commands/cmd_save_file.cpp

@@ -53,9 +53,14 @@ public:
 
   void showProgressWindow() {
     startJob();
+
+    if (isCanceled()) {
       fop_stop(m_fop);
     }
 
+    waitJob();
+  }
+
 private:
 
   // Thread to do the hard work: save the file to the disk.
@@ -89,6 +94,10 @@ static void save_document_in_background(Document* document, bool mark_as_saved)
     Console console;
     console.printf(fop->error.c_str());
   }
+  // If the job was cancelled, mark the document as modified.
+  else if (fop_is_stop(fop)) {
+    document->impossibleToBackToSavedState();
+  }
   else {
     App::instance()->getRecentFiles()->addRecentFile(document->getFilename().c_str());
     if (mark_as_saved)

src/app/commands/cmd_sprite_size.cpp

@@ -243,6 +243,7 @@ void SpriteSizeCommand::onExecute(Context* context)
     {
       SpriteSizeJob job(reader, new_width, new_height, resize_method);
       job.startJob();
+      job.waitJob();
     }
 
     ContextWriter writer(reader);

src/app/document.cpp

@@ -210,6 +210,11 @@ void Document::markAsSaved()
   m_associated_to_file = true;
 }
 
+void Document::impossibleToBackToSavedState()
+{
+  m_undo->impossibleToBackToSavedState();
+}
+
 //////////////////////////////////////////////////////////////////////
 // Loaded options from file
 

src/app/document.h

@@ -130,6 +130,12 @@ namespace app {
     bool isAssociatedToFile() const;
     void markAsSaved();
 
+    // You can use this to indicate that we've destroyed (or we cannot
+    // trust) the file associated with the document (e.g. when we
+    // cancel a Save operation in the middle). So it's impossible to
+    // back to the saved state using the UndoHistory.
+    void impossibleToBackToSavedState();
+
     //////////////////////////////////////////////////////////////////////
     // Loaded options from file
 

src/app/document_undo.cpp

@@ -74,6 +74,11 @@ void DocumentUndo::markSavedState()
   return m_undoHistory->markSavedState();
 }
 
+void DocumentUndo::impossibleToBackToSavedState()
+{
+  m_undoHistory->impossibleToBackToSavedState();
+}
+
 void DocumentUndo::pushUndoer(undo::Undoer* undoer)
 {
   return m_undoHistory->pushUndoer(undoer);

src/app/document_undo.h

@@ -55,6 +55,7 @@ namespace app {
 
     bool isSavedState() const;
     void markSavedState();
+    void impossibleToBackToSavedState();
 
     // UndoHistoryDelegate implementation.
     undo::ObjectsContainer* getObjects() const OVERRIDE { return m_objects; }

src/app/file/ase_format.cpp

@@ -321,25 +321,25 @@ bool AseFormat::onSave(FileOp* fop)
     // Frame duration
     frame_header.duration = sprite->getFrameDuration(frame);
 
-    /* the sprite is indexed and the palette changes? (or is the first frame) */
+    // The sprite is indexed and the palette changes? (or is the first frame)
     if (sprite->getPixelFormat() == IMAGE_INDEXED &&
         (frame == 0 ||
          sprite->getPalette(frame.previous())->countDiff(sprite->getPalette(frame), NULL, NULL) > 0)) {
-      /* write the color chunk */
+      // Write the color chunk
       ase_file_write_color2_chunk(f, &frame_header, sprite->getPalette(frame));
     }
 
-    /* write extra chunks in the first frame */
+    // Write extra chunks in the first frame
     if (frame == 0) {
       LayerIterator it = sprite->getFolder()->getLayerBegin();
       LayerIterator end = sprite->getFolder()->getLayerEnd();
 
-      /* write layer chunks */
+      // Write layer chunks
       for (; it != end; ++it)
         ase_file_write_layers(f, &frame_header, *it);
     }
 
-    /* write cel chunks */
+    // Write cel chunks
     ase_file_write_cels(f, &frame_header, sprite, sprite->getFolder(), frame);
 
     // Write the frame header
@@ -348,6 +348,9 @@ bool AseFormat::onSave(FileOp* fop)
     // Progress
     if (sprite->getTotalFrames() > 1)
       fop_progress(fop, (float)(frame.next()) / (float)(sprite->getTotalFrames()));
+
+    if (fop_is_stop(fop))
+      break;
   }
 
   // Write the missing field (filesize) of the header.

src/app/job.cpp

@@ -54,24 +54,12 @@ Job::Job(const char* job_name)
 
 Job::~Job()
 {
+  ASSERT(!m_timer->isRunning());
+  ASSERT(m_thread == NULL);
+
   if (m_alert_window != NULL)
     m_alert_window->closeWindow(NULL);
 
-  // The job was canceled by the user?
-  {
-    base::scoped_lock hold(*m_mutex);
-    if (!m_done_flag)
-      m_canceled_flag = true;
-  }
-
-  if (m_timer->isRunning())
-    m_timer->stop();
-
-  if (m_thread) {
-    m_thread->join();
-    delete m_thread;
-  }
-
   if (m_progress)
     delete m_progress;
 
@@ -83,6 +71,25 @@ void Job::startJob()
 {
   m_thread = new base::thread(&Job::thread_proc, this);
   m_alert_window->openWindowInForeground();
+
+  // The job was canceled by the user?
+  {
+    base::scoped_lock hold(*m_mutex);
+    if (!m_done_flag)
+      m_canceled_flag = true;
+  }
+}
+
+void Job::waitJob()
+{
+  if (m_timer->isRunning())
+    m_timer->stop();
+
+  if (m_thread) {
+    m_thread->join();
+    delete m_thread;
+    m_thread = NULL;
+  }
 }
 
 void Job::jobProgress(double f)

src/app/job.h

@@ -41,6 +41,8 @@ namespace app {
     // monitoring the progress with onMonitorTick() event.
     void startJob();
 
+    void waitJob();
+
     // The onJob() can use this function to report progress of the
     // background job being done. 1.0 is completed.
     void jobProgress(double f);

src/undo/undo_history.cpp

@@ -71,7 +71,7 @@ void UndoHistory::clearRedo()
   // that state again, so we have to put a value in m_diffSaved
   // impossible to be equal to m_diffCount.
   if (m_diffCount < m_diffSaved)
-    m_diffSaved = -1;
+    impossibleToBackToSavedState();
 }
 
 Undoer* UndoHistory::getNextUndoer()
@@ -100,6 +100,11 @@ void UndoHistory::markSavedState()
   m_diffSaved = m_diffCount;
 }
 
+void UndoHistory::impossibleToBackToSavedState()
+{
+  m_diffSaved = -1;
+}
+
 void UndoHistory::runUndo(Direction direction)
 {
   UndoersStack* undoers = ((direction == UndoDirection)? m_undoers: m_redoers);

src/undo/undo_history.h

@@ -48,6 +48,7 @@ namespace undo {
 
     bool isSavedState() const;
     void markSavedState();
+    void impossibleToBackToSavedState();
 
     ObjectsContainer* getObjects() const { return m_delegate->getObjects(); }