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

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

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

Comment 1

[Jorg K (CEST = GMT+2)]

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?

Comment 2

[Jorg K (CEST = GMT+2)]

Sorry, I saw it.

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Jorg K (CEST = GMT+2)]

Now I saw it. What we try to append is null and that now crashes :-(

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1493934-no-null-append.patch

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 :-(

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1493934-no-null-append.patch (v1b)

Oops, I didn't want the nsDependentCString().

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f600e1fb24b3
Don't Append(null) in nsMailboxUrl::GetNormalizedSpec(). rs=bustage-fix DONTBUILD

Comment 8

[:aceman]

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?

Comment 9

[Jorg K (CEST = GMT+2)]

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 10

[:aceman]

Comment on attachment 9011753 [details] [diff] [review]
1493934-no-null-append.patch (v1b)

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

Thanks.

Comment 11

[Henri Sivonen (:hsivonen)]

Sorry about that. Filed bug 1493977.

Comment 12

[Jorg K (CEST = GMT+2)]

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.

Comment 13

[Jorg K (CEST = GMT+2)]

Make that five :-(