1527236 - Massive Mozmill/Xpcshell/Mochitest failure 2019-02-12

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

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

Comment 1

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/43767631d146
No bug - Disable Nightly builds . a=jorgk DONTBUILD

Comment 2

[Jorg K (CEST = GMT+2)]

The MozMill log says:
EXCEPTION: Component returned failure code: 0x80550016 [nsIMsgFolder.createSubfolder]

That would take down a lot of tests :-(

Comment 3

[Geoff Lankow (:darktrojan)]

I'm extremely suspicious of bug 1517025. We do use % signs in the hostname part of our server URLs, and I think the error is thrown here.

Comment 4

[Geoff Lankow (:darktrojan)]

Yes, backing out https://hg.mozilla.org/mozilla-central/rev/2b799d86aeff fixes the tests. That said, I don't think I'm the right person to fix this, so over to you, … somebody.

Comment 5

[Jorg K (CEST = GMT+2)]

Valentin, what can we do here. Looks like disallowing % throws TB off the rails.

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #3)

We do use % signs in the hostname part of our server URLs, and I think the error is thrown

Well as imap://[e-mail]/.... yes, are there other cases? That's not really in the host name.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

I would hate to back this out because of thunderbird... As far as I can tell, we have the option of making the code preffable, but that would cause a small runtime perf issue. Would an #ifdef work?

Comment 8

[Jorg K (CEST = GMT+2)]

Try with M-C rev backed out:

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

Comment 9

[Jorg K (CEST = GMT+2)]

Sure and #ifdef would work, but then your new test will all fail unless you disable them, too.

I'm still not sure whether the M-C change is right. I'd have to look on which URL it's failing, but as Magnus already said, the % doesn't appear to be in the hostname, the stuff after the @.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Let me know if I can help. This patch was an important step towards being more spec-compliant, so I really hope it sticks.
If TB uses non-standard URLs, then we should try to fix that, or move it towards a separate URL parser (at least for the non-standard ones)

Comment 11

[Jorg K (CEST = GMT+2)]

But % is still allowed in the username part?

Comment 12

[Valentin Gosu [:valentin] (he/him)]

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

But % is still allowed in the username part?

Yes, it is, as invalid characters are percent encoded.

Comment 13

[Jorg K (CEST = GMT+2)]

OK, running an "easy" Xpcshell test, I get:

0:01.45 pid:8556 [8556, Main Thread] WARNING: NS_ENSURE_SUCCESS(mStatus, mStatus) failed with result 0x804B000A: file c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsIURIMutator.h, line 610
0:01.45 pid:8556 [8556, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file c:/mozilla-source/comm-central/comm/mailnews/base/util/nsMsgDBFolder.cpp, line 3112

The code is:
rv = NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID).SetSpec(mURI).Finalize(url);
So we're setting the spec.

Adding a bit of debug there shows:
=== failed on URL: |mailbox://nobody@Local%20Folders|

Ouch :-(

That's basically a file system folder name in those mailbox: URLs. The other variation of mailbox: URLs is this for example:
mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/
kp2r4jae.Testing%202/Mail/Local%20Folders/June%202018?number=16
which is a "real" file name percent encoded.

Hmm, Valentin, any idea how to work around this? Would the second one be interpreted as a server name, too?

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Ah, ok... the issue is that nsStandardURL should only be used for http/https/ftp/file/ws/ws (There might be a couple others, but that's about it).
All other types should use NS_SIMPLEURIMUTATOR_CONTRACTID or better yet go through the protocol handler nsIOService.newURI(spec)

Comment 15

[Jorg K (CEST = GMT+2)]

OK, but we're using an nsIURL here, not nsIURI:

nsCOMPtr<nsIURL> url;
rv = NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID).SetSpec(mURI).Finalize(url);
if (NS_FAILED(rv)) printf("=== failed on URL: |%s|\n", mURI.get());
NS_ENSURE_SUCCESS(rv, rv);

Comment 16

[Jorg K (CEST = GMT+2)]

Anyway, changing this to rv = NS_MutateURI(NS_SIMPLEURIMUTATOR_CONTRACTID).SetSpec(mURI).Finalize(url); fails just the same.

Comment 17

[Valentin Gosu [:valentin] (he/him)]

The solution would be to use nsIOService.newURI - you get back an nsIURI. Then you QI to nsIURL and use GetFilename, otherwise it's an nsIURI and all we have is GetFilePath.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1527236-url-no-percent.patch

Thanks for the suggestion, but this fails just the same:

0:01.31 pid:14560 === failed on URL: |mailbox://nobody@Local%20Folders|
0:01.31 pid:14560 [14560, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file c:/mozilla-source/comm-central/comm/mailnews/base/util/nsMsgDBFolder.cpp, line 3117

Comment 19

[Jorg K (CEST = GMT+2)]

I don't have a good idea how to proceed here. I can see these options:

1. Do an #ifdef in M-C and switch off the tests which will fail as a consequence.
2. Rework the entire URLs for mailboxes, that's not a quick fix.
3. Hack to s/%/$/ after escaping the folder names. That might fly, but I found[1] which is still using RDF.

[1] https://searchfox.org/comm-central/rev/9bc82e1a0ba1abd185447d1f645fcfd8408e7950/mailnews/base/util/nsMsgDBFolder.cpp#3536.
No idea where RDF gets the URL from an whether this can be tweaked.

Ben, any ideas?

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: Bug 1527236 - Allow % in URLs for Thunderbird.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Patch for M-C: 1527236-allow-percent.patch

Carrying forward Valentin's r+ from Phabricator.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa39bdeb511f
Allow % in URLs for Thunderbird. r=valentin

Comment 23

[Ben Campbell]

I've run into something along these lines before.
At one point I tried switching to using NS_MutateURI() to normalise some folder URIs (on the grounds that it was the most "canonical" encoder I could find), but discovered that the encoding used by most of the folder code had slightly different rules.
Most of the folder URI code is pretty ad-hoc - lots of string appends and searching backwards for '/' etc...
From memory, the root URI is set up by nsMsgIncomingServer::GetServerURI(), but I can't remember the details of how each extra component is encoded (if at all).
Having a poke about now I'll leave notes on Bug 1527462.

Comment 24

[Jorg K (CEST = GMT+2)]

Actually, no, this is the bug to work on, bug 1527462 will just be the backout. Or you can open a new bug for the Mailnews work and we close this one here for the temporary fix.

Comment 25

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

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c9979c174e0a
No bug - Backed out changeset 43767631d146 to re-enable Nightly builds after bug 1527236 and bug 1452600 got fixed. a=backout DONTBUILD

Comment 27

[Ben Campbell]

Forgot to post my thoughts on this one so here they are:

The base URI is set up by nsMsgIncomingServer::GetServerURI() which applies percent-encoding to the hostname.
However, strictly speaking, host names have a very restricted character set: a-z, 0-9 and hyphen, with dots to separate the components. Additionally, each component is restricted to 63 chars, with a 253-char limit overall.

https://en.wikipedia.org/wiki/Hostname#Restrictions_on_valid_hostnames

So TB doesn't correctly sanitise hostnames, and when M-C got strict about it we got tripped up.
Changing GetServerURI() to produce 'correct' URIs wouldn't be too hard.

But if we do that... then suddenly any place a URI is stored could potentially break things.
eg the virtualFolders.dat config file references other folders by URI.

I'm not sure how pervasive this issue is.

Off the top of my head I can see three solutions to this:

1. do nothing, rely on the TB special case in M-C forever.

2. have some code which checks for bad URIs and fixes them on startup or when the config files are read, say. (eg as we load virtualFolders.dat we clean up the URIs as we go)

3. leave bad URIs as they are in config files, but make sure any URI lookups clean up uris before lookup (eg maybe the folder-lookup-service "normalises" uris to their sanitised form).

It'd be nice to fix it. The choice between 2 & 3 seems to depend on how many config files (and extensions?) rely on storing/matching URIs...

Comment 28

[Jorg K (CEST = GMT+2)]

This got fixed by an M-C hack, see comment #22. Follow-up work in bug 1527462.