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

RESOLVED FIXED in Thunderbird 67.0

Status

defect
RESOLVED FIXED
3 months ago
a month ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 67.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Updated

3 months ago
Keywords: leave-open

Comment 1

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/43767631d146
No bug - Disable Nightly builds . a=jorgk DONTBUILD
Assignee

Comment 2

3 months ago

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

That would take down a lot of tests :-(

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.

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.

Assignee

Comment 5

3 months ago

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

Flags: needinfo?(valentin.gosu)
Assignee

Updated

3 months ago
Blocks: 1517025

(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://username%40example@foobar.com/.... yes, are there other cases? That's not really in the host name.

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?

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

Comment 9

3 months ago

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 @.

Flags: needinfo?(jorgk)

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)

Assignee

Comment 11

3 months ago

But % is still allowed in the username part?

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

Assignee

Comment 13

3 months ago

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?

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)

Assignee

Comment 15

3 months ago

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);

Assignee

Comment 16

3 months ago

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

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.

Assignee

Comment 18

3 months ago
Posted patch 1527236-url-no-percent.patch (obsolete) — Splinter Review

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

Assignee: nobody → jorgk
Assignee

Comment 19

3 months ago

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?

Flags: needinfo?(benc)
Assignee

Comment 20

3 months ago
Attachment #9043270 - Attachment is obsolete: true
Assignee

Comment 21

3 months ago

Carrying forward Valentin's r+ from Phabricator.

Attachment #9043240 - Attachment is obsolete: true
Attachment #9043344 - Flags: review+
Assignee

Updated

3 months ago
Attachment #9043344 - Attachment description: 1527236-allow-percent.patch → Patch for M-C: 1527236-allow-percent.patch
Assignee

Updated

3 months ago
Keywords: checkin-needed

Comment 22

3 months ago

Pushed by dvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa39bdeb511f
Allow % in URLs for Thunderbird. r=valentin

Keywords: checkin-needed
Assignee

Updated

3 months ago
Blocks: 1527462

Comment 23

3 months ago

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.

Flags: needinfo?(benc)
Assignee

Comment 24

3 months ago

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 26

3 months ago
Pushed by mozilla@jorgk.com:
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

3 months ago

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...

Assignee

Comment 28

a month ago

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

Status: NEW → RESOLVED
Last Resolved: a month ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0
You need to log in before you can comment on or make changes to this bug.