1688782 - Dragging a folder from a not UTF8=ACCEPT server to a not yet authenticated UTF8=ACCEPT server fails

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[gene smith]

This issue was pointed out in bug 1687727 comment 51 and bug 1687727 comment 64 but we decided to move this to a new bug.

<<<<<<<<<<< Moved this 1st half discussion to a separate new Bug 1690093.

The first aspect of this is independent of the UTF8=ACCEPT feature. If you drag a folder from account A to an existing folder or the root of account B and account B uses plaintext (normal) password and has not yet connected with the IMAP server during the current tb session, the copy/move operation never occurs and no error is indicated. But looking at the IMAP log during this activity, the indication of a problem is clear:

D/IMAP Try to log in
D/IMAP IMAP auth: server caps 0x20044e3325, pref 0x1006, failed 0x0, avail caps 0x1004
D/IMAP (GSSAPI = 0x1000000, CRAM = 0x20000, NTLM = 0x100000, MSN = 0x200000, PLAIN = 0x1000, LOGIN = 0x2, old-style IMAP login = 0x4, auth external IMAP login = 0x20000000, OAUTH2 = 0x800000000)
D/IMAP Trying auth method 0x1000
E/IMAP IMAP: password prompt failed or user canceled it
E/IMAP login failed entirely
D/IMAP SetConnectionStatus(0x80004005)
D/IMAP URL failed with code 0x80004005 (imap://[e-mail]:143/ensureExists%3E.INBOX.unique-%26AOT%2Cww-)
I/IMAP 0x7fddd30b1000:mail.tana.it:NA:ProcessCurrentURL: aborting queued urls
I/IMAP 0x7fddd30b1000:mail.tana.it:NA:TellThreadToDie: close socket connection
D/IMAP ImapThreadMainLoop leaving [this=0x7fddd30b1000]
I/IMAP 0x7fddd7211800:mobile.charter.net:S-unique-&AOT,ww-:SendData: 16 close
I/IMAP 0x7fddd7211800:mobile.charter.net:S-unique-&AOT,ww-:SendData: 17 logout
I/IMAP 0x7fddd7211800:mobile.charter.net:S-unique-&AOT,ww-:TellThreadToDie: close socket connection

The stored password is not obtained and never sent to the server so login process fails and the connection is dropped. Again, this is independent of UTF8=ACCEPT and occurs when the destination server uses plaintext (normal) password. It does not occur when the destination server uses OAUTH2, e.g., gmail. It may also occur for other authentication methods that I haven't tested.

<<<<<<<<<< Moved the above to new Bug 1690093

Even with the above password issue resolved or with destination servers that use OAUTH2, if the destination server is UTF8=ACCEPT capable and the folder name to copy/move is non-ascii and the destination server is not yet IMAP connected, there is a problem. The folder name created at the destination will be encoded as MUTF-7 instead of UTF-8 and the IMAP append into the new folder will fail, so the destination folder contains no messages. This is because the imap append command uses the correct UTF-8 encoded folder name but the folder was created with MUTF-7 encoded string. For example, here the folder is created with MUTF-7 ascii characters:

IMAP]: I/IMAP 0x7fa3cc3cf000:imap.gmail.com:A:SendData: 12 create "unique-&AOT,ww-"
IMAP]: D/IMAP ReadNextLine [rv=0x0 stream=0x7fa3cc5ef9d0 nb=15 needmore=0]
IMAP]: I/IMAP 0x7fa3cc3cf000:imap.gmail.com:A:CreateNewLineFromSocket: 12 OK Success

But the append into the folder is directed to the same folder name but UTF-8 encoded:

I/IMAP 0x7fa3cc3cf000:imap.gmail.com:A:ProcessCurrentURL:imap://[e-mail]:993/appendmsgfromfile%3E%5Eunique-%C3%A4%EF%BF%83: = currentUrl
I/IMAP 0x7fa3cc3cf000:imap.gmail.com:A:SendData: 15 append "unique-äￃ" (\Seen) "20-Sep-2019 20:39:31 -0500" UTF8 (~{449}
D/IMAP ReadNextLine [rv=0x0 stream=0x7fa3cc5ef9d0 nb=12 needmore=0]
I/IMAP 0x7fa3cc3cf000:imap.gmail.com:A:CreateNewLineFromSocket: + go ahead
D/IMAP SetConnectionStatus(0x0)
I/IMAP 0x7fa3cc3cf000:imap.gmail.com:A:SendData: To: [e-mail]
From: Gene Smith <[e-mail]>
Subject: send from mba
Message-ID: <[e-mail]>
Date: Fri, 20 Sep 2019 20:39:31 -0400
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:60.0)
Gecko/20100101 Thunderbird/60.8.0
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
2021-01-25 23:17:54.938167 UTC - [Parent 160842: IMAP]: I/IMAP Content-Language: en-US

test save to 2018

I/IMAP 0x7fa3cc3cf000:imap.gmail.com:A:SendData: )
D/IMAP ReadNextLine [rv=0x0 stream=0x7fa3cc5ef9d0 nb=51 needmore=0]
I/IMAP 0x7fa3cc3cf000:imap.gmail.com:A:CreateNewLineFromSocket: 15 NO [TRYCREATE] Folder doesn't exist. (Failure)

This could be written as two independent bugs but since they are closely related, I've only made one. Now two independent bugs, see Bug 1690093 for the issue described in the top part of this comment.

Comment 1

[gene smith]

The password issue described in this comment is moved to Bug 1690093
The problem with the password occurs here: https://searchfox.org/comm-central/rev/7922a3609264e98d1f190eb090d2bc7c1104b3bd/mailnews/imap/src/nsImapProtocol.cpp#8195

If there has been no folder selected for the server it has no "window" yet. So the NS_ENSURE_TRUE causes the GetPassword() to return an error.

I think this check is done, as the code says, for "biff". This must prevent a password prompt from occurring during "biff" (timed check for new mail).

Commenting out the NS_ENSURE_TRUE allows password to be obtained and the authentication to occur. However, I'm sure that's not the correct solution. Don't know what is.

Comment 2

[gene smith]

The problem with appending to the destination folder having a MUTF-7 name when it should be UTF-8 is caused by the set-up of the "EnsureExists" URL before any connection is established to the destination server. This occurs here where the encoding is selected to be MUTF-7 or UTF-8: https://searchfox.org/comm-central/rev/7922a3609264e98d1f190eb090d2bc7c1104b3bd/mailnews/imap/src/nsImapService.cpp#2275

If there is no connection yet, the flag utf8AcceptEnabled will return false and the URI is encoded MUTF-7.

A possible solution is to maybe defer the encoding until it is know if UTF8=ACCEPT is supported, after the first authentication of the destination server occurs.

Another possibility is to notify the user that a connection to the destination server must be made before a copy/move of a folder to a destination server can occur.

Another possibility is to first force a "select" url to INBOX of the destination server before encoding the "EnsureExists" url so the UFT8=ACCEPT flag is known. It may be helpful if utf8AcceptEnable is a 3-value enum instead of boolean: NotSet, UTF8, MUTF7. So if the value returns is NotSet, only then do we do the "select" url and wait for result to become UTF8 or MUTF7. (This sounds too complicated for nsImapService, not sure.)

Comment 3

[gene smith]

Attachment: fix-utf8-drag-drop-1st-try.diff

A possible solution is to maybe defer the encoding until it is know if UTF8=ACCEPT is supported, after the first authentication of the destination server occurs.

To me this seems like the easiest thing to try. The diff does this and it seems to work when I dd a non-ascii named folder from my isp into gmail. It would fail before.

It checks if an URL occurs before the connection is authenticated. If it does and the URL turns out to be the imap action "EnsureExists" and the folder doesn't exist and UTF8=ACCEPT is in effect then I convert the ascii folder name from MUTF-7 to UTF-16 and then from UTF-16 to UTF-8 and then create the folder.

I had to duplicate some code due to a "const" attribute on the folder name. Maybe there's a better way. Also, I probably didn't guess the right string types or conversion functions so I'm sure you (Klaus B.) can do some optimization and improvements, but it does work.

(This doesn't address the other issue regarding password so it only fixes gmail.)

Comment 4

[Klaus B.]

Comment on attachment 9199188 [details] [diff] [review]
fix-utf8-drag-drop-1st-try.diff

Review of attachment 9199188 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable, thank.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +6332,5 @@
>      Subscribe(aSourceMailbox);
>    } else {
> +          MOZ_LOG(IMAP, LogLevel::Debug,
> +                  ("create mailbox"));
> +    if (m_urlOccurredWhenNonAuthenticated &&  m_allowUTF8Accept) {

Double space after && (but clang-format will fix that). m_allowUTF8Accept is for the target folder, yes?

@@ +6335,5 @@
> +                  ("create mailbox"));
> +    if (m_urlOccurredWhenNonAuthenticated &&  m_allowUTF8Accept) {
> +      // Convert aSourceMaibox from MUTF-7 to UTF-16
> +      nsAutoString utf16BoxName;
> +      CopyMUTF7toUTF16(nsDependentCString(aSourceMailbox), utf16BoxName);

Should that be
CopyFolderNametoUTF16(nsDependentCString(aSourceMailbox), utf16BoxName); ?
Do we know for sure that the source is MUTF-7? Sure, that's the case you're focused on here, but doesn't the problem also arise went copying between two UTF-8 servers?

Comment 5

[Klaus B.]

s/thank/thanks/ ;-) - If I edit that comment, it messes up completely.

Comment 6

[alta88]

Why create yet another variable when auth state is already obtainable.

nsIImapIncomingServer.QueryInterface(Ci.nsIImapServerSink).userAuthenticated;

And any user initiated action (meaning not a biff, nor a background move/copy filter, etc.) should obviously raise an auth prompt if not yet authed and auth is required.

Comment 7

[gene smith]

While working with this I noticed something else and filed a new bug: Bug 1689014. It doesn't seem to be related to the UTF8/MUTF7 issues.

Comment 8

[gene smith]

Attachment: fix-utf8-drag-drop-2nd-try.diff

The MOZ_LOGs I added are just for personal debug and are temporary. I just wanted to see where they occur in relation to the other log entries. So added them instead of printf.

Should that be
CopyFolderNametoUTF16(nsDependentCString(aSourceMailbox), utf16BoxName); ?
Do we know for sure that the source is MUTF-7? Sure, that's the case you're focused on here, but doesn't the problem also arise went copying between two UTF-8 servers?

It is only necessary to convert to UTF-8 if the authenticated destination server connection has utf8=accept enable and the destination folder passed into nsImapProtocol::OnEnsureExistsFolder() is ascii. So I'm essentially doing the 1st half of CopyFolderNameToUTF16() inline.

This eliminates the need for m_urlOccurredWhenNonAuthenticated as suggested in comment 6 but for a different reason. If the ensureExists url occurs before the connection is authenticated, the folder name will be encoded as ascii. If it's actually MUTF-7 ascii it will get changed to UTF-8 to create the folder, otherwise the folder is just created with the ascii char string or UTF-8 octet string passed in as-is.

doesn't the problem also arise went copying between two UTF-8 servers?

Yes, but the folder name string encoded at nsImapService::EnsureFolderExists() only depends on the state of the destination server. The source server being UTF8 or not is n/a.

Comment 9

[Klaus B.]

Sorry, I'm still confused. I thought the issue is that you don't know the encoding on the destination server. Now you do, in m_allowUTF8Accept? Or is this for the source? But why test its existence? How can you produce anything for the destination without knowing its encoding?

Generally there are four cases:

1. MUFT-7 to MUTF-7 2) MUTF-7 to UTF-8 3) UTF-8 to MUTF-7 and 4) UTF-8 to UTF-8.

Assuming m_allowUTF8Accept is for the destination, in the patch you've covered 1) and 2). For case 3) you start with testé on the source server in UTF-8, m_allowUTF8Accept is not set, we put testé as UTF-8 onto a MUFT-7 server.

Why is the snipped that's needed any different to all the other hunks we have now?

CopyFolderNametoUTF16(sourceMailbox, utf16BoxName);
if (m_allowUTF8Accept) {
  CopyUTF16toUTF8(utf16BoxName, boxNameUtf7or8);
} else {
  CopyUTF16toMUTF7(utf16BoxName, boxNameUtf7or8);
}

If m_allowUTF8Accept were for the source, then it would be different.

Sorry, all this may make on sense at all since I'm really confused and don't understand the context of the patch, particularly given that the 1st patch was about missing authentication when the second one just uses m_allowUTF8Accept.

Comment 10

[gene smith]

I hope this explains the context. Sorry, it is kind of long and complex to explain and took a while to write, but here's my attempt:

Copying (dragging) a folder between accounts causes the UI to eventually trigger a call to EnsureFolderExists() here: https://searchfox.org/comm-central/rev/42554810f7d3b26099cdfec046df7dce7111f42b/mailnews/imap/src/nsImapService.cpp#2244. Passed into this is the name of the folder being copied, newFolderName, encoded in UTF-16. Also passed into EnsureFolderExists() is the "parent" folder which is the folder on the destination server the copy is going to. Then in EnsureFolderExists(), "parent", after conversion to "imapFolder" is used to determine if UTF8=ACCEPT is in effect for the destination (parent) folder.

If the destination folder (actually mailbox) has been imap connected and authenticated, then the state of UTF8=ACCEPT will be known (i.e., utf8AcceptEnabled will be set true or false in EnsureFolderExists().

But if the an imap connection for the destination folder has yet to occur, utf8AcceptEnabled will be set false. This will cause the newFolderName to be copied from UTF-16 to MUTF-7. This cause EnsureFolderExists() to encode and pass to nsImapProtocol.cpp the "EnsureExists" URL with the folder name incorrectly encoded in MUTF-7 (assuming the destination folder is, in the future, determined to be UTF8=ACCEPT capable).

Note: It is important to understand that UTF8=ACCEPT capability is determined for each authenticated IMAP connection that occurs for a server. There is no server-global UTF8=ACCEPT capability flag.

Of course, if EnsureFolderExists() sees utf8AcceptEnable true, it will copy newFolderName to UTF-8 and encode the result in the "EnsureExists" URL in UTF-8 and there is no ambiguity because the destination folder (mailbox) has an authenticated imap connection when EnsureFolderExists() is called.

Now we move to nsImapProtocol.cpp where the "EnsureExists" URL is processed in OnEnsureExistsFolder() which is the subject of the patch. See https://searchfox.org/comm-central/rev/42554810f7d3b26099cdfec046df7dce7111f42b/mailnews/imap/src/nsImapProtocol.cpp#6304 showing the unmodified code.

The running of the URL occurs in OnEnsureExistsFolder() and will only occur if the imap connection is in authenticated state even though the URL was possibly triggered in nsImapService::EnsureFolderExists() before a connection occurred (and the state of UTF8=ACCEPT was not yet set).

Passed into OnEnsureExistsFolder() is "aSourceMailbox" which is actually the name of the destination folder (not source) we want to ensure exists before messages are copied into it. It contains the folder name as a string of bytes (chars*) encoded as either MUTF-7 or UTF-8 as obtained from the URI string produced in nsImapService::EnsureFolderExists().

In the patch, after determining that aSourceMailbox does not exist and must be created, I first check if UTF8=ACCEPT is in effect by looking at m_allowUTF8Accept. If it is not set, the folder is created using aSourceMailbox as is. If m_allowUTF8Accept is true, I then look at aSourceMailbox and determine if it is all US-ascii bytes or if it contains UTF-8 non-US-ascii bytes. If it contain UTF-8 bytes, the folder is also created using aSourceMailbox as is.

But if m_allowUTF8Accept is true and aSourceMailbox is US-ascii, then it's possible the URL was created incorrectly with MUTF-7 encoding because the connection and authentication had not yet occurred. So at this point the name might be US-ascii or MUTF-7 (both are technically MUTF-7), so now copy the MUTF-7 string to unicode UTF-16. Finally, copy the UTF-16 to UTF-8 and create the folder with the UTF-8 string (which still may be US-ascii if the intended destination folder for the copy was actually US-ascii).

3. you start with testé on the source server in UTF-8, m_allowUTF8Accept is not set, we put testé as UTF-8 onto a MUFT-7 server.

To be sure, I tried this between gmail (UTF-8=ACCEPT capable) and my ISP's server (not UTF8=ACCEPT) and MUTF-7 was copied to ISP. Anyhow, I'm somewhat unclear on the other comments and questions. So read my thesis and maybe/probably you will see some holes in it. Thank :).

Comment 11

[Klaus B.]

Let me try: EnsureFolderExists() is called, it calculates the wrong folder name since the UTF-8-ness of the target isn't yet known. So instead of UTF-8 you get MUTF-7. Then GetImapConnectionAndLoadUrl(imapUrl, nullptr, url); supposedly triggers OnEnsureExistsFolder() with the incorrectly encoded folder name. At that stage, the connection is now authenticated, so you can now rely on m_allowUTF8Accept for the target folder, right?

So it the folder name is ASCII and potentially MUTF-7, we move it back to UTF-16 and then UTF-8. I guess I understood this now. I'll make a suggestion for a clearer/longer comment.

So let's go through the four cases to see whether there is anything missing. Let's do the "not yet authenticated case first":
MUTF-7 or UTF-8 (no auth, looks like MUTF-7) to UTF-8: you come in with (wrong) MUTF-7 and turn that into UTF-8. Fine.
MUTF-7 or UTF-8 (no auth, looks like MUTF-7) to MUTF-7: you come in with (wrong) MUTF-7 and don't touch it. Also Fine.

Now the auth case:
MUTF-7 or UTF-8 to UTF-8: you come in with (right) UTF-8, if that's pure ASCII you still go via UTF-16 to UTF-8, and the result is unchanged. Fine.
MUTF-7 or UTF-8 to MUTF-7: you come in with (right) MUTF-7, and don't touch it. Also Fine.

OK, looks like the logic is right :-) - So here my suggestion for a comment:

// We need to handle the following edge case where the destination server wasn't authenticated
// when the folder name was encoded in `EnsureFolderExists()`. In this case we always get
// MUTF-7. Here we are authenticated and can rely on `m_allowUTF8Accept`.
// If the folder appears to be MUTF-7 and we need UTF-8, we re-encode it. If it's not ASCII, it must
// be already correct in UTF-8. And if it was ASCII to start with, it doesn't matter that we MUTF-7 decode
// and UTF-8 re-encode.
if (m_allowUTF8Accept) {

The last sentence is more like sales gloss since it's not strictly true. Heuristic like that leads converting plain ASCII that accidentally is a valid MUTF-7 string to be mal-interpreted.

Comment 12

[Klaus B.]

Let me fix the string issues in the patch ;-)

Comment 13

[gene smith]

The last sentence is more like sales gloss since it's not strictly true. Heuristic like that leads converting plain ASCII that accidentally is a valid MUTF-7 string to be mal-interpreted.

Yes I think one of your 1st comments a few weeks (days?) ago involved intentionally creating a us-ascii folder with a valid MUTF-7 string and it shows up as something else in UTF-8. Is that what you are fixing in the patch? (No need to answer, I'll wait and see!)

Comment 14

[Klaus B.]

Attachment: fix-utf8-drag-drop-2nd-try.diff (Klaus)

Fixed commit message, string processing, comments and doubled-up code. Yes, you can assign to an input parameter ;-) - However, why do we fix the name sooo late? Don't the other calls also need the correct name? Should that code not move up to the start of the function?

Comment 15

[Klaus B.]

Is that what you are fixing in the patch?

No that's a general issue and possibly wrong all over the place.

Comment 16

[Klaus B.]

Oops, that patch was too "elegant" for it's own sake, the string will be destroyed when utf8BoxName goes out of scope. Let me try again.

Comment 17

[Klaus B.]

Attachment: fix-utf8-drag-drop-2nd-try.diff (Klaus)

OK, this should do it. If you move the block up, it might be nicer to add a:
const char* sourceMailbox = aSourceMailbox; and then assign to the former. I can have another look then.

Comment 18

[gene smith]

Looks good!

Yes, you can assign to an input parameter ;-)

I was going to do exactly what you did but I saw that "const char*" on the parameter and thought it would prevent me from assigning a different value to the string and I never even tried it. (Ok, looked it up. It means I can't change the original string aSourceMailbox points to, but I can change the pointer itself, like you did. I think, maybe...)

The first part is not real clear to me what it does, something about allocating canonical strings. I'll have to look deeper into that.

Comment 19

[gene smith]

Attachment: fix-utf8-drag-drop-3rd-try-gene.diff

I agree. The name fix-up needs to occur at the beginning so moved it there.

However discovered a problem with the original fix-up method. The aSourceMailbox can be multi-level with hierarchy delimiters. The higher levels are already UTF-8 and only the leaf level potentially is incorrectly ascii. So we must check for ascii only on the leaf and not the whole aSourceMailbox.

I saw the problem when I tried to drop the non-ascii folder onto a UTF-8 non-ascii folder in gmail. I guess I had only tested by dropping the folder at root or into a ascii named gmail folder.

Anyhow, here's my probably non-optimal attempt to deal with just the leaf. It's based on other code doing similar things I found in nsImapProtocol.cpp.

P/s: It looked like you last diff had some redundant code after call the Subscribe() that I took out. It's late here and I could be wrong on this.

Comment 20

[Klaus B.]

Comment on attachment 9199719 [details] [diff] [review]
fix-utf8-drag-drop-3rd-try-gene.diff

Review of attachment 9199719 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I'm just a style pedant :-(
The string guide https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings suggests to use auto strings for local variables.
I can't speak to whether this actually works. You only want to treat the leaf name?
What happens with a sub-sub-folder? You drag the original folder testé3 from testé1/testé2/testé3 to desté1/desté2/desté3 so it becomes desté1/desté2/desté3/testé3. Don't you need to re-encode the entire path?

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +6311,5 @@
> +  // doesn't matter that we MUTF-7 decode and UTF-8 re-encode.
> +
> +  // aSourceMailbox is a path with hierarchy delimiters possibly. For the edge
> +  // case we only want to check the leaf folder for ascii and is only
> +  // necessary when m_allowUTF8Accept is true.

`aSourceMailbox`, `m_allowUTF8Accept`, ASCII.

@@ +6314,5 @@
> +  // case we only want to check the leaf folder for ascii and is only
> +  // necessary when m_allowUTF8Accept is true.
> +
> +  // Need this at top level so stays in scope.
> +  nsCString oldBoxName(aSourceMailbox);

nsAutoCString.

@@ +6320,5 @@
> +    char onlineDirSeparator = kOnlineHierarchySeparatorUnknown;
> +    m_runningUrl->GetOnlineSubDirSeparator(&onlineDirSeparator);
> +
> +    int32_t leafStart = oldBoxName.RFindChar(onlineDirSeparator);
> +    nsCString leafName;

nsAutoCString

@@ +6321,5 @@
> +    m_runningUrl->GetOnlineSubDirSeparator(&onlineDirSeparator);
> +
> +    int32_t leafStart = oldBoxName.RFindChar(onlineDirSeparator);
> +    nsCString leafName;
> +

Lose empty line.

@@ +6322,5 @@
> +
> +    int32_t leafStart = oldBoxName.RFindChar(onlineDirSeparator);
> +    nsCString leafName;
> +
> +    if (-1 == leafStart) {

leafStart == kNotFound

@@ +6323,5 @@
> +    int32_t leafStart = oldBoxName.RFindChar(onlineDirSeparator);
> +    nsCString leafName;
> +
> +    if (-1 == leafStart) {
> +      leafName = oldBoxName;  // this is a root level box

Move comment to its own line:
// This is not a root level box.

@@ +6334,5 @@
> +
> +    if (NS_IsAscii(leafName.get())) {
> +      MOZ_LOG(IMAP, LogLevel::Debug,
> +              ("re-encode leaf of mailbox %s to UTF-8", aSourceMailbox));
> +      nsAutoString utf16BoxName;

Should now be: utf16LeafName

@@ +6338,5 @@
> +      nsAutoString utf16BoxName;
> +      CopyMUTF7toUTF16(leafName, utf16BoxName);
> +
> +      // Convert UTF-16 to UTF-8 to create the folder.
> +      nsAutoCString utf8BoxName;

utf8LeafName

Comment 21

[gene smith]

From comment 20:

What happens with a sub-sub-folder? You drag the original folder testé3 from testé1/testé2/testé3 to desté1/desté2/desté3 so it becomes desté1/desté2/desté3/testé3. Don't you need to re-encode the entire path?

No. The reason is that the desté1/desté2/desté3 is already existing and known mailbox name at the destination server. They are correctly encoded into the EnsureExists URL as UTF-8 and is derived from the "parent" parameter passed into nsImapService::EnsureFolderExists(). The only thing potentially needing re-encoding is the leaf testé3 derived from parameter newFolderName.

However, your example did bring up another possibility that I hadn't tested and didn't expect to work. That is, what happens if you drag source folder testé1 (at the top of the hierarchy) to, say, desté2. Will it copy just testé1 or will it copy testé1 and everything below (recursively)? If it copies recursively, it looks like the leaf will be be testé3 and, with my latest diff, only it will be re-encoded.

It turns out it does copy recursively but each sub-folder is copied separately from highest to lowest level. So testé1 gets re-encoding since it is the "bad" leaf (due to not yet connected and authenticated) on the first EnsureExists URL. Then the testé2 and then testé3 are encoded as the leaf in separate EnsureExists but now the connection is established and authenticated so the EnsureExists URL leaf is encoded with UTF-8 and no re-encode is needed. So it actually works!

Comment 22

[Klaus B.]

OK, thanks. You can fix the nits and present it for review then. Also "Need this at top level so it stays in scope" should have "it" inserted.

Comment 23

[gene smith]

Attachment: fix-utf8-drag-drop-v0.patch

Sorry for the delay in submitting this. Needed to do some non-tb stuff.
Also, got side-tracked trying to fix the "top half" of this bug's comment 0 regarding passwords and never found a satisfactory fix for that. So I moved the "top half" regarding passwords to a separate bug to keep from holding up any longer the re-encoding to UTF-8 fix (bottom half of comment 0) contained in this patch.

Comment 24

[Klaus B.]

Comment on attachment 9200527 [details] [diff] [review]
fix-utf8-drag-drop-v0.patch

Review of attachment 9200527 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks fine to me but I'm not a peer here.

Comment 25

[Ben Campbell]

Comment on attachment 9200527 [details] [diff] [review]
fix-utf8-drag-drop-v0.patch

Review of attachment 9200527 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, looks fine to me.
But... ouch - the whole MUTF-7/UTF-8 stuff is a mess. I'm going to file a new bug to try and clean up the madness a little.

EDIT: I filed Bug 1690403

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/aa5b7f9770fa
Change encoding from MUTF-7 to UTF-8 if necessary when running EnsureExists URL. r=benc

Comment 27

[Klaus B.]

Comment on attachment 9200527 [details] [diff] [review]
fix-utf8-drag-drop-v0.patch

[Approval Request Comment]
Regression caused by (bug #): bug 1571672
User impact if declined: Dragging of IMAP folders sometimes doesn't work, especially on Gmail
Risk to taking this patch (and alternatives if risky):
This should join bug 1687727 on TB 86 beta. It's not particularly risky, just a small encoding tweak that didn't make it into bug 1687727 (which landed on branch day hours before the branch).

Comment 28

[Wayne Mery (:wsmwk)]

Comment on attachment 9200527 [details] [diff] [review]
fix-utf8-drag-drop-v0.patch

[Triage Comment]
Approved for beta

AIUI, none of this is covered by tests?

Comment 29

[Klaus B.]

To my knowledge, there are no tests for dragging anything anywhere in Mozilla since the dragging action can't be simulated.

That said, there is poor test coverage for all the stuff that was repaired in bug 1687727 and all the other regressions of bug 1571672 which weren't about dragging at all. Check Ben's testing ambitions in bug 1690403.

Comment 30

[Magnus Melin [:mkmelin]]

There are some dragging tests: e.g. around https://searchfox.org/comm-central/search?q=synthesizeDragOver&path=&case=true&regexp=false
Though not for this.

Comment 31

[Rob Lemley [:rjl]]

Thunderbird 86.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/9feea402684c

Comment 32

[gene smith]

(In reply to Wayne Mery (:wsmwk) from comment #28)

AIUI, none of this is covered by tests?

As you understand it seems correct. :)