Closed Bug 1906992 Opened 4 months ago Closed 4 months ago

NS_ERROR_MALFORMED_URI on startup

Categories

(Thunderbird :: General, defect, P1)

Product:
Thunderbird
Component:
General
Version:
Thunderbird 130
Type:
defect

Tracking

(thunderbird_esr128 unaffected)

Status:
RESOLVED FIXED
Milestone:
130 Branch
Tracking Flags:
Tracking Status
thunderbird_esr128 --- unaffected

People

(Reporter: aleca, Assigned: babolivier)

References

(Regression)

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

WIP: Bug 1906992 - Preserve Thunderbird-specific feature from mozilla-central deps when regenerating gkrust's Cargo.toml.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1906992 - Special case pseudo-hosts with space for IDNA in mailnews builds. r=kershaw,john.bieling,darktrojan
48 bytes, text/x-phabricator-request
Details | Review
WIP: Bug 1906992 - Ensure URLs are properly normalized for local folders and smart mailboxes.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1906992 - Fix tests that use non-standard hostnames. r=#thunderbird-reviewers
48 bytes, text/x-phabricator-request
Details | Review
SetSpec() success cases recorded from local C-C mochitest in June 2020.
201.57 KB, text/plain
Details

Thunderbird is busted with an NS_ERROR_MALFORMED_URI on startup.

NI the author of the patch in bug 1889536 to see if there's something we need to do to port those changes.

Flags: needinfo?(hsivonen)
Severity: -- → S1
tracking-firefox129: --- ? → ? ---
tracking-firefox130: --- → ?
tracking-firefox130: ? → ---
Priority: -- → P1

What's the input to the IDN service that now gets rejected?

Flags: needinfo?(hsivonen)

Things to check:

  • Does the IsMailNews() check in nsStandardURL actually return true as expected?
  • Is the rejection via nsStandardURL or via another entry to IDNA code?

I haven't had much time to investigate, and I'm not currently in a position to do so, but

  • I don't actually remember where I saw the NS_ERROR_MALFORMED_URI, but definitely did when I first started digging.
  • There's something going wrong with a domain of Local%20Folders, I made NS_DomainToDisplayAndASCII return NS_OK for that domain only and things got better for the local account.
  • Other accounts are still broken, perhaps it's because their user part contains %, e.g. geoff%40thunderbird.net? That's not the domain, but I dunno what else the cause could be. A typical URL would be imap://geoff%40thunderbird.net@imap.gmail.com/INBOX.
  • Does the IsMailNews() check in nsStandardURL actually return true as expected?

I think so. Not sure if I tested it directly, but I hard-coded the return true and there was no change. I also tried hard-coding the MAILNEWS and GLYPHLESS deny lists in both places where they're used and nothing changed.

  • Is the rejection via nsStandardURL or via another entry to IDNA code?

I'm guessing nsStandardURL but can't be sure.

Just to be clear: Is Local%20Folders what gets passed to the IDNA code and not the percent-decoded Local Folders? The MAILNEWS ASCII deny list specifically is about allowing the percent sign. GLYPHLESS tolerates the percent also. (URL and GECKO deny the percent sign.) However, the deny lists URL, GECKO, MAILNEWS, and GLYPHLESS all deny the space and allowing the space overall would open up the opportunity for a variety of weird behavior and accidents given that 1) actual domains don't have spaces and 2) UTS 46 maps various things, at least certain standalone accents characters, that don't look like they contain a space into containing a space.

(In general, getting something like Local%20Folders, which clearly isn't a DNS host name or even a NetBIOS host name passed through code that's can do unwanted things to inputs that aren't really domain / host names is bad. Long term, it would be great to get mailnews to a point where it didn't pass non-domains through code that's about processing domains.)

Another question: Can mozilla-central first-party Rust code detect that it's being compiled as part of a comm-central compilation? How? I.e. can the IsMailNews check move from nsStandardURL to idna_glue and can idna_glue do more comm-central special-casing without requiring all mozilla-central callers to signal mailnewsness?

Okay, I've been able to dig a little deeper.

Yes, it's Local%20Folders.

  • Does the IsMailNews() check in nsStandardURL actually return true as expected?

Yes.

  • Is the rejection via nsStandardURL or via another entry to IDNA code?

Both.

I've discovered a place where we call ConvertToDisplayIDN with the same nsCString for input and output. This isn't working, but we can stop doing that to fix it. It doesn't solve everything, but I'll go looking for other things like it.

Ah, this call is failing with an unescaped hostname.

Henri, NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID).SetSpec (like we do here) doesn't work for URLs like mailbox://nobody@Local%20Folders. Should it? Is there something else we can do?

Flags: needinfo?(hsivonen)

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

Henri, NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID).SetSpec (like we do here) doesn't work for URLs like mailbox://nobody@Local%20Folders. Should it?

That's surprising. I don't know about the finer points of NS_MutateURI.

Is there something else we can do?

It seems to me the normal thing to do would be NS_NewURI(getter_AddRefs(url), mURI, UTF_8_ENCODING);. It's not immediately clear to me why that code is doing something more complex in the first place. Is the purpose that the caller is forcing the type of URL via the contract ID instead of letting NS_NewURI pick the right class? If so, perhaps the mismatch with what NS_NewURI would do is the problem?

Flags: needinfo?(hsivonen)

I suspect it was put there by somebody under pressure solve a broken build and who, like I just did, forgot that NS_NewURI exists.

However, NS_NewURI doesn't work either, it also returns NS_ERROR_MALFORMED_URI.

Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/b5a33aebffff Disable the Daily build while all the tests are failing. rs=me CLOSED TREE
Attached file WIP: Bug 1906992 (obsolete) —
status-thunderbird_esr128: --- → unaffected
Version: unspecified → Thunderbird 130

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

However, NS_NewURI doesn't work either, it also returns NS_ERROR_MALFORMED_URI.

What idna_glue entry point does a debugger show and with what input?

What happens if you remove @ from the list of characters in https://searchfox.org/mozilla-central/source/netwerk/base/idna_glue/src/lib.rs#23 (the MAILNEWS deny list)?

(In reply to Henri Sivonen (:hsivonen) from comment #12)

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

However, NS_NewURI doesn't work either, it also returns NS_ERROR_MALFORMED_URI.

What idna_glue entry point does a debugger show and with what input?

What happens if you remove @ from the list of characters in https://searchfox.org/mozilla-central/source/netwerk/base/idna_glue/src/lib.rs#23 (the MAILNEWS deny list)?

There are many crashing bugs at this moment.
Even though I removed "@" from the list, the xpcshell test I tested in particular won't run due to the
MOZ_CRASH error mentioned in Bug 1907113

See Also: → 1907113, 1906529

I've been looking into this bug in Geoff's absence.

As Geoff mentioned, the issue with ConvertToDisplayIDN is that we were using the same string as both source and destination. However, I can confirm that URLs looking like mailbox://nobody@Local%20Folders cause NS_ERROR_MALFORMED_URI errors.

(In reply to Henri Sivonen (:hsivonen) from comment #12)

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

However, NS_NewURI doesn't work either, it also returns NS_ERROR_MALFORMED_URI.

What idna_glue entry point does a debugger show and with what input?

What happens if you remove @ from the list of characters in https://searchfox.org/mozilla-central/source/netwerk/base/idna_glue/src/lib.rs#23 (the MAILNEWS deny list)?

After spending some time fiddling with gdb, it looks like it's coming from mozilla_net_domain_to_display_and_ascii_impl (via nsStandardURL::NormalizeIDN).

Removing @ from the MAILNEWS deny list does not fix the issue.

Duplicate of this bug: 1906529
Duplicate of this bug: 1907113
No longer duplicate of this bug: 1907113

I have removed a MOZ_CRASH I see with a patch in
https://phabricator.services.mozilla.com/D216180

But obviously there are other issues.

Right, I think I got to the bottom of this. In mozilla_net_domain_to_display_and_ascii_impl, we percent_decode the domain: https://searchfox.org/mozilla-central/rev/0b1d02b2cb5736511139cf0e40b318273e825899/netwerk/base/idna_glue/src/lib.rs#106

This means Local%20Folders becomes Local Folders, which then fails the validation because the deny list has deny_glyphless set to true: https://searchfox.org/mozilla-central/rev/0b1d02b2cb5736511139cf0e40b318273e825899/netwerk/base/idna_glue/src/lib.rs#23

I don't think simply turning that boolean to false is a good solution since it means allowing spaces in domain names, which looks like a good way to create even more problems (since there seems to be a lot of expectations in both mozilla-central and comm-central that domains can't include spaces). So I think we might want to figure out a way to specifically special-case %20 when doing percent-decoding.

Worth noting: somewhere else I found that we try to create URLs for IMAP folders after normalizing them, e.g. imap://Local Folders here: https://searchfox.org/comm-central/rev/ba069c614fc2385c66992cc9c1ebb5998125b135/mailnews/base/src/nsMsgAccountManager.cpp#1850-1877 - the call to ConvertToDisplayIDN ends up calling mozilla_net_domain_to_display_impl, according to gdb.

This means that the URL mutation ends up failing here: https://searchfox.org/mozilla-central/rev/0b1d02b2cb5736511139cf0e40b318273e825899/netwerk/base/nsURLParsers.cpp#537-539

Considering this check in nsAuthURLParser::ParseServerInfo is not new, I believe the percent-decoding that's now done on the %20s in the hostname was added by bug 1889536 - which it looks like brings us back to decoding %20 causing the breakage in Thunderbird we're seeing today.

Henri, I'm not sure I have all the context needed to figure out a fix that's right for both Firefox and Thunderbird (at least one that doesn't involve a massive rework of how we refer to folders in TB), probably because I'm not familiar enough with ICU4X. Any idea?

Flags: needinfo?(hsivonen)

You're aware of bug 1527462 which predicted major bustage? Bug 1527462 comment #5 mentions supporting a mailnews: scheme which TB doesn't actually use.

BTW, why did you dupe bug 1906529 here? That was regressed by bug 1869685 a while ago. According to comment #6, the current failure is triggered at
https://searchfox.org/comm-central/rev/ba069c614fc2385c66992cc9c1ebb5998125b135/mailnews/base/src/nsMsgAccountManager.cpp#1853
whereas the output which is subject to bug 1906529 is from a few lines later:
https://searchfox.org/comm-central/rev/ba069c614fc2385c66992cc9c1ebb5998125b135/mailnews/base/src/nsMsgAccountManager.cpp#1879

I think if we can handle " " or "%20" in a manner that Thunderbird wants, the problem would go away.
Other characters do not seem to be causing problems.
I say this due to some statistical data from test log.

Below is the snippet from a local mochitest log summary (created by my local script)
from a run on June 25. (The latest one today is not complete due to the issue discussedhere and so I picked up an old log.)

Several years ago, I noticed a strange SetSpec failure and started recording the failing URIs.
Obviously some (JavaScript?) routines pass bogus URIs as well as otherwise valid URIs that contain a problematic
" " character in host part.
The number at the beginning of each line is the frequency of occurrences during mochitest run of FULL DEBUG version of C-C TB.
The list is sorted by the frequency of occurrences.

   2154 SetSpec failed. : aSpec=imap://Local Folders
   2009 SetSpec failed. : aSpec=toolkit/global/arrowscrollbox.ftl
   1663 SetSpec failed. : aSpec=toolkit/global/textActions.ftl
   1276 SetSpec failed. : aSpec=toolkit/main-window/findbar.ftl
   1276 SetSpec failed. : aSpec=messenger/messenger.ftl
   1156 SetSpec failed. : aSpec=messenger/openpgp/openpgp.ftl
   1156 SetSpec failed. : aSpec=messenger/openpgp/openpgp-frontend.ftl
   1156 SetSpec failed. : aSpec=messenger/openpgp/msgReadStatus.ftl
   1156 SetSpec failed. : aSpec=messenger/messageheader/headerFields.ftl
   1156 SetSpec failed. : aSpec=calendar/calendar-invitation-panel.ftl
    734 SetSpec failed. : aSpec=messenger/preferences/preferences.ftl
    516 SetSpec failed. : aSpec=messenger/treeView.ftl
    501 SetSpec failed. : aSpec=branding/brand.ftl
    402 SetSpec failed. : aSpec=messenger/addressbook/vcard.ftl
    402 SetSpec failed. : aSpec=messenger/addressbook/aboutAddressBook.ftl
    396 SetSpec failed. : aSpec=messenger/appmenu.ftl
    338 SetSpec failed. : aSpec=toolkit/updates/history.ftl
    338 SetSpec failed. : aSpec=toolkit/about/config.ftl
    338 SetSpec failed. : aSpec=security/certificates/deviceManager.ftl
    338 SetSpec failed. : aSpec=security/certificates/certManager.ftl
    338 SetSpec failed. : aSpec=messenger/syncAccounts.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/system-integration.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/sync-dialog.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/receipts.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/permissions.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/passwordManager.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/offline.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/notifications.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/new-tag.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/languages.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/fonts.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/dock-options.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/cookies.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/connection.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/colors.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/attachment-reminder.ftl
    338 SetSpec failed. : aSpec=messenger/aboutDialog.ftl
    338 SetSpec failed. : aSpec=calendar/preferences.ftl
    338 SetSpec failed. : aSpec=calendar/category-dialog.ftl
    250 SetSpec failed. : aSpec=imap://Reply Identity Testing
    145 SetSpec failed. : aSpec=imap://smart mailboxes
    120 SetSpec failed. : aSpec=messenger/about3Pane.ftl
     73 SetSpec failed. : aSpec=toolkit/global/mozMessageBar.ftl
     67 SetSpec failed. : aSpec=messenger/otr/chat.ftl
     48 SetSpec failed. : aSpec=toolkit/neterror/netError.ftl
     48 SetSpec failed. : aSpec=toolkit/neterror/certError.ftl
     48 SetSpec failed. : aSpec=toolkit/global/mozSupportLink.ftl
     48 SetSpec failed. : aSpec=toolkit/global/mozFiveStar.ftl
     48 SetSpec failed. : aSpec=toolkit/about/aboutAddons.ftl
     48 SetSpec failed. : aSpec=messenger/extensionPermissions.ftl
     48 SetSpec failed. : aSpec=messenger/aboutAddonsExtra.ftl
     48 SetSpec failed. : aSpec=imap://Test Local Folders
     36 SetSpec failed. : aSpec=toolkit/global/resetProfile.ftl
     36 SetSpec failed. : aSpec=toolkit/global/processTypes.ftl
     36 SetSpec failed. : aSpec=toolkit/about/aboutSupport.ftl
     36 SetSpec failed. : aSpec=messenger/aboutSupportMail.ftl
     36 SetSpec failed. : aSpec=messenger/aboutSupportChat.ftl
     36 SetSpec failed. : aSpec=messenger/aboutSupportCalendar.ftl
     26 SetSpec failed. : aSpec=toolkit/global/notification.ftl
     22 SetSpec failed. : aSpec=messenger/aboutImport.ftl
     19 SetSpec failed. : aSpec=imap://Redirect Addresses Testing
     19 SetSpec failed. : aSpec=imap://BCC Reply Testing
     17 SetSpec failed. : aSpec=moz-icon://https://www.mozilla.org/?size=16&contentType=
     12 SetSpec failed. : aSpec=undefined
     12 SetSpec failed. : aSpec=DTD/xhtml1-strict.dtd
      8 SetSpec failed. : aSpec=calendar/calendar-widgets.ftl
      6 SetSpec failed. : aSpec=messenger/accountManager.ftl
      4 SetSpec failed. : aSpec=imap://New Msg Compose Identity Testing
      4 SetSpec failed. : aSpec=imap://Draft Identity Testing
      3 SetSpec failed. : aSpec=toolkit/about/aboutProfiles.ftl
      3 SetSpec failed. : aSpec=
      2 SetSpec failed. : aSpec=notarealaddress
      2 SetSpec failed. : aSpec=moz-icon://chrome://calendar/content/sound.wav?size=16
      2 SetSpec failed. : aSpec=moz-icon://
      1 SetSpec failed. : aSpec=null

The only valid-looking URIs prefixed by protocol (imap:) are as follows: They all contain problematic " ", but otherwise they look OK.

   2154 SetSpec failed. : aSpec=imap://Local Folders
    250 SetSpec failed. : aSpec=imap://Reply Identity Testing
    145 SetSpec failed. : aSpec=imap://smart mailboxes
     48 SetSpec failed. : aSpec=imap://Test Local Folders
     19 SetSpec failed. : aSpec=imap://Redirect Addresses Testing
     19 SetSpec failed. : aSpec=imap://BCC Reply Testing
      4 SetSpec failed. : aSpec=imap://New Msg Compose Identity Testing
      4 SetSpec failed. : aSpec=imap://Draft Identity Testing

Those prefixed with "moz-icon://" are screwed up URIs, it seems. They miss complete host part or have another protocol appended in the host part.

     17 SetSpec failed. : aSpec=moz-icon://https://www.mozilla.org/?size=16&contentType=
      2 SetSpec failed. : aSpec=moz-icon://chrome://calendar/content/sound.wav?size=16
      2 SetSpec failed. : aSpec=moz-icon://

xpcshell test on July 4 also shows something similar. A space character " " seems to be the culprit.
Oh, there is a URI(?) that starts with document.getElementByID(). This may need an attention, but it is not prefixed with a protocol.

   3103 SetSpec failed. : aSpec=imap://Local Folders
    572 SetSpec failed. : aSpec=imap://Local Folder
    201 SetSpec failed. : aSpec=imap://smart mailboxes
      5 SetSpec failed. : aSpec=imap://Smart Mailboxes
      3 SetSpec failed. : aSpec=document.getElementById(
      1 SetSpec failed. : aSpec=imap://Local Folders-1

So my guess is handling " " satisfactorily would solve the issue.
But how?
I am afraid we need a bandage solution like passing an extra argument to tell the underlying routine that the call is from TB or FF, etc. until a long-term solution is implemented.
Ugly. But I can't think of an easy to implement short term solution.
Just my two cents worth.

PS: The error dump was done inside netwerk/base/nsIURIMutator.idl and in NS_MutateURI& SetSpec(const nsACString& aSpec)
when SetSpec failed.
https://searchfox.org/mozilla-central/source/netwerk/base/nsIURIMutator.idl#341

PPS: In the long run, we need to eradicate the code that pass bogus URIs to SetSpec for the sake of efficiency. I think we are wasting plenty of CPU cycles for the bogus URI handling.

In today's failing local mochitest log (after my patch in Bug https://bugzilla.mozilla.org/show_bug.cgi?id=1907113 to avoid the MOZ_CRASH issue solely),
I notice that the following URIs now are reported as failing. This is the problem that causes the
startup failure, and again %20, i.e., " " need to be handled in a satisfactory manner.

    482 SetSpec failed. : aSpec=mailbox://nobody@Local%20Folders
     65 SetSpec failed. : aSpec=mailbox://nobody@Local%20Folders/Drafts
     61 SetSpec failed. : aSpec=mailbox://nobody@Local%20Folders/Sent
     59 SetSpec failed. : aSpec=mailbox://nobody@Local%20Folders/Templates
      7 SetSpec failed. : aSpec=mailbox://nobody@Local%20Folders/Unsent%20Messages

xpcshell test today also shows additional mailbox URIs failing. They all have '%20' in them.

   5153 SetSpec failed. : aSpec=mailbox://nobody@Local%20Folders
     50 SetSpec failed. : aSpec=mailbox://nobody@Local%20Folders/Sent
     50 SetSpec failed. : aSpec=mailbox://nobody@Local%20Folders/Drafts
     25 SetSpec failed. : aSpec=mailbox://nobody@Local%20Folders/Templates
     20 SetSpec failed. : aSpec=mailbox://nobody@smart%20mailboxes
      2 SetSpec failed. : aSpec=mailbox://nobody@Local%20Folders/folder?number=2
      1 SetSpec failed. : aSpec=mailbox://%7Bd6cfc7a2-3d69-42b5-bd85-d21766d24f30%7D@Local%20Folder
      1 SetSpec failed. : aSpec=mailbox://%7Ba61e68cb-a825-4729-8d25-9cd56640de22%7D@Local%20Folder
      1 SetSpec failed. : aSpec=mailbox://%7B92317bd8-11fa-4626-b5d3-9af8e98a4b0d%7D@Local%20Folder
      1 SetSpec failed. : aSpec=mailbox://%7B02a37e48-b1d8-4c3c-b4c2-32f240f49fc7%7D@Local%20Folder

So as far as C-C TB xpcshell test and mochitest are concerned, the handling of " " and/or '%20' are the key to the solution.

So as far as C-C TB xpcshell test and mochitest are concerned, the handling of " " and/or '%20' are the key to the solution.

We are aware of this. Thank you.

My previous understanding of bug 1527462 was that Thunderbird was doing double percent escaping. Since that's apparently not the case, the quick hack would be to allow the space in the MAILNEWS ASCII deny list. However, then all the cases that don't go through parsing would still break, and you'd need a mailnews-specific space carveout for what now uses GLYPHLESS. (This is why I asked earlier if there's a way for m-c Rust code to detect that it's being compiled as part of c-c.)

In general, letting things that are not actually domain names go through the processing that's meant for domain names is badness, and not erroring out on spaces in supposedly domain names is particularly likely to open things up for accidents, I think the ASCII deny lists in effect for m-c should not start allowing the ASCII space.

(As for using the same XPCOM string instance as both the input and the output, I asked an XPCOM expert if the code I was writing needed to support that case and was told not. Apparently c-c is using the IDN service in a way that's against previously unenforced XPCOM rules.)

Flags: needinfo?(hsivonen)

We're seriously considering changing the hostname of the two problematic accounts to only use characters that are allowed.

Pros:

  • It would allow us to remove the hacks that were created to allow what we currently have.
  • It would prevent potential misuse of those hacks on URLs they were not intended for. I don't think this is an actual problem that exists though.
  • It's early in the ESR cycle. It's also early in the 130 cycle. There is time to catch edge cases.

Cons:

  • The Local Folders account has been around like this since the 1990s, and the URL has been the definitive way to refer to a folder for just as long. There's potential for it to be recorded in all sorts of places. We might need to add some "just in case" code in important functions.
  • We'd have to migrate every existing profile, very early in the start-up sequence.
  • This migration wouldn't be backwards-compatible.
  • Getting rid of URLs for folders altogether is on the medium-term plan anyway, as part of a much larger bit of work. If we make this change now there's going to be three (instead of two) possible states a profile can be in, and that makes things even more complicated.

I think we probably are going to change the hostnames. Investigating that solution is quite advanced. However I'd like to get things back up and running ASAP with a simple temporary fix, even if it is a bit of a hack. That would take the time pressure off such a significant change.

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

We're seriously considering changing the hostname of the two problematic accounts to only use characters that are allowed.

If there are two problematic fixed strings, those could be special-cased in idna_glue. However, for performance reasons, I'd like to scope such a hack only to the case where idna_glue is being compiled as part of c-c, so that m-c-only builds don't spend time comparing every domain for "Local Folders".

(I'm assuming above that "Local Folders" is a fixed token and not localized or user-editable.)

Yeah, "Local Folders" and "smart mailboxes" are the only hostnames with non-standard characters. At least, they should be. It is possible to create others if you write some bad code, but I think that would fail now.

(In reply to Henri Sivonen (:hsivonen) from comment #26)

If there are two problematic fixed strings, those could be special-cased in idna_glue. However, for performance reasons, I'd like to scope such a hack only to the case where idna_glue is being compiled as part of c-c, so that m-c-only builds don't spend time comparing every domain for "Local Folders".

Makes sense. It looks like we could special-case c-c builds by checking mozbuild::config::MOZ_APP_NAME (https://searchfox.org/mozilla-central/rev/01aaa47e62a2015e7641f26ab0bc2bb00ab579b8/build/rust/mozbuild/generate_buildconfig.py#104), which should be "thunderbird" when building as part of c-c.

For the compiler to optimize things out in the m-c case, checking against a string seems problematic.

I suggest introducing a mailnews Cargo feature on idna_glue and setting the Cargo feature on idna_glue from https://searchfox.org/comm-central/source/rust/gkrust/Cargo.toml .

Attachment #9412239 - Attachment is obsolete: true

(In reply to Henri Sivonen (:hsivonen) from comment #29)

For the compiler to optimize things out in the m-c case, checking against a string seems problematic.

I suggest introducing a mailnews Cargo feature on idna_glue and setting the Cargo feature on idna_glue from https://searchfox.org/comm-central/source/rust/gkrust/Cargo.toml .

I think that could work. It would need fiddling with our automation, since we generate our gkrust/Cargo.toml file to keep up to date with the dependencies from m-c, but I have a solution for this in the patch above (https://phabricator.services.mozilla.com/D216282).

Henri, since you've probably got more knowledge and context on/around idna_glue than I do, would you be able to take care of this special casing for Local Folders and smart folders? No worries if not, I can look into it in this case.

Henri, since you've probably got more knowledge and context on/around idna_glue than I do, would you be able to take care of this special casing for Local Folders and smart folders? No worries if not, I can look into it in this case.

Sorry, I meant smart mailboxes, not smart folders.

(In reply to Brendan Abolivier [:babolivier] from comment #32)

(In reply to Henri Sivonen (:hsivonen) from comment #29)

For the compiler to optimize things out in the m-c case, checking against a string seems problematic.

I suggest introducing a mailnews Cargo feature on idna_glue and setting the Cargo feature on idna_glue from https://searchfox.org/comm-central/source/rust/gkrust/Cargo.toml .

I think that could work. It would need fiddling with our automation, since we generate our gkrust/Cargo.toml file to keep up to date with the dependencies from m-c, but I have a solution for this in the patch above (https://phabricator.services.mozilla.com/D216282).

It doesn't have to be that file. Any c-c Cargo.toml that adds a dependency on idna_glue and sets the mailnews feature should work.

Henri, since you've probably got more knowledge and context on/around idna_glue than I do, would you be able to take care of this special casing for Local Folders and smart folders? No worries if not, I can look into it in this case.

I uploaded a completely untested patch. (Please test that the equality comparisons actually work!) I'm unlikely to have time to work on the patch more this week, so please take the patch and edit as needed.

In the patch, I'm assuming that existing code still expects the IDNA code to change "Local Folders" to lower case, but if not, then you can edit the "Local Folders" cases to look more like the "smart mailboxes" cases.

(In reply to Henri Sivonen (:hsivonen) from comment #35)

(In reply to Brendan Abolivier [:babolivier] from comment #32)

(In reply to Henri Sivonen (:hsivonen) from comment #29)

For the compiler to optimize things out in the m-c case, checking against a string seems problematic.

I suggest introducing a mailnews Cargo feature on idna_glue and setting the Cargo feature on idna_glue from https://searchfox.org/comm-central/source/rust/gkrust/Cargo.toml .

I think that could work. It would need fiddling with our automation, since we generate our gkrust/Cargo.toml file to keep up to date with the dependencies from m-c, but I have a solution for this in the patch above (https://phabricator.services.mozilla.com/D216282).

It doesn't have to be that file. Any c-c Cargo.toml that adds a dependency on idna_glue and sets the mailnews feature should work.

Unfortunately since we don't depend on idna_glue directly in one of our crate, any Cargo.toml file that could be a good target will have automated generation getting in the way.

Henri, since you've probably got more knowledge and context on/around idna_glue than I do, would you be able to take care of this special casing for Local Folders and smart folders? No worries if not, I can look into it in this case.

I uploaded a completely untested patch. (Please test that the equality comparisons actually work!) I'm unlikely to have time to work on the patch more this week, so please take the patch and edit as needed.

In the patch, I'm assuming that existing code still expects the IDNA code to change "Local Folders" to lower case, but if not, then you can edit the "Local Folders" cases to look more like the "smart mailboxes" cases.

Thanks a ton! I've made a few experiments on my end and I think I know how to modify this patch to make things work for TB.

Assignee: nobody → brendan
Status: NEW → ASSIGNED

(In reply to Brendan Abolivier [:babolivier] from comment #36)

Unfortunately since we don't depend on idna_glue directly in one of our crate, any Cargo.toml file that could be a good target will have automated generation getting in the way.

You can add a new c-c crate that declares a dependency on idna_glue in its Cargo.toml without actually doing anything with it in its lib.rs.

Attachment #9411975 - Attachment is obsolete: true
Attachment #9412293 - Attachment description: WIP: Bug 1906992 - Special case pseudo-hosts with space for IDNA in mailnews builds → Bug 1906992 - Special case pseudo-hosts with space for IDNA in mailnews builds. r=kershaw,john.bieling,darktrojan
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/integration/autoland/rev/9da81db4aeec Special case pseudo-hosts with space for IDNA in mailnews builds. r=necko-reviewers,john.bieling,kershaw

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

Yeah, "Local Folders" and "smart mailboxes" are the only hostnames with non-standard characters. At least, they should be. It is possible to create others if you write some bad code, but I think that would fail now.

Thank you for working on the quick fix. It would be great if we can build and test C-C TB soon.

However, maybe my thought on " " alone is the good enough target may be premature.

A few years back, I recorded the SetSpec success cases for comparison to SetSpec failure cases. It was last done in June 2020 using a local mochitest under linux. (The mochitest back then may not be as complete as today's. So I see the posted patch handles the cases which did not get recorded in June 2020.)

We are talking about mailbox: or mailbox-message:, I suppose. Then I see the following URIs that uses "%20" in OTHER places (other
than "Local Folders" or "Smart mailboxes". I wonder if the fix we are talking about take care of the following cases.

Some seem to be already taken care of in the attached patch. I mark them with
"*" at the beginning of the file.

*     1 {debug} SetSpec succeeded. : aSpec=mailbox-message://nobody@Local%20Folders/Forward%20Content%20Testing#1?fetchCompleteMessage=true
*     2 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Draft%20Identity%20Testing

      Note the lower case "folder" in the next URI. Ugh.
?     1 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@local%20folders/Forward%20Content%20Testing

     10 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Local%20Folders/Forward%20Content%20Testing
     17 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Local%20Folders/My%20Folder
     52 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Local%20Folders/test-tabmail-closing%20folder
      2 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Local%20Folders/Uninteresting%20Folder

      Again, note the lower case "folders" on the next line.
      6 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@local%20folders/Unsent%20Messages
    139 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Local%20Folders/Unsent%20Messages

    Very intereting use of "Local_Folders":
      2 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Local_Folders/Unsent%20Messages

*      4 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@New%20Msg%20Compose%20Identity%20Testing
*      3 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@New%20Msg%20Compose%20Identity%20Testing/Inbox
*      3 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@New%20Msg%20Compose%20Identity%20Testing/Trash

*      7 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Reply%20Identity%20Testing
*      5 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Reply%20Identity%20Testing/Inbox
*     78 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Reply%20Identity%20Testing/Msgs4Reply
*     32 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Reply%20Identity%20Testing/Replies


      2 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@smart%20mailboxes/My%20Smart%20Folder%20A
      2 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@smart%20mailboxes/My%20Smart%20Folder%20B

*      4 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Test%20Local%20Folders
*      6 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Test%20Local%20Folders/Another%20Folder
*      4 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Test%20Local%20Folders/Inbox
*      7 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Test%20Local%20Folders/Test%20Folder
*      4 {debug} SetSpec succeeded. : aSpec=mailbox://nobody@Test%20Local%20Folders/Trash

      mailbox-message:

      1 {debug} SetSpec succeeded. : aSpec=mailbox-message://nobody@Local%20Folders/Forward%20Content%20Testing#1?fetchCompleteMessage=true

Well, if we expand the search to other protocol prefixes (other than mailbox:, mailbox-message:), There are many others. I see something like as follows. (Not complete list.) They use other esoteric characters other than " ".

      2 {debug} SetSpec succeeded. : aSpec=about:neterror?e=blockedByPolicy&u=file%3A///NEW-SSD/moz-obj-dir/objdir-tb3/dist/bin/chrome/messenger/content/messenger/about-support/aboutSupport.xhtml&c=UTF-8&d=Your%20organization%20has%20blocked%20access%20to%20this%20page%20or%20website.

      1 {debug} SetSpec succeeded. : aSpec=data:text/css;charset=utf-8,body%20%7B%20background-color%3A%20darkred%3B%20%7D

      I have no idea what the following is.
      1 GECKO(1273596) {debug} SetSpec succeeded. : aSpec=data:text/plain,var knownOrigins = (function () {  return ['http://mochi.test:8888', 'http://127.0.0.1:80', 'http://127.0.0.1:8888', 'http://test:80', 'http://mochi.test:8888', 'http://test1.mochi.test:8888', 'http://sub1.test1.mochi.test:8888', 'http://s

Oh, the SetSpec string was cut off at 256 (or 255) position, and the last example line is cut short.

There are many other esoteric cases.

All the success cases from SetSpec() on June 2020 are in the attachment I will upload sorted alphabetically with frequencies of occurrences at the beginning of each line.

This is the list of URIs that get passed to SetSpec() and succeeded in NS_MutateURI& SetSpec(const nsACString& aSpec) in netwerk/base/nsIURIMutator.idl.

The data was collected from local mochtest under linux using C-C TB Debug version.

So there MAY be some tests that may no longer be there, but there may be other tests that introduce
URIs that use " " in unexpected places.

Pushed by smolnar@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/ae67a0e098b4 Special case pseudo-hosts with space for IDNA in mailnews builds. r=necko-reviewers,john.bieling,kershaw
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/cc7b21451775 Preserve Thunderbird-specific feature from mozilla-central deps when regenerating gkrust's Cargo.toml. r=darktrojan,kaie https://hg.mozilla.org/comm-central/rev/43eb3874368d Ensure URLs are properly normalized for local folders and smart mailboxes. r=darktrojan,kaie https://hg.mozilla.org/comm-central/rev/0db2d66d3a78 Fix tests that use non-standard hostnames. r=kaie CLOSED TREE
Keywords: leave-open
Target Milestone: --- → 130 Branch
Duplicate of this bug: 1907113

Backed out the disabling of the Daily cron job:
https://hg.mozilla.org/comm-central/rev/543068fbcb85816e8ab9a859bce65fbf237d2434

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
See Also: 1907113, 1906529 →
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: