1746348 - Problems if foldername has trailing white-space (webexperiment, compFields.fcc)

Status: RESOLVED FIXED

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

Description

[Günter Gersdorf]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:95.0) Gecko/20100101 Firefox/95.0

Steps to reproduce:

Create a folder with trailing white-space.
Select this folder as sent folder
Send a message

In a webexperiment, set the compFields.fcc to a folder with trailing white-space

Actual results:

A new folder without the trailing white-space is created and used as sent folder.
Even worse, if the original foldername contains non-Ascii characters (e.g. 'Späce '), the newly created folder contains encoded characters (e.g. 'Sp&AOQ-ce').

the fcc is ignored and the default sent folder is used.

Expected results:

Foldername should be used as-is.

Comment 1

[Thomas D. (:thomas8)]

Hi Günter, thanks, and I believe you... but why on earth would anyone create a mail folder name with trailing whitespace? What type of whitespace is it? Such folder names should be prohibited imo. But if they are created on IMAP server, we'd probably need to handle them anyway. Can you elaborate where and why the whitespace was added to the folder name?

Comment 2

[John Bieling (:TbSync)]

Are you able to provide an STR which does not involve an Experiment? Can this bug be triggered as a standard user interacting with a default Thunderbird?

Comment 3

[Mark Banner (:standard8)]

My bet is that this is most likely one of the many regressions that bug 1571672 has caused. There's a ton of fixes which I believe are going into a 91.4.1 dot release, so they may help.

Comment 4

[Günter Gersdorf]

@Thomas
No one would create a foldername with trailing whitespace, but it happens. My add-on 'CopySent2Current' makes heavy use of the compFields.fcc field. One of my users ask me why the add-on doesn't work on a particular folder. He send me a picture of his folders and i recognize the trailing whitespace.

I would think this problem is caused by a call to trim() in the function getFcc() in the module MimeMessageUtils.jsm

@John
what is 'STR'?
I think the problem with compFields.fcc can't be triggered without an experiment, since the fcc field is not exposed in the compose API. But point one of my original post can be done by anyone.

@Mark
I don't think that the bug is related to bug 1571672. There is no problem with german umlauts if the foldername does not have a trailing white-space.

Comment 5

[John Bieling (:TbSync)]

STR = steps to reproduce

I think the problem with compFields.fcc can't be triggered without an experiment, since the fcc field is not exposed in the compose API. But point one of my original post can be done by anyone.

There is no internal/core use of the fcc field?

Comment 6

[Günter Gersdorf]

I think the fcc field is not normally used. If its empty the identity.fccFolder is used.

Comment 7

[John Bieling (:TbSync)]

Ok, thanks for reporting. I will check what we can do.

Comment 8

[John Bieling (:TbSync)]

I think this should be moved elsewhere, as it is not related to WebExtension API.

I tried to have a look at the code. We have

• getMsgFolderURIFromPrefs() -> uses identiy.fccFolder
• getFcc() -> uses identiy.fcc

I have no idea why we have those two different places to store the fcc. Do you Günter?

In getFcc() the value gets trimmed:
https://searchfox.org/comm-central/rev/68c2c1641ee0e1c9f320ff304fb8276d540a18ea/mailnews/compose/src/MimeMessageUtils.jsm#201

So it checks whether the folder exists and if so, returns the trimmed folder name, which will probably cause your issue down the road, when the returned fcc value is used to get the actual folder.

Who could know the reason behind the trim line? Magnus?

Comment 9

[John Bieling (:TbSync)]

This is probably where the wrong folder than gets created:
https://searchfox.org/comm-central/rev/68c2c1641ee0e1c9f320ff304fb8276d540a18ea/mailnews/compose/src/MessageSend.jsm#907

Günther, could you verify that with TB 91.4.1 or TB Beta, the issue with the non-ascii characters has been fixed and only the trim issue remains?

Comment 10

[Günter Gersdorf]

getFcc does not use identity.fcc (this does not exist), it uses compFields.fcc

And neither 91.4.1 nor 96b3 fixes the problem 1 of my original post.

If you create a folder with trailing space (e.g. 'INBOX/Space ') and select that folder as sent folder identity.fccFolder is set to 'INBOX/Space' (without the trailing space).

Comment 11

[John Bieling (:TbSync)]

getFcc does not use identity.fcc (this does not exist), it uses compFields.fcc

Thanks.

And neither 91.4.1 nor 96b3 fixes the problem 1 of my original post.
If you create a folder with trailing space (e.g. 'INBOX/Space ') and select that folder as sent folder identity.fccFolder is set to 'INBOX/Space' (without the trailing space).

I meant the äöü issue, to confirm Marks statement in Comment #3.

Edit: Removed an observation which turned out to be wrong, caused by a misconfiguration on my side.

Comment 12

[John Bieling (:TbSync)]

@Günter: I do not know if removing the trim mentioned in Comment 8 is feasible or if that has other side effects. While waiting a response from Magnus, I was looking at the compFields.fcc2 field, which is accessible through the composer UI and which I would like to make available through the WebExtension set/getComposeDetails() function.

Will your add-on work using that field or is there a specific reason you need to use compFields.fcc?

Comment 13

[Günter Gersdorf]

There is no issue (and has never been) with äöü. The problem only arises if the foldername has a trailing space AND the folder is used as a special folder.

And i think, both compFields.fcc and compFields.fcc2 are normally used. compFields.fcc contains the sent folder from the identity and compFields.fcc2 is set to the folder choosen in the compose UI (and defaults to 'nocopy://')

And my add-on also uses both fields. fcc is set to the folder which can be choosen from the composer UI (and replaces the sent folder from the identity) and fcc2 is set to the sent folder from the identity if the user has set an 'additionally copy to sent folder' global option.

Comment 14

[John Bieling (:TbSync)]

There is no issue (and has never been) with äöü. The problem only arises if the foldername has a trailing space AND the folder is used as a special folder.

The topic of this bug is "Problems if foldername has trailing white-space or non-ASCII characters like "äöü" (webexperiment, compFields.fcc)". Additionally, you stated in Comment #0:

Even worse, if the original foldername contains non-Ascii characters (e.g. 'Späce '), the newly created folder contains encoded characters (e.g. 'Sp&AOQ-ce').

All I want to find out: Has that Späce issue been resolved by now, as Mark suggested, so I can concentrate on the trailing space issue? Or is it still using the bad encoding when creating the new folder due to the trailing space?

Comment 15

[Günter Gersdorf]

The 'or non-ASCII characters like "äöü" (webexperiment, compFields.fcc)' part of the topic was added by someone else, it was not part of the original topic.

And yes, the 'Späce' issue is fixed (in the sense that this was never an issue!)

Comment 16

[John Bieling (:TbSync)]

If I understood the code correct, setting .fcc allows to temporarily disable or override the default fcc per message, which is not used in core. And .fcc2 is mapped to the UI selection the user can use to set an additional fcc.

Would your add-on benefit from making these two fields accessible via WebExt API? My suggestion: Setting the WebExt primaryFcc/secondaryFcc to false would disable them and setting them to a WebExt folder would set/override them.

Comment 17

[Mark Banner (:standard8)]

I think the problem may be that when you are setting compFields.fcc, you are not using an encoded url. An encoded url would have the space encoded, which would then not get trimmed.

The .trim that is present seems to be a translation of the original code to fix the header: https://searchfox.org/comm-esr91/rev/b4fc9021bb71e2e7ff454b2181a95f4ccaa6f857/mailnews/compose/src/nsMsgSend.cpp#2160

That code isn't quite equivalent to the new code, but I'm guessing we probably didn't need all the old code anyway.

I think that would work. I've been trying to simulate this with going offline & sending a message and then going back online and letting it send, but it looks like that is getting messed up elsewhere and a separate folder gets created.

Comment 18

[Magnus Melin [:mkmelin]]

In general I don't think any of our code (wx nor core) should allow creating folders with trailing spaces. I'm not sure how widely servers (happen) to support it either. Exchange doesn't allow it.

Comment 19

[Günter Gersdorf]

@John

Would your add-on benefit from making these two fields accessible via WebExt API?

This would probably allow me to make the add-on pure webext.

@Mark
I've tried so set an encoded url but that does not work.
Please note the difference of uri and url. For a folder 'Space '
folder.URI contains 'imap://[e-mail]/INBOX/Space ' while
folder.folderURL contains 'imap://[e-mail]/INBOX/Space%20'
so i think fcc has to be set to an uri not an url.

@Magnus
As long as there are IMAP servers and mailprogramms/webmailers which allow creating mailboxes with trailing spaces Thunderbird must handle them. And well, currently TB has no problems with such folders, e.g. it's possible to move messages to such folders. The problem only arises if the folder is used as a special folder!

Comment 20

[John Bieling (:TbSync)]

Attachment: Bug 1746348 - Do not trim valid fcc uri. r=mkmelin

Comment 21

[John Bieling (:TbSync)]

Attachment: Bug 1746348 - Expose msgCompFields.fcc and msgCompFields.fcc2 via compose.ComposeDetails. r=mkmelin

Depends on D144419

Comment 22

[John Bieling (:TbSync)]

@Günter: The patch exposes fcc2 (the value connected to the UI selection) as ComposeDetails.fcc. (Nobody besides you knows that it is fcc2 internally). The internal fcc value is not set by Thunderbird and I hesitate to expose that (as it might be subject to removal).

What is the purpose of setting it in your add-on? Just to be able to create two copies?

Comment 23

[Günter Gersdorf]

No. I set fcc to override the sent folder selected in the account settings for this one mail.

The normal case is: Mail is saved to the folder defined in the accounts settings, if that is defined. If the user selects a folder via UI the mail is additionally saved to that folder.

If i would use fcc2, always two copies would be saved (as long as a sent folder is defined in the accounts settings, which i think is the normal case)

Comment 24

[John Bieling (:TbSync)]

So, I should expose fcc as "defaultFcc" and fcc2 as "additionalFcc". Other suggestions for nice human readable names?

In addition, should I make defaultFcc return the actual used standard folder if it is not set, instead of ""?

Comment 25

[Günter Gersdorf]

So, I should expose fcc as "defaultFcc" and fcc2 as "additionalFcc". Other suggestions for nice human readable names?

Sounds good

In addition, should I make defaultFcc return the actual used standard folder if it is not set, instead of ""?

Probably yes, this would allow to distinguish between "Sent folder defined" and "No sent folder defined" in the accounts settings.

Comment 26

[John Bieling (:TbSync)]

@Günter: Returning the identity fcc was not working out so great. You can now set "defaultFcc" to default, disabled or a MailFolder.

What I changed is, that overriding the default fcc by setting a MailFolder will now always do fcc, even though the user pref is disabled. I think this is a better fit for the override character of the setting: Either the default setting is used or the override.

The old code skipped getFcc(), if that pref was set and bypassed the override:
https://searchfox.org/comm-central/rev/16430df4c00f639809ba0aea5cf6c31575d55419/mailnews/compose/src/MimeMessageUtils.jsm#188

Objections?

Comment 27

[Günter Gersdorf]

I believe that if you remove the test for userIdentity.doFcc from getFcc() disabling the 'Place a copy in' from the 'copies and folder' account settings won't work any longer.

And if think the

    if (composeFields.fcc.startsWith("nocopy://")) {

line from your patch should be something like

    if (composeFields.fcc=="" || composeFields.fcc.startsWith("nocopy://")) {

because if 'Place a copy in' is disabled fcc is ""

Comment 28

[John Bieling (:TbSync)]

Thanks for your feedback.

I did not remove the doFcc check, I just moved it below the initial check if an fcc value has been set. I still return "" if doFcc is false and no fcc override has been set. But if you think it is better to have just a partial override, I can restore the old behaviour (the folder set in defaultFcc will be ignored, if the used identity has fcc disabled). But it will break my overall concept (see below).

For your second remark: The "disabled" value does not reflect the state of the identity, but the state of the override. This is what I want to achieve:

"default" : follow the default behaviour
"disabled": force the fcc to be disabled (regardless what the default identity setting is)
MailFolder: force the fcc to store a copy in that folder (regardless what the default identity setting is)

That is the most straightforward interface, I think. Do you agree, or am I missing something?

Comment 29

[Günter Gersdorf]

Understood. Ok so far.

Is this correct: if i listen to onBeforeSend and no one had set fcc (e.g. via setComposeDetails()), than 'disabled' in the fcc field of the details reflects the fcc state of the account?

Comment 30

[John Bieling (:TbSync)]

Is this correct: if i listen to onBeforeSend and no one had set fcc (e.g. via setComposeDetails()), than 'disabled' in the fcc field of the details reflects the fcc state of the account?

"disabled" in the defaultFcc field will only be set, if someone has manually called setComposeDetails() and set it to "disabled" or if an Experiment has set composeFields.fcc = "nocopy://". The untouched value will be "default" (regardless of what that means).

I think I should rename composeDetails.defaultFcc to composeDetails.fcc, as it never returns the state of the default identity fcc, but the fcc configuration of that single email: Either it is using the default, enforces a custom value or enforces no fcc at all.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a6c96dac60c2
Do not trim valid fcc uri. r=mkmelin

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8490805c05d5
Expose msgCompFields.fcc and msgCompFields.fcc2 via compose.ComposeDetails. r=mkmelin