1588391 - Folders with space are not found when passed to various APIs

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect, P1)

Description

[Philipp Kewisch [:Fallen][☁️📆]]

STR:

1. Create a folder with a space in it (/INBOX/with space for example)
2. Do something like this:

let folder = await messenger.accounts.get("account1").folders.find(folder => folder.name == "with space");
await messenger.mailTabs.update({ displayedFolder: folder });

Result:
Folder "/INBOX/with space" for account "account1" not found.

Expected:
Folder is correctly selected.

Addl Info:
The API encodes the folder URI to imap://[e-mail]/INBOX/with%20space while the real folder uri is imap://[e-mail]/INBOX/with space. The folder lookup service expects the folder path to be a string exact match to the folder uri, not an uri match. imap folders don't seem to escape spaces to %20, so the folder is not found.

I'm sensing there may be other edge cases, and it might even depend on the protocol type. When fixing this bug we should find out how to make sure we always use the correct path.

I'm marking this P1 since it breaks a bunch of APIs that work with folders (go to folder, copy/move messages to folders, etc). Geoff, do you have cycles to look into this?

Comment 1

[Geoff Lankow (:darktrojan)]

Space appears to be encoded in the URLs of POP or local folders but not IMAP folders. Can we be sure this is the case for all IMAP accounts?

Comment 2

[Jorg K (CEST = GMT+2)]

Can you show us some examples. Going through my e-mail archive I found
imap://[e-mail]:993/fetch%3EUID%3E.INBOX.Jordi%20Knobloch%3E139

So clearly the space is encoded as %20.

Comment 3

[Magnus Melin [:mkmelin]]

Seems to me too we are encoding spaces, and that we should do so.

Comment 4

[Geoff Lankow (:darktrojan)]

But we're not always encoding spaces, e.g. these two folders I just created:

mailbox://nobody@Local%20Folders/Trash/foo%20bar
imap://user@localhost/INBOX/foo bar

The question is, if we change the WE code to handle this based on the server being IMAP, is it going to work for all IMAP servers?

On the other hand if we change the IMAP code to always encode spaces, that has potential to break many things.

Comment 5

[Jorg K (CEST = GMT+2)]

Where did you get that imap: URL from? Mine is from some debug in the bowels of the backend. And can is imap://user@localhost/INBOX/foo bar a real thing? Or did you substitute the real server name with localhost.

Comment 6

[Geoff Lankow (:darktrojan)]

I'm writing a test. That's where the URL is from.

But to answer my own question, I've just created a folder called "this is a test" in my GMail account, loaded it, and gFolderDisplay.displayedFolder.folderURL has encoded spaces. But MailServices.folderLookup.getFolderForURL with encoded spaces doesn't find it. This is messy.

Comment 7

[Jorg K (CEST = GMT+2)]

For folder lookup issues consult your fellow compatriot from the island further south ;-)

Comment 8

[Geoff Lankow (:darktrojan)]

Huh. The folderURL and URI properties of a folder differ. One encodes the spaces, the other does too, but not for IMAP.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1588391-folders-with-spaces-1.diff

This seems wrong but it works.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9101010 [details] [diff] [review]
1588391-folders-with-spaces-1.diff

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

Looks ok. I wonder if this wasn't also broken for IMAP servers using . or something else as separator... (unusual, but exists). 
I do think we should fix the folderURL and URI discrepancy, but let's do that elsewhere.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9101010 [details] [diff] [review]
1588391-folders-with-spaces-1.diff

I guess you want this uplifted.

Comment 12

[Geoff Lankow (:darktrojan)]

Yeah, but I want to see if Ben has anything insightful to say about it.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 9101010 [details] [diff] [review]
1588391-folders-with-spaces-1.diff

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

::: mail/components/extensions/parent/ext-mail.js
@@ +1255,5 @@
>    if (path == "/") {
>      return rootURI;
>    }
> +  if (server.type == "imap") {
> +    return rootURI + path;

Maybe some comment here would be useful to explain why we're doing that, together with an example.

Comment 14

[Jorg K (CEST = GMT+2)]

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

Yeah, but I want to see if Ben has anything insightful to say about it.

OK, I won't land it then. But we'll be building TB 70 beta 4 today or tomorrow.

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1588391-folders-with-spaces-2.diff

Now with a comment.

Comment 16

[Philipp Kewisch [:Fallen][☁️📆]]

Maybe it makes sense to normalize the URI in https://searchfox.org/comm-central/rev/364e5ba7492c8a13e104662a7e43769819e339f7/mailnews/base/src/folderLookupService.js#118 instead? This way you could pass whatever you want and the lookup would find it.

Comment 17

[Jorg K (CEST = GMT+2)]

I'm still confused. Shouldn't the (folder)URI always be percent-encoded? Why should we need to cater for "whatever you want"?

I do think we should fix the folderURL and URI discrepancy, but let's do that elsewhere.

I agree with that.

Comment 18

[Ben Campbell]

Firstly, if Geoff's patch fixes the problem and doesn't break anything, then I say we run with it for now.

It's a symptom of not being 100% clear on the naming/encoding rules in various places. And complicated by the fact that there are names already in the wild we need to continue supporting (eg URIs in virtualfolders.dat).

In this particular case:

What is folderURL? How does it differ to the folder's URI? The comments in the IDL don't help much...

From looking at the implementations, I'm guessing it's for retrieving a URL which you can actually open and request data from?

• For IMAP, folderURL is an escaped version of URI ("imap:" protocol).
• For local folders, folderURL is based on a URL pointing to the filesystem (but with "mailbox:" replacing "file:")
• For news folders, it returns a "news:" or "snews:" url, using the hostname, port and folder name (with some escaping).

(if anyone can confirm this for me, I'll add some comments to the folderURL definition in the IDL file!)

For folder URIs in particular we need to do two things:

1. decide on an canonical encoding/escaping scheme for folder URIs.
   My naive suggestion, under "Folder URIs": https://bugzilla.mozilla.org/show_bug.cgi?id=124287#c26 (all utf8, individual components urlencoded).
   I'm sure there's a proper W3C URI spec somewhere that covers this stuff :-)
   After removing RDF, maybe they're not strictly URIs any more? Just TB-specific folder IDs.

2. figure out how to standardise the URI scheme without breaking existing code and data.
   eg: the folder lookup service could use more tolerant matching, eg:
   "imap:INBOX/foo bar" should equate to "imap:INBOX/foo%20bar" (but not "imap:INBOX%2Ffoo%20bar").

Comment 19

[Ben Campbell]

I just wrote up URI normalisation in folder-lookup-service as Bug 1588944.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7fc4bc60717a
In WebExtensions folder lookup, don't encode path components for IMAP servers; r=mkmelin

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 9101200 [details] [diff] [review]
1588391-folders-with-spaces-2.diff

I don't think there'll be another TB 70 beta.

Comment 22

[Jorg K (CEST = GMT+2)]

TB 68.2.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/a28f7418c659b51b93b8852b779d252810dd30aa

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 9101200 [details] [diff] [review]
1588391-folders-with-spaces-2.diff

Missed TB 70 beta 4.