440366 - all keywords/tags of a mail will be deleted if I cut off (not delete) attachments

Status: RESOLVED FIXED

(Thunderbird :: General, defect, P1)

Description

[X. Em]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: DE Thunderbird Version 2.0.0.14 (20080421)

all keywords of a mail are no longer present after I cut off (not delete) attachments

Reproducible: Always

Steps to Reproduce:
1. add keyword to mail
2. cut off attachment of this mail

Actual Results:  
added keyword is no longer present

Expected Results:  
added keyword is still present

Comment 1

[Wayne Mery (:wsmwk)]

confirm, happens for message in imap or local
1. tag
2. delete attachment

David do you remember seeing this?  sounds like a dupe but I'm not finding one - it's not bug 370440

Comment 2

[David :Bienvenu]

Wayne, yikes. I have not heard of this bug before. I'll look into it.

confirming based on your confirm comment :-)

Comment 3

[Volker Englisch]

I am seeing the same thing.  When I detach (or remove) an attachment from a message with only one attachment, the tags for that mail message are being removed.  
This is happening for TB on Windows and Linux.

Please note: 
- I've tagged one message with three tags (keyboard input 1, 2, 3).  After 
  detaching the attachment the first two tags were removed, the third one was 
  still available.  
- When I'm using just one tag, that tag is removed.
- When I'm tagging (3, 2, 1) the third tag only is preserved
- When I'm tagging (7, 8, 9) all tags are removed.

Comment 4

[Wayne Mery (:wsmwk)]

didn't test, but perhaps affects other things too - star, ...

Comment 5

[David :Bienvenu]

marking blocking

Comment 6

[David :Bienvenu]

taking - I was able to reproduce this.

Comment 7

[David :Bienvenu]

Attachment: one possible approach

I started down this path, which is to make detaching attachments use CopyFileMessage with an original message header, so the copy action can delete the original message, and propagate the various flags naturally. I got this working for local messages, with some scary changes, and then tested IMAP, which didn't work. I'll spend a little time looking at why IMAP broke, but I suspect a much less risky approach is going to be to make the detach code propagate properties from the old header to the new header. It would be a really nice simplification if I could get the copy service to do the work for me, but changing the core copy service is scary.

Comment 8

[David :Bienvenu]

IMAP CopyFileMessage doesn't delete the original message, even if msgToReplace is set - I could fix that, but I'm going to go for the safe approach for b1.

Comment 9

[David :Bienvenu]

Attachment: proposed fix

It occurred to me that it might be useful for other callers of CopyFileMessage to be able to set keywords (e.g., if an extension wanted to create a message from a file, and set tags on that message), so I bit the bullet and changed all the callers and impls to pass in an optional keyword string. This way, we can set the keyword directly when we append the msg to the imap server, in the imap case.

I had to rearrange some of the notification code so that I could set the keywords on the local messages after we've released the semaphore. This allowed me to get the test I added to pass, but it should help other listeners should there be any.

Comment 10

[David Ascher (:davida)]

messed up assignment.  undoing.

Comment 11

[David :Bienvenu]

Attachment: missed some test changes in my previous patch

Seeing if Standard8 has time for this review - I'd like to get this in for b1 since it is dataloss of a sort.

Comment 12

[[e-mail]]

Comment on attachment 335572 [details] [diff] [review]
missed some test changes in my previous patch

I think there must be something missing from this patch, my build stopped at nsMessenger.cpp with these errors:
nsMessenger.cpp(1637) : error C2660: 'nsIMsgCopyService::CopyFileMessage' : function does not take 8 arguments
nsMessenger.cpp(2570) : error C2660: 'nsIMsgCopyService::CopyFileMessage' : function does not take 8 arguments

>   rv = copyRequest->Init(nsCopyFileMessageType, fileSupport, dstFolder,
>                          isDraft, aMsgFlags, listener, window, PR_FALSE);
>   if (NS_FAILED(rv)) goto done;
>+  copyRequest->m_newMsgKeywords = aNewMsgKeywords;
IMHO it would be cleaner to add a parameter to Init.

>   copyService.CopyFileMessage(bugmail1, gLocalInboxFolder, null, false, 0,
>-                              copyListener, null);
>+                              "", copyListener, null);
There are a number of changes like this, but that extra parameter is an const nsACString&, not a const char*, so this should be EmptyCString(), right? (I've seen VC allow something similar for NS_NewURI when it shouldn't.)

>+      if (aKeywords  && (supportedFlags & kImapMsgSupportUserFlag))
Nit: you have two spaces before &&

sr=me once this compiles ;-)

Comment 13

[David :Bienvenu]

the nsIMsgCopyService.idl changes were missing from the patch somehow...

>   copyService.CopyFileMessage(bugmail1, gLocalInboxFolder, null, false, 0,
>-                              copyListener, null);
>+                              "", copyListener, null);
>There are a number of changes like this, but that extra parameter is an const
>nsACString&, not a const char*, so this should be EmptyCString(), right? (I've
>seen VC allow something similar for NS_NewURI when it shouldn't.)

these are js changes so "" is OK, isn't it? All the c++ calls use EmptyCString().

I'll fix the Init call and attach a patch that should build :-)

Comment 14

[David :Bienvenu]

Attachment: address Neil's comments...

carrying forward Neil's sr, after addressing comments, requesting r from Standard8 for new patch.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 335882 [details] [diff] [review]
address Neil's comments...

I just tried this on an IMAP message that I'd just sent myself and tagged with the default tags (probably keys 1 and 2). When I detached the attachment, the tags weren't kept.

This did work on local folders though.


>diff --git a/mailnews/base/public/nsIMsgCopyService.idl b/mailnews/base/public/nsIMsgCopyService.idl

This file needs a uuid bump.

> NS_IMETHODIMP
> nsMsgLocalMailFolder::OnCopyCompleted(nsISupports *srcSupport, PRBool moveCopySucceeded)
> {
>   if (mCopyState && mCopyState->m_notifyFolderLoaded)
>     NotifyFolderEvent(mFolderLoadedAtom);
> 
>-  delete mCopyState;
>-  mCopyState = nsnull;
>   (void) RefreshSizeOnDisk();
>   // we are the destination folder for a move/copy
>+  PRBool haveSemaphore;
>+  nsresult rv = TestSemaphore(static_cast<nsIMsgLocalMailFolder*>(this), &haveSemaphore);
>+  if(NS_SUCCEEDED(rv) && haveSemaphore)

nit: not you fault, but as you're touching it, please insert a space between the if and the (

>+// Test of setting keywords with CopyFileMessage
>+
>+const copyService = Cc["@mozilla.org/messenger/messagecopyservice;1"]
>+                      .getService(Ci.nsIMsgCopyService);
>+const bugmail11 = do_get_file("../mailnews/test/data/bugmail11");
>+                     
>+// main test
>+
>+var hdrs = [];
>+
>+// tag used with test messages
>+var tag1 = "istag";
>+
>+function run_test()
>+{
>+
>+  loadLocalMailAccount();
>+
>+  // bugmail11 has no keywords.    
>+  copyService.CopyFileMessage(bugmail11, gLocalInboxFolder, null, false, 0, tag1,
>+                              copyListener, null);

This should have a call to do_test_pending() before it...(I'm assuming this call is likely to be async).

>+  return true;
>+}
>+
>+// nsIMsgCopyServiceListener implementation
>+var copyListener = 
>+{
>+  OnStartCopy: function() {},
>+  OnProgress: function(aProgress, aProgressMax) {},
>+  SetMessageKey: function(aKey)
>+  {
>+    hdrs.push(gLocalInboxFolder.GetMessageHeader(aKey));
>+  },
>+  SetMessageId: function(aMessageId) {},
>+  OnStopCopy: function(aStatus)
>+  {
>+    var copiedMessage = gLocalInboxFolder.GetMessageHeader(hdrs[0]);
>+    do_check_eq(copiedMessage.getStringProperty("keywords"), tag1);

Now that you've finished the test, you need a do_test_finished();

Comment 16

[David :Bienvenu]

Attachment: fix addressing Standard8's comments

My guess is that your server doesn't support user-defined keywords.  I can deal with that, but I'd rather do it in a follow on...

Comment 17

[David :Bienvenu]

Mark sent me a protocol log that showed his server does NOT support user-defined keywords, so the behavior he's seeing is expected.

Comment 18

[David :Bienvenu]

fix checked in

Comment 19

[palo misik]

Will be this fix available for Thunderbird 2.* ?

Comment 20

[Magnus Melin [:mkmelin]]

No.