While bug 1685033, bug 1686034, bug 1685450, bug 1687412 and bug 1687452 have fixed the fallout from bug 1571672, style and correctness issues from bug 1571672 comment #86 still need to be addressed.

It also needs to be checked whether all the calls to CopyUTF16toMUTF7()
https://searchfox.org/comm-central/search?q=CopyUTF16toMUTF7&path=&case=false&regexp=false
are correct.

These five
https://searchfox.org/comm-central/rev/0b9e961cd522f59b0bcf7e15062781a87319df2b/mailnews/base/src/nsMsgUtils.cpp#508 (in NS_MsgCreatePathStringFromFolderURI())
https://searchfox.org/comm-central/rev/0b9e961cd522f59b0bcf7e15062781a87319df2b/mailnews/imap/src/nsImapMailFolder.cpp#7155 (in nsImapFolderCopyState::OnStopRunningUrl())
https://searchfox.org/comm-central/rev/0b9e961cd522f59b0bcf7e15062781a87319df2b/mailnews/imap/src/nsImapMailFolder.cpp#8116 (in nsImapMailFolder::RenameSubFolders())
https://searchfox.org/comm-central/rev/0b9e961cd522f59b0bcf7e15062781a87319df2b/mailnews/imap/src/nsImapService.cpp#2282 (in nsImapService::EnsureFolderExists())
https://searchfox.org/comm-central/rev/0b9e961cd522f59b0bcf7e15062781a87319df2b/mailnews/search/src/nsMsgFilter.cpp#623 (in nsMsgFilter::ConvertMoveOrCopyToFolderValue())
are unconditional and not based on whether UTF8=ACCEPT is enabled and there is a likeliness that they are incorrect now. A test case for each of these five code paths needs to be created.

This fixes the comments without checking whether they are correct. There is some truly frightening stuff like:
If it's not UTF-8, it cannot be MUTF-7, either. We just ignore it.
The caller has already converted MUTF-7 to UTF-8, which is a problem.
So change to MUTF-7 not needed at all it seems.

Overall I have the impression there is widespread confusion of when folder names are in UTF-8 always or can be in UTF-8 or MUTF-7 depending on the server. So comments like these:
online name is in MUTF-7 - leave it that way
convert name back from MUTF-7
are most likely wrong now. I'll try to make sense of it in the next set of patches.

This fixes all the issues from bug 1571672 comment #86 except "The code doesn't match the comment", see next paragraph for that. I removed a condition based on m_onlineFolderName.IsEmpty(). The comment was confusing and it wasn't clear why that was needed. If it is indeed needed, it should be documented.

So even after this, we must revisit all the comments hit in the first part and make sure which encoding is used.

Additionally, we must visit the five call sites pointed out in comment #0 and check that MUTF-7 is really the right thing to use unconditionally.

Maybe Gene wants to chime in on the latter.

May I suggest that in the future, really BIG changes are landed with proper review so that not six further bugs are needed afterwards.

Maybe Gene wants to chime in on the latter.

Sorry for the delay, had some non-tb stuff to take care of.
I'll look at these but I'm no expert on this char encoding stuff (obviously).

Looking at the coding patch above, I see a change to UnescapeSlashes near the end. Is that addressing a similar issue as Ben has proposed in his "robustify" patch here: bug 1580262 ?

Indeed, bug 1580262 fixes UnescapeSlashes() but it repeats the code. Ben will spot it when he reviews the patch here.

The first feedback I need is why this code was changed:
https://hg.mozilla.org/comm-central/rev/0054ea73e658#l18.131
I reverted the change since I couldn't see the need and didn't understand "Avoids replacement chars".

The next tasks is to check the five calls to CopyUTF16toMUTF7() listed in comment #0. As far as I can see, this will still produce MUTF-7 for servers can handle UFT-8.

The first feedback I need is why this code was changed:
https://hg.mozilla.org/comm-central/rev/0054ea73e658#l18.131
I reverted the change since I couldn't see the need and didn't understand "Avoids replacement chars".

I took out the additional check I added for && !onlineName.IsEmpty() and I now don't see invalid chars in the utf8 strings (online names) sent to the imap server. This is with a folder name with multi-byte UTF-8 chars on a server with UTF8=ACCEPT capability and enabled.
At one point in the process this seemed to be needed (I had it marked a a temporary change in an obsolete diff) but taking it out now doesn't seem to help or hurt per my testing.

However, I do see something strange now. Probably a partially implemented new feature, but when I open an attachment (.jpg) the picture takes over the whole message reading area instead of opening in an application. The only way to get back the message text is to select another message and then come back.

taking it out now doesn't seem to help or hurt per my testing.

OK, thanks, so I'm removing it in the patch. You can f+ the patch then ;-)

I do see something strange now.

Bug 1687937.

Next we need to check the five call sites from comment #0.

Gene, I removed the comment here. "It seems" isn't nice. All the code does is convert UTF-16 to UTF-8. The original folder name may already contain MUTF-7. So maybe we add a comment:
// folderName contains MUFT-7 or UTF-8 as required by the server here.
Can you please confirm?

I also made your change here and subscribe/unsubscribe still works on UTF8=ACCEPT and !UTF8=ACCEPT capable servers. I checked the value of folderName on two servers, so I think this confirms what you asked:

folderName on Courier IMAP with UTF8=ACCEPT -- looks like the folder name äbcￃin unicode:

(gdb) p folderName
$1 = (const nsAString &) @0x7fffffff5418: {<mozilla::detail::nsTStringRepr<char16_t>> = {mData = 0x7fffbc843d00 u"INBOX/äbcￃ", mLength = 10, 
    mDataFlags = mozilla::detail::StringDataFlags::TERMINATED, mClassFlags = mozilla::detail::StringClassFlags::NULL_TERMINATED}, 
  static kMaxCapacity = 1073741817}

folderName on Openwave IMAP with no UTF8=ACCEPT -- looks like the folder name unique-ä as MUTF-7 ascii encoded in <char16_t>

(gdb) p folderName
$3 = (const nsAString &) @0x7fffffff4b98: {<mozilla::detail::nsTStringRepr<char16_t>> = {mData = 0x7fffbcb6d720 u"unique-&AOQ-", mLength = 12, 
    mDataFlags = mozilla::detail::StringDataFlags::TERMINATED, mClassFlags = mozilla::detail::StringClassFlags::NULL_TERMINATED}, 
  static kMaxCapacity = 1073741817}

There is a bit of an problem when you subscribe to gmail. Unsubscribe doesn't make the folder disappear. Collapse/expand of account doesn't either. You have to restart tb. Even then the folder is still there. Finally when you select/click-on the folder it goes away. I've seen this before so I don't think it has anything to do with UTF8 changes. And also, you have to restart tb for subscribe to take effect (for the folder to appear) on gmail.

I didn't see this problem on other server, even that do and don't support UTF8=ACCEPT.

Thanks, I'll update the patch/comment accordingly. The UnescapeSlashes() can move to bug 1580262.

So now we come to the bigger task of checking the five call sites from comment #0. Can you also please look through the comments mentioned in comment #1 to see whether we can clarify them. As I said, it would be great to know what encoding is carried in the variables at all times.

Rebased to bug 1580262. Comment tweaks.

Fixed typo.

These five

Should I apply your patches in this bug before testing or looking at the 5? I've only look at the 1st one so far and see this (without your patches):

https://searchfox.org/comm-central/rev/0b9e961cd522f59b0bcf7e15062781a87319df2b/mailnews/base/src/nsMsgUtils.cpp#508 (in NS_MsgCreatePathStringFromFolderURI())

This one seems to only involve nntp. I connected to new.gmane.io to see what happens.
First bp at line 508. Hits line 508 and examine pathPiece

(gdb) p pathPiece
$6 = {<nsTString<char16_t>> = {<nsTSubstring<char16_t>> = {<mozilla::detail::nsTStringRepr<char16_t>> = {
mData = 0x7fffffff8d84 u"gmane.comp.mozilla.thunderbird.user", mLength = 35,
mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::INLINE),
mClassFlags = (mozilla::detail::StringClassFlags::INLINE | mozilla::detail::StringClassFlags::NULL_TERMINATED)},
static kMaxCapacity = 1073741817}, <No data fields>}, static kStorageSize = 64, mInlineCapacity = 63,
mStorage = u"gmane.comp.mozilla.thunderbird.user\000", '' <repeats 16 times>, 'ꪪ' <repeats 12 times>}

Then step over CopyUTF16toMUTF7(pathPiece, tmp); and examine tmp:

(gdb) p tmp
$7 = {<nsTString<char>> = {<nsTSubstring<char>> = {<mozilla::detail::nsTStringRepr<char>> = {mData = 0x7fffffff8c94 "gmane.comp.mozilla.thunderbird.user",
mLength = 35, mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::INLINE),
mClassFlags = (mozilla::detail::StringClassFlags::INLINE | mozilla::detail::StringClassFlags::NULL_TERMINATED)},
static kMaxCapacity = 2147483637}, <No data fields>}, static kStorageSize = 64, mInlineCapacity = 63,
mStorage = "gmane.comp.mozilla.thunderbird.user\000", '\344' <repeats 15 times>, "䪪\252\252\252\252\252\252\252\252\252\252"}

Then step over CopyASCIItoUTF16(tmp, pathPiece); and examine pathPiece again:

(gdb) p pathPiece
$8 = {<nsTString<char16_t>> = {<nsTSubstring<char16_t>> = {<mozilla::detail::nsTStringRepr<char16_t>> = {
mData = 0x7fffffff8d84 u"gmane.comp.mozilla.thunderbird.user", mLength = 35,
mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::INLINE),
mClassFlags = (mozilla::detail::StringClassFlags::INLINE | mozilla::detail::StringClassFlags::NULL_TERMINATED)},
static kMaxCapacity = 1073741817}, <No data fields>}, static kStorageSize = 64, mInlineCapacity = 63,
mStorage = u"gmane.comp.mozilla.thunderbird.user\000", '' <repeats 28 times>}

No idea if this is what you are wanting.

The patches shouldn't change the behaviour but it would be safer to apply them.

Yes, that's exactly what we need. We need to see that we don't accidentally use MUFT-7 unconditionally when we should be using UTF-8.

Sorry, I missed that the first call site is protected by if (aIsNewsFolder) { - I think the code totally doesn't make any sense, why would we use MUTF-7 on a news folder? Looks like a total hack to get rid of non-ASCII characters. Anyway, we leave that alone.

Four to go :-)

#2 https://searchfox.org/comm-central/rev/0b9e961cd522f59b0bcf7e15062781a87319df2b/mailnews/imap/src/nsImapMailFolder.cpp#7155 (in nsImapFolderCopyState::OnStopRunningUrl())

I can get this occur when I select a folder with the mouse and drag it to another folder. So far, that's the only way I can get url "Ensure Exists" to occur. I'm dragging a non-ascii named folder to another non-ascii named folder. Here's the results before and after the call to CopyUTF16toMUTF7(folderName, utf7LeafName);:

(gdb) p folderName
$5 = {<nsTSubstring<char16_t>> = {<mozilla::detail::nsTStringRepr<char16_t>> = {mData = 0x7fffda71e748 u"gds-ääa-envoyés", mLength = 15,
mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::REFCOUNTED),
mClassFlags = mozilla::detail::StringClassFlags::NULL_TERMINATED}, static kMaxCapacity = 1073741817}, <No data fields>}
(gdb) n
7153 rv = m_curDestParent->FindSubFolder(utf7LeafName,
(gdb) p utf7LeafName
$6 = {<nsTSubstring<char>> = {<mozilla::detail::nsTStringRepr<char>> = {mData = 0x7fffbafac748 "gds-&AOQA5A-a-envoy&AOk-s", mLength = 25,
mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::REFCOUNTED),
mClassFlags = mozilla::detail::StringClassFlags::NULL_TERMINATED}, static kMaxCapacity = 2147483637}, <No data fields>}

To store the copied folder on filesystem, tb creates the file gds-&AOQA5A-a-envoy&AOk-s.msf which is MUTF-7 chars (all ascii) so OK. However, if the imap folder with same name was created manually, rather than copied in, it would be in UTF-8 form, at least on linux, i.e., gds-ääa-envoyés.msf. (The source and destination server supports UTF8=ACCEPT.)

#3 https://searchfox.org/comm-central/rev/0b9e961cd522f59b0bcf7e15062781a87319df2b/mailnews/imap/src/nsImapMailFolder.cpp#8116 (in nsImapMailFolder::RenameSubFolders())

First I re-tested to make sure folder rename works on UTF8 and non-UTF8 servers with ascii and non-ascii folder names. Seems to work OK. On linux the .msf name is encoded as MUTF-7 for non-UTF8 servers and as UTF8 for UTF8 servers. (I'm not sure where the .msf names are determined and encoded.

#3 actually handles the rename of subfolders. So to trigger it you have to rename a top level IMAP folder and it iterates through the subfolders. Folders that have .msf files as MUTF7 (copied in as in #2) keep the MUTF7 file name while folders with UTF8 file names keep the UTF8 file names. Here's the result for two sub-folders. Again, only the top level folder was renamed. It doesn't appear here.

8116 rv = CopyUTF16toMUTF7(folderName, utf7LeafName);
(gdb) p folderName
$8 = {<nsTSubstring<char16_t>> = {<mozilla::detail::nsTStringRepr<char16_t>> = {mData = 0x7fffbae89348 u"gds-ääa-envoyés", mLength = 15,
mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::REFCOUNTED),
mClassFlags = mozilla::detail::StringClassFlags::NULL_TERMINATED}, static kMaxCapacity = 1073741817}, <No data fields>}
(gdb) n
8117 NS_ENSURE_SUCCESS(rv, rv);
(gdb) p utf7LeafName
$9 = {<nsTSubstring<char>> = {<mozilla::detail::nsTStringRepr<char>> = {mData = 0x7fffbafaccc8 "gds-&AOQA5A-a-envoy&AOk-s", mLength = 25,
mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::REFCOUNTED),
mClassFlags = mozilla::detail::StringClassFlags::NULL_TERMINATED}, static kMaxCapacity = 2147483637}, <No data fields>}
(gdb) c

The folder above has a MUTF7 filename since it was copied in from a non-UTF8 server.

Here's another subfolder:

(gdb) p folderName
$10 = {<nsTSubstring<char16_t>> = {<mozilla::detail::nsTStringRepr<char16_t>> = {mData = 0x7fffbe16ff08 u"äbcￃ-abc", mLength = 8,
mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::REFCOUNTED),
mClassFlags = mozilla::detail::StringClassFlags::NULL_TERMINATED}, static kMaxCapacity = 1073741817}, <No data fields>}
(gdb) n
8117 NS_ENSURE_SUCCESS(rv, rv);
(gdb) p utf7LeafName
$12 = {<nsTSubstring<char>> = {<mozilla::detail::nsTStringRepr<char>> = {mData = 0x7fffbf35dc68 "&AOQ-bc&,8M--abc", mLength = 16,
mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::REFCOUNTED),
mClassFlags = mozilla::detail::StringClassFlags::NULL_TERMINATED}, static kMaxCapacity = 2147483637}, <No data fields>}
(gdb) c

This folder äbcￃ-abchas filename äbcￃ-abc.msf.

#4 https://searchfox.org/comm-central/rev/0b9e961cd522f59b0bcf7e15062781a87319df2b/mailnews/imap/src/nsImapService.cpp#2282 (in nsImapService::EnsureFolderExists())

Like #2, this is triggered by dragging and dropping a folder between servers. Regardless of the source server, tb always creates a MUTF7 encoded destination .msf filename. It doesn't seem to matter whether the source or destination server is UTF8 capable.

2278 CopyUTF16toMUTF7(newFolderName, utfNewName);
(gdb) p newFolderName
$17 = (const nsAString &) @0x7fffffff9970: {<mozilla::detail::nsTStringRepr<char16_t>> = {mData = 0x7fffda71e108 u"Skräppost--åeds", mLength = 15,
mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::REFCOUNTED),
mClassFlags = mozilla::detail::StringClassFlags::NULL_TERMINATED}, static kMaxCapacity = 1073741817}
(gdb) n
2279 nsCString escapedFolderName;
(gdb) p utfNewName
$18 = {<nsTString<char>> = {<nsTSubstring<char>> = {<mozilla::detail::nsTStringRepr<char>> = {mData = 0x7fffffff9884 "Skr&AOQ-ppost--&AOU-eds", mLength = 23,
mDataFlags = (mozilla::detail::StringDataFlags::TERMINATED | mozilla::detail::StringDataFlags::INLINE),
mClassFlags = (mozilla::detail::StringClassFlags::INLINE | mozilla::detail::StringClassFlags::NULL_TERMINATED)},
static kMaxCapacity = 2147483637}, <No data fields>}, static kStorageSize = 64, mInlineCapacity = 63,
mStorage = "Skr&AOQ-ppost--&AOU-eds\000", '\344' <repeats 15 times>, "䪪", '\252' <repeats 22 times>}
(gdb) c

#5 https://searchfox.org/comm-central/rev/0b9e961cd522f59b0bcf7e15062781a87319df2b/mailnews/search/src/nsMsgFilter.cpp#623 (in nsMsgFilter::ConvertMoveOrCopyToFolderValue())
are unconditional and not based on whether UTF8=ACCEPT is enabled and there is a likeliness that they are incorrect now. A test case for each of these five code paths needs to be created.

I can't seem to find what triggers this. Looks like a simple filter to move inbox messages to another folder should. I have one of these and it works but it doesn't trigger #5 or functions that call it. I'll look some more tomorrow.

Thanks for testing. As suspected, there are a few things going wrong here:

#2: Comment #14 (quote): tb creates the file gds-&AOQA5A-a-envoy&AOk-s.msf which is MUTF-7 chars.
#3: If I read comment #15 correctly, there are still some MUTF-7 folders occurring on UTF-8 servers, right? That should be fixed, no?
#4: Is there a typo in comment #16 and you mean #4 instead of #2? Again, MUTF-7 should not be created on UTF-8 servers.
#5: Need to find a way to trigger this code path.

Altogether, I believe, we need to create a function along the lines of ShouldUseUTF8FolderName() that needs to be called in at lease cases #2 to #4. A call to this function can then replace the duplicated code which was added here:
https://hg.mozilla.org/comm-central/rev/0054ea73e658#l23.41
https://hg.mozilla.org/comm-central/rev/0054ea73e658#l23.74

I'll make a patch for that.

OK, this should fix call sites #2, #3 and #4.
DISCLAIMER: 100% untested, I've just made it compile.

As we saw, #1 is some crazy NNTP code and #5 is protected by if (filterVersion == k45Version) {, so it looks like some (very) old filter versions are processed there. Let's ignore #5 then.

Gene, when applying the patches, bug 1580262 needs to go first.

In part 5 I will review the strange comments pointed out in comment #1.

Gene, when applying the patches, bug 1580262 needs to go first.

That has landed, but there's a second patch that corrects an off-by-one error. Please apply that otherwise it won't work at all.

Instead of guessing what the trash folder encoding should be, we use the server flag.

Lightly tested on Gmail with UTF-8 enabled (and it lints OK). Gene, does this still work?

I ended up throwing away a lot of obviously wrong and unhelpful comments. Two were referring to bug 63186 and bug 264071. I changed a couple of variable names since the variables are not guaranteed to carry MUTF-7 any more. And finally I think I found a bug where a CopyASCIItoUTF16() was missed. So on that code patch a UTF-8 folder wouldn't have worked. Maybe Gene wants to confirm that.

That concludes the cleanup.

And finally I think I found a bug where a CopyASCIItoUTF16() was missed.

Maybe that causes what was reported in bug 1686034 comment #37.

In comment 18 Klaus B. said this:

#3: If I read comment #15 correctly, there are still some MUTF-7 folders occurring on UTF-8 servers, right? That should be fixed, no?

Last night, I didn't think this matters. However, now on tb restart when I look at the folder tree in tb, those folders names are showing as literal MUTF-7, e.g., gds-&AOQA5A-a-envoy&AOk-s.

#4: Is there a typo in comment #16 and you mean #4 instead of #2? Again, MUTF-7 should not be created on UTF-8 servers.

#4 and #2 are triggered by the same user action: dragging a folder and dropping into another folder. It does cause folder names and file names to appear as MUTF-7 on UTF8 accounts/servers if the source is non-UTF8.

I haven't digested everything after comment 18 but it sounds like you may have a fix for this in the various patches.

Just pull C-C, apply the second patch from bug 1580262, then the five patches here and then see what happens ;-)

That's what I did. Looks good so far. Not creating filenames with MUTF-7 ascii but with true UTF-8. I'll try some more things...

I see a problem: If I drag a folder called unique-äￃfrom a non-UTF8 account to a folder on a UTF8 account, the folder is created OK with UTF-8 chars on at the destination UTF8 account. But destination imap server produces the error Must create mailbox before append. The problem is with the encoding of the strings into the imap APPEND (from imap log):

append "INBOX.INBOX^new-one^unique-&AOT,ww-"

Looks like 3 problems:

• INBOX occurs twice
• The hierarchy delimiter is wrong; ^ should be .
• The leaf destination folder name is encoded as MUTF-7 as it was on the source account.

Imap sends what it sees in the URL. Here's what the log show for the "appendmsg" URL:

2021-01-23 20:55:15.031203 UTC - [Parent 48088: IMAP]: I/IMAP 0x7f7600580000:mail.tana.it:S-INBOX.Skrä-pail-ä:ProcessCurrentURL:imap://gds@mail.tana.it:143/appendmsgfromfile%3E%5EINBOX%5Enew-one%5Eunique-%26AOT%2Cww-: = currentUrl

Also, just noticed there are two destination .msf files:
unique-&AOT,ww-.msf
unique-äￃ.msf

Did that get broken with the patches or was that broken before? If the former, which particular change broke it? Looks like the "append" still uses the MUTF-7 when when it should have encoded to UTF-8, right? Where does this happen? Maybe before the patches the copy was in MUTF-7 completely, so no UTF-8 was used at all.

EDIT: You're copying between servers with different hierarchy delimiters. That worked before? I see you have an account at tana.it, that's the server of the reporter of all those UTF-8 bugs, right? I can only use Gmail.

Looking into this a bit:
AppendMessageFromFile() gets the folder name here:
https://searchfox.org/comm-central/rev/5e2a193b811cd20e275bb8216e20799b24141c25/mailnews/imap/src/nsImapService.cpp#2025

And GetFolderName() is here:
https://searchfox.org/comm-central/rev/5e2a193b811cd20e275bb8216e20799b24141c25/mailnews/imap/src/nsImapService.cpp#124

It has this comment "online name is in imap utf-7 - leave it that way" which I removed in part 5 since I believe that the online name can be UTF-8 as well now. So I'm not sure where it goes wrong.

1st, easy answers:

I see you have an account at tana.it, that's the server of the reporter of all those UTF-8 bugs, right? I can only use Gmail.

The tana test account is from the original requestor of the UTF8=ACCEPT feature, Alessandro Vesly. It's a Courier server. (I think the reporter of UTF8 bugs was in France and not Italy).

I tried the same d&d between non-UTF8 account (my ISP) and gmail. The same problems occurred except for doubled INBOX (gmail doesn't put folders under INBOX).

In my above tests from yesterday I was dragging between gmail and tana. So I've never done exactly that before. (I think I implied I did in my earlier comments but I didn't.)

Yes, I was referring to Alessandro, not the French reporter of the fallout bugs which are all fixed now.

So the patches fix the UTF-8 (Gmail) to UTF-8 (tana) drag. That's already something. I guess if you drag between UTF-8 and non-UTF-8 or vice versa, some re-encoding of the name will need to happen. Perhaps you can pinpoint it ... or even fix it in part 6 ;-) - A starting point would be to get the folder name of the destination folder at the spot I suggested in comment #29.

Does aFolder->GetOnlineName(onlineName); get a MUTF-7 or an UTF-8 name for the target folder? I assume it would be UTF-8 now, no?
https://searchfox.org/comm-central/rev/5e2a193b811cd20e275bb8216e20799b24141c25/mailnews/imap/src/nsImapService.cpp#132

If it's not already UTF-8 you could try re-encoding ... EDIT

   nsCString onlineName;
   // Online name is in MUTF-7 or UTF-8.
   rv = aFolder->GetOnlineName(onlineName);
   NS_ENSURE_SUCCESS(rv, rv);
+
+  // Change it to whatever we need.
+  nsAutoString utf16Name;
+  if (NS_SUCCEEDED(CopyFolderNameToUTF16(onlineName, utf16Name))) {
+    bool utf8AcceptEnabled;
+    nsCOMPtr<nsIMsgImapMailFolder> imapFolder = do_QueryInterface(aImapFolder);
+    rv = imapFolder->GetShouldUseUTF8FolderName(&utf8AcceptEnabled);
+    NS_ENSURE_SUCCESS(rv, rv);
+    if (utf8AcceptEnabled) {
+      CopyUTF16toUTF8(utf16Name, onlineName);
+    } else {
+      CopyUTF16toMUTF7(utf16Name, onlineName);
+    }
+  }

EDIT continued: I tried that and it didn't work. I guess the hierarchy delimiter isn't right either, see comment #33.

Or maybe there is something wrong with the "online name" of the target folder and that got set as MUTF-7 instead of UTF-8 to start with.

I tried dragging a folder testä from my ISP (non-UTF8) to Gmail. The folder gets created, but remains empty. The error I see is:
The current command did not succeed. The mail server for the account ... responded: [TRYCREATE] Folder doesn't exist. (Failure).

This is totally weird. When dropping the folder, I get an online name of INBOX.test&AOQ-, at a later stage I see debug go past that has INBOX/testä.
EDIT: That matches Gene's observation that differently named folders show up later.
EDIT2: My guess from all that is that the target folder is initially incorrectly created.

(In reply to Klaus B. from comment #32)

I tried dragging a folder testä from my ISP (non-UTF8) to Gmail. The folder gets created, but remains empty. The error I see is:
The current command did not succeed. The mail server for the account ... responded: [TRYCREATE] Folder doesn't exist. (Failure).

That's exactly what I see too when dd to tana or gmail. It creates the folder ok (so empty) but when it tries to imap append to the folder it uses the bad uri (containing MUTF7 and bad delimiter etc.).

This is totally weird. When dropping the folder, I get an online name of INBOX.test&AOQ-, at a later stage I see debug go past that has INBOX/testä.

Not sure what you mean...

Not sure what you mean...

I added a print to nsImapService::GetFolderName(). After a while, like 30 seconds, I see all the IMAP folders be printed out with the correct name. At the time of the drag, they are "wrong". Please note the edits in comment #31 and #33.

This sort of "rings a bell" about my previous tests from several months ago. The hierarchy delimiter is returning as ^ when I enter AppendMessageFromFile(). This is defined here: https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapCore.h#67
I don't remember what I did to get around this before but that's why we see ^ in the append URI. I'm still running with patches to "part 5" above. Haven't tried any changes yet. I'll keep poking.

As suggested, if I back-out the patch part 3 above (fix-MUFT7-usage.patch) it seems to work fine when I drag a MUFT7 folder to a UTF8 folder. However, if I look at the imap log, I see that the destination folder is created with MUFT7 ascii and the imap append now has the correct hierarchy separator but the path is also encoded as MUTF7 ascii. The destination folder name looks OK in tb also. However, when I look at webmail, both gmail and tana, I see that the folder I copied in is raw MUTF7 ascii instead of the expected non-ascii chars.

Specifically, in tb I see unique-äￃ while on webmail I see unique-&AOT,ww-

On tb restart, if the copied in folder is a subfolder of a folder with non-ascii UTF8 chars, initially it displays as ascii MUTF7. But after folder discovery by tb, it changes to the correct form, e.g., unique-&AOT,ww- --> unique-äￃ.

Also, the filename in tb for the copied in folder is MUTF7: unique-&AOT,ww-.msf

From comment 33:

My guess from all that is that the target folder is initially incorrectly created.

Yes, that's the first problem. I put back all your 5 patches and added the comment 31 addition.

In nsImapService::EnsureFolderExists() the call to GetShouldUseUTF8FolderName(&utf8AcceptEnabled) set utf8AccceptEnabled to false when the source folder is non-UTF8 and the destination is UTF8. I don't know right now if it is looking at the source or destination or if it is just returning the wrong boolean value. If I force utf8AcceptEnabled to true the correct UTF8 string is used to create the folder instead of a MUTF7 string and the imap create looks good in imap log, on tb and on gmail webmail (not mutf7 ascii).

I think your addition to GetFolderName() in comment 31 has the same problem and would work if the utf8AcceptEnable got set right.

Ok, the problem for create folder is that the destination folder has not yet authenticated so the utf8 flag is not yet set. If I select the destination folder on gmail or tana before doing the d&d, the folder is creating UTF8 and not MUTF7. At this point, not sure how to work around that.

Probably the same issue occurs during the append. This may also be why I saw an "unknown hierarchy delimiter" at one point on tana.

Summarising comment #37 to #39:
Part 3 is necessary since otherwise we get unwanted MUTF-7 on UTF-8 servers.
The extra code from comment #31 should not be necessary, it was just a "desperate" hack to try to bend the folder name into shape.
The problem is that the UTF-8-ness of the destination isn't known in time, so the destination uses MUTF-7 when it shouldn't, for a while. That explains my observation that at first, I see lots of wrong names coming back from GetOnlineName(), after a while, I see them correctly.
"If I select the destination folder on gmail or tana before doing the d&d" --> How do you select the destination folder if that's created in the drag'n drop? Or do you mean the parent?

If the properties of the destination system are known too late, did drag an drop between systems of different hierarchy delimiters, or to a system with a non-standard hierarchy delimiter ever work? If so, how did we get the correct delimiter? Couldn't we get the UTF-8-ness at that point?

I've been studying the original changeset https://hg.mozilla.org/comm-central/rev/0054ea73e658 and found some more "interesting" things:
Some separator related code was changed here: https://hg.mozilla.org/comm-central/rev/0054ea73e658#l18.176
That happens in nsImapMailFolder::GetDBFolderInfoAndDB() which also runs (*folderInfo)->SetProperty("onlineName", autoOnlineName); which be believe sets the wrong folder name at the beginning.
The UTF-8 feature is enabled here: https://hg.mozilla.org/comm-central/rev/0054ea73e658#l19.171 in nsImapProtocol::ProcessAfterAuthenticated(), and there is this interesting warning here: https://hg.mozilla.org/comm-central/rev/0054ea73e658#l19.184
Does that ever come out? In which case the capability wouldn't be set correctly.

I've merged parts 2 to 5 into one. That reduces the overall patch size since we don't visit the same code more than once. I also found another bit in nsMsgImapSearch.cpp that needed simplification. Functionally, it's the same as the four separate patches.

From comment 40:

"If I select the destination folder on gmail or tana before doing the d&d" --> How do you select the destination folder if that's created in the drag'n drop? Or do you mean the parent?

I do mean the parent. I've been testing this by dragging a folder into an existing folder. So I mean I have to select/click-on/visit the parent folder first to set the utf8=accept flag.
I'm not sure what would happen if I just dragged the folder to the "root" of the account and not into an existing mailbox folder.
Maybe an existing (cached) connection is getting reused by the new destination mailbox folder so the utf8=accept flag is still set. I definitely need to look closer at how this works.

From comment 41:

The UTF-8 feature is enabled here: https://hg.mozilla.org/comm-central/rev/0054ea73e658#l19.171 in nsImapProtocol::ProcessAfterAuthenticated(), and there is this interesting warning here: https://hg.mozilla.org/comm-central/rev/0054ea73e658#l19.184
Does that ever come out? In which case the capability wouldn't be set correctly.

I don't think I've ever seen a null m_imapServerSink, so I've never seen that warning occur.

I haven't seen the warning. ProcessAfterAuthenticated() has two call sites, and for both m_imapServerSink seems to be set. You could add a MOZ_ASSERT(m_imapServerSink) in there instead of the warning. Strangely enough ProcessAfterAuthenticated() seems to be called every once in a while, I see GetServerStateParser().fUtf8AcceptEnabled; be returned false and true, for my ISP's server and Gmail, as if this were polled regularly.

I just try to drag a subfolder of the the non-UTF-8 Inbox at the ISP to the Gmail Inbox. Even if dragged to a subfolder of the Gmail Inbox and with visiting that subfolder before, it doesn't work for me. I guess I can't be of much further help here.

Here's my debugging patch. I can see that the correct UTF-8 ability is retrieved from the source and destination servers. However, this is the output, so the folder name is derived from the URI and that still has the MUFT-7 bit:

=== set online (2) INBOX/testä2
=== set online (1) INBOX/testä2
=== |INBOX/test&AOQ-2|
=== |imap://klaus.bartosch%40gmail.com@imap.gmail.com/INBOX/test&AOQ-2|
=== set online (2) INBOX/test&AOQ-2

I suggest to check where that URI is constructed. Could it be that it takes the folder URI from the source and doesn't adapt it to the destination?

More debug:
=== nsMsgDBFolder::Init |imap://klaus.bartosch%40gmail.com@imap.gmail.com/INBOX/test&AOQ-2|
JS Stack:
0 getOrCreateFolderForURL(uri = ""imap://klaus.bartosch%40gmail.com@imap.gmail.com/INBOX/test&AOQ-2"") ["resource:///modules/FolderLookupService.jsm":95:13]
this = [object Object]
Someone requesting the folder with the bad URL.

Found it :-( - My fault. Checked the source flag here:

          nsCOMPtr<nsIMsgImapMailFolder> imapFolder =
              do_QueryInterface(m_curSrcFolder);  <=== This should be m_curDestParent
          rv = imapFolder->GetShouldUseUtf8FolderName(&utf8AcceptEnabled);
          NS_ENSURE_SUCCESS(rv, rv);
          if (utf8AcceptEnabled) {
            CopyUTF16toUTF8(folderName, utfLeafName);
          } else {
            CopyUTF16toMUTF7(folderName, utfLeafName);
          }
          rv = m_curDestParent->FindSubFolder(utfLeafName,
                                              getter_AddRefs(newMsgFolder));

I'll update the patch.

One line change from the previous version. Seems to be working now.

Yes, this is about consistency. The main point is the "7" in the name, it's a 7bit encoding, so it's using the US-ASCII charset.

Actually, "mUTF-7" is more canonical that "MUTF-8" :).

Anyhow, I can now drop a folder into gmail or tana folder creates and then content appends as long as I have a prior IMAP connection with them.

I typically run with my test accounts like gmail and tana set to not check for new mail at startup and don't poll for new mail every X minutes. Avoids filling my log with stuff I don't care about. So no imap connection occurs until I click on a gmail or tana folder. That means the utf8=accept flag is still false when tb tries to create and append the folder so mutf-7 encoding is used. But this is not a problems when "check new mail at startup" is set since at least one connection occurs and sets the flag appropriately. So maybe it needs a 3rd state for the flag like "never set" or "unknown". If never set/unknown, maybe the UI should inhibit the copy/move.

I see another problem when dragging a folder to the top level server name (root) of gmail with a prior connection. The resulting folder is created as a sub-folder of a phantom folder (gray with a previously deleted folder's name) and contains no content. gmail reports errors like [TRYCREATE] or "unknown folder" when I try to access or delete the folder in tb. Have to go to webmail to get rid of it (remove the label).
When I do the same operation with tana (drop the folder on the top-level server name) nothing happens and don't see any activity in the log.

For both servers, when I drop to an existing folder (not top level server name), it works fine now (assuming a prior connection).

I tried to address the review issues from the original bug. And that's done now. The resulting patch is already big and confusing enough. If you want to fix some edge cases (no initial connection, drop onto root), we should do that in a separate bug and I'm happy to help. OK? So please put the f+ onto the patch so that Ben knows that it "mostly works"(TM). It's certainly much better than without the patch. If we hurry up, we can get it landed before the next beta tomorrow.

Can you also please check the trash folder selection, the code in am-server.js.

Comment on attachment 9198904 [details] [diff] [review]
Part 2: 1687727-coding-issues.patch

Sorry, I entered feedback+ too soon without testing the Trash folder. Wasn't sure exactly what the Trash folder issue was and hadn't looked at it during this go-around.

Looking at the original bug 1571672 Comment 66, I'm seeing the same problems with the latest patch. I designated a UTF-8 multi-byte folder as trash, it display the appropriate icon and it worked OK to delete a message into. But on restart I still see the trash icon on it but on Server settings page it says "Choose Folder" instead of the already selected folder.

So retracting the feedback+ for now.

(deleted, was all wrong)

Sadly the landed code was wrong:
https://searchfox.org/comm-central/rev/0510f14df4723c90f03be54c359553782a1b33eb/mailnews/base/prefs/content/am-server.js#523
returns a JS unicode string with the folder name. If it's not MUTF-7, it can be used directly, no need to make raw UTF-8 from it. I removed the useless catch blocks which papered over any logic error.

Sorry, another comment tweak.

Comment on attachment 9198929 [details] [diff] [review]
Part 2: 1687727-coding-issues.patch

Looks good, works good.

From comment 51:

I see another problem when dragging a folder to the top level server name (root) of gmail with a prior connection. The resulting folder is created as a sub-folder of a phantom folder (gray with a previously deleted folder's name) and contains no content. gmail reports errors like [TRYCREATE] or "unknown folder" when I try to access or delete the folder in tb. Have to go to webmail to get rid of it (remove the label).
When I do the same operation with tana (drop the folder on the top-level server name) nothing happens and don't see any activity in the log.

Whatever this was its seemed to fix itself for gmail. I remove the patches for this bug going back to the current latest trunk. Still saw the problem. Then I ran the latest beta 85.0b3 (before recent changes hit the fan) and the problem went away. (Oh, I forgot to mention, the problem occurred just creating a folder from root level, no d&d required.) Then I returned to trunk version and I could still create folder at gmail root level! Applied the patches for this bug and it still works without the phantom folder.

Before run the beta, I had deleted all the gmail profile files and let them be restored on header downloads and that didn't help.

The phantom folder contained the "envoy" string that I tested with several days ago. It didn't appear in the gmail imap profile files or on webmail before creating the root level folder. I think it may have been coming from the panacea.dat file. It contained some references to the phantom string containing "envoy" and it still does. I know the ultimate clean-up is to delete all the profile files for the account and the panacea.dat but that causes all messages/headers to be re-downloaded, so I haven't done that.

I think its not possible to create a root level folder on tana because INBOX is the primary namesapace and it's already at root level. So I don't think there is problem at all with tana. (You can only create new folders inside tana's INBOX namespace and that works fine.)

(In reply to Ben Campbell from comment #58)

You want a single line summary for the patch. I'd move the existing lines

Will do.

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

Looks "good", none of the failures come from here.

Hmm. I wonder what the tree referred to here is?

Pre-existing legacy comment.

Should be "UTF8" rather than "Utf8" - i.e. shouldUseUTF8FolderName?

I had that at first, but we have:
https://hg.mozilla.org/comm-central/rev/0054ea73e658#l13.34
attribute boolean utf8AcceptEnabled;
https://hg.mozilla.org/comm-central/rev/0054ea73e658#l14.13
void setServerUtf8AcceptEnabled(in boolean aEnabled);

So I changed it to what we have now for consistency.

Setting the trash folder to a non-ASCII name on a non-UTF-8 server was broken as well due to this incorrect change here:
https://hg.mozilla.org/comm-central/rev/0054ea73e658#l16.50
https://hg.mozilla.org/comm-central/rev/0054ea73e658#l16.238
That depends on whether the server uses UTF-8 or now. The code only worked for UTF-8 servers now :-( - Fixed it while I was there.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/eefbb5b2f82a
Unify spelling of UTF-8 and MUTF-7 in IMAP code. r=benc
https://hg.mozilla.org/comm-central/rev/7463adb63b60
Fixes to IMAP folder name encoding. r=benc

For the record, the fixes for the incorrect code mentioned in comment #61 are here:
https://hg.mozilla.org/comm-central/rev/7463adb63b60#l6.12
https://hg.mozilla.org/comm-central/rev/7463adb63b60#l6.134
https://hg.mozilla.org/comm-central/rev/7463adb63b60#l6.152
With the corrected comments and the adjusted variable names it's quite clear now which encoding we're expecting at any point in the processing.

Change of topic. Gene, is comment #51 still relevant? Quote:
I typically run with my test accounts like gmail and tana set to not check for new mail at startup and don't poll for new mail every X minutes. [...]. So no imap connection occurs until I click on a gmail or tana folder. That means the utf8=accept flag is still false when tb tries to create and append the folder so mutf-7 encoding is used.

If so, that needs to be addressed in another bug. Can you please file one. I'm a bit surprised that it would access a server without authenticating first, so I'd imagine that for a copy operation it would first have to authenticate the target folder/server.

(In reply to Klaus B. from comment #63)

Change of topic. Gene, is comment #51 still relevant? Quote:
I typically run with my test accounts like gmail and tana set to not check for new mail at startup and don't poll for new mail every X minutes. [...]. So no imap connection occurs until I click on a gmail or tana folder. That means the utf8=accept flag is still false when tb tries to create and append the folder so mutf-7 encoding is used.

If so, that needs to be addressed in another bug. Can you please file one. I'm a bit surprised that it would access a server without authenticating first, so I'd imagine that for a copy operation it would first have to authenticate the target folder/server.

Yes, still relevant. I'm not exactly clear on the sequence of events that happen here but it's definitely a bug that needs to be filed.
I've seen similar effects if you right click on a server or folder for an imap account that is not yet connected, for example:

• Subscribe comes up empty
• Quota for folder has no data
• Drag a folder to another unconnected account (even pre-UTF8=ACCEPT has this issue).

(In reply to Klaus B. from comment #61)

Created attachment 9198948 [details] [diff] [review]
Part 2: 1687727-coding-issues.patch

Setting the trash folder to a non-ASCII name on a non-UTF-8 server was broken as well due to this incorrect change here:
https://hg.mozilla.org/comm-central/rev/0054ea73e658#l16.50
https://hg.mozilla.org/comm-central/rev/0054ea73e658#l16.238
That depends on whether the server uses UTF-8 or now. The code only worked for UTF-8 servers now :-( - Fixed it while I was there.

I'm sure your new code is correct, but I was curious and went back to the 85.0b3 before any of yours or Ping's changes occurred. I was able to assigned a non-ascii named folder as trash for a non-UTF8=ACCEPT account and it worked OK.
Also, I reverted all uncommited patches for this bug back to trunk version and did hg pull -u and rebuilt. Still working fine.

Edit: Re comment 66 below:
Klaus B. wrote:

Well, assigning it in the account settings worked, but in the folder tree the it didn't get a trash icon. And the setting was lost after a restart since the flag never got set on the folder. That's what I observed before I fixed it (with Ping's patches applied, of course

That's exactly what I did witht the 85.0b3 and it worked OK.

Let's move on to the next issue from comment #64 :-)

'nuf said, agreed!

I was curious and went back to the 85.0b3 before any of yours or Ping's changes occurred.

Sounds like a difficult "post mortem". The initial bug forgot to "propagate" all the API changes from ACString to AUTF8String. Ping added that. Whether the various deficiencies compensated each other is hard to tell. In general, code must be formally correct and also work ;-)

I was able to assigned a non-ascii named folder as trash for a non-UTF8=ACCEPT account and it worked OK.

Well, assigning it in the account settings worked, but in the folder tree the it didn't get a trash icon. And the setting was lost after a restart since the flag never got set on the folder. That's what I observed before I fixed it (with Ping's patches applied, of course).

Let's move on to the next issue from comment #64 :-)