281002 - During folder drag and drop, some folders can be lost

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[William Bonnet]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.7.5) Gecko/20041108 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.7.5) Gecko/20041108 Firefox/1.0

If you drag and drop a folder containing subfolders (local folders), if the move
of a folder fails, the copy stops and ALL the folders are deleted

Reproducible: Always

Steps to Reproduce:
Reproduceable everytime with the *good* data. You just have to drag and drop
folders, but you can see this bug only if one folder cannot be copied
Actual Results:  
Some folders where lost (about 120mb of emails) without warning the user that
move failed. This is a critical problem because data are lost, and if it happens
deep inside a folder tree the top folders are moved and you don t see the the
bottom folders are lost

Expected Results:  
Move all the folders or rollback

Comment 1

[William Bonnet]

Attachment: This is a fixed version of the copyFolderLocal method

This bug fixing is VERY important because it fixes a major data loss. In one
drag and drop you can loose hundred of meg of email.

During folder move the copy can fail (it happens on my mail db). Somewhere in
the recursive callstack on copyFolderLocal one copydir fails, but the error was
not returned at to topmost level. Thus recursive copy stopped without error
code, and all the folders are deleted

I fix that, and i added a "kink of rollback". If the copy fails i deleted the
target folder and let the srcFolder without deleting it. It works on my
computer

Comment 2

[timeless]

Attachment: patch against trunk (not compiled) tabs converted to spaces, no else after break/return after else, !NS_SUCCEEDED=>NS_FAILED

(not compiled)
tabs converted to spaces
no else after break/return after else
!NS_SUCCEEDED=>NS_FAILED
trailing whitespace removed
lines limited to 80 cols

Comment 3

[William Bonnet]

Attachment: This patch copy the local folder to another local folder handling copy error on filespec in the case .msf file does not exist or is zero size. In such case specfile is not copied, and recreated later by a GetDatabaseWithReparse call

This patch complete the fixing of the lost folders when drag and dropping
folder.

When a folder is moved it is done by a recursive copy then recursive delete
algorithm. If for any reason the CopyFolderLocal fails (not at toplevel), the
error code was not thrown up to the root of the recursive call. Thus at top
level error was ignored, and the recursive delete was call.

This patch add error handling, and rollback if the copy fails. The source is
not deleted, and the destination is destroyed (since partially copied).

For the user the folders just don't move (better than loosing a part of the
data).

This patch add a second fix. Most of the time the copy fails because the .msf
file does not exist. If the copy fails for the filespec, i test if the filespec
exist or not, if not i update the folder (call to UpdateFolder) and try to copy
again. If it fails again it's another error. But most of the time it will
reconstruct index and continue

Comment 4

[William Bonnet]

Comment on attachment 173473 [details] [diff] [review]
This patch copy the local folder to another local folder handling copy error on filespec in the case .msf file does not exist or is zero size. In such case specfile is not copied, and recreated later by a GetDatabaseWithReparse call

Index: nsLocalMailFolder.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v
retrieving revision 1.495
diff -u -p -1 -0 -r1.495 nsLocalMailFolder.cpp
--- nsLocalMailFolder.cpp	1 Feb 2005 17:29:58 -0000	1.495
+++ nsLocalMailFolder.cpp	6 Feb 2005 01:05:43 -0000
@@ -21,20 +21,21 @@
  *
  * Contributor(s):
  *   [e-mail]
  *   [e-mail]
  *   [e-mail]
  *   [e-mail]
  *   [e-mail]
  *   [e-mail]
  *   Pierre Phaneuf <[e-mail]>
  *   Howard Chu <[e-mail]>
+ *   William Bonnet <[e-mail]>
  *
  * Alternatively, the contents of this file may be used under the terms of
  * either of the GNU General Public License Version 2 or later (the "GPL"),
  * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
  * in which case the provisions of the GPL or the LGPL are applicable instead
  * of those above. If you wish to allow use of your version of this file only
  * under the terms of either the GPL or the LGPL, and not to allow others to
  * use your version of this file under the terms of the MPL, indicate your
  * decision by deleting the provisions above and replace them with the notice
  * and other provisions required by the GPL or the LGPL. If you do not delete
@@ -1886,21 +1887,22 @@ nsMsgLocalMailFolder::CopyFolder( nsIMsg

   if (isMoveFolder)   // isMoveFolder == true when "this" and srcFolder are on
same server
     rv = CopyFolderLocal(srcFolder, isMoveFolder, msgWindow, listener );
   else
     rv = CopyFolderAcrossServer(srcFolder, msgWindow, listener );

   return rv;
 }

 NS_IMETHODIMP
-nsMsgLocalMailFolder::CopyFolderLocal(nsIMsgFolder *srcFolder, PRBool
isMoveFolder,
+nsMsgLocalMailFolder::CopyFolderLocal(nsIMsgFolder *srcFolder, 
+				       PRBool isMoveFolder,
				       nsIMsgWindow *msgWindow,
				       nsIMsgCopyServiceListener *listener )
 {
   nsresult rv;
   mInitialized = PR_TRUE;
   nsCOMPtr<nsIMsgFolder> newMsgFolder;
   PRBool isChildOfTrash=PR_FALSE;
   rv = IsChildOfTrash(&isChildOfTrash);
   if (isChildOfTrash)	
   {
@@ -1961,24 +1963,38 @@ nsMsgLocalMailFolder::CopyFolderLocal(ns
     newPath.CreateDirectory();
   }

   rv = CheckIfFolderExists(folderName.get(), this, msgWindow);
   if(NS_FAILED(rv)) 
     return rv;

   nsFileSpec path = oldPath;

   rv = path.CopyToDir(newPath);   //copying necessary for aborting.... if
failure return
-  NS_ENSURE_SUCCESS(rv, rv);	  //would fail if a file by that name exists
+  NS_ENSURE_SUCCESS(rv, rv);	   //would fail if a file by that name exists

-  rv = summarySpec.CopyToDir(newPath);
-  NS_ENSURE_SUCCESS(rv, rv);
+  // Copy to dir can fail if filespec does not exist. If copy fails, we test
+  // if the filespec exist or not, if it does not that's ok, we continue 
+  // without copying it. If it fails and filespec exist and is not zero sized
+  // there is real problem
+  rv = summarySpec.CopyToDir(newPath);      // Copy the filespec to the new
dir
+  if (! NS_SUCCEEDED(rv))		     // Test if the copy is successfull
+  {					   
+    if (summarySpec.Exists() == PR_TRUE)    // Test if filespec exist
+    {					     // 
+      if (summarySpec.GetFileSize() > 0)    // Test if the filespec has data
+	 NS_ENSURE_SUCCESS(rv, rv);	     // Yes, it should have worked !
+      // else case is filespec is zero sized, no need to copy it, that's not 
+      // an error
+    }
+    // else case is filespec does not exist that's not an error
+  }

   // linux and mac are not good about maintaining the file stamp when copying
folders
   // around. So if the source folder db is good, set the dest db as good too.
   nsCOMPtr <nsIMsgDatabase> destDB;
   if (summaryValid)
   {
     nsCAutoString folderLeafName;
     folderLeafName.Adopt(path.GetLeafName());
     newPath += folderLeafName.get();
     nsCOMPtr<nsIMsgDBService> msgDBService =
do_GetService(NS_MSGDB_SERVICE_CONTRACTID, &rv);
@@ -2010,23 +2026,38 @@ nsMsgLocalMailFolder::CopyFolderLocal(ns
   nsCOMPtr<nsISupports> supports;
   rv = aEnumerator->First();
   nsresult copyStatus = NS_OK;
   while (NS_SUCCEEDED(rv) && NS_SUCCEEDED(copyStatus))
   {
     rv = aEnumerator->CurrentItem(getter_AddRefs(supports));
     folder = do_QueryInterface(supports);
     rv = aEnumerator->Next();
     if (folder)
     {
-      nsCOMPtr <nsIMsgLocalMailFolder> localFolder =
do_QueryInterface(newMsgFolder);
+      nsCOMPtr <nsIMsgLocalMailFolder> localFolder =
+	 do_QueryInterface(newMsgFolder);
       if (localFolder)
-	 copyStatus = localFolder->CopyFolderLocal(folder, PR_FALSE, msgWindow,
listener);  // PR_FALSE needed to avoid un-necessary deletions
+      {
+	 // PR_FALSE needed to avoid un-necessary deletions
+	 copyStatus = localFolder->CopyFolderLocal(folder, PR_FALSE, msgWindow,
listener);
+	 // Test if the call succeeded, if not we have to stop recursive call
+	 if (NS_FAILED(copyStatus))
+	 {
+	   // Copy failed we have to notify caller to handle the error and stop
+	   // moving the folders. In case this happens to the topmost level of
+	   // recursive call, then we just need to break from the while loop
and
+	   // go to error handling code.
+	   if (!isMoveFolder)
+	     return copyStatus;
+	   break;
+	 }
+      }
     }
   }  

   if (isMoveFolder && NS_SUCCEEDED(copyStatus))
   {
     //notifying the "folder" that was dragged and dropped has been created.
     //no need to do this for its subfolders - isMoveFolder will be true for
"folder"
     NotifyItemAdded(newMsgFolder);

     nsCOMPtr<nsIMsgFolder> msgParent;
@@ -2047,20 +2078,44 @@ nsMsgLocalMailFolder::CopyFolderLocal(ns
       rv = parentPathSpec->GetFileSpec(&parentPath);
       NS_ENSURE_SUCCESS(rv,rv);

       AddDirectorySeparator(parentPath); 
       nsDirectoryIterator i(parentPath, PR_FALSE);
       // i.Exists() checks if the directory is empty or not 
       if (parentPath.IsDirectory() && !i.Exists())
	 parentPath.Delete(PR_TRUE);
     }
   }
+  else
+  {
+    // This is the case where the copy of a subfolder failed.
+    // We have to delete the newDirectory tree to make a "rollback".
+    // Someone should add a popup to warn the user that the move was not
+    // possible.
+    if (isMoveFolder && NS_FAILED(copyStatus))
+    {
+      nsCOMPtr<nsIMsgFolder> msgParent;
+      newMsgFolder->ForceDBClosed();
+      newMsgFolder->GetParentMsgFolder(getter_AddRefs(msgParent));
+      newMsgFolder->SetParent(nsnull);
+      if (msgParent)
+      {
+	 msgParent->PropagateDelete(newMsgFolder, PR_FALSE, msgWindow);
+	 newMsgFolder->Delete();
+	 newMsgFolder->ForceDBClosed();
+	 AddDirectorySeparator(newPath);
+	 newPath.Delete(PR_TRUE);  //berkeley mailbox
+      }
+      return NS_ERROR_FAILURE;
+    }
+  }
+
   return NS_OK;
 }

 NS_IMETHODIMP
 nsMsgLocalMailFolder::CopyFileMessage(nsIFileSpec* fileSpec, nsIMsgDBHdr*
				       msgToReplace, PRBool isDraftOrTemplate,
				       nsIMsgWindow *msgWindow,
				       nsIMsgCopyServiceListener* listener)
 {
   nsresult rv = NS_ERROR_NULL_POINTER;
@@ -3030,21 +3085,21 @@ NS_IMETHODIMP nsMsgLocalMailFolder::Sele
   {
     nsCAutoString newuri;
     nsBuildLocalMessageURI(mBaseMessageURI, mDownloadSelectKey, newuri);
     mDownloadWindow->SelectMessage(newuri.get());
     mDownloadState = DOWNLOAD_STATE_DIDSEL;
   }
   return NS_OK;
 }

 NS_IMETHODIMP nsMsgLocalMailFolder::DownloadMessagesForOffline(
-	nsISupportsArray *aMessages, nsIMsgWindow *aWindow)
+  nsISupportsArray *aMessages, nsIMsgWindow *aWindow)
 {
   if (mDownloadState != DOWNLOAD_STATE_NONE) 
     return NS_ERROR_FAILURE; // already has a download in progress

   // We're starting a download...
   mDownloadState = DOWNLOAD_STATE_INITED;

   MarkMsgsOnPop3Server(aMessages, POP3_FETCH_BODY);

   // Pull out all the PARTIAL messages into a new array
@@ -3618,24 +3673,24 @@ nsMsgLocalMailFolder::GetUidlFromFolder(
     {
       size = aState->m_header.Length();
       if (!size)
	 break;
       len -= size;
       // account key header will always be before X_UIDL header
       if (!accountKey)
       {
	 accountKey = strstr(aState->m_header.get(),
HEADER_X_MOZILLA_ACCOUNT_KEY);
	 if (accountKey)
-	{
+	 {
	   accountKey += strlen(HEADER_X_MOZILLA_ACCOUNT_KEY) + 2;
-	  aState->m_accountKey = accountKey;
-	}
+	   aState->m_accountKey = accountKey;
+	 }
       } else
       {
	 aState->m_uidl = strstr(aState->m_header.get(), X_UIDL);
	 if (aState->m_uidl)
	 {
	   aState->m_uidl += X_UIDL_LEN + 2; // skip UIDL: header
	   break;
	 }
       }
     }

Comment 5

[William Bonnet]

Comment on attachment 173473 [details] [diff] [review]
This patch copy the local folder to another local folder handling copy error on filespec in the case .msf file does not exist or is zero size. In such case specfile is not copied, and recreated later by a GetDatabaseWithReparse call

The reindexation while copying has not been successfully tested in all cases. I
remove it. It works fine with creation of filespec on the  fly

Comment 6

[William Bonnet]

Attachment: Final version of patch fixing folder drag and drop

Final version of the patch

. fixes folder drag and drop (copy delete)
. handles copy of folders with missing or zero sized specfile
. does not create missing specfile while copy

Comment 7

[Brendan Eich [:brendan]]

Confirming, putting on tb1.0.1 radar.

mscott, can you review or should bienvenu?

/be

Comment 8

[sairuh (rarely reading bugmail)]

what would be the best way to test this? start to copy something, then somehow
interrupt the copying process, if so, how?

Comment 9

[William Bonnet]

Hi,

I submitted a atch to fix this bug.

I think the best to do is what is implemented in the patch. That is

1/ Start copying
2/ If an error occur stop copying
3/ Deleting new files (what was copied so far to destination)
4/ Do not change anything to old files (source)

It sounds good to me because the user has exactly the same data ine the same the
data were before he called the function that failed.

The only missing thing in my patch is that there is no popup warning the user
that the move failed for some reasons

Comment 10

[William Bonnet]

It looks like my previous answer was inaccurate... I was talking about the
algorithm, not the way to test it.

If you want to test it you have several ways of doing it.

First you can create an empty index file (zero sized .msf) somewhere in the
folder tree, copy will fail.

Or you can change read permission (remove read permission) on a folder or on a
file you have to copy.

Comment 11

[Scott MacGregor]

moving to 1.1 for investigation

Comment 12

[William Bonnet]

Hi

> moving to 1.1 for investigation

Can you please tell me what is there to investigate ? 

I detailed the way to reproduce the bug 100% of the time, and submited a patch
that fix it. Even if the source of the problem (bug 281222) can be fixed later
after more investigation (since its based on code investigation, not on a
reproducable bug that was fixed), i personnaly believe that this bug needs an
EMERGENCY fix for people working under Linux. That means, even if we don't fix
the origin of the data loss, at least we prevent more data loss.

I remember you that the first patch was submited on the 2 of February, that is
more than a month ago. I also remember you that i discovered this bug after
loosing 120Mb of email in one single drag and drop.

Don't you think it should be fixed in 1.0.1 ?

Tell me please if you need some more information on this bug.

Comment 13

[Asa Dotzler [:asa]]

Thunderbird 1.0.1 is a security release that we're already late in delivering to
our users and we aren't taking further non-critical-security fixes into that
release. Perhaps this can make the 1.1 release or a later 1.0.2 release. 

Comment 14

[David :Bienvenu]

Comment on attachment 173496 [details] [diff] [review]
Final version of patch fixing folder drag and drop

William, thx for this patch. We're going to try this on the trunk, once we get
it reviewed. A couple comments:

+    if (summarySpec.Exists() == PR_TRUE)    // Test if filespec exist
+    {					     // ...

this should just be 

if (summarySpec.Exists() && summarySpec.GetFileSize() > 0)
  NS_ENSURE_SUCCESS(rv, rv)

the rest looks OK, except that the change to
nsMsgLocalMailFolder::DownloadMessagesForOffline is superfluous, isn't it?

Comment 15

[William Bonnet]

Hi,

> if (summarySpec.Exists() && summarySpec.GetFileSize() > 0)
>   NS_ENSURE_SUCCESS(rv, rv)

Yes you're right :)

> nsMsgLocalMailFolder::DownloadMessagesForOffline is superfluous, isn't it?

oopss...  once again i had some space/tab replacement with my xemacs :( I'll be
more carefull.

Comment 16

[William Bonnet]

Hi

> Thunderbird 1.0.1 is a security release that we're already late in delivering to

I totally understand that the release is late and that it is difficult to
include new patches at the last minute. 

I was just meaning that for a end user security is also security of the data.
Security is not only virus and spam fighting. On my personnal point of view, the
risk of having a virus is less critical than the risk of loosing 100 megs of
email. Loosing my data is a "switch reason" for me. 

On the windows platform there is anyways a security risk with virus, but at
least outlook or eudora does not "delete" my data. Moreover, there is hardly no
virus risk on Unix platform, but there is the data loss risk... critical isn't it? 

I believe that this is a security matter also.

> we aren't taking further non-critical-security fixes into that release. 

Loosing data is non-critical ? 

yes you are right it is not a critical problem, it is a "drop that software and
switch to outlook" problem

If the reason is "too late for 1.0.1 it is already frozen, but it will be
included next week in 1.0.2" that's ok :) But i feel that 1.1 is too far away.

Anyways it is up to you guys, i am not the project leader. But i am just
surprised that such a important bug is not yet fixed. 

In my company we have been using a patched version for more than a month now. It
is difficult for me to imagine people using the unix version in a professional
context and having having this Damocles sword hanging over their heads, waiting
for the next drag and drop to fall and destroy some folders.

Comment 17

[David :Bienvenu]

Attachment: patch with review comments addressed

this is the same patch, with the review comments addressed. We'd like to try
this on the trunk quickly.

Comment 18

[David :Bienvenu]

Attachment: fix last patch

last patch had an extra }

Comment 19

[David :Bienvenu]

fixed on trunk, thx William

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

chofmann -- what makes this patch 'thunderbird only' (status whiteboard)?  It
touches /mailnews/ without |ifdef|s, so I'm assuming Seamonkey is affected by
this as well.

Comment 21

[Graham Klyne]

Is this truly resolved?  The log here suggests a fix should be in Thunderbird
1.0.2, but I have experienced similar in a new installation of 1.0.6 (except
that I lost a Gb of amail, not a mere 100Mb ;-)  (But I had backup, so no big
deal for me.)

See also comments on https://bugzilla.mozilla.org/show_bug.cgi?id=271877.

#g

Comment 22

[Wayne Mery (:wsmwk)]

*** Bug 271877 has been marked as a duplicate of this bug. ***