User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; .NET CLR 1.1.4322) Build Identifier: version 2.0.0.6 (20070728) In "Tools | Options | Composition | Send Options | Plain Text Domains", I entered lists.berkeley.edu, which is only one server in our domain - the listserv machine. Then I try to send an html formatted email to myself, donb@berkeley.edu. It is received as text only. When I remove the lists.berkeley.edu spec, and send it again, it is received with proper formatting. I uninstalled all add-ons except the default Talkback, and the problem persisted. Reproducible: Always Steps to Reproduce: 1.In "Tools | Options | Composition | Send Options | Plain Text Domains", add xxxx.YourMailServerDomain 2.Send yourself some red or blue text. 3.Receive it looking black. Actual Results: Email is received without html formatting. Expected Results: Email should be received with html formatting. My listserv server does not handle html coded email.

Confirmed in 2.0.0.16 on a Mac. More specifically, it appears to be accepting a partial match instead of an exact match. If either "test.example.com" or "example.com.test" are set as plain text domains and you write to someone@example.com, it will get sent in plain text. Actually, it's a little worse than that -- a message to someone@ample.com will also get sent in plain text, which is really confusing and unexpected. It should require an exact match (possibly with wildcards). If not, the screen should get updated to reflect the fact that it isn't, because there's nothing there right now to indicate that partial matches are happening.

this has probably existed forever

(A) There are mailnews.plaintext_domains and mailnews.html_domains. > http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#4887 > 4887 NS_GetLocalizedUnicharPreferenceWithDefault(prefBranch, "mailnews.plaintext_domains", EmptyString(), > 4888 plaintextDomains); > 4889 NS_GetLocalizedUnicharPreferenceWithDefault(prefBranch, "mailnews.html_domains", EmptyString(), > 4890 htmlDomains); (B) mailnews.plaintext_domains and mailnews.html_domains looks used when recipient's text or html preference is not defined in address book(Unknown). > 4908 // if we don't have a prefer format for a recipient, check the domain in > 4909 // case we have a format defined for it (C) If domainpart of mail address(string after @) is substring of mailnews.plaintext_domains or mailnews.html_domains, mail&news looks to consider that the domainpart is registered as plaintext_domain or html_domain. > 4913 PRInt32 atPos = recipient.mEmail.FindChar('@'); > 4914 if (atPos >= 0) > 4915 { > 4916 domain = Substring(recipient.mEmail, atPos + 1); > 4917 if (CaseInsensitiveFindInReadable(domain, plaintextDomains)) > 4918 recipient.mPreferFormat = nsIAbPreferMailFormat::plaintext; > 4919 else > 4920 if (CaseInsensitiveFindInReadable(domain, htmlDomains)) > 4921 recipient.mPreferFormat = nsIAbPreferMailFormat::html; > 4922 } Above code doesn't look to care for case of "@xxx.yyy.zzz is used and xxx.yyy.zzz is placed in mailnews.<plaintext or html>_domains, and @yyy.zzz is used too." It was very rare case, I think. Ben Bucksch, is my guess right? If so, bug 647522 is perhaps "mail address in address book with text/html preference=HTML" case.

(In reply to WADA from comment #8) > (C) If domainpart of mail address(string after @) is substring of > mailnews.plaintext_domains or mailnews.html_domains, mail&news looks to > consider that the domainpart is registered as plaintext_domain or > html_domain. > > 4913 PRInt32 atPos = recipient.mEmail.FindChar('@'); > > 4914 if (atPos >= 0) > > 4915 { > > 4916 domain = Substring(recipient.mEmail, atPos + 1); > > 4917 if (CaseInsensitiveFindInReadable(domain, plaintextDomains)) > > 4918 recipient.mPreferFormat = nsIAbPreferMailFormat::plaintext; > > 4919 else > > 4920 if (CaseInsensitiveFindInReadable(domain, htmlDomains)) > > 4921 recipient.mPreferFormat = nsIAbPreferMailFormat::html; > > 4922 } > > Ben Bucksch, is my guess right? Indeed, that's precisely the bug, nail on head. This comparison is just wrong. We need to split the string at ",", then compare the right-most part, and only get a hit, if the email address domain string is equal or longer than the pref domain string, not the other way around as now. Ben

(and before string comparison, add a dot before both domains, to avoid matching "@nopaypal.com" when "paypal.com" is configured.)

This fixes it so that the domain must match exactly, or at least end with dot+the domain. So if the recipient is foo@cc.example.com it would match a domain of cc.example.com or example.com, but not ample.com

Comment on attachment 823030 [details] [diff] [review] proposed fix Review of attachment 823030 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for picking this up. ::: mailnews/compose/src/nsMsgCompose.cpp @@ +4912,5 @@ > + > + nsDependentSubstring emailDomain = Substring(recipient.mEmail, atPos + 1); > + nsAutoString domain; > + > + // Check plain text domains. Refactor out an 'isInDomainList' function so that we don't need to duplicate this code for each of the plaintext and html lists. @@ +4922,5 @@ > + if (right == kNotFound) > + right = plaintextDomains.Length(); > + domain = Substring(plaintextDomains, left, right); > + nsAutoString dotDomain(domain); > + dotDomain.AppendLiteral("."); We want the dot on the *beginning* of the domain string, don't we?

Thx for the review! Updated, and added a test.

Comment on attachment 8358839 [details] [diff] [review] proposed fix, v2 Review of attachment 8358839 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the tests. Almost there ;-> ::: mailnews/compose/src/nsMsgCompose.cpp @@ +3028,5 @@ > + * Subdomains are also considered to match. > + * @param aDomain - the domain name to check > + * @param aDomainList - a comman separated string of domain names > + */ > +bool IsInDomainList(const nsString &aDomain, const nsString &aDomainList) The parameters should be nsAString&, see https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide#As_function_parameters @@ +3042,5 @@ > + right = aDomainList.FindChar(',', left); > + if (right == kNotFound) > + right = aDomainList.Length(); > + nsAutoString domain; > + domain = Substring(aDomainList, left, right); nsDependentSubstring domain = Substring(aDomainList, left, right); should avoid allocating heap memory and copying the substring. @@ +3044,5 @@ > + right = aDomainList.Length(); > + nsAutoString domain; > + domain = Substring(aDomainList, left, right); > + nsAutoString dotDomain = NS_LITERAL_STRING("."); > + dotDomain.Append(domain); nsString dotDomain = NS_LITERAL_STRING(".") + domain; should similarly avoid copying, see https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide#String_Concatenation @@ +3045,5 @@ > + nsAutoString domain; > + domain = Substring(aDomainList, left, right); > + nsAutoString dotDomain = NS_LITERAL_STRING("."); > + dotDomain.Append(domain); > + if (aDomain.Equals(domain, nsCaseInsensitiveStringComparator()) || Do this test (and return true) before you construct dotDomain, to avoid the extra work (micro-optimizations 'R' us ;-) e.g. if (aDomain.Equals(...)) return true; nsString dotDomain = .... if (stringEndsWith(aDomain, dotDomain...)) return true; @@ +4889,5 @@ > + if (atPos < 0) > + continue; > + > + nsAutoString emailDomain; > + emailDomain = Substring(recipient.mEmail, atPos + 1); Use 'nsDependentSubstring emailDomain = Substring(...)' here; that lets the Substring object pass through without copying the data.

Adressing review comments, except for the one about plussing the strings. That's not allowed for the external string api.

Comment on attachment 8365709 [details] [diff] [review] proposed fix, v3 Review of attachment 8365709 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Unfortunately I just thought of a case where both the old and new code will give the wrong answer, but I think it's an obscure enough issue that I'd be OK with filing a new bug and fixing it later: If the user configures domain.example => plaintext, sub.domain.example => HTML, and the user sends mail to user@sub.domain.example, we'll match ".domain.example" in the plaintext check and not bother doing the HTML check, so we'll send user@sub.domain.example the wrong format. ::: mailnews/compose/src/nsMsgCompose.cpp @@ +5168,5 @@ > recipientsStr.Append(recipient.mAddress); > } > > + // Add recipients to the nonHtmlRecipient list if they haven't > + // explicitely allowed it. nittiest of comment nits: s/explicitely/explicitly/

https://hg.mozilla.org/comm-central/rev/268ad6c0803b -> FIXED

Backed out for bustage on all platforms. https://hg.mozilla.org/comm-central/rev/55b7979abab7 https://tbpl.mozilla.org/php/getParsedLog.php?id=33809111&tree=Thunderbird-Trunk

External api trap :/ Apparently this has to be + nsAutoString dotDomain; + dotDomain.Assign(NS_LITERAL_STRING(".")); + dotDomain.Append(domain); Relanded as https://hg.mozilla.org/comm-central/rev/9668ba473b98

.AssignLiteral ?

That should work too. The think I didn't expect was that nsAutoString dotDomain = NS_LITERAL_STRING("."); works find under external api but doesn't compile w/ internal.