Closed Bug 1533320 Opened 6 years ago Closed 6 years ago

During local debug build of TB 64-bit I get the assertion failure: ASSERTION: uninitialized: 'mHost.mLen >= 0', file ../mozilla/netwerk/base/nsStandardURL.cpp, line 1603

Categories

(Thunderbird :: General, defect)

Product:
Thunderbird
Component:
General
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 67.0

People

(Reporter: ishikawa, Assigned: jorgk-bmo)

References

Details

Attachments

(2 files, 2 obsolete files)

log of the test with symbolic stacktrace
7.24 KB, text/plain
Details
1533320-CreateURL.patch (v3)
5.67 KB, patch
valentin
: review+
Details | Diff | Splinter Review

With the source code updated about two days ago,
I get the following assertion error when I test the TB 64-bit versio binary with |make mozmill| test suite.

ASSERTION: uninitialized: 'mHost.mLen >= 0', file /NREF-COMM-CENTRAL/mozilla/netwerk/base/nsStandardURL.cpp, line 1603

This happens in the test of
TEST-START | /NREF-COMM-CENTRAL/mozilla/comm/mail/test/mozmill/attachment/test-attachment.js | test_attachment_name_click

What irks me is that the test succeeds as a whole:

TEST-PASS | /NREF-COMM-CENTRAL/mozilla/comm/mail/test/mozmill/attachment/test-attachment.js | test-attachment.js::test_attachment_list_expansion

[26486, Main Thread] ###!!! ASSERTION: uninitialized: 'mHost.mLen >= 0', file /NREF-COMM-CENTRAL/mozilla/netwerk/base/nsStandardURL.cpp, line 1603
#01: mozilla::net::nsStandardURL::SetUserPass(nsTSubstring<char> const&) (/NREF-COMM-CENTRAL/mozilla/netwerk/base/nsStandardURL.cpp:1603 (discriminator 1))
#02: mozilla::net::nsStandardURL::TemplatedMutator<mozilla::net::nsStandardURL>::SetUserPass(nsTSubstring<char> const&, nsIURIMutator**) (/NREF-COMM-CENTRAL/mozilla/netwerk/base/nsStandardURL.h:323)
#03: nsMsgMailNewsUrl::SetUserPass(nsTSubstring<char> const&) (/KERNEL-SRC/moz-obj-dir/objdir-tb3/dist/include/nsIURIMutator.h:489)
#04: nsMsgMailNewsUrl::Mutator::SetUserPass(nsTSubstring<char> const&, nsIURIMutator**) (/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/util/nsMsgMailNewsUrl.h:80)
#05: NS_InvokeByIndex (/NREF-COMM-CENTRAL/mozilla/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:110)

I'm not sure how this would happen. We check that the URL is initialized, but it's not clear how the URL gets to have no host.

Jorg, can you check if this is a fresh mutator, what is the current value of its URL, and what we're setting it to? Thanks!

status-firefox67: affected → ---
Component: Networking → General
Flags: needinfo?(jorgk)
Product: Core → Thunderbird

I'm not sure how this would happen. We check that the URL is initialized, but it's not clear how the URL gets to have no host.

Please recall that this is a local test done at home.
There COULD be a local DNS setup issue that might interfere with the ordinary |make mozmil| operation.

For example, I think I am using ".example.org" domain name for home local LAN, which should NOT resolve to any valid domain name outside in the open Internet. (YES, I KNOW THERE WERE dummy entries to catch such doain name usage.)
The mozilla test code MAY be using such a host domain name for internal testing purposes and the my local home DNS may
play a funny trick(?)
if
If I could run the test in the mozilla build/test farm, I can verify if it happens or not there.
Somehow I have not been able to submit build/test job successfully
since the great file directory layout change of last April timeframe.

TIA

Many MozMill tests are currently busted due to XBL issues, so I need to return to this after we have fixed that bustage.

(In reply to Jorg K (GMT+1) from comment #3)

Many MozMill tests are currently busted due to XBL issues, so I need to return to this after we have fixed that bustage.

Fair enough. Thank you for the work on XBL.

So I do not have to report any local errors seemingly related to XBL, I presume.

This error is not due to XBL, but I can't run the test now since it's mostly disabled due to XBL bustage:
https://hg.mozilla.org/comm-central/rev/67022d52f8ed7b8e270f29f2b774ce5ec236f753#l2.1

That said, test_attachment_name_click isn't disabled so let me get back to this.

OK, I did run the test now and I see:

[1728, Main Thread] ###!!! ASSERTION: uninitialized: 'mHost.mLen >= 0', file c:/mozilla-source/comm-central/netwerk/base/nsStandardURL.cpp, line 1603
#01: nsMsgMailNewsUrl::SetUserPass (c:\mozilla-source\comm-central\comm\mailnews\base\util\nsMsgMailNewsUrl.cpp:527)
#02: XPTC__InvokebyIndex[c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\bin\xul.dll +0x7e6f742]

So some JS uses someURL.userPass = xxx, hmm, but I can't see that.

The failing code is just return NS_MutateURI(m_baseURL).SetUserPass(aUserPass).Finalize(m_baseURL);. Dumping out the spec I get:
mailbox:///C:/Users/jorgk/AppData/Local/Temp/mozmillprofile/Mail/Local%20Folders/AttachmentA?number=2&part=1.2&filename=ubik.txt&type=text/plain&filename=ubik.txt
so it looks kosher, although M-C isn't happy with some of our URLs, especially the mailbox ones ;-)

So Valentin, back to Core::Networking?

Flags: needinfo?(jorgk) → needinfo?(valentin.gosu)

The question is: should nsMsgMailNewsUrl have a user & password? If the answer is no, then the problem is how we instantiate m_baseURL. NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID) creates an nsStandardURL with the default type URLTYPE_STANDARD - which must have a hostname. If nsMsgMailNewsUrl is more like a file URL, then it should be initialized like this

Flags: needinfo?(valentin.gosu)

nsMsgMailNewsUrl can be lots of things. There are six classes derived from it, see:
https://searchfox.org/comm-central/search?q=public+nsMsgMailNewsUrl&case=false&regexp=false&path=

Yes, the mailbox: one is more like a file, but the other ones have host names. I caught it in the debugger, this is the JS stack:

0 initIntro(url = [xpconnect wrapped (nsISupports, nsIURI, nsIURL) @ 0x1d8526f5dc0 (native @ 0x1d856164e20)], filename = "ubik.txt", displayname = "ubik.txt") ["resource://gre/modules/HelperAppDlg.jsm":571:24]
    this = [object Object]
1 initDialog() ["resource://gre/modules/HelperAppDlg.jsm":442:9]
    this = [object Object]
2 onload(event = [object Event]) ["chrome://mozapps/content/downloads/unknownContentType.xul":1:7]
    this = [object ChromeWindow]
3 waitFor(callback = [function], message = "Timeout waiting for modal dialog to open.", timeout = 10000, interval = 100, thisObject = [object Object]) ["chrome://mozmill/content/modules/utils.jsm":391:11]
    this = [object Object]
4 WindowWatcher_waitForModalDialog(aWindowType = "unknownContentType", aTimeout = undefined) ["file:///c:/mozilla-source/comm-central/comm/mail/test/mozmill/shared-modules/test-window-helpers.js":379:10]
    this = [object Object]
5 wait_for_modal_dialog(aWindowType = "unknownContentType") ["file:///c:/mozilla-source/comm-central/comm/mail/test/mozmill/shared-modules/test-window-helpers.js":612:16]
6 test_attachment_name_click() ["file:///c:/mozilla-source/comm-central/comm/mail/test/mozmill/attachment/test-attachment.js":244:2]

So here:
https://searchfox.org/comm-central/rev/dbc2627276adff44df57ec3c20f63e6fa463287f/mozilla/toolkit/mozapps/downloads/HelperAppDlg.jsm#571

Looks like the test clicks onto some attachment with unknown(?) type ".txt" and some helper is meant to show up which for some reason sets the user password to "".

So where from here?

(In reply to Jorg K (GMT+1) from comment #8)

nsMsgMailNewsUrl can be lots of things. There are six classes derived from it, see:
https://searchfox.org/comm-central/search?q=public+nsMsgMailNewsUrl&case=false&regexp=false&path=

Yes, the mailbox: one is more like a file, but the other ones have host names. I caught it in the debugger, this is the JS stack:

nsMailboxUrl::SetSpecInternal should not simply delegate to nsMsgMailNewsUrl::SetSpecInternal, but should initialize m_baseURL to what makes sense.

I took a look at the other implementations, but I don't know how the URLs should look like, so I can't tell if they are correct or not. Do we have documentation for what each scheme is used for?

Documentation, not that I know of ;-(

You already know the mailbox: URL. IMAP, NNTP, POP3, SMTP and the JA thing certainly have a hostname, also with a user name, like: imap://tbtest%40jorgk%2Ecom@mail.your-server.de:143/fetch%3EUID%3E.INBOX.AAA%3E2.

So the only one causing trouble is the mailbox: URL.

So where from here? Change nsMailboxUrl::SetSpecInternal() to use a file-style URL?

Flags: needinfo?(valentin.gosu)

Yes. Something like this:

  return NS_MutateURI(new nsStandardURL::Mutator())
      .Apply(NS_MutatorMethod(&nsIFileURLMutator::MarkFileURL))
      .Apply(NS_MutatorMethod(&nsIStandardURLMutator::Init,
                              nsIStandardURL::URLTYPE_NO_AUTHORITY, -1, buf,
                              charset, base, nullptr))
      .Finalize(result);

You can skip the MarkFileURL if nsMailboxUrl doesn't also implement nsIFileURL

Flags: needinfo?(valentin.gosu)

Umm, now I'm confused. Where do want to put that code?

That's not to replace the nsresult rv = nsMsgMailNewsUrl::SetSpecInternal(aSpec); surely since the return would skip the later processing in nsMailboxUrl::SetSpecInternal():
https://searchfox.org/comm-central/rev/cb674605dabf543dfb315457479b1e8186e2120c/mailnews/local/src/nsMailboxUrl.cpp#418
Also, your code doesn't set a spec.

Is we don't call into nsMsgMailNewsUrl::SetSpecInternal(), we don't get this processing:
https://searchfox.org/comm-central/rev/cb674605dabf543dfb315457479b1e8186e2120c/mailnews/base/util/nsMsgMailNewsUrl.cpp#468

Flags: needinfo?(valentin.gosu)

I think the best way is to declare a virtual
already_AddReffed<nsIURI> CreateURI(nsACString& spec); on nsMsgMailNewsUrl. The nsMsgMailNewsUrl implementation will simply create a URI via NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID).SetSpec(aSpec).Finalize(m_baseURL);. We'll overwrite it in nsMsgMailNewsUrl to create the URI using nsIStandardURLMutator::Init.

And of course, we call CreateURI in nsMsgMailNewsUrl::SetSpecInternal

Flags: needinfo?(valentin.gosu)
Attached patch 1533320-CreateURL.patch (obsolete) — — Splinter Review

OK, no more assertion when running
mozmake mozmill-one SOLO_TEST=attachment/test-attachment.js

Let's see whether the rest of the system likes this:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f8ed04bd95354eee1d4719a29ae14d840adab2c7

Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #9051398 - Flags: review?(valentin.gosu)
Comment on attachment 9051398 [details] [diff] [review] 1533320-CreateURL.patch Not ready for prime-time yet :-( TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-image-display.js | test-image-display.js::test_cid_image_compose TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_mailboxProtocol.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_mailboxProtocol.js | run_test - [run_test : 40] "mailbox:///" == "mailbox://user@localhost/" TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3PasswordFailure3.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3PasswordFailure3.js | endTest - [endTest : 162] 0 == 1 TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3PasswordFailure.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3PasswordFailure.js | - 0 == 1 TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3PasswordFailure2.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3PasswordFailure2.js | endTest - [endTest : 160] 0 == 1 There is even a variation with "mailbox://user@localhost/", hmm.
Attachment #9051398 - Flags: review?(valentin.gosu)

I should have known better, those URLs can also have the form mailbox://user@domain@server/folder?number=nn :-(

Attached patch 1533320-CreateURL.patch (v2) (obsolete) — — Splinter Review

OK, this fixes the failing tests, I ran them all manually.

Attachment #9051398 - Attachment is obsolete: true
Attachment #9051418 - Flags: review?(valentin.gosu)
Comment on attachment 9051489 [details] [diff] [review] 1533320-CreateURL.patch (v3) Review of attachment 9051489 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Make sure we have a test for this.
Attachment #9051489 - Flags: review?(valentin.gosu) → review+

Thanks Valentin, what would to like to test (and how)?

Target Milestone: --- → Thunderbird 67.0

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/583bb771fcb6
introduce nsMsgMailNewsUrl::CreateURL() and override it for mailbox: URLs. r=valentin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED

(In reply to Jorg K (GMT+1) from comment #21)

Thanks Valentin, what would to like to test (and how)?

A simple xpcshell-test should suffice. Similar to netwerk/test/unit/test_URIs.js or netwerk/test/unit/test_standardurl.js
Use Services.io.newURI to create a few URIs of each type, and make sure that setting the username for mailbox URIs throws without triggering the assert.

Blocks: 1535964

Thanks Valentin, I filed bug 1535964.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: