1401821 - Test failures caused by changes to nsTextFormatter in bug 1388789

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jorg K (CEST = GMT+2)]

In opt mode we have:

Xpcshell failure:
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_formatFileSize.js | test_formatFileSize - [test_formatFileSize : 39] "0 bytes" == "1 bytes"

Mozmill:
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\folder-display\test-message-size.js | test-message-size.js::test_byte_message_size [log…]
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\folder-display\test-summarization.js | test-summarization.js::test_summary_updates_when_new_message_added_to_collapsed_thread

It gets much worse in debug mode where we have a eight failures:
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_formatFileSize.js | xpcshell return code: 1 [log…]
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_compactColumnSave.js | xpcshell return code: 1 [log…]
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_folderCompact.js | xpcshell return code: 1 [log…]
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js | xpcshell return code: 1 [log…]
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_compaction.js | xpcshell return code: 1 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_compactOfflineStore.js | xpcshell return code: 1 [log…]
TEST-UNEXPECTED-FAIL | mailnews/local/test/unit/test_duplicateKey.js | xpcshell return code: 1 [log…]
TEST-UNEXPECTED-FAIL | mailnews/local/test/unit/test_over4GBMailboxes.js | xpcshell return code: 1 [log…]

All crashes are the same:
ID 4884 | Assertion failure: false, at c:/builds/moz2_slave/tb-c-cen-w32-d-000000000000000/build/mozilla/xpcom/string/nsTextFormatter.cpp:634 [log…]
PROCESS-CRASH | mailnews/base/test/unit/test_formatFileSize.js | application crashed [@ nsTextFormatter::dosprintf(nsTextFormatter::SprintfStateStr *,char16_t const *,mozilla::Span<nsTextFormatter::BoxedValue,4294967295>)] [log…]

Mozmill has a lot of crashes at nsTextFormatter.cpp:634 as well.
        if (nextNaturalArg >= aValues.Length() || !aValues[nextNaturalArg].IntCompatible()) {
          // A correctness issue but not a safety issue.
          MOZ_ASSERT(false);  <=== Line 634
        } else {
          prec = aValues[nextNaturalArg++].mValue.mInt;
        }

Tom, any advice for us here? I still need look at the call stack.

Comment 1

[Jorg K (CEST = GMT+2)]

OK, that was easy:
"Assertion failure: false, at c:/mozilla-source/comm-central/mozilla/xpcom/string/nsTextFormatter.cpp:634"
"#01: nsTextFormatter::vssprintf (c:\\mozilla-source\\comm-central\\mozilla\\xpcom\\string\\nstextformatter.cpp:865)"
"#02: FormatFileSize (c:\\mozilla-source\\comm-central\\mailnews\\base\\util\\nsmsgutils.cpp:529)"

So here:
https://dxr.mozilla.org/comm-central/rev/3d6725fd02884bc7cd58d80d0c83f4788686604a/mailnews/base/util/nsMsgUtils.cpp#523

Looks like the behaviour of nsTextFormatter::ssprintf() has changed here, in fact:
https://hg.mozilla.org/mozilla-central/rev/5a294c3eff7c#l1.750

Comment 2

[Jorg K (CEST = GMT+2)]

Similar code to what we have in nsMsgUtils.cpp#523 can be found here:
https://dxr.mozilla.org/comm-central/search?q=Get+rid+of+insignificant+bits+by+truncating+to+1+or+0+de&redirect=false

So it a little hard to see why this is suddenly failing.

Comment 3

[Jorg K (CEST = GMT+2)]

I've done a bit of debugging
        if (nextNaturalArg >= aValues.Length() || !aValues[nextNaturalArg].IntCompatible()) {
          // A correctness issue but not a safety issue.
          printf("###################@@@@@@@@@@@@@@ %d %d\n", (int)nextNaturalArg, (int)aValues.Length());
          if (nextNaturalArg < aValues.Length())
          printf("###################@@@@@@@@@@@@@@ %d\n", !aValues[nextNaturalArg].IntCompatible()?1:0);
          MOZ_ASSERT(false);
        } else {
and see:
 0:01.91 PROCESS_OUTPUT: Thread-1 (pid:1296) "###################@@@@@@@@@@@@@@ 1 2"
 0:01.91 PROCESS_OUTPUT: Thread-1 (pid:1296) "###################@@@@@@@@@@@@@@ 1"
So looks like the value isn't "int compatible". Strange, I thought the code is trying to output a real number with some digits behind the decimal point, no?

Comment 4

[Jorg K (CEST = GMT+2)]

Tom, unless we're doing something obviously wrong in our call to nsTextFormatter::ssprintf(), I have the impression what this function has changed its behaviour to a state where one could call it broken.

Landing this 12 hours before the merges was really bad timing. So may I suggest a quick backout of bug 1388789 while you investigate.

Comment 5

[Tom Tromey :tromey]

Sorry about this.  I see the problem and I'll fix shortly.

Comment 6

[Tom Tromey :tromey]

Attachment: Bug 1401821 - nsTextFormatter "*" width argument comes before the actual argument;

Bug 1388789 introduced a bug breaking formats like "%*.f".  The problem
was that the next "natural" argument was taken before the "*" width
argument.

Review commit: https://reviewboard.mozilla.org/r/182268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182268/

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8910800 [details]
Bug 1401821 - nsTextFormatter "*" width argument comes before the actual argument;

https://reviewboard.mozilla.org/r/182268/#review187634

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5fec539b167
nsTextFormatter "*" width argument comes before the actual argument; r=froydnj

Comment 9

[Jorg K (CEST = GMT+2)]

Thanks for the quick fix. All the failing Xpcshell test pass locally now. Please request beta uplift.

Comment 10

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/mozilla-central/rev/a5fec539b167

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8910800 [details]
Bug 1401821 - nsTextFormatter "*" width argument comes before the actual argument;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1388789.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: In Thunderbird.

This fixes a problem introduced in bug 1388789 found in TB testing. A test in M-C was also added here. Please uplift.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910800 [details]
Bug 1401821 - nsTextFormatter "*" width argument comes before the actual argument;

Fix a test, taking it.
Should be in 57b3.

Comment 13

[Jorg K (CEST = GMT+2)]

Thanks, just to clarify: The TB test-suite found a real bug/problem in the rewritten nsTextFormatter.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a5fec539b167

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/da0d57213476