370440 - Tags are lost on messages with attachment in IMAP (when the server doesn't supports any keywords)

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Satya]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20061208 Firefox/2.0.0.1
Build Identifier: Thunderbird/2.0b2 2007011615

I have some messages on Scalix IMAP (I don't know if it supports keywords). Tags on messages with attachments in the IMAP folders are lost on the next refresh.

Reproducible: Always

Steps to Reproduce:
1. Tag some messages with attachments on IMAP
2. Wait for refresh or manually refresh

Actual Results:  
Tags were gone

Expected Results:  
Tags should be preserved

Comment 1

[Magnus Melin [:mkmelin]]

Try creating an imap protocol log http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap

Comment 2

[Michael Peters]

My Thunderbird (Version 2.0.0.4 (20070604)) lost the tag(s) in a message when I delete an attachment in this message...

Comment 3

[Daniel Pryden]

I appear to be experiencing the same issue.  

My steps to reproduce are:
1. Create a Saved Search folder that searches an IMAP server for a certain tag
2. Tag a message with an attachment
3 [review]. Go to the Saved Search folder.  The message will appear.
4. Return to the actual IMAP folder.  The message will no longer have the tag associated with it.

This is with Thunderbird 2.0.0.9 (Windows/20071031) and a new profile.  My platform is Windows XP.  The remote IMAP server is at Everyone.net; I'm not sure what software they use.

I'm attaching an IMAP log, but I'm not sure how much help it is (I don't see anything obviously wrong).  Let me know if there's any other information I can provide.

Comment 4

[Daniel Pryden]

Attachment: IMAP log

Comment 5

[Magnus Melin [:mkmelin]]

The server says it doesn't support labels of any kind. (Not even the few predefined.)

Comment 6

[Gervase Markham [:gerv]]

Magnus: so does this mean tags aren't functional on that sort of server? If so, shouldn't we change the UI to prevent people using them?

Gerv

Comment 7

[Magnus Melin [:mkmelin]]

The tags will usually still work - but on the local machine only. (There's special code for that tries to handle this.)

Comment 8

[Wayne Mery (:wsmwk)]

similar to bug 411869 - warn if server doesn't supported feature like marking message with star.  In that case, message gets starred in UI, click away and then back to folder then stars are gone.

Comment 9

[Daniel Pryden]

OK, so it looks like it's the fault of the server for not keeping tags.  But then why can I set tags on messages that don't have an attachment, and they stick just fine?

Presumably if the server can't store the tags, then TB is doing some magic to keep them locally.  But then why doesn't the magic work if there's an attachment?

Even if the tags have to be local, which is sub-optimal, I can live with it as long as they actually persist.

Comment 10

[Wayne Mery (:wsmwk)]

(In reply to comment #9)
> OK, so it looks like it's the fault of the server for not keeping tags.  But
> then why can I set tags on messages that don't have an attachment, and they
> stick just fine?

perhaps david knows

Comment 11

[David :Bienvenu]

I can't think why messages w/ attachments should be treated any differently. Can you try a test and see if this happens on a non-built in tag, i.e., a 6th or 7th tag? We could be hitting code that clears the built in tags (though I kinda doubt it if the server doesn't support keywords at all)

Comment 12

[Daniel Pryden]

Thanks for your attention, David.  Unfortunately, I just tested it and it makes no difference if it's a built-in tag or one that I added.

Is there anything else I can do to help troubleshoot this?

Comment 13

[David :Bienvenu]

the offline playback is generating a bogus url to set the keyword on the server. Working on a patch.

Comment 14

[Wayne Mery (:wsmwk)]

requesting blocking based on dataloss and prospects for patch

Comment 15

[Daniel Pryden]

By the way, if it helps for purposes of getting the blocking flag set, I can confirm that this bug occurs on other platforms than x86 Linux -- I've consistently confirmed it with Windows XP, for example.  I'm pretty sure the platform field should be "All".

Comment 16

[Magnus Melin [:mkmelin]]

Daniel: can you test if it's still a problem with a build from here?
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-central/

Comment 17

[Daniel Pryden]

Yes, it's still an issue as of the build "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090129 Shredder/3.0b2pre".  Steps to reproduce are the same as in Comment #3.

Comment 18

[David :Bienvenu]

So, the server claims not to support any non-default flags, but has a special keyword set when a message has an attachment (X-EON-HAS-ATTACHMENT). We're taking that keyword and crunching the local keywords the user has set. I'm not quite sure how we should handle this - we could just ignore keywords on the server if the server say there aren't any non-default permanent flags, though I can imagine that it might be useful for extensions to get hold of those server-set keywords. Or we could avoid crunching the local keywords if the server says it doesn't store user-defined keywords. But then if the keyword gets removed on the server, we won't have any way of storing that. We could have two properties for headers for this case, one for user set keywords and one for server set keywords, but that makes our implementation more complicated.

Comment 19

[Kent James (:rkent)]

(In reply to comment #18)
>
> we could avoid crunching the local keywords if the
> server says it doesn't store user-defined keywords.

I don't understand what is wrong with that solution. Why would an extension want to get the pseudo-keyword added by the server? I guess I don't really understand why the server is adding this bogus flag. (I assume by "has a special keyword set" what you mean is that the server adds another named flag).

If an extension wants the value of the flags from the server, can't it just ask directly?

Comment 20

[David :Bienvenu]

(In reply to comment #19)

> I don't understand what is wrong with that solution. Why would an extension
> want to get the pseudo-keyword added by the server? I guess I don't really
> understand why the server is adding this bogus flag. (I assume by "has a
> special keyword set" what you mean is that the server adds another named flag).

Yes, we can't know why a server would add a flag, so we can't know why an extension might want to know about the flag. In enterprises, I've come across server-side processes that add their own keywords to messages, and wanted to have extensions be able to access those keywords.

> 
> If an extension wants the value of the flags from the server, can't it just ask
> directly?

Not synchronously or easily.

All that being said, I'm going to go with a variant of this solution. I'd much rather encourage servers to turn on support for user-set keywords.

Comment 21

[Daniel Pryden]

I concur with the sentiment of comment #19.  It seems the case where an extension wants to get at the keywords is more of an enhancement (I'm not aware of any extensions that look for this kind of keyword) whereas the case where we crunch the local keywords is a dataloss bug.

Are we really worried that "special" (i.e. non-standard) keywords that were set by the server might change over time?  I doubt that the server is going to remove the X-EON-HAS-ATTACHMENT keyword unexpectedly.  I would think the same is probably true of the other enterprise-specific keywords you mention (although admittedly this is a wild guess).

If I understand the last sentence of comment #18 correctly, that alternative makes sense (storing user-set and server-set keywords separately).  Then when the server says it doesn't support user-set keywords, we don't allow it to make changes to our local user-set keywords, but it can still make changes to server-set keywords.  This seems like the most ideal solution, although it is more complicated for very little practical gain.

Comment 22

[David :Bienvenu]

Attachment: proposed fix

This makes HandleCustomFlag take a boolean which says whether the server supports custom keywords. If it doesn't, we still parse for things like $Junk, NonJunk, etc, but we don't crunch the msg hdr keywords property.

I cleaned up nsImapFlagAndUIDState to get rid of an unused constructor, and unused initial flags parameter.

Comment 23

[[e-mail]]

Comment on attachment 365304 [details] [diff] [review]
proposed fix

>+  /**
>+   * Set of flags the server supports storing per message. See nsImapCore.h
>+   * for the set of flags. Note that the setter OR's the  passed in flags
>+   * with the previous flags because we want to accumulate the FLAGS
>+   * and PERMANENTFLAGS response.
>+   */
>+  attribute unsigned short supportedUserFlags;
IMHO This is unacceptable behaviour for an attribute. Please
a) either make the attribute readonly or change it to getSupportedUserFlags()
and
b) restore setSupportedUserFlags and optionally name it orSupportedUserFlags.

>+                                 userFlags & kImapMsgSupportUserFlag,
PRBool should be either PR_TRUE or PR_FALSE. Please
a) pass in all the flags, and test in HandleCustomFlags
or
b) just change the type of the argument to PRUint16
or
c) use !!( & ) to force the result to a boolean.

Comment 24

[David :Bienvenu]

Comment on attachment 365304 [details] [diff] [review]
proposed fix

yeah, you're right - this wouldn't be an interface at all if it weren't for xpcom proxying, but since it is one, we should play by the rules.

Comment 25

[David :Bienvenu]

Attachment: fix addressing Neil's comments

Comment 27

[David :Bienvenu]

fix checked in.