Closed Bug 1401821 Opened 2 years ago Closed 2 years ago

Test failures caused by changes to nsTextFormatter in bug 1388789

Categories

(Core :: XPCOM, defect, blocker)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- fixed

People

(Reporter: jorgk-bmo, Assigned: tromey)

References

Details

(Whiteboard: [Thunderbird-testfailure: XZ all])

Attachments

(1 file)

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.
Flags: needinfo?(ttromey)
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
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.
Whiteboard: [Thunderbird-testfailure: XZ all]
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?
Severity: normal → blocker
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.
Blocks: 1388789
Summary: Test failures caused by new nsTextFormatter → Test failures caused by changes to nsTextFormatter in bug 1388789
Sorry about this.  I see the problem and I'll fix shortly.
Assignee: nobody → ttromey
Flags: needinfo?(ttromey)
Comment on attachment 8910800 [details]
Bug 1401821 - nsTextFormatter "*" width argument comes before the actual argument;

https://reviewboard.mozilla.org/r/182268/#review187634
Attachment #8910800 - Flags: review?(nfroyd) → review+
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a5fec539b167
nsTextFormatter "*" width argument comes before the actual argument; r=froydnj
Thanks for the quick fix. All the failing Xpcshell test pass locally now. Please request beta uplift.
https://hg.mozilla.org/mozilla-central/rev/a5fec539b167
Status: NEW → RESOLVED
Closed: 2 years ago
Component: General → XPCOM
Product: Thunderbird → Core
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
Flags: needinfo?(sledru)
Attachment #8910800 - Flags: approval-mozilla-release?
Attachment #8910800 - Flags: approval-mozilla-release? → approval-mozilla-beta?
Comment on attachment 8910800 [details]
Bug 1401821 - nsTextFormatter "*" width argument comes before the actual argument;

Fix a test, taking it.
Should be in 57b3.
Flags: needinfo?(sledru)
Attachment #8910800 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Thanks, just to clarify: The TB test-suite found a real bug/problem in the rewritten nsTextFormatter.
You need to log in before you can comment on or make changes to this bug.