Closed Bug 364082 Opened 16 years ago Closed 13 years ago

mail lost when moving from local to remote folder in offline mode

Categories

(Thunderbird :: Mail Window Front End, defect, P1)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: again, Assigned: Bienvenu)

References

(Blocks 1 open bug, )

Details

(Keywords: dataloss)

Attachments

(4 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20061202 Firefox/2.0
Build Identifier: Thunderbird/1.5.0.8

Hi!

Thunderbird 1.5.0.8 reproducably loses mails that are moved from local folders to remote IMAP folders in offline mode on at least Windows XP and Linux.

Best regards
Christian

Reproducible: Always

Steps to Reproduce:
1. When not in offline mode, create a mail and save it to a remote folder.
2. Drag the mail to a local folder.
3. Switch to offline mode without downloading messages for offline use.
4. Drag the mail back to a remote folder.
5. Turn off offline mode.
Actual Results:  
The mail is lost and it seems to be impossible to recover.

Expected Results:  
As long as in offline mode, the mail should be cached somewhere.  It should be displayed (including message body) when viewing the remote folder.  When switching to online mode, it should be uploaded to the remote folder.
Version: unspecified → 1.5
WFM on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20061215 Thunderbird/2.0b1 ID:2006121504

(though it seems to take a few seconds before it gets "back in view" after going online again)
I can also reproduce the bug with Thunderbird 2.0b1 (Windows/20061206).
I have updated the URL field to point to a screencast exposing the defective behavior.

http://schlotter.org/pub/screencast_364082.gif (768kB)
User-Agent: Thunderbird 1.5.0.8 (X11/20061110)
Nice screencast. (BTW, what did you use to make it?)
Tried it again. Can't reproduce, maybe it's a windows only bug...
Whiteboard: win only?
Version: 1.5 → 2.0
(In reply to comment #4)
> Nice screencast. (BTW, what did you use to make it?)

byzanz (http://people.freedesktop.org/~company/byzanz/)

> Tried it again. Can't reproduce, maybe it's a windows only bug...

I can also reproduce it on Linux with Thunderbird 2.0b1 (X11/20061217), so it is not a windows only bug.

Could it be related to the IMAP server used?  I tested on two different IMAP servers, one of them is Cyrus IMAP server.
Possibly... I tried with two, one is Courier-IMAP, don't remember the other, but I think it's not Cyrus.

Perhaps looking at an imap protocol log could help (though I'm not sure what to look for)
http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap
Whiteboard: win only?
OK, I looked at the log.  One server is Cyrus IMAP4 v2.0.16, the other one I couldn't find out, but I think the bug is not related to the IMAP server.

The following is an excerpt from the logfile, in which I echoed messages in capital letters to identify which action triggers which IMAP behavior:

...
-1220699968[805b548]: 894d0e0:imap.profimailer.de:NA:TellThreadToDie: close socket connection
-1295836256[894dd48]: ImapThreadMainLoop leaving [this=894d0e0]
WENT OFFLINE
DRAG TO REMOTE
WENT ONLINE -- LOST
EXIT

As one can see, Thunderbird does not reconnect to the server after switching to online mode -- so there is no chance for the message to reach the server.  (If I force Thunderbird to connect by clicking on another remote folder, the message does not get uploaded either.)
Keywords: dataloss
can you paste or e-mail me a bit more of the log? The snippet you attached isn't very useful by itself.  Thx!
Assignee: mscott → bienvenu
Status: UNCONFIRMED → NEW
Ever confirmed: true
er, never mind - a log would not be useful. I was able to reproduce the problem, and I'm working on a fix.
Attached patch proposed fixSplinter Review
In the case of move from a local folder to an imap folder, the source msg hdr gets deleted before we try to play back the operation. So, I need to cache the msg size and flags in the offline operation, and retrieve them, when we try to play back the move.

I also need to make sure we look at local folders as well as imap folders when trying to find folders needing playback of offline ops.

This fix is not perfect - we'll still lose tags when moving a local message to an imap server while offline, but at least now we won't lose data.

A more general fix is a bit scary at this point - ideally, I think we need to store message headers for offline moved messages in a separate table in the db, and retrieve those during playback, and clear them out when playback is done...
Attachment #249187 - Flags: superreview?(mscott)
Please find attached the fix from attachment 249187 [details] [diff] [review] against mailnews from Thunderbird 2.0b1 (Thunderbird 2.0b1 (X11/20061220)).  I can confirm that this *sometimes* fixes the bug.

The first time I tried to reproduce this bug, I created a Draft with Recipient "Test" and Subject "Test".  Then I moved it to a local folder, switched to offline mode, dragged the mail to my remote "Drafts" folder and switched online again (some procedure as described in my bug report and as shown in the screencast).  After that, a mail appeared in the drafts folder that had an empty Subject and Recipient, and the creation date was set to Jan 1st 1970.  While trying to reproduce this exact case, the following malformed emails were created also:

To: Test, Subject: Test
http://schlotter.org/pub/notcorrect.png

To: Test, Subject: Test, Message: Test
http://schlotter.org/pub/notcorrect2.png

To: Test, Subject: Test
http://schlotter.org/pub/notcorrect3.png

The following mail is correct:
To: Test, Subject: Test
http://schlotter.org/pub/correct.png

If is wrong to test all this with a patched Thunderbird 2.0b1, please disregard this comment.
I don't think you attached the right file - it's not a diff.

So I'm a little confused - what steps did work with the fix, and what steps didn't? It looks like the cases that didn't work had a problem whereby the From header was escaped (we shouldn't be including the envelope line at all). I didn't see that in my tests, though I was working on the trunk. I can try my patch with 2.0...
(In reply to comment #12)
> I don't think you attached the right file - it's not a diff.

Sorry, David.  Here's the diff...

> So I'm a little confused - what steps did work with the fix, and what steps
> didn't?

All I did was repeat the steps shown in the screencast I posted earlier.  Sometimes all worked as intended, sometimes something went wrong.  It always happened that after switching to online mode again the message was uploaded to the remote folder, but sometimes it was flawed (as seen on the screenshots).

Unfortunately I am not able to reproduce this anymore, at the moment your patch does what it should do as long as Thunderbird is not quit.  However, when I'm in offline mode and drag a mail from a local to a remote folder, then quit Thunderbird and restart it, it still gets lost.  If I'm in offline mode and save a mail directly to a remote folder, the mail survives the restart.  I have made another screencast showing this:
http://schlotter.org/pub/screencast2.gif (2.6MB)
(This is Thunderbird 2.0b1 (X11/20061220) plus your patch.)

In the screencast there are two mails created.  The first in online mode, which is dragged to a local folder.  After that I switch to offline mode and create a second mail, which is automatically saved to the remote Drafts folder.  Then I restart Thunderbird, switch to the Drafts folder and only the mail written in offline mode appears.
Attachment #249228 - Attachment is obsolete: true
I tried that patch with a 2.0 build on a different machine, and it worked, even in the case where I shutdown and restarted in between doing the offline operation and playing it back. I do have TB configured to startup in the last mode it was in, so it starts up offline. I can try switching it to always start up online and see if it makes a difference.

yes, that was the issue - in this case, we only play back offline operations when you click on a folder with an offline operation. And when you click on the imap folder, we get confused and think we need to remove the offline op for the move - (offline move ops get stored in both the source and destination folder, which means we need to remove the op from one of the folders when the op is played back).
Attachment #249187 - Flags: superreview?(mscott) → superreview+
I do still need to fix the case where you start up online...but I think it's separate from this fix.
I've checked in the patch to the trunk and branch. I still need to fix the offline startup case so I'm leaving this open.
Status: NEW → ASSIGNED
I saw that Thunderbird 2.0's RC1 is out now, but this bug is still open.  Will the fix for this whole bug not be in the final version?  I want to emphasize that this bug causes data loss and is also present at least in 1.5.
Flags: wanted-thunderbird3?
Flags: blocking-thunderbird3?
Flags: wanted-thunderbird3? → wanted-thunderbird3+
Whiteboard: [only offline startup case remaining to fix per comment 17]
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3+
Priority: -- → P1
Target Milestone: --- → Thunderbird 3.0b2
IMO should block b2
Moving out to b2 since this doesn't effect pseudo-offline mode, which is what we're hoping to turn on for b1.
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
moving to b3 since this is a lot harder to reproduce now.
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Version: 2.0 → Trunk
This is the fix for the remaining issue of moving a local message to an imap folder, while offline, shutting down, and restarting in online mode.

I detect that we've switched network state from offline to online on startup, and set a pref mailnews.playback_offline to true.

Then, in the same place on startup where we check if we should send unsent messages (as good a place as any), I added a check for that pref, and if it's set, I playback all offline events.
Attachment #370528 - Flags: review?(mkmelin+mozilla)
Comment on attachment 370528 [details] [diff] [review]
proposed fix for remaining issue of copying local message to imap folder

Seems to work fine! r=mkmelin

>+      // check if we shut down offline, and restarted online, in which case
>+      // we may have offline events to playback.
>+      let playbackOfflineEvents = false;
>+      try {
>+        playbackOfflineEvents = gPrefBranch.getBoolPref("mailnews.playback_offline");
>+      }
>+      catch(ex) {}
>+      if (playbackOfflineEvents)

Is the try catch really needed? If not, you don't need the temp variable at all. 
Might as well capitalize the comment sentence while you're at it...

>+      {
>+        gPrefBranch.setBoolPref("mailnews.playback_offline", false);
>+        MailOfflineMgr.offlineManager.goOnline(false, true, msgWindow);
>+      }
>+        

^^^^ a bunch of spaces
Attachment #370528 - Flags: review?(mkmelin+mozilla) → review+
Thanks for the patch!

(From attachment 370528 [details] [diff] [review])
>+//   If the user shutdown offine, and is now starting up in online

Here is a typo              ^^^^^^
the offlineStartup.js part needs sr as well since it's shared with SM. And SM probably wants a similar fix to its msgMail3PaneWindow.js code...
Attachment #371246 - Flags: superreview?(bugzilla)
Attachment #371246 - Flags: review+
Whiteboard: [only offline startup case remaining to fix per comment 17] → [has patch, needs sr standard8]
Blocks: TB2SM
Comment on attachment 371246 [details] [diff] [review]
fix addressing mkmelin's comments

>diff --git a/mail/base/content/msgMail3PaneWindow.js b/mail/base/content/msgMail3PaneWindow.js
>--- a/mail/base/content/msgMail3PaneWindow.js
>+++ b/mail/base/content/msgMail3PaneWindow.js
>@@ -915,23 +915,39 @@ function loadStartFolder(initialUri)
>       dump('Exception in LoadStartFolder caused by no default account.  We know about this\n');
>     }
> 
>     // if gLoadStartFolder is true, then we must have just created a POP3 account
>     // and we aren't supposed to initially download mail. (Bug #270743)
>     if (gLoadStartFolder)
>       MsgGetMessagesForAllServers(defaultServer);
> 
>-    // If appropriate, send unsent messages. This may end up prompting the user,
>-    // so we need to get it out of the flow of the normal load sequence.
>-    function checkUnsent() {
>-      if (MailOfflineMgr.isOnline() && MailOfflineMgr.shouldSendUnsentMessages())
>-        SendUnsentMessages();
>+    if (MailOfflineMgr.isOnline()) {
>+      // Check if we shut down offline, and restarted online, in which case
>+      // we may have offline events to playback. Since this is not a pref
>+      // the user should set, it's not in mailnews.js, so we need a try catch.
>+      let playbackOfflineEvents = false;
>+      try {
>+        playbackOfflineEvents = gPrefBranch.getBoolPref("mailnews.playback_offline");
>+      }
>+      catch(ex) {}
>+      if (playbackOfflineEvents)
>+      {
>+        gPrefBranch.setBoolPref("mailnews.playback_offline", false);
>+        MailOfflineMgr.offlineManager.goOnline(false, true, msgWindow);
>+      }
>+
>+      // If appropriate, send unsent messages. This may end up prompting the user,
>+      // so we need to get it out of the flow of the normal load sequence.
>+      function checkUnsent() {
>+        if (MailOfflineMgr.shouldSendUnsentMessages())
>+          SendUnsentMessages();
>+      }
>+      setTimeout(checkUnsent, 0);
>     }
>-    setTimeout(checkUnsent, 0);
> }
> 
> function AddToSession()
> {
>   var mailSession = Components.classes["@mozilla.org/messenger/services/session;1"]
>                               .getService(Components.interfaces.nsIMsgMailSession);
>   var nsIFolderListener = Components.interfaces.nsIFolderListener;
>   var notifyFlags = nsIFolderListener.intPropertyChanged | nsIFolderListener.event;
>diff --git a/mailnews/extensions/offline-startup/js/offlineStartup.js b/mailnews/extensions/offline-startup/js/offlineStartup.js
>--- a/mailnews/extensions/offline-startup/js/offlineStartup.js
>+++ b/mailnews/extensions/offline-startup/js/offlineStartup.js
>@@ -48,16 +48,18 @@ const kAlwaysOffline = 3;
> ////////////////////////////////////////////////////////////////////////
> //
> //   nsOfflineStartup : nsIObserver
> //
> //   Check if the user has set the pref to be prompted for
> //   online/offline startup mode. If so, prompt the user. Also,
> //   check if the user wants to remember their offline state
> //   the next time they start up.
>+//   If the user shutdown offline, and is now starting up in online
>+//   mode, we will set the boolean pref "mailnews.playback_offline" to true.
> //
> ////////////////////////////////////////////////////////////////////////
> 
> var nsOfflineStartup =
> {
>   onProfileStartup: function()
>   {
>     debug("onProfileStartup");
>@@ -74,33 +76,35 @@ var nsOfflineStartup =
>         return;
>       }
>     }
> 
>     var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>                           .getService(Components.interfaces.nsIPrefBranch);
>     var manageOfflineStatus = prefs.getBoolPref("offline.autoDetect");
>     gOfflineStartupMode = prefs.getIntPref(kOfflineStartupPref);
>+    let wasOffline = !prefs.getBoolPref("network.online");
> 
>     if (gOfflineStartupMode == kAlwaysOffline)
>     {
>       ioService.manageOfflineStatus = false;
>       ioService.offline = true;
>     }
>     else if (gOfflineStartupMode == kAlwaysOnline)
>     {
>       ioService.manageOfflineStatus = manageOfflineStatus;
>+      if (wasOffline)
>+        prefs.setBoolPref("mailnews.playback_offline", true);
>       // If we're managing the offline status, don't force online here... it may
>       // be the network really is offline.
>       if (!manageOfflineStatus)
>         ioService.offline = false;
>     }
>     else if (gOfflineStartupMode == kRememberLastState)
>     {
>-      var wasOffline = !prefs.getBoolPref("network.online");
>       ioService.manageOfflineStatus = manageOfflineStatus && !wasOffline;
>       // If we are meant to be online, and managing the offline status
>       // then don't force it - it may be the network really is offline.
>       if (!manageOfflineStatus || wasOffline)
>         ioService.offline = wasOffline;
>     }
>     else if (gOfflineStartupMode == kAskForOnlineState)
>     {
>@@ -120,16 +124,18 @@ var nsOfflineStartup =
> 
>       var result = promptService.confirmEx(null, title, desc,
>         (promptService.BUTTON_POS_0 * promptService.BUTTON_TITLE_IS_STRING) +
>         (promptService.BUTTON_POS_1 * promptService.BUTTON_TITLE_IS_STRING),
>         button0Text, button1Text, null, null, checkVal);
>       debug ("result = " + result + "\n");
>       ioService.manageOfflineStatus = manageOfflineStatus && result != 1;
>       ioService.offline = result == 1;
>+      if (result != 1 && wasOffline)
>+        prefs.setBoolPref("mailnews.playback_offline", true);
>     }
>   },
> 
>   observe: function(aSubject, aTopic, aData)
>   {
>     debug("observe: " + aTopic);
> 
>     if (aTopic == "profile-change-net-teardown")
Attachment #371246 - Flags: superreview?(bugzilla) → superreview+
Whiteboard: [has patch, needs sr standard8] → [ready to land]
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [ready to land]
You need to log in before you can comment on or make changes to this bug.