Closed Bug 1140884 Opened 5 years ago Closed 5 years ago

An error occurred while sending mail garbled

Categories

(MailNews Core :: Composition, defect, major)

defect
Not set
major

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, seamonkey2.35 fixed, seamonkey2.36 fixed)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
seamonkey2.35 --- fixed
seamonkey2.36 --- fixed

People

(Reporter: mkmelin, Assigned: sshagarwal)

References

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #952493 +++

Seems there's something wrong with the error messages. 
I just tried sending from mr@exaple.invalid - the server obviously complained.

The result is just garbled, see the screenshot.

Trying tb31 shows the normal "An error occurred while sending mail. The mail server responded:  5.1.8 <mr@example.invalid>: Sender address rejected: Domain not found. Please check the message recipient magnus@netti.fi and try again"
(And that the message is totally misleading is another bug - it's of course the sender that is incorrect, not the recipient.)
Summary: relaying failed (and other error messages?) garbled → An error occurred while sending mail (and other error messages?) garbled
Looks like the message strings have %s when they should have %S?
Yes, they do. But that was the case even before bug 952493.
Some of them are processed with nsTextFormatter::*smprintf() which probably requires %s.

The screenshot seems to contain the string:
errorSendingRcptCommand=An error occurred while sending mail. The mail server responded:  \n%1$S.\n Please check the message recipient "%2$S" and try again.

This one was changed from lower case $s to uppercase. I wonder if we shouldn't use the same formatting function everywhere. If only sending substitution arguments to FormatStringFromName() wasn't so awkward.
Attached image Screen Shot 2015-03-15 at 1.25.17.png (obsolete) —
But this is what I see. Did I not follow the proper STRs?
Attached patch Patch v1 (obsolete) — Splinter Review
Okay, so the issue is because of the toolkit handling of S and s:
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/glue/nsTextFormatter.cpp#1119

So, instead of going through fixing it that way, I've reverted the string case
to the way it was before 952493 and now the message shows up properly for me.
Please let me know, if we can take this in given the time constraints; or we are supposed to go the way of fixing the calls for getting the error message string.

Thanks.
Attachment #8582614 - Flags: review?(mkmelin+mozilla)
Attachment #8582614 - Flags: feedback?(acelists)
Would fixing it the right way (without this textFormatter) go without changing other strings? Aren't there some other strings that would then need change %s -> %S?
(In reply to :aceman from comment #6)
> Would fixing it the right way (without this textFormatter) go without
> changing other strings?
Ya but that would require changing and handling va_args as well. And other
function calls that are currently part of vsmprintf. If we want to go that way,
maybe, we can get this in so that the release is proper and fix that in a follow up? :)
Or, if we want it to be done here itself, just let me know.
 
> Aren't there some other strings that would then need
> change %s -> %S?
I couldn't find any other case discrepancies in bug 952493's patch. There were a few in
the comments but the strings seemed same to me.

Thanks.
Sure, we need to fix this quickly for TB 38. Reading random memory due to the bogus placeholders may have nasty side effects.
Attached patch Patch v2 (obsolete) — Splinter Review
Since patch v1 requires string changes, its hard to get it in aurora at this time.
So, here's an attempt for a workaround.
I used NS_ConvertUTF8toUTF16() and not ASCII converters as
NS_ConvertASCIItoUTF16 and ToNewUnicode because the page says they are risky:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings

Though, if you want I can use those calls.
The patch works for me and I see the proper error message (no garbled text).

Thanks.
Attachment #8583322 - Flags: review?(mkmelin+mozilla)
Attachment #8583322 - Flags: feedback?(acelists)
Attached patch Patch variant 2 (obsolete) — Splinter Review
Please ignore Patch v2, its the same as v1.
Attachment #8583322 - Attachment is obsolete: true
Attachment #8583322 - Flags: review?(mkmelin+mozilla)
Attachment #8583322 - Flags: feedback?(acelists)
Attachment #8583326 - Flags: review?(mkmelin+mozilla)
Attachment #8583326 - Flags: feedback?(acelists)
Comment on attachment 8583326 [details] [diff] [review]
Patch variant 2

Review of attachment 8583326 [details] [diff] [review]:
-----------------------------------------------------------------

If those double get()s are needed :)
I haven't tested it but this solution seems good enough for TB38 to avoid the string change.
Attachment #8583326 - Flags: feedback?(acelists) → feedback+
(In reply to :aceman from comment #11)
> Comment on attachment 8583326 [details] [diff] [review]
> Patch variant 2
> 
> Review of attachment 8583326 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If those double get()s are needed :)
> I haven't tested it but this solution seems good enough for TB38 to avoid
> the string change.

ToNewUnicode() removes both the get() calls as it takes nsCString and returns
const char16_t* 
So, given the risk, if we intend to use this, let me know.

Thanks.
Comment on attachment 8582614 [details] [diff] [review]
Patch v1

Since this patch requires string changes which are hard to get in because of the time constraints, discarding this patch. If someone feels otherwise, feel free to consider it.

Thanks.
Attachment #8582614 - Attachment is obsolete: true
Attachment #8582614 - Flags: review?(mkmelin+mozilla)
Attachment #8582614 - Flags: feedback?(acelists)
(In reply to Suyash Agarwal (:sshagarwal) from comment #12)
> ToNewUnicode() removes both the get() calls as it takes nsCString and returns
> const char16_t* 

So will there be a new patch with ToNewUnicode()?
(In reply to Suyash Agarwal (:sshagarwal) from comment #12)
> ToNewUnicode() removes both the get() calls as it takes nsCString and returns
> const char16_t* 

So will there be a new patch with ToNewUnicode()?
Status: NEW → ASSIGNED
(In reply to :aceman from comment #15)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #12)
> > ToNewUnicode() removes both the get() calls as it takes nsCString and returns
> > const char16_t* 
> 
> So will there be a new patch with ToNewUnicode()?

No, actually Neil said on IRC that we normally want to avoid ToNewUnicode unless you've got a wchar_t** outparam.
(In reply to aceman from comment #11)
> If those double get()s are needed :)
The inner one isn't; NS_Convert* functions accept XPCOM string parameters.
Assignee: nobody → syshagarwal
Summary: An error occurred while sending mail (and other error messages?) garbled → An error occurred while sending mail garbled
Comment on attachment 8583326 [details] [diff] [review]
Patch variant 2

Review of attachment 8583326 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. r=mkmelin with the unnecessary inner get()s removed
Attachment #8583326 - Flags: review?(mkmelin+mozilla) → review+
Thanks.
Attachment #8577654 - Attachment is obsolete: true
Attachment #8583326 - Attachment is obsolete: true
Attachment #8585203 - Flags: review+
Keywords: checkin-needed
Please check if this needs to be uplifted.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(syshagarwal)
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
(In reply to aleth [:aleth] from comment #21)
> Please check if this needs to be uplifted.

Ya, this needs to be uplifted. Its a visible issue and a regression.

Thanks.
Flags: needinfo?(syshagarwal)
Comment on attachment 8585203 [details] [diff] [review]
Patch for check-in

[Approval Request Comment]

We'll uplift after nightly cycles
Attachment #8585203 - Flags: approval-comm-beta?
Duplicate of this bug: 1140308
You need to log in before you can comment on or make changes to this bug.