1636318 - MOZ_ASSERT() triggered in nsTextFormatter::dosprintf when showing error to send message

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Kai Engert (:KaiE:)]

With Thunderbird, we crash at this assertion in TextFormatter.cpp:

      case 'S':
        MOZ_ASSERT(thisArg->mKind == STRING16);
        // Type-based printing below.
        break;

The TB code uses two one-byte strings as parameters.

Stack:

#6  0x0000768ad5091e53 in nsTextFormatter::dosprintf(nsTextFormatter::SprintfStateStr*, char16_t const*, mozilla::Span<nsTextFormatter::BoxedValue, 18446744073709551615ul>)
    (aState=<optimized out>, aFmt=0x768abc30d492 u".\nPlease check the message recipient \"%2$S\" and try again.", aValues=...) at /home/user/moz/commcent/mozilla/xpcom/string/nsTextFormatter.cpp:707
#7  0x0000768ad50922b5 in nsTextFormatter::vssprintf(nsTSubstring<char16_t>&, char16_t const*, mozilla::Span<nsTextFormatter::BoxedValue, 18446744073709551615ul>) (aOut=..., aFmt=0x768adeeb68b0 <_IO_stdfile_2_lock> u"", aValues=...)
    at /home/user/moz/commcent/mozilla/xpcom/string/nsTextFormatter.cpp:852
#8  0x0000768ad4e3bc35 in nsExplainErrorDetails(nsISmtpUrl*, nsresult, char const*, char const*)
    (aSmtpUrl=<optimized out>, aCode=-2141900513, arg1=0x768abd19a488 "550 5.1.1 <[e-mail]>: Recipient address rejected: User unknown in local recipient table", arg2=0x768ab592a108 "[e-mail]") at /home/user/moz/commcent/obj-thunder-opt/dist/include/nsTextFormatter.h:118
#9  0x0000768ad4e42e4c in nsSmtpProtocol::SendRecipientResponse() (this=0x768a962c7800) at /home/user/moz/commcent/mozilla/comm/mailnews/compose/src/nsSmtpProtocol.cpp:1777

Could it be related to the recent changes to "Span", bug 1634023, bug 1634014 ?

Comment 1

[Kai Engert (:KaiE:)]

To reproduce, compose an email to a nonexisting recipient, at the domain managed by your own email provider. Changes are high that the SMTP server will reject the email address, and you should run into the same error path listed in the stack above.

Comment 2

[Kai Engert (:KaiE:)]

Attachment: 1636318-v1.patch

Apparently TextFormatter expects 2-byte strings as parameters.

With this patch I no longer crash and get the expected error prompt.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9146644 [details] [diff] [review]
1636318-v1.patch

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

Looks correct. Not sure I get why this passed the complier checks...

Comment 4

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #3)

Looks correct. Not sure I get why this passed the complier checks...

In C the "printf" class of functions use a variable parameter list, which aren't use the usual strict typing, but which are interpreted according to the format string.

Comment 5

[Kai Engert (:KaiE:)]

Commit message:

Bug 1636318 - Use 16-bit string type for strings passed to nsTextFormatter::ssprintf. r=mkmelin

Comment 6

[Kai Engert (:KaiE:)]

Attachment: 1636318-v2.patch

For your convenience, a patch that includes the commit message.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e5d6fe83becb
Use 16-bit string type for strings passed to nsTextFormatter::ssprintf. r=mkmelin

Comment 8

[Magnus Melin [:mkmelin]]

Looks like this is causing test failures across the board:

TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_sendMailAddressIDN.js | xpcshell return code: 1
Assertion failure: thisArg->mKind == STRING, at /builds/worker/checkouts/gecko/xpcom/string/nsTextFormatter.cpp:712

Comment 9

[Kai Engert (:KaiE:)]

So apparently the code sometimes expects a 8-bit char, and at other times a 16-bit char.
I'm trying to have a deeper look.

Comment 10

[Kai Engert (:KaiE:)]

This file maps error codes to error messages:
https://searchfox.org/comm-central/source/mailnews/compose/src/nsComposeStrings.cpp

The messages are listed here:
https://searchfox.org/comm-central/source/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties

The problem is that some messages use a lower case %s which means 8-bit string, and some messages use an upper case %S which means 16-bit string.

I don't know if that difference makes sense, or if upper/lowercase were chosen without paying attention.

Function nsExplainErrorDetails doesn't try to distinguish or detect, in the past it simply passed an 8-bit string for all of them (and today we changed it to 16-bit string for all of them). So we're crashing whenever there's a mismatch.

We have the following choices:

• change all strings in the properties file to use a lowercase %s, and change the string identifiers as necessary. Risk: Will localizers notice the difference?
• make function nsExplainErrorDetails smarter. Detect if the string contains lower or uppercase parameters, and set arg1 and arg2 accordingly. This would be failsafe against localizers being imprecise.

Comment 11

[Kai Engert (:KaiE:)]

If error code NS_ERROR_COMMUNICATIONS_ERROR can happen, then function nsExplainErrorDetails will pass a string to the formatter, which will then attempt to interpret it as a decimal. Fun.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Bug 1636318 - Failsafe nsExplainErrorDetails implementation. r=mkmelin

Comment 13

[Kai Engert (:KaiE:)]

phab has a bigger patch that attempts to make this code failsafe. I tests for the actual contents of the format string. If inconsistencies are detected, it falls back to the simple numeric error code reporting. If it seems consistent, we use either 8-bit or 16-bit strings, either one or two strings.

I couldn't use the (a?b:c) syntax for the call to ssprintf that used either 8 or 16 bit, the compiler rejected it, that's why the code is elaborate.

try build
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0b04acae4da9cc6ea5e856fce469cc8ac602993a

Comment 14

[Kai Engert (:KaiE:)]

updated the patch, so here's a newer try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=64d701f15b34bca76dbc3ad43f2805e4a9f9d4a6

Comment 15

[Magnus Melin [:mkmelin]]

Backed out for now while we get it sorted out
https://hg.mozilla.org/comm-central/rev/316d93a7923d53d05bc08b918b04649665a73a1b

Comment 16

[Wayne Mery (:wsmwk)]

Will this be wanted on beta?

Comment 17

[Kai Engert (:KaiE:)]

Beta doesn't crash for me. It might still be good to uplift the fix for testing the new approach (if r+).

Comment 18

[Jorg K (CEST = GMT+2)]

Well, those MOZ_ASSERT()s have been there since 2017:
https://hg.mozilla.org/integration/autoland/rev/5a294c3eff7c#l1.1021
I guess no one tested those errors in a debug build since then. It's not an issue in a non-debug build.

Comment #10 is an interesting read. Funny mix of %s and %S in composeMsgs.properties with %s being in the minority. How about making them all %S, and while we're there, turning %d into %S in smtpPermSizeExceeded1, also see comment at nsSmtpProtocol.cpp line 121.

Comment 19

[Kai Engert (:KaiE:)]

(In reply to Jorg K (CEST = GMT+2) from comment #18)

I guess no one tested those errors in a debug build since then. It's not an issue in a non-debug build.

That could explain it.

Comment #10 is an interesting read. Funny mix of %s and %S in composeMsgs.properties with %s being in the minority. How about making them all %S, and while we're there, turning %d into %S in smtpPermSizeExceeded1, also see comment at nsSmtpProtocol.cpp line 121.

And change all affected string identifiers?

The latest patch I've attached doesn't require a change to the strings. It makes the code smarter. It allows both upper %S or lowercase %s and also a single %d, and will pass the appropriate type.

Comment 20

[Jorg K (CEST = GMT+2)]

And change all affected string identifiers?

I guess so, there are 11 :-(

Yes, I saw your patch, it's quite long and twisted and IMHO only serves for "covering up" bad choices that were made in the past. Even if localisers make a mistake, the system will still work, only debug builds may run into the MOZ_ASSERT().

Comment 21

[Kai Engert (:KaiE:)]

Magnus, do you prefer the failsave code (phab patch) or do you prefer making all strings consistent wrt %s/%S ?

I think if we want to be really clean, we might have to check all the callers of this function, and look at the parameters being passed in. Maybe it currently works, because the strings passed always match the type specified in the properties?

I personally prefer a failsafe approach, because future mistakes are easy to be made.

Comment 22

[Jorg K (CEST = GMT+2)]

Well, your extra code in nsSmtpProtocol.cpp won't prevent mistakes elsewhere.

I agree, a review of all call sites is necessary, there aren't too many. However, my first spot check already found an error:
https://searchfox.org/comm-central/rev/99f79dafbc912022853d27c0d78c39afc2e5afc5/mailnews/compose/src/nsMsgSend.cpp#4008
That call is correct since mSavedToFolderName appear so to be UTF-16, but two lines down, "?" is passed with doesn't match the %S in copyMessageStart (should be u"?").

Found another one:
pop3ServerDoesNotSupportTopCommand has %S but gets a 8bit string here:
https://searchfox.org/comm-central/rev/99f79dafbc912022853d27c0d78c39afc2e5afc5/mailnews/local/src/nsPop3Protocol.cpp#3300

Comment 23

[Kai Engert (:KaiE:)]

Thanks for making the call. I'd like to ask for help with the review / the fix, so reverting assignee.

Comment 24

[Jorg K (CEST = GMT+2)]

I can do it if we agree to go with %S throughout. BTW, there's more scope for clean-up. The special treatment of NS_ERROR_ILLEGAL_LOCALPART here:
https://searchfox.org/comm-central/rev/99f79dafbc912022853d27c0d78c39afc2e5afc5/mailnews/compose/src/nsSmtpProtocol.cpp#102
introduced here:
https://searchfox.org/comm-central/diff/915316c9e62dd9cc12afe519522fdcd770dd82c7/mailnews/compose/src/nsSmtpProtocol.cpp#94
doesn't appear to be necessary since NS_ERROR_ILLEGAL_LOCALPART is included in errorStringNameForErrorCode() here:
https://searchfox.org/comm-central/rev/99f79dafbc912022853d27c0d78c39afc2e5afc5/mailnews/compose/src/nsComposeStrings.cpp#96

EDIT: There's a hacky fix for changing all the strings: We can read the string, then replace %d and %s with %S. We could also hit the L10N repository.

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1636318-dosprintf.patch

I'd do it like this. If we don't want the hack, we need to change 12 strings, one with %d and 11 with %s. Magnus?

I'll send a separate patch for the survey of incorrect call sites, see comment #22. Then I'll do a try run.

BTW, comm/mailnews/compose/test/unit/test_sendMailAddressIDN.js passes in a debug build.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: 1636318-fix-other-incorrect-use.patch

I haven't found anything else apart from the ones mentioned in comment #22, but I looked at all call sites of nsTextFormatter::ssprintf(). I'll start a try run in a minute.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1636318-fix-other-incorrect-use.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7faed77878b9f093e8e944c4b076e7f2ae1d3321
(new patch, needed to fix HG header)

EDIT: Looks green apart from the "usual" failures.

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 9147268 [details] [diff] [review]
1636318-dosprintf.patch

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

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +116,5 @@
>        bundle->GetStringFromName(exitString, eMsg);
> +      // Some error strings contain %d, others %s. We're supplying 16-bit
> +      // strings, so replace those with %S. It's a bit hacky. but it saves
> +      // us tweaking some pretty old message strings.
> +      eMsg.ReplaceSubstring(NS_LITERAL_STRING("%d"), NS_LITERAL_STRING("%S"));

maybe we should fix the one case (smtpPermSizeExceeded1) and drop this %d conversion

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 9147396 [details] [diff] [review]
1636318-fix-other-incorrect-use.patch

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

Thx! r=mkmelin

Comment 30

[Jorg K (CEST = GMT+2)]

maybe we should fix the one case (smtpPermSizeExceeded1) and drop this %d conversion

Do I understand that correctly? Leave the hack for 11 strings which have %s (which BTW is rare, most error string also in other files (which I visited during my survey) use %S), and change the %d to %S causing a retranslation due to the change of smtpPermSizeExceeded1 to smtpPermSizeExceeded2. Seems like a fair middle ground.

Comment 31

[Magnus Melin [:mkmelin]]

Yeah that was what I'm suggesting.

Comment 32

[Jorg K (CEST = GMT+2)]

Attachment: 1636318-dosprintf.patch

After some consideration I decided to go the other way and leave the special treatment of smtpPermSizeExceeded1. BTW, arg2 is null in that case, but the compiler insists of a type. That's minimal change, no string changes and pretty fail-safe. I hope you agree.

Landing order doesn't matter, but landing 1636318-dosprintf.patch first feels nicer ;-)

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d58c5ca03202
Use 16-bit string type for strings passed to nsTextFormatter::ssprintf. r=mkmelin
https://hg.mozilla.org/comm-central/rev/03bb86addb5d
fix incorrect uses of nsTextFormatter::ssprintf(). r=mkmelin