Closed Bug 1493934 Opened Last year Closed Last year

Xpcshell test failure (10 failing tests) on 2018-09-25 and MozMill crashes (even on Daily's opt builds)

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: jorgk, Assigned: jorgk)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

PROCESS-CRASH | comm/mailnews/base/test/unit/test_copyToInvalidDB.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | comm/mailnews/base/test/unit/test_folderCompact.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | comm/mailnews/db/gloda/test/unit/test_index_messages_local.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | comm/mailnews/local/test/unit/test_mailboxProtocol.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | comm/mailnews/local/test/unit/test_over4GBMailboxes.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | comm/mailnews/local/test/unit/test_pop3FilterActions.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | comm/mailnews/local/test/unit/test_pop3MoveFilter.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | comm/mailnews/local/test/unit/test_pop3PasswordFailure3.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | comm/mailnews/local/test/unit/test_pop3PasswordFailure2.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | comm/mailnews/local/test/unit/test_pop3PasswordFailure.js | application crashed [@ libc-2.23.so + 0x8b746]

Also toolkit test failures:
PROCESS-CRASH | toolkit/components/places/tests/history/test_async_history_api.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | toolkit/components/places/tests/unit/test_isvisited.js | application crashed [@ libc-2.23.so + 0x8b746]
PROCESS-CRASH | toolkit/components/places/tests/unit/test_isURIVisited.js | application crashed [@ libc-2.23.so + 0x8b746]

M-C last good: b7aa0f3bdf687053b0000a3ab9a0f98eca
M-C first bad: c5a9878baf35a354cb913b4f06542e2336
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b7aa0f3bdf687053b0000a3ab9a0f98eca&tochange=c5a9878baf35a354cb913b4f06542e2336
Running a test locally gives:
 0:01.22 pid:3308 console.log: Login storage: _findLogins: returning 0 logins
 0:01.22 pid:3308 console.log: LoginManagerPrompter: ===== promptPassword called() =====
 0:01.36 TEST_END: Test FAIL, expected PASS. Subtests passed 11/11. Unexpected 0 - xpcshell return code: 1
PROCESS-CRASH | comm/mailnews/local/test/unit/test_pop3PasswordFailure.js | application crashed [unknown top frame]

Running it in the debugger we crash here:
ucrtbase.dll!00007ffe68280431()
xul.dll!nsTSubstring<char>::Append(const char * aData, unsigned int aLength, const std::nothrow_t & aFallible) Line 844
	at c:\mozilla-source\comm-central\xpcom\string\nsTSubstring.cpp(844)
xul.dll!nsTSubstring<char>::Append(const char * aData, unsigned int aLength) Line 831
	at c:\mozilla-source\comm-central\xpcom\string\nsTSubstring.cpp(831)
xul.dll!nsMailboxUrl::GetNormalizedSpec(nsTSubstring<char> & aPrincipalSpec) Line 163
	at c:\mozilla-source\comm-central\comm\mailnews\local\src\nsMailboxUrl.cpp(163)
xul.dll!nsMsgMailNewsUrl::SetSpecInternal(const nsTSubstring<char> & aSpec) Line 411
	at c:\mozilla-source\comm-central\comm\mailnews\base\util\nsMsgMailNewsUrl.cpp(411)
xul.dll!nsMailboxUrl::SetSpecInternal(const nsTSubstring<char> & aSpec) Line 416
	at c:\mozilla-source\comm-central\comm\mailnews\local\src\nsMailboxUrl.cpp(416)
xul.dll!nsMailboxService::NewURI(const nsTSubstring<char> & aSpec, const char * aOriginCharset, nsIURI * aBaseURI, nsIURI * * _retval) Line 567
	at c:\mozilla-source\comm-central\comm\mailnews\local\src\nsMailboxService.cpp(567)
xul.dll!mozilla::net::nsIOService::NewURI(const nsTSubstring<char> & aSpec, const char * aCharset, nsIURI * aBaseURI, nsIURI * * result) Line 702
	at c:\mozilla-source\comm-central\netwerk\base\nsIOService.cpp(702)
xul.dll!XPTC__InvokebyIndex() Line 99
	at c:\mozilla-source\comm-central\xpcom\reflect\xptcall\md\win32\xptcinvoke_asm_x86_64.asm(99)

Crashing here:
nsTSubstring<T>::Append(const char_type* aData, size_type aLength,
                        const fallible_t& aFallible)
{
  if (MOZ_UNLIKELY(aLength == size_type(-1))) {
    aLength = char_traits::length(aData);  <=== 844
  }

Our code in nsMailboxUrl.cpp is:

  spec += NS_LITERAL_CSTRING("?number=");
  spec.Append(messageKey);
  PR_Free(messageKey);

So that would make it:
00df8305ed36 Henri Sivonen - Bug 1484938 - Make XPCOM string appends and copying assignments write the terminator after writing content. r=froydnj

Henri, what's wrong with out code?
Flags: needinfo?(hsivonen)
Sorry, I saw it.
Flags: needinfo?(hsivonen)
Actually, no.

We have
  char* messageKey = extractAttributeValue(spec.get(), "number=");
and extractAttributeValue() returns a string allocated with PL_strdup(). So we append that and then free it.
Flags: needinfo?(hsivonen)
Now I saw it. What we try to append is null and that now crashes :-(
Attached patch 1493934-no-null-append.patch (obsolete) — Splinter Review
This fixes the test I tried locally, but we might append null in other places.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=39c8f8ceffca966a0d0b367c906dd3298918dae3

Sadly Daily is pretty much hosed :-(
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f600e1fb24b3
Don't Append(null) in nsMailboxUrl::GetNormalizedSpec(). rs=bustage-fix DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Flags: needinfo?(hsivonen)
Target Milestone: --- → Thunderbird 64.0
Attachment #9011753 - Flags: review?(acelists)
Comment on attachment 9011753 [details] [diff] [review]
1493934-no-null-append.patch (v1b)

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

::: mailnews/local/src/nsMailboxUrl.cpp
@@ +159,5 @@
>    }
>  
> +  if (messageKey) {
> +    spec += NS_LITERAL_CSTRING("?number=");
> +    spec.Append(messageKey);

So if messageKey was null, did we append a "?number=" with no number and the handler of this still coped?

So we know why there is no messageKey? Is that a valid scenario?
Attachment #9011753 - Flags: feedback+
Yes. There doesn't have to be a number and no one ever noticed :-( - Appending "?number=" was a bug, but since this is just about providing a "normalised" URL, it didn't matter. It should be r+ ;-)
Comment on attachment 9011753 [details] [diff] [review]
1493934-no-null-append.patch (v1b)

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

Thanks.
Attachment #9011753 - Flags: review?(acelists) → review+
Sorry about that. Filed bug 1493977.
Keywords: crash
Crash Signature: [@ strlen | nsTSubstring<T>::Append ]
Crash Signature: [@ strlen | nsTSubstring<T>::Append ] → [@ strlen | nsTSubstring<T>::Append ] [@ ucrtbase.dll@0x20431 ] [@ ucrtbase.dll@0x20451 ] [@ ucrtbase.dll@0x46f30 ]
Since I retriggered the Dailies, this fault was out in the wild for about three hours. Funny it created four different crash signatures all at comm/mailnews/local/src/nsMailboxUrl.cpp:162.
Make that five :-(
Crash Signature: [@ strlen | nsTSubstring<T>::Append ] [@ ucrtbase.dll@0x20431 ] [@ ucrtbase.dll@0x20451 ] [@ ucrtbase.dll@0x46f30 ] → [@ strlen | nsTSubstring<T>::Append ] [@ ucrtbase.dll@0x20431 ] [@ ucrtbase.dll@0x20451 ] [@ ucrtbase.dll@0x46f30 ] [@ libc-2.27.so@0xb1646 ]
You need to log in before you can comment on or make changes to this bug.