Debug version of TB crashes when SMTP server responded with unknown user error.: mozilla/xpcom/string/nsTextFormatter.cpp:712 MOZ_ASSERT(thisArg->mKind == STRING)
Categories
(Core :: XPCOM, defect)
Tracking
()
People
(Reporter: ishikawa, Assigned: ishikawa)
References
Details
(Keywords: assertion, Whiteboard: [tbird crash])
Attachments
(5 files)
Incomplete SMTP host configuration.
This is under Debian GNU/Linux. But I suspect it applies to all the platforms.
TB was locally build and M-C/C-C is about a week old now. (I am trying to see if I can run TB's mochitest under valgrind and wanted a stable binary for now. That is why I have not updated the source file right now. See Bug 1629433 and Bug 1636945 if you are interested in valgrind testing.)
What happened.
I tried to send an e-mail message with an attachment to a local e-mail address of the form, ishikawa@localhost.
The local host has an SMTP server running (acutally the SMTP subsystem of dovecot mail suite.)
ps axg | grep dovecot
1117 ? Ss 0:00 /usr/sbin/dovecot -F
1169 ? S 0:00 dovecot/anvil
1170 ? S 0:00 dovecot/log
1173 ? S 0:00 dovecot/config
121554 ? S 0:00 dovecot/stats
714617 pts/5 S+ 0:00 grep dovecot
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$
What happened.:
The local SMTP server seems to have responded that it does not know the mail recipient name and refused to accept the message transfer.
TB seems to try to print the error message (I have compiled TB with full DEBUG, -DDEBUG=1) and experienced MOZ_ASSERT().
The stack trace from the failing TB.
Program /NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/thunderbird-bin (pid = 714240) received signal 11.
Stack:
#01: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x62e0df5]
#02: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x14140]
#03: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x10873bf]
#04: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x1a31451]
#05: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x1bef5ff]
#06: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x1bef7a9]
#07: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x1beea52]
#08: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x1b64742]
#09: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x270fdd3]
#10: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x2710011]
#11: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x27157d2]
#12: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x685f73e]
#13: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x68738e6]
#14: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x68670d3]
#15: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x6873206]
#16: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x687382e]
#17: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x6873df8]
#18: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x6f5f0ff]
#19: ???[/NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/libxul.so +0x6f5f70d]
#20: ??? (???:???)
Sleeping for 300 seconds.
Type 'gdb /NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/thunderbird-bin 714240' to attach your debugger to this thread.
... from a different terminal I did ...
ishikawa@ip030:/home/ishikawa$ gdb /NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/thunderbird-bin 714240
GNU gdb (Debian 10.1-2) 10.1.90.20210103-git
Copyright (C) 2021 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<https://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/thunderbird-bin...
Attaching to program: /NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/thunderbird-bin, process 714240
[New LWP 714244]
[New LWP 714245]
[New LWP 714246]
[New LWP 714247]
[New LWP 714249]
[New LWP 714250]
[New LWP 714253]
[New LWP 714257]
[New LWP 714258]
[New LWP 714259]
[New LWP 714260]
[New LWP 714262]
[New LWP 714264]
[New LWP 714265]
[New LWP 714266]
[New LWP 714267]
[New LWP 714268]
[New LWP 714269]
[New LWP 714270]
[New LWP 714273]
[New LWP 714279]
[New LWP 714280]
[New LWP 714281]
[New LWP 714282]
[New LWP 714283]
[New LWP 714284]
[New LWP 714285]
[New LWP 714286]
[New LWP 714287]
[New LWP 714288]
[New LWP 714289]
[New LWP 714290]
[New LWP 714291]
[New LWP 714292]
[New LWP 714293]
[New LWP 714294]
[New LWP 714295]
[New LWP 714296]
[New LWP 714297]
[New LWP 714298]
[New LWP 714300]
[New LWP 714301]
[New LWP 714302]
[New LWP 714303]
[New LWP 714304]
[New LWP 714305]
[New LWP 714306]
[New LWP 714307]
[New LWP 714308]
[New LWP 714309]
[New LWP 714310]
[New LWP 714311]
[New LWP 714312]
[New LWP 714314]
[New LWP 714317]
[New LWP 714318]
[New LWP 714319]
[New LWP 714320]
[New LWP 714321]
[New LWP 714322]
[New LWP 714339]
[New LWP 714340]
[New LWP 714341]
[New LWP 714342]
[New LWP 714343]
[New LWP 714344]
[New LWP 714345]
[New LWP 714346]
[New LWP 714347]
[New LWP 714348]
[New LWP 714349]
[New LWP 714350]
[New LWP 714351]
[New LWP 714352]
[New LWP 714353]
[New LWP 714354]
[New LWP 714355]
[New LWP 714356]
[New LWP 714357]
[New LWP 714358]
[New LWP 714359]
[New LWP 714360]
[New LWP 714361]
[New LWP 714367]
[New LWP 714370]
[New LWP 714373]
[New LWP 714379]
[New LWP 714380]
[New LWP 714381]
[New LWP 714382]
[New LWP 714390]
[New LWP 714393]
[New LWP 714398]
[New LWP 714415]
[New LWP 714416]
[New LWP 714417]
[New LWP 714418]
[New LWP 714419]
[New LWP 714429]
[New LWP 714430]
[New LWP 714431]
[New LWP 714432]
[New LWP 714433]
[New LWP 714434]
[New LWP 714435]
[New LWP 714436]
[New LWP 714437]
[New LWP 714438]
[New LWP 714439]
[New LWP 714440]
[New LWP 714441]
[New LWP 714442]
[New LWP 714443]
[New LWP 714444]
[New LWP 714445]
[New LWP 714446]
[New LWP 714447]
[New LWP 714448]
[New LWP 714449]
[New LWP 714450]
[New LWP 714451]
[New LWP 714452]
[New LWP 714453]
[New LWP 714454]
[New LWP 714455]
[New LWP 714456]
[New LWP 714457]
[New LWP 714458]
[New LWP 714459]
[New LWP 714460]
[New LWP 714461]
[New LWP 714462]
[New LWP 714508]
[New LWP 714515]
[New LWP 714516]
[New LWP 714517]
[New LWP 714518]
[New LWP 714519]
[New LWP 714520]
[New LWP 714521]
[New LWP 714522]
[New LWP 714523]
[New LWP 714524]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
--Type <RET> for more, q to quit, c to continue without paging--q
Quit
A program is being debugged already. Kill it? (y or n) n
Program not killed.
Redefine command "prun"? (y or n) [answered Y; input not from terminal]
Redefine command "pu"? (y or n) [answered Y; input not from terminal]
Redefine command "ps"? (y or n) [answered Y; input not from terminal]
Loading JavaScript value pretty-printers; see js/src/gdb/README.
If they cause trouble, type: disable pretty-printer .* SpiderMonkey
SpiderMonkey unwinder is disabled by default, to enable it type:
enable unwinder .* SpiderMonkey
(gdb) where
#0 0x00007fa8775efc61 in __GI___clock_nanosleep
(clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7ffc2e0f8990, rem=rem@entry=0x7ffc2e0f8990)
at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:48
#1 0x00007fa8775f5443 in __GI___nanosleep
(requested_time=requested_time@entry=0x7ffc2e0f8990, remaining=remaining@entry=0x7ffc2e0f8990) at nanosleep.c:27
#2 0x00007fa8775f537a in __sleep (seconds=0) at ../sysdeps/posix/sleep.c:55
#3 0x00007fa86f1c0b49 in common_crap_handler(int, void const*)
(signum=11, aFirstFramePC=0x7fa86f19bdf5 <nsProfileLock::FatalSignalHandler(int, siginfo_t*, void*)+197>)
at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/toolkit/xre/nsSigHandlers.cpp:95
#4 0x00007fa86f1c0b6d in ah_crap_handler(int) (signum=<optimized out>)
at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/toolkit/xre/nsSigHandlers.cpp:103
#5 0x00007fa86f19bdf5 in nsProfileLock::FatalSignalHandler(int, siginfo_t*, void*) (signo=11, info=0x7ffc2e0f8bf0, context=0x7ffc2e0f8ac0)
at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/toolkit/profile/nsProfileLock.cpp:183
#6 0x00007fa877929140 in <signal handler called> ()
at /lib/x86_64-linux-gnu/libpthread.so.0
#7 nsTextFormatter::dosprintf(nsTextFormatter::SprintfStateStr*, char16_t const*, mozilla::Span<nsTextFormatter::BoxedValue, 18446744073709551615ul>) (aState=
0x7ffc2e0f90f0, aFmt=0x5586fd79846e u". Please verify that your email address is correct in your account settings and try again.", aValues=...)
--Type <RET> for more, q to quit, c to continue without paging--c
at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/string/nsTextFormatter.cpp:712
#8 0x00007fa86a8ec451 in nsTextFormatter::vssprintf(nsTSubstring<char16_t>&, char16_t const*, mozilla::Span<nsTextFormatter::BoxedValue, 18446744073709551615ul>) (aOut=..., aFmt=0x5586fd7983e8 u"An error occurred while sending mail. The mail server responded: %s. Please verify that your email address is correct in your account settings and try again.", aValues=...) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/string/nsTextFormatter.cpp:852
#9 0x00007fa86aaaa5ff in nsTextFormatter::ssprintf<char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*>(nsTSubstring<char16_t>&, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*, char16_t const*) (aFmt=<optimized out>, aOut=...) at /NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/mozilla/Span.h:289
#10 nsStringBundleBase::FormatString(char16_t const*, nsTArray<nsTString<char16_t> > const&, nsTSubstring<char16_t>&) (aFormatStr=<optimized out>, aParams=const class nsTArray<nsTString<char16_t> > & = {...}, aResult=...) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/intl/strres/nsStringBundle.cpp:679
#11 0x00007fa86aaaa7a9 in nsStringBundleBase::FormatStringFromName(char const*, nsTArray<nsTString<char16_t> > const&, nsTSubstring<char16_t>&) (this=this@entry=0x5586fdbc3600, aName=0x5586fd423448 "errorSendingFromCommand", aParams=const class nsTArray<nsTString<char16_t> > & = {...}, aResult=...) at /NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/nsTString.h:161
#12 0x00007fa86aaa9a52 in nsStringBundleBase::FormatStringFromAUTF8Name(nsTSubstring<char> const&, nsTArray<nsTString<char16_t> > const&, nsTSubstring<char16_t>&) (this=0x5586fdbc3600, aName=<optimized out>, aParams=const class nsTArray<nsTString<char16_t> > & = {...}, aResult=...) at /NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/nsTString.h:161
#13 0x00007fa86aa1f742 in NS_InvokeByIndex () at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:101
#14 0x00007fa86b5cadd3 in CallMethodHelper::Invoke() (this=0x7ffc2e0f9438) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1631
#15 CallMethodHelper::Call() (this=this@entry=0x7ffc2e0f9438) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1184
#16 0x00007fa86b5cb011 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (ccx=..., mode=<optimized out>) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1130
#17 0x00007fa86b5d07d2 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (cx=<optimized out>, argc=<optimized out>, vp=0x5586fa731050) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:921
#18 0x00007fa86f71a73e in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) (cx=0x5586fa9c0810, native=native@entry=0x7fa86b5d0470 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, reason=reason@entry=js::CallReason::Call, args=...) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/src/vm/Interpreter.cpp:401
#19 0x00007fa86f72e8e6 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) (cx=<optimized out>, args=..., construct=<optimized out>, reason=js::CallReason::Call) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/src/vm/Interpreter.cpp:488
#20 0x00007fa86f7220d3 in js::CallFromStack(JSContext*, JS::CallArgs const&) (args=<optimized out>, cx=<optimized out>) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/src/vm/Interpreter.cpp:552
#21 Interpret(JSContext*, js::RunState&) (cx=0x5586fa9c0810, state=...) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/src/vm/Interpreter.cpp:3255
#22 0x00007fa86f72e206 in js::RunScript(JSContext*, js::RunState&) (cx=0x5586fa9c0810, state=...) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/src/vm/Interpreter.cpp:370
#23 0x00007fa86f72e82e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) (cx=<optimized out>, args=..., construct=js::NO_CONSTRUCT, reason=<optimized out>) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/src/vm/Interpreter.cpp:520
#24 0x00007fa86f72edf8 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) (cx=<optimized out>, fval=Python Exception <class 'gdb.error'> value has been optimized out:
, fval@entry=$JS::Value((class JSObject *) 0x5ea90704c58 [object Function "_onCommand"]), thisv=Python Exception <class 'gdb.error'> value has been optimized out:
, thisv@entry=$JS::Value((class JSObject *) 0x5ea90705878 [object Object]), args=..., rval=$JS::UndefinedValue(), reason=reason@entry=js::CallReason::Call) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/src/vm/Interpreter.cpp:565
#25 0x00007fa86fe1a0ff in js::jit::InvokeFunction(JSContext*, JS::Handle<JSObject*>, bool, bool, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (cx=<optimized out>, obj=Python Exception <class 'gdb.error'> value has been optimized out:
, constructing=<optimized out>, ignoresReturnValue=<optimized out>, argc=1, argv=0x7ffc2e0fa200, rval=$JS::UndefinedValue()) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/src/jit/VMFunctions.cpp:780
#26 0x00007fa86fe1a70d in js::jit::InvokeFromInterpreterStub(JSContext*, js::jit::InterpreterStubExitFrameLayout*) (cx=0x5586fa9c0810, frame=0x7ffc2e0fa1d8) at /NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/js/RootingAPI.h:1197
#27 0x00001e5ba82f8e8f in ()
#28 0x00007ffc2e0fa218 in ()
#29 0x00007ffc2e0fa1d8 in ()
#30 0x0000000000000000 in ()
(gdb)
Please note that the signal is raised in TB's sprintf routine.
So let me figure out what arguments are passed ssprintf.
After going up the stack a few times, I realize the error is caused by MOZ_ASSERT().
(gdb) up
#7 nsTextFormatter::dosprintf (aState=0x7ffc2e0f90f0,
aFmt=0x5586fd79846e u". Please verify that your email address is correct in your account settings and try again.", aValues=...)
at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/string/nsTextFormatter.cpp:712
712 MOZ_ASSERT(thisArg->mKind == STRING);
(gdb)
https://searchfox.org/mozilla-central/source/xpcom/string/nsTextFormatter.cpp#712
is this:
case 'S':
MOZ_ASSERT(thisArg->mKind == STRING16);
// Type-based printing below.
break;
case 's':
MOZ_ASSERT(thisArg->mKind == STRING); <--- line 712
// Type-based printing below.
break;
I am including the case statement above for reasons that would become clear later in this post.
Above the big switch statement we have block comment which I quote below.
// Several `MOZ_ASSERT`s below check for argument compatibility
// with the format specifier. These are only debug assertions,
// not release assertions, and exist to catch problems in C++
// callers of `nsTextFormatter`, as we do not have compile-time
// checking of format strings. In release mode, these assertions
// will be no-ops, and we will fall through to printing the
// argument based on the known type of the argument.
So we are hitting a bad call of this function and DEBUG build triggered it.
Going up a bit further, hampered by the lack of optimized-away values, I did find that
the aVaule(s) passed to does not have mkind that is STRING, but actually is STRING16.
Subtle and yet valid difference to trigger the assert.
(gdb) print aState
$1 = (nsTextFormatter::SprintfStateStr *) 0x7ffc2e0f90f0
(gdb) print *aState
$2 = {stuff =
0x7fa86a8efd40 <nsTextFormatter::StringStuff(nsTextFormatter::SprintfStateStr*, char16_t const*, unsigned int)>,
base = 0x5586feb3ab88 u"An error occurred while sending mail. The mail server responded: ",
cur = 0x5586feb3ab88 u"An error occurred while sending mail. The mail server responded: ",
maxlen = 0, stuffclosure = 0x7ffc2e0f94b0}
(gdb) print aFmt
$3 = 0x5586fd79846e u". Please verify that your email address is correct in your account settings and try again."
(gdb) print aValues
$4 = <optimized out>
(gdb) print aValue
No symbol "aValue" in current context.
(gdb) print c
$5 = <optimized out>
(gdb) print flags
$6 = <optimized out>
(gdb) print width
$7 = <optimized out>
(gdb) print radix
$8 = 10
(gdb) print prec
$9 = <optimized out>
(gdb) print hexp
$10 = <optimized out>
(gdb) print nextNaturalArg
$11 = 1
(gdb) print sawNumberedArg
$12 = <optimized out>
(gdb) print sawWidth
$13 = <optimized out>
(gdb) print hex
$14 = u"0123456789abcdef"
(gdb) print radix
$15 = 10
(gdb) print c
$16 = <optimized out>
(gdb) print thisArg
$17 = <optimized out>
(gdb) print *thisArg
value has been optimized out
(gdb) up
#8 0x00007fa86a8ec451 in nsTextFormatter::vssprintf (aOut=...,
aFmt=0x5586fd7983e8 u"An error occurred while sending mail. The mail server responded: %s. Please verify that your email address is correct in your account settings and try again.",
aValues=...) at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/string/nsTextFormatter.cpp:852
852 dosprintf(&ss, aFmt, aValues);
(gdb) print aValues
$18 = {static extent = <optimized out>,
storage_ = {<mozilla::span_details::extent_type<18446744073709551615>> = {size_ = 10},
data_ = 0x7ffc2e0f9150}}
(gdb) print aValues.data
Cannot take address of method data.
(gdb) Quit
(gdb) print aValues.data_
There is no member or method named data_.
(gdb) print aValues.storage
There is no member or method named storage.
(gdb) print aValues.storage_
$19 = {<mozilla::span_details::extent_type<18446744073709551615>> = {size_ = 10},
data_ = 0x7ffc2e0f9150}
(gdb) print aValues.storage_.data_
$20 = (mozilla::Span<nsTextFormatter::BoxedValue, 18446744073709551615>::pointer) 0x7ffc2e0f9150
(gdb) print *(aValues.storage_.data_)
$21 = {mKind = nsTextFormatter::STRING16, mValue = {mInt = 94038270578152,
mUInt = 94038270578152, mPtr = 0x5586fdb68de8, mDouble = 4.6461078886988561e-310,
mString = 0x5586fdb68de8 "B", mString16 = 0x5586fdb68de8 u"Bad sender address syntax",
mIntPtr = 0x5586fdb68de8}}
(gdb) print aFmt
$22 = 0x5586fd7983e8 u"An error occurred while sending mail. The mail server responded: %s. Please verify that your email address is correct in your account settings and try again."
(gdb)
So I think the format specifier used should be "%S" instead of "%s" (note the upper vs lower difference.)
What do people think?
I wonder why this issue was not detected by mochitest on treeherder and other tests, but obviously we have failed to test cases of failing SMTP server interaction throughly using Debug build.
PS: I have set up TB's SMTP server configuration MANUALLY or rather kept it incomplete since I avoided the initial AUTOMATIC SETUP because it is such a bother during local building and testing, and chose manual setup. Obviously the local SMTP server did not grok my specified local ID, it seemed.) Still, TB should not abort in the face of such error situation.
|
Assignee | |
Comment 1•4 years ago
|
||
As I said, I have not configured the SMTP server entry very much.
Still TB should NOT crash.
You can see that necessary information SMTP is not specified yet pretty much.
Of course, my SMTP host doesn't know the user ID (empty?).
|
|
Assignee | |
Comment 2•4 years ago
•
|
||
"%s" seems to come from the following property.
https://searchfox.org/comm-central/source/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties#20
errorSendingFromCommand=An error occurred while sending mail. The mail server responded: %s. Please verify that your email address is correct in your account settings and try again.
Unless I change this into "%S", I can't seem to test what happens if I change other settings of SMTP host (when my ID is not recognized by SMTP), and so I modify this line and rebuild TB now.
|
|
Assignee | |
Comment 3•4 years ago
|
||
I may have to be careful. It is possible that my ID passed to SMTP host was empty string. I wonder what happens when TB sees an empty ID thrown back from SMTP server. Does it assume it to be STRING or STRING16?
(Aha, maybe I needed to print more arguments to |TextFormater|. At the sametime, I see that the data types seen in the stack trace converge to char16_t data types. So I think %S is preferred.
|
|
Assignee | |
Comment 4•4 years ago
•
|
||
After I changed "%s" to "%S" as in comment 9, I could get the proper error message without crash.
So the response from SMTP server is passed to the TextFormat routine after type changes as char16_t instead of char8_t.
I did not have to re-build TB. That was good.
What is NOT good is this.
In the same message file,
I see mixture of "%s" and "%S" which are used for SMTP server response.
I think they need to be all "%S" due to the same reason in this bugzilla.
It happens to work, .i.e. readable strings are printed as of now on little endian machine in the release build.
On big endian machines, I think the user would see garbage.
To wit, only partial excerpt from the message file. I modified the first couple of lines to use "%S" instead of "%s".
## LOCALIZATION NOTE (errorSendingFromCommand): argument %S is the Outgoing server (SMTP) response
errorSendingFromCommand=An error occurred while sending mail. The mail server responded: %S. Please verify that your email address is correct in your account settings and try again.
## LOCALIZATION NOTE (errorSendingDataCommand): argument %s is the Outgoing server (SMTP) response
errorSendingDataCommand=An Outgoing server (SMTP) error occurred while sending mail. The server responded: %s.
## LOCALIZATION NOTE (errorSendingMessage): argument %s is the Outgoing server (SMTP) response
errorSendingMessage=An error occurred while sending mail. The mail server responded: %s. Please check the message and try again.
postFailed=The message could not be posted because connecting to the news server failed. The server may be unavailable or is refusing connections. Please verify that your news server settings are correct and try again.
errorQueuedDeliveryFailed=An error occurred while delivering the unsent messages.
sendFailed=Sending of the message failed.
## LOCALIZATION NOTE (sendFailedUnexpected): argument %X is a hex error code value
sendFailedUnexpected=Failed due to unexpected error %X. No description is available.
## LOCALIZATION NOTE (smtpSecurityIssue): argument %S is the Outgoing server (SMTP) response
smtpSecurityIssue=The configuration related to %S must be corrected.
There are much more lines that refer to server response and with mixed "%s" and "%S" usage.
Someone in the know ought to take a serious look at this issue.
|
||
Comment 5•4 years ago
|
||
This all goes through stringbundle formatStringFromName through XPCOM and there are both AUTF8String and not callers - https://searchfox.org/mozilla-central/source/intl/strres/nsIStringBundle.idl#55,61. The formatter assertion seems basically flawed for that.
In practice there's probably no problem as I don't think Thunderbird exists for any big-endian system
|
|
Assignee | |
Comment 6•4 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #5)
This all goes through stringbundle formatStringFromName through XPCOM and there are both AUTF8String and not callers - https://searchfox.org/mozilla-central/source/intl/strres/nsIStringBundle.idl#55,61. The formatter assertion seems basically flawed for that.
Do you mean there are cases where char16_t pointer is passed and other cases where char8_t pointer is passed?
If so, that is tough.
In practice there's probably no problem as I don't think Thunderbird exists for any big-endian system
Well, then we need to ask Tom Tromey and Nathan Froyd
who did the work of making the routine more robust at run-time in Bug 1388789 according to the log/blame info.
Summary: we have hit MOZ_ASSERT(thisArg->mKind == STRING) on nsTextFormatter.cpp:712 when an error message from the SMTP server is displayed with DEBUG version of TB, and in this particular case, the argument has mKind which is STRING16, so I changed where "%s" in format string is coming from (in a message property file) to %S and I could avoid the assertion.
However, there are other mixed usage of %s and %S in the property file that handles the messages from SMTP server.
Magnus thinks the particular |MOZ_ASSERT()| may not be useful and we are OK to ignore it since we don't run TB on a big endian machine as of now.
What is the proper way to handles this? Remove MOZ_ASSERT() altogether?
|
||
Comment 7•4 years ago
|
||
Summary: we have hit MOZ_ASSERT(thisArg->mKind == STRING) on nsTextFormatter.cpp:712 when an error message from the SMTP server is displayed with DEBUG version of TB, and in this particular case, the argument has mKind which is STRING16, so I changed where "%s" in format string is coming from (in a message property file) to %S and I could avoid the assertion.
However, there are other mixed usage of %s and %S in the property file that handles the messages from SMTP server.
Magnus thinks the particular |MOZ_ASSERT()| may not be useful and we are OK to ignore it since we don't run TB on a big endian machine as of now.
What is the proper way to handles this? Remove MOZ_ASSERT() altogether?
I don't understand the big-endian comment. It doesn't seem relevant.
I don't recall exactly why I added the asserts. Maybe this is what happened before?
Or maybe I was being overly pedantic.
They aren't strictly necessary, because this has a template wrapper that passes through the
compiler-generated type information. So this kind of checking is actually a bit redundant.
I would support making %s and %S equivalent, in the sense that each format could
allow either underlying string type. This would be done by extending the assertions, like:
case 's':
case 'S':
MOZ_ASSERT(thisArg->mKind == STRING || thisArg->mKind == STRING16);
break;
You'd have to double-check that this is all that's needed.
You could even generalize this further if you really wanted, to avoid asserts and
make all %-formats "DWIM". Of course then why bother having different %-formats at all.
|
|
Assignee | |
Comment 8•4 years ago
•
|
||
(In reply to Tom Tromey :tromey from comment #7)
Summary: we have hit MOZ_ASSERT(thisArg->mKind == STRING) on nsTextFormatter.cpp:712 when an error message from the SMTP server is displayed with DEBUG version of TB, and in this particular case, the argument has mKind which is STRING16, so I changed where "%s" in format string is coming from (in a message property file) to %S and I could avoid the assertion.
However, there are other mixed usage of %s and %S in the property file that handles the messages from SMTP server.
Magnus thinks the particular |MOZ_ASSERT()| may not be useful and we are OK to ignore it since we don't run TB on a big endian machine as of now.
What is the proper way to handles this? Remove MOZ_ASSERT() altogether?
I don't understand the big-endian comment. It doesn't seem relevant.
I don't recall exactly why I added the asserts. Maybe this is what happened before?
Or maybe I was being overly pedantic.They aren't strictly necessary, because this has a template wrapper that passes through the
compiler-generated type information. So this kind of checking is actually a bit redundant.
I would support making %s and %S equivalent, in the sense that each format could
allow either underlying string type. This would be done by extending the assertions, like:case 's':
case 'S':
MOZ_ASSERT(thisArg->mKind == STRING || thisArg->mKind == STRING16);
break;You'd have to double-check that this is all that's needed.
You could even generalize this further if you really wanted, to avoid asserts and
make all %-formats "DWIM". Of course then why bother having different %-formats at all.
Thank you for the comment.
For now, the suggested patch seems the way to go then.
Let me change the code locally and see how the local test and treeherder test go.
|
|
Assignee | |
Comment 9•4 years ago
|
||
I created this patch and build TB locally and
on try-comm-central.
It did not produce any unknown new errors.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=13995f91cffa1c8d0971e8d60cfe5b9e79edae81
|
|
Assignee | |
Updated•4 years ago
|
|
|
Assignee | |
Updated•4 years ago
|
|
||
Updated•4 years ago
|
|
|
||
Updated•4 years ago
|
|
||
Comment 10•4 years ago
|
||
I don't claim to understand C++ code, but why are you changing the code instead of fixing the strings?
.properties should use "%S" or "$1$S", not lowercase %s. That's how tools expected placeholders to be formatted, and they definitely produce different results in some cases (IIRC, %s would only show the first character).
|
|
Assignee | |
Comment 11•4 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #10)
I don't claim to understand C++ code, but why are you changing the code instead of fixing the strings?
.properties should use "%S" or "$1$S", not lowercase
%s. That's how tools expected placeholders to be formatted, and they definitely produce different results in some cases (IIRC,%swould only show the first character).
So the comment 7 is not quite approrpriate?
I would support making %s and %S equivalent, in the sense that each format could
allow either underlying string type. This would be done by extending the assertions, like:
case 's':
case 'S':
MOZ_ASSERT(thisArg->mKind == STRING || thisArg->mKind == STRING16);
break;
(We need to cleanup quite a few entries in L10N message files (in manylanguages) as well from the viewpoint of TB.)
|
|
||
Comment 12•4 years ago
|
||
See for example old bug 1133554, bug 1171870. The assertion was introduced for a reason.
Again, I would like to understand why fixing the strings (you'll need new IDs BTW) shouldn't be the fix here.
|
|
Assignee | |
Comment 13•4 years ago
•
|
||
I have not much opinion here.
I was under the impression just modifying the code as in my patch was just fine in today's code base.
Again see comment 5, and comment 7.
I looked at bug 1133554 and bug 1171870.
Here are a few data points.
[0] Despite Bug 1133554 , it seems the code has changed significantly.
With my patch in, TB can print response from the server without truncation. That is %s and %S does not make a difference.
It seems that comment 5 and comment 7 are true.
That is, TB today does not seem to have a big fuss over the difference of %s and %S. (But I am not sure. See my comment [1] below.)
BTW, I had to make sure if I am using the correct properties file and the current one, just in case. (%s vs %S)
I added the message ID at the end of the string as in
## LOCALIZATION NOTE (errorSendingFromCommand): argument %s is the Outgoing server (SMTP) response
errorSendingFromCommand=An error occurred while sending mail. The mail server responded: %s. Please verify that your email address is correct in your account settings and try again.\n [errorSendingFromCommand]
So that I know for sure, what string in .properties file TB is using. Note the small 's' in %s.
The above line is printed in the dialog when the error dialog is shown.
And you see the full message from the server.
[1] Are there UTF-16 documents in C-C tree?
The message printed in the dialog comes from
/NREF-COMM-CENTRAL/mozilla/comm/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
and this file is, according to linux's |file| command, in UTF-8 code.
I think no sane SMTP server on the internet tries to use UTF-16 in response at all. After all, it took years to achieve 8-bit clean network path.
So I think using %s is fine in this particular error case.
And for that matter, as long as I can tell, there are only handful of UTF-16 files within
mozilla/comm subdirectory. And I think this suggests use of %s is just fine as far as the data comes from within C-C tree.
And the external source of such data is very unlikely (see my comment regarding no sane SMTP server tries to response in UTF-16).
Finding how many UTF-16 files are in C-C tree.
find . -type f -print | xargs file | grep UTF | grep 16
./mailnews/compose/test/unit/data/test-UTF-16LE.txt: Little-endian UTF-16 Unicode text, with CRLF line terminators
./mailnews/compose/test/unit/data/test-UTF-16BE.txt: Big-endian UTF-16 Unicode text, with CRLF line terminators
./mailnews/test/data/16-plaintext+HMTL.eml: UTF-8 Unicode text
./mailnews/import/test/unit/resources/WindowsLiveMail/donhallimap/donhallimap{testimap}.oeaccount: XML 1.0 document, Little-endian UTF-16 Unicode text, with CRLF line terminators
./mailnews/import/test/unit/resources/WindowsLiveMail/donhallnntp/donhallnntp{testnntp}.oeaccount: XML 1.0 document, Little-endian UTF-16 Unicode text, with CRLF line terminators
./mailnews/import/test/unit/resources/WindowsLiveMail/news.mozilla.org/account{B3B3}.oeaccount: XML 1.0 document, Little-endian UTF-16 Unicode text, with CRLF line terminators
./mailnews/import/test/unit/resources/WindowsLiveMail/MicrosoftCommunities/account{2E23}.oeaccount: XML 1.0 document, Little-endian UTF-16 Unicode text, with CRLF line terminators
./mailnews/import/test/unit/resources/WindowsLiveMail/pop3.test.test/account{D244}.oeaccount: XML 1.0 document, Little-endian UTF-16 Unicode text, with CRLF line terminators
./mailnews/import/test/unit/resources/utf16_addressbook.csv: Little-endian UTF-16 Unicode text, with very long lines, with CRLF line terminators
./suite/components/places/tests/autocomplete/test_416214.js: UTF-8 Unicode text
./mail/components/search/mdimporter/English.lproj/schema.strings: Little-endian UTF-16 Unicode text
./mail/components/search/mdimporter/English.lproj/InfoPlist.strings: Little-endian UTF-16 Unicode text
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla/comm$
A few files above were listed because there was "16" in its path string.
But I have to ask someone in the know.
Oh, to cope with the mess in .properties file regarding the use of %s and %S, and given that the code now supports either of the format specifier (if so), I would prefer the code change instead of the properties file change which have been translated into a few dozen languages now. {But of course, editing is doable.]
Messages strings used only in error path may NOT have been TESTED FULLY IMHO.
I say this because manually running locally-built DEBUG-version of TB report this problem for the first time after MOZ_ASSERT() was introduced.
|
|
Assignee | |
Comment 14•4 years ago
|
||
(In reply to Tom Tromey :tromey from comment #7)
Summary: we have hit MOZ_ASSERT(thisArg->mKind == STRING) on nsTextFormatter.cpp:712 when an error message from the SMTP server is
... [omission] ...
I don't understand the big-endian comment. It doesn't seem relevant.
If UTF-16 code for 'a', 'b', 'c' each 'a', 'b', 'c' is stored as in
\0, 'a', \0, 'b', \0, 'c' in big-endian form
vs
'a', \0, 'b', \0, 'c', \0 in little-endian form
Here, 'a', 'b', and 'c' stand for any 8-bit octet character that is mapped to UTF-16 representation by appending \0 octet in its upper (as UTF-16) position.
Then ordinary C format %s handles the former as empty / null string because it sees \0 in the first character position,
whereas the second string is
handled as "a". The upper octet terminating the C-like string.
See comment 5, and [0] in my comment 13.
I think there does not seem to be any internal UTF-16 source that would be used for printing via %s (or %S) today. (I may be wrong.)
But to be honest, checking whether something is printed via %s from an external source (maybe a message being edited?)
is something beyond today's TB developer community IMHO.
Well, I DID notice there is a test file for importing (?) csv address book in UTF-16 and that may ring a bell.
In that case, messages related to importing address book may need to be scrutinized a bit.
I am totally fine with the laid-back approach, though. If an error is reported for a particular error case, then we take a look at the error situation and modify the message if necessary (and split the message if necessary if there can be UTF-16 and UTF-8 cases).
But if %s and %S no longer need distinguishing today as Tom suggested, that will be mighty fine.
|
|
Assignee | |
Comment 15•4 years ago
•
|
||
Aha, now I see JavaScript internally holds string as UTF-16 string.
But will it seep to C++ world without proper XPCOM change?
That is, going through the API mentioned in comment 5?
(These string bundles are supposed to be used through bundle API and I thought proper string representation change would have been performed by the time the string hits the original MOZ_ASSERT(). Or what? Maybe I was wrong.
But at least the subsequent processing code TODAY does not seem to have an issue of digesting UTF-16 and UTF-8 character strings without a problem as far as DEBUG version of TB is tested via its mochitest and xpcshell test. So I think in this sense, the MOZ_ASSERT() in the original code is useless for TB. There may be a few cases where it is NOT so in principle, but that has not been reported so far from real world error situation. Yeah, that last sentence is more like the situation that summarizes what is an issue here. I hit an easy-to-reproducible case where the mail client, based on user input on the "To:" line of newly composed message tries to send an e-mail to non-existing mail address to the local SMTP server. )
|
|
||
Comment 16•4 years ago
|
||
Since the string get passed through stringbundle AString formatStringFromName(in AUTF8String aName, in Array<AString> params); (which is an oddly mixed string-type signature) the params will be UTF16 yes. So the proper fix would be to change the %s to %S and change the l10n when doing so. Could be done, but since it doesn't appear to cause any problems....
|
|
||
Comment 17•4 years ago
|
||
|
|
||
Comment 18•4 years ago
|
||
(In reply to Francesco Lodolo [:flod] (OOO until Jan 3rd) from comment #12)
See for example old bug 1133554, bug 1171870. The assertion was introduced for a reason.
Again, I would like to understand why fixing the strings (you'll need new IDs BTW) shouldn't be the fix here.
Fixing the strings is fine and harmless. However, it leaves open the possibility to introduce a similar bug once again.
It's true that, in the past, the UTF-8 and UTF-16 cases were handled differently by nsTextFormatter. However, that situation changed with Bug 1388789. There, we changed the API so that the compiler annotates each actual argument with its type (this is the BoxedValue stuff). This change enables the formatting engine to detect mismatches, and the assertion is not really needed -- it does not provide any additional safety, it is just a pedanticism.
I don't understand the big-endian comment. It doesn't seem relevant.
If UTF-16 code for 'a', 'b', 'c' each 'a', 'b', 'c' is stored as in
\0, 'a', \0, 'b', \0, 'c' in big-endian form
Ok. I see -- this isn't relevant to this code any more. You can see this by starting with the API in nsTextFormatter.h. A call with a UTF-16 string goes via:
explicit BoxedValue(const char16_t* aArg) : mKind(STRING16) {
mValue.mString16 = aArg;
}
(see how ssprintf boxes all of its arguments.)
Then in the .cc file, the actual printing is handled by a switch:
// If we get here, we want to handle the argument according to its
// actual type; modified by the flags as appropriate.
switch (thisArg->mKind) {
...
case STRING:
rv = cvt_s(aState, thisArg->mValue.mString, width, prec, flags);
break;
case STRING16:
rv = cvt_S(aState, thisArg->mValue.mString16, width, prec, flags);
break;
What you can see here is that the actual format character -- the 's' or 'S' -- is irrelevant. (This is just a brief outline, to really convince yourself you will have to read a bit more of the code.) Instead the decision on how to format is handled based on the type of the actual argument to ssprintf, as supplied by the C++ compiler.
This is why I think it's fine to erase the distinction between these cases.
|
|
Assignee | |
Comment 19•3 years ago
|
||
This has been sitting here for almost a year.
I think Tom explained why the patch here is OK in comment 17 and 18.
Is it OK to land the patch?
Without this patch, the DEBUG version of C-C TB crasshed hitting MOZ_ASSERT().
|
|
||
Comment 20•3 years ago
|
||
Probably, but you need submit it via phabricator and get it formally reviewed. Try r=#xpcom-reviewers
|
|
Assignee | |
Comment 21•3 years ago
|
||
I see.
|
|
Assignee | |
Comment 22•3 years ago
|
||
|
|
Assignee | |
Comment 23•3 years ago
|
||
I used moz-phab for the first time.
(Well, I am not sure how to reconcile this with my still old habit of holding many local MQ patches...)
|
||
Updated•3 years ago
|
|
||
Comment 24•3 years ago
|
||
|
|
||
Updated•3 years ago
|
|
||
Comment 25•3 years ago
|
||
| bugherder | ||
|
||
Updated•3 years ago
|
Description
•