1428666 - Unwanted Trash folder created in second inbox

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: double inbox+trash.png - screenshot

+++ This bug was initially created as a clone of Bug #1335982 +++

This appears to be a regression from bug 1335982. Every time I startup TB it creates an unwanted second Inbox folder and in it a Trash folder.

I can delete those folders and also unsubscribe from them in TB 52, but when I use Daily, they are coming back.

This is quite annoying since it's hard to get rid of the extra folders. Once I got rid of them in TB 52, they are coming back on the second restart of Daily.

I've confirmed the regression by backing out rev bcfa74cb6921.

That's about the opposite of what bug 1335982 tried to fix :-(

Comment 1

[Jorg K (CEST = GMT+2)]

Hmm, I have two IMAP accounts in my configuration and I see this debug twice when I print out the trash name:
|INBOX/Trash|
However, only one of the accounts creates a another INBOX with a Trash inside. Strange.

Comment 2

[gene smith]

Jorg,
Do you know the imap server type? It seems to be behaving like the OVH/Dovecot server that is the subject of Bug 1427725. On the server creating the ghost INBOX.Inbox.Trash that you show in the attachment, does it have a personal namespace of "INBOX." ? If so, that's the problem.

I have a test account on OVH/Dovecot from the bug reporter and I reverted the bug 1335982 patch and the problem on OVH/Dovecot still occurs. I have identified a fix but haven't submitted a patch since I want more info from the reporter.

Comment 3

[Jorg K (CEST = GMT+2)]

As I said, I have two IMAP accounts, the one that does NOT show a phantom INBOX says:
[CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE NAMESPACE AUTH=PLAIN AUTH=LOGIN] Dovecot ready.

The troublesome says for which I have e-mailed you the login details says:
CAPABILITY IMAP4 IMAP4rev1 UIDPLUS CHILDREN NAMESPACE THREAD=ORDEREDSUBJECT THREAD=REFERENCES SORT QUOTA IDLE ACL ACL2=UNION
I can't find a "name" reply for this server.

In both cases the personal namespace is "INBOX.".

Deleting the unwanted folders in TB 52, emptying the trash and then also unsubscribing from INBOX finally gets rid of them. Resetting the personal namespace to "" and making sure the server can't overwrite it does in fact fix the problem. However, I get mail.server.server1.trash_folder_name set to INBOX/Trash and that's undesired and caused by bug 1335982. I'm still curious to know why accepting full path names and not the leaf name was a good idea. Sadly in my review I picked on the string processing instead of focussing on the overall picture.

Letting the server overwrite the the personal namespace again and backing out rev bcfa74cb6921 from bug 1335982 also fixes the problem here.

I've e-mailed you the access details for the problem server, if you need anything else, please let me know.

Comment 4

[gene smith]

So are you saying with the fixes for  bug 1335982 removed and a default setup, you don't see a problem on either account? I do see the problem of the internal inbox/inbox/trash being created on tb startup or just by collapse/expand of the folder tree for the [e-mail] account.

The account you sent is a Courier imap server. Does the dovecot server that you say works OK have the structure where everything is under INBOX? Or can you create folders at the same level as inbox too? Just curious. The tbtest account you sent only creates folders under inbox.

From what I can see, the fix for  bug 1335982 is OK. It loops through all the folders marked with the trash flag and if there is more than one marked, it clears the flag from those not having the path leaf (e.g., INBOX/trash) equal to mail.server.serverX.trash_folder_name. If there is only 1 marked as trash (always do, never seen more than one marked) and its leaf path doesn't match  trash_folder_name, it sets trash_folder_name to the folder url leaf path.

The string parsing was wrong because for trash folder url imap://user@account/INBOX/Trash it determined the leaf value to be "Trash" and not "INBOX/Trash" so when it compared it to trash_folder_name that is INBOX/Trash they were not equal. So it set trash_folder_name to "Trash". Then on the next restart (or collapse/expand), at another place in the code a new folder Trash (at same level as Inbox) is created since it is found not to exist.

The parsing changes for bug 1335982 fixes the problem for when the Trash folder is placed under Inbox or inside other folders and there is no personal namespace prefix. But for servers having the personal prefix or a '.' separator, as I see on the Courier imap, OVH/Dovecot as well as Elmar's Dovecot, the other place in the code that I mentioned above has a problem that has to be fixed too. This fix I intended to do under Bug 1427725. Also, some of the changes in the patch for Bug 1427507 are also needed for this (specifically, the changes in nsImapProtocol.cpp).

Following the procedure you describe I can make the test account work without creating the phantom inbox.trash. However, when I go into server settings and set the trash folder destination from "Choose folder" to the actual folder, it creates the inner inbox.trash again on restart or collapse/restore. This is with the bug 1335982 fix and the other fixes all backed out (a pristine tree pulled mid-December).

Comment 5

[gene smith]

Attachment: 1428666-review-changes0.patch

I went ahead and made the patch needed to avoid the unwanted phantom trash creation assuming the other trash parsing patch in nsImapIncomingServer.cpp is in place. This seems to fix the problem.

There were two problems in this code that caused failure to realize that the trash folder exists (trashExists set false incorrectly).

1. When the trash path is at the second or deeper level, e.g., Inbox/Trash and the server delimiter is '.', the function AllocateCononicalPath set the serverTrashName to Inbox^Trash. It should remain just Inbox/Trash.

2. If the server has a personal namespace prefix, e.g., pers-prefix or Inbox, the trashMatch string gets set to, e.g., pers-prefix/Inbox/Trash or Inbox/Inbox/Trash that also caused a trashExists to remain false.

So the change is to not call AllocateCanonicalPath() since the trashMatch string passed to it is already "canononical" or slash separated and the correct slashed get escaped/changed to incorrect ^.

Also, I no longer pass the prefix string to CreatePossibleTrashName() to avoid problem 2. 

It appears the trash is working fine on my 3 accounts with . separators and 2 accounts with personal namespace strings. Also on OK on other accounts with more typical / separators and no namespaces.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to gene smith from comment #4)
> So are you saying with the fixes for  bug 1335982 removed and a default
> setup, you don't see a problem on either account? I do see the problem of
> the internal inbox/inbox/trash being created on tb startup or just by
> collapse/expand of the folder tree for the [e-mail] account.
Hmm, somehow we're not coming together here :-(
jorgk.com is my personal domain and I have been using IMAP accounts with default configuration there for ages and I have never seen any phantom folders.

I've just created a new profile 1 with TB 52 and looked at the account. All was well. I started various times, I looked at subscribe (currently broken in Daily, so please keep your mid-December tree for a while). I collapsed/expanded the tree. No phantom folder in sight.

Next I backed out rev bcfa74cb6921 and built. I repeated the same procedure as for TB 52. New profile 2, looked around various times, etc. No phantom folder.

Next I stripped the backout (so bcfa74cb6921 in place again) and repeated the procedure on the new profile 3. Right after I started, I didn't get the phantom folder. I checked the pref and saw mail.server.server1.trash_folder_name set to INBOX/Trash. When I collapsed and expanded the Inbox, the phantom folder showed up.

Next I visited profile 2, the one created with the patch backed out, using the binary which had the patch in place. The phantom folder showed up straight away.

Next I compiled with the patch attached here and created a new profile 4. The phantom folder showed up immediately.

So summarising:
rev bcfa74cb6921 from bug 1335982 DOES cause this problem for me. The patch attached here does NOT fix the problem for me.

I'll discuss the rev bcfa74cb6921 in the next comment.

Comment 7

[Jorg K (CEST = GMT+2)]

So let's look at https://hg.mozilla.org/comm-central/rev/bcfa74cb6921 which I reviewed focussing on string manipulation issues rather than trying it out and looking at the bigger picture. First of all, I think it's not OK, since it causes the regression I described in comment #6 for a usually well behaved IMAP server.

As we saw, the pref trash_folder_name can end up being set to INBOX/Trash, or if I use a Spanish version of TB 58 beta 3 to where this change got uplifted, I see a value of "Bandeja de entrada/garbage" if I select "garbage" as trash folder in the test account. (That "garbage" folder also doesn't receive the trash folder icon, but that's bug 1427507.) If I restart TB 58 b3 ES I now get another phantom folder "Bandeja de entrada" with "garbage" inside. Reverting back to beta 2, I can remove the two phantom folders. And revert back to normal.

Another experiment: Using TB 52 and setting mail.server.server1.trash_folder_name to INBOX/Trash will create the phantom folder after a restart. So again I have reason to believe that the non-leaf value in the preference introduced in bug 1335982 is the source of the problem.

Why do I think the preference should only ever contain a leaf name? For one, the comment (which is still there) says so, not a good argument, I admit.

Secondly, the pref value is returned by nsImapIncomingServer::GetTrashFolderName(). That function gets called a few times around the code base, and I think the callers all expect a leaf name, at least these two:

https://dxr.mozilla.org/comm-central/rev/007155df7eb96eaa992085feb7e086d61d96e577/mailnews/imap/src/nsImapMailFolder.cpp#397
https://dxr.mozilla.org/comm-central/rev/007155df7eb96eaa992085feb7e086d61d96e577/mailnews/imap/src/nsImapMailFolder.cpp#853

So with your kind permission I'd like to backout bug 1335982 and it's unfortunate that this already slipped into a beta.

But let's also consider your comment #4:
> The string parsing was wrong because for trash folder url
> imap://user@account/INBOX/Trash it determined the leaf value to be "Trash"
> and not "INBOX/Trash" so when it compared it to trash_folder_name that is
> INBOX/Trash they were not equal. So it set trash_folder_name to "Trash".
> Then on the next restart (or collapse/expand), at another place in the code
> a new folder Trash (at same level as Inbox) is created since it is found not
> to exist.
You're saying that the comparison fails to detect the trash if the preference was set to INBOX/Trash. I absolutely agree. But nothing in the world should have ever set the preference to that value. I've googled trash_folder_name and I've read various articles that suggested to set a localised name in there, but none to include a path with a slash.

Comment 8

[gene smith]

OK, I must be missing something here. I will revisit this. Maybe for the Elmar bug I was fixing the symptom and not the cause. Thanks for testing of of this.

Comment 9

[Jorg K (CEST = GMT+2)]

Fixed by backing out bug 1335982.

Comment 10

[gene smith]

Observation: With mid-Dec pristine code, except for some printf's I've added, when I set the trash folder in server settings to, e.g., foo/bar/snaf/fu/Trash and click OK, I see SetTrashFolderName(const nsAstring& chvalue) called with the string "foo/bar/snaf/fu/Trash" (per my printf) and when I look at the trash_folder_name in config editor it is also "foo/bar/snaf/fu/Trash". So the UI sets trash_folder_name to a non-leaf/full path value. The trash icon appears at the correct place, but after 2 or 3 collapse/expands I see a new folder named Trash appear at top level, also having the trash icon. Deleted emails are moved to "foo/bar/snaf/fu/Trash" as expected, not to the new top level Trash. 

This is not with your test account but just with my normal charter account. So there is no namespace or separator issues like with dovecot or courier imap servers.

A question to you is do you know where SetTrashFolderName() is actually called? I don't see it in the .cpp code or .js or any of the UI code. I have used DXR and grep'd the whole comm-central, case-insensitive, and can't find where it's called. It sets the trash_folder_name pref to the folder path (not just the leaf) after the trash folder is picked in the server settings. When I look with debugger it seems to be called from assembly.

After a bug was fixed in nsImapIncomingServer.cpp that added the code we are dealing with, you found a very similar error indicating that the bug was not 100% fixed. Not sure if I understand the significance of the language transitions mentioned.
see https://bugzilla.mozilla.org/show_bug.cgi?id=1156669#c28

For most accounts that don't have personal namespace the Trash folder is at the top level so trash_folder_name starts as and remains just, e.g., "Trash". The patch in Bug 1156669 changes trash_folder_name from its configured value (that can be a full path depending on where the trash folder is configured to be). It changes it to the leaf value when trash_folder_name has more than one level, e.g., foo/bar/snaf/fu/Trash to just Trash. (This is what I saw as the original bug that you have now reverted.)

Here is how/where trash_folder_name gets reduced down to the leaf in nsImapIncomingServer.cpp. Other than in SetTrashFolderName() I don't think trash_folder_name is set anywhere else.

> if (numFolders == 1)
> {
>   // We got here because the preferred trash folder does not
>   // exist, but a folder got discovered to be the trash folder.
>   SetUnicharValue(PREF_TRASH_FOLDER_NAME, nameUnicode);
>   continue;
> }

With your test account and with the other test account on OVH-Dovecot server (both have personal namespace "INBOX.") and with none of my patches applied, I can remove any "phantom" Inbox.Inbox.Trash folders and they don't come back after tb restarts or multiple collapse/expands. However, if I then change to a different trash folder, the phantoms come back. Of course, they can be deleted again and then it is OK again as long as I stay with the same trash folder. 

So I expect that since you probably always run with the same trash folder you are OK and don't see a problem.

On Elmar's test account I am not so lucky. An outer trash folder is always created even after it is deleted.

Let me ask one more thing. You definitely ran with both patches in place, i.e., the patch attached above and the patch you backed-out / reverted? I wasn't 100% sure from reading your previous comments. With both in place all accounts work fine for me. Without the patches, things are usually bad.

Comment 11

[Jorg K (CEST = GMT+2)]

I've done further tests:
With a binary without rev bcfa74cb6921 from bug 1335982 I set the trash folder to INBOX/child-of-inbox/Ta-rash with a breakpoint on SetTrashFolderName(). The call comes from JS as you can see from _NS_InvokeByIndex() in the stack.
On Windows the trick is to now inspect any variable and get the system to evaluate |DumpJSStack()|. That gives:
["chrome://messenger/content/AccountManager.js":878], so the call comes from here:
https://dxr.mozilla.org/comm-central/rev/007155df7eb96eaa992085feb7e086d61d96e577/mailnews/base/prefs/content/AccountManager.js#878
Remember that trashFolderName is an attribute in nsIImapIncomingServer.idl, you SetTrashFolderName() gets called when you do
server.trashFolderName = "xxx"; in JS. Of course |dest[slot] = typeArray[slot];| as in AccountManager.js is about as obscure as you can get.

You were right and mail.server.server1.trash_folder_name is set to INBOX/child-of-inbox/Ta-rash.

Next I repeated test 4 from my comment 4, that is the patch from bug 1335982 and the patch from this bug here (re-applied).

At first I created a new profile 5 on the test account and the undesired INBOX.INBOX.Trash (that's what it's called according to IMAP debugging) didn't appear immediately as I had stated yesterday in comment #4, but it did appear after a restart of TB.

I deleted both Trash and INBOX into the "real" trash and emptied it. My experience yesterday was that I needed to unsubscribe from the INBOX in the trash to not have it reappear, but sadly the subscribe dialogue is currently broken in TB 59 Daily due to bug 1425962. So I tried unsubscribing using TB 52, which immediately brought back another set of INBOX + Trash due to the INBOX/Trash preference setting. Sigh.

Again with both patches applied, I started on another new profile 6 and fake INBOX appeared immediately this time, but i can't rule out that there were some remnants of it left on the server from the previous attempt.

So in TB 52 I cleaned up the account and made sure all the undesired folders were gone. Next, again with both patches applied I started again on a new profile 7. As for my test 5 above, the was no extra INBOX on the first start. Also not after collapsing and expanding the folder tree. I checked the preference, I saw INBOX/Trash and restarted various times and the folder DID NOT reappear \o/. I also changed the trash to INBOX/child-of-inbox/Ta-rash and after a restart that folder got the correct icon. I switched back to trash and it all kept working.

So in summary:
Thank you for convincing me that the preference trash_folder_name is indeed set to a path value. That makes bug 1335982 much more acceptable. If I reland it, I will add a HUGE comment to that effect and of course remove the faulty comment talking about a leaf!! Next I will take a good look at the patch here. I'll also revisit bug 1156669 where the leaf extraction was introduced. Time-consuming!!

Thank you for your persistence, it hasn't been easy.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8940965 [details] [diff] [review]
1428666-review-changes0.patch

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +4998,5 @@
>          if (GetDeleteIsMoveToTrash() && !onlineTrashFolderExists &&
>              adoptedBoxSpec->mAllocatedPathName.Find(m_trashFolderName, /* ignoreCase = */ true) != -1)
>          {
>            bool trashExists = false;
> +          nsCString trashMatch(CreatePossibleTrashName(""));

Can you please explain this change? Also, looking at the function, it just prepends the prefix. So no need to call it with "", you can just use m_trashFolderName;

Comment 13

[Jorg K (CEST = GMT+2)]

OK, even in TB 52 you end up with a preference value of INBOX/Trash if you set the trash folder via the Account Manager. And when I restart, the phantom folder is there, and that clearly without bug 1335982. So while you were trying to make things better in bug 1335982 by consistently storing the full path in the preference as the Account Manager does, you, or I, tripped over that other bug.

So I'll re-land bug 1335982 once we settled on a patch for this bug here.

Comment 14

[Jorg K (CEST = GMT+2)]

Hold on, whilst I agree now that storing the *path* in bug 1335982 is OK, I can surely see "Bandeja de entrada/Trash" and I don't think we want to store the localised name, right? Or even worse, using the account manager in my Spanish version I get "Bandeja de entrada/Papelera". Next time you run an English version on the same profile, that is bound to mess up, only ever the real server folder *path* should be stored. This is a hornets' nest!!!

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1428666-review-changes0.patch (JK)

OK, I simplified your patch but I still want to know why it's OK to remove all that processing.

Comment 16

[Jorg K (CEST = GMT+2)]

Grrr, coming back to my comment #14:
Yes, I see "Bandeja de entrada/ ..." when choosing the folder in the Account Manager, that's the error. In TB 53 b3 with the patch from bug 1335982, the path is perfectly well stored as "INBOX/Trash" when starting on a new profile. So there's no fault in bug 1335982, the fault is in the account manager!

So the way forward to cut through this horrible mess is:
1) reland bug 1335982 with a FAT comment.
2) negotiate the patch here
3) File another bug on the Account Manager that messes up localised versions by storing localised strings
   in the preference which should hold the server's folder path.

Do you agree?

Comment 17

[gene smith]

(In reply to Jorg K (GMT+1) from comment #14)
> Hold on, whilst I agree now that storing the *path* in bug 1335982 is OK, I
> can surely see "Bandeja de entrada/Trash" and I don't think we want to store
> the localised name, right? Or even worse, using the account manager in my
> Spanish version I get "Bandeja de entrada/Papelera". Next time you run an
> English version on the same profile, that is bound to mess up, only ever the
> real server folder *path* should be stored. This is a hornets' nest!!!

I remembered that I think said you saw trash_folder_name = Bandeja de entrada/Trash at one point. That worried me too and would or could cause problems. Frankly, I haven't looked at all at how localization works. On the French user of the OVH/Dovecot his inbox is called "Courrier Entrant" or something like that. Not sure if TB uses the Spanish or French internally instead of INBOX.

The code associated with this bug's patch (attached above) uses a hardcoded "INBOX/" to compare strings. I don't really understand why especially if INBOX might be changed to "Bandeja de entrada" or "Courrier Entrant" internally. I assume that shouldn't happen. Maybe that is the "Account Manager" problem you point out.

I need to look closer at this patch. Basically I found that pre-pending the namespace prefix broke the code and caused another trash folder creation. Also, the attempt to make the  path string "canonical", that I think mainly means all separators are converted to slashes, really messed up the result when the separator is '.'. You ended up with a string like INBOX/Inbox^Trash instead of the desired INBOX/Inbox/Trash. This caused trashExists flag to remain false and caused the re-creation of the trash folder at another place in the file.
 
Also, not sure I understand the significance of checking for "starts with INBOX/" and what the comparisons do when it does or doesn't. I will look closer at these issues.

Comment 18

[gene smith]

(In reply to Jorg K (GMT+1) from comment #16)
> 
> Do you agree?

Yes. I will look closer at the attached patch. I assume you will check on the localization issue and do the fat comment on the other patch. But, I think maybe these should be combined into one unified patch instead of separate patches.

Comment 19

[Jorg K (CEST = GMT+2)]

I'm going back to the other bug and will modify *lots* of comments everywhere. I don't think we should combine these bugs. And yes, in a third step we must make sure that the Account Manager stores INBOX/Trash and not Bandera de Entrada/Trash. That's doable, we just need to get the folder URI and massage it like we do in the other bug.

Thanks for your help!

Comment 20

[gene smith]

You're welcome!

Another question. Have you been working with the tbtest account within the last hour? I just noticed a phantom INBOX/INBOX/Trash again but I see via my printfs that my local tb is not creating it. I haven't even used "Trash" as the trash folder for a long time.

Comment 21

[Jorg K (CEST = GMT+2)]

To look at this patch now, I reapplied the patch from bug 1335982. I added prints to see what's going on and with the patch here I get:

=== pathName Trash
=== m_trashFolderName INBOX/Trash
=== adoptedBoxSpec->mAllocatedPathName INBOX/Trash
=== trash exists 1

No phantoms show.

Taking the patch out and adding more prints I get:

=== pathName Trash
=== trashMatch INBOX.INBOX/Trash
=== serverTrashName INBOX/INBOX^Trash
=== m_trashFolderName INBOX/Trash
=== adoptedBoxSpec->mAllocatedPathName INBOX/Trash
=== trash exists 0

=== pathName INBOX/Trash
=== trashMatch INBOX.INBOX/Trash
=== serverTrashName INBOX/INBOX^Trash
=== m_trashFolderName INBOX/Trash
=== adoptedBoxSpec->mAllocatedPathName INBOX/INBOX/Trash
=== trash exists 0

And the unwanted inbox is back.

I'm not sure why I see this twice, maybe the second one is a result of having created a phantom folder.

So certainly you patch works in my case, so maybe you can motivate why the code changes are OK. Looks like forming trashMatch by prepending the "INBOX." doesn't help at all. Forming a serverTrashName from there isn't much use either.

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8941384 [details] [diff] [review]
1428666-review-changes0.patch (JK)

OK, let's see whether you can warm up to my version.

Comment 23

[gene smith]

I haven't had a chance to try your patches but on the surface they look like the same functionality that I am running with now. However, I did see one bad thing happen that has delayed me testing the patches. During one of many tb restarts, a phantom folder appeared in the OVH/Dovecot test account. I have done many more restarts and collapse/expand on all the accounts and haven't seen the problem again so I think it must have been a race or timing effect that hit at just the right (or wrong) time.

I was using printf's at the time so maybe they affected the timing, not sure. Anyhow here is what I recorded:

This is printed from the code from DiscoverMailboxSpec(). I had the "more-stuff-under-less" set as the trash folder and it is determined to exist since trashExist is 1 and then a flag is set that the trash mailbox exists.

gds: 2trashMatch with w/o prefix is INBOX/less-stuff/more-stuff-under-less
gds: 2allocatedPathName is INBOX/less-stuff/more-stuff-under-less
gds: 2serverTrashName = INBOX/less-stuff/more-stuff-under-less
gds: 2inside INBOX
gds: trashExists = 1

Then later in MailboxDiscoveryFinished() the same flag that was supposedly set above is checked and found to *not* be set; so no trash mailbox is assumed to exist. This causes the trash mailbox to be created (even though it already exists).

gds: 1trashName with prefix is INBOX.INBOX/less-stuff/more-stuff-under-less
gds: 1creating trash mbox name = INBOX.INBOX.less-stuff.more-stuff-under-less

At this point where the trash mailbox is created, the namespace "INBOX." is pre-pended. Having the prefix in front (an extra INBOX.) causes the phantom folder path to be created. Note: this is the same place in the code that the phantom mailbox (or extra mailbox for Elmar's bug) is always created and the only place a "missing" trash folder is ever created.

I think that in MailboxDiscoveryFinished() the mailbox creation code there should not include the prefix. Without this, the occasional but unneeded attempt to create trash mailbox at this point will just result in a harmless (and, I think, not reported) imap error that the mailbox already exists instead of the phantom trash path being created and corrupting the folder tree.

I was somewhat concert about the case where the personal prefix is something other than "INBOX." I set up a local dovecot server and set the personal namespace to "wolf." instead of "INBOX.". The "wolf" folder appears on the same level as the actual Inbox and all other folders are under "wolf" and wolf remains grey (not selectable and only contains folders) which is good. 

My current trash_folder_name is now "wolf.top-level.RoundFile". 
The code that re-creates the trash folder and pre-pends "wolf" causes this imap command would be sent:
AAA create wolf.wolf.top-level.RoundFile
and produces the phantom. So leaving the first "wolf" off here causes no harm when this create occurs, only a mailbox already exists error.

So I propose that the the code where the trash folder is created be modified to not include the prefix and be part of your patch.
nsImapProtocol.cpp at about line 7650 in function MailboxDiscoveryFinished():

    // There is no Trash folder (either LIST'd or LSUB'd), and we're using the
    // Delete-is-move-to-Trash model, and there is a personal namespace
    if (!trashFolderExists && GetDeleteIsMoveToTrash() && ns)
    {
      nsCString trashName(CreatePossibleTrashName(ns->GetPrefix()));   <<<<<< leave off prefix
      printf("gds: 1trashName with prefix is %s\n", trashName.get());
      nsCString onlineTrashName;
      m_runningUrl->AllocateServerPath(trashName.get(), ns->GetDelimiter(),
                                       getter_Copies(onlineTrashName));
      
      GetServerStateParser().SetReportingErrors(false);
      printf("gds: 1creating trash mbox name = %s\n", onlineTrashName.get());
      bool created = CreateMailboxRespectingSubscriptions(onlineTrashName.get());
      GetServerStateParser().SetReportingErrors(true);










I w

Comment 24

[Jorg K (CEST = GMT+2)]

Yes, my patch here and the one in bug 1335982 are functionally identical. The one in bug 1335982 just has a heap more comments and the one here uses the observation that
  nsCString trashName(CreatePossibleTrashName(ns->GetPrefix())); 
is the same as using m_trashFolderName since the function just prepends the prefix and returns the value.

(In reply to gene smith from comment #23)
>       nsCString trashName(CreatePossibleTrashName(ns->GetPrefix()));  
> <<<<<< leave off prefix
OK, can you please modify my patch to reflect this. Also apply the simplification of not using the variable trashName at all.

I'm not sure why the modified detection logic should fail and attempt to create the trash folder twice which would then silently fail.

Comment 25

[Jorg K (CEST = GMT+2)]

Actually, there are three calls to CreatePossibleTrashName() in nsImapProtocol.cpp. So far you've proposed to remove two of them, to then we should review the third usage and at least remove the function and inline the code if needed.

Comment 26

[gene smith]

namespace, you get the phatom.(In reply to Jorg K (GMT+1) from comment #25)
> Actually, there are three calls to CreatePossibleTrashName() in
> nsImapProtocol.cpp. So far you've proposed to remove two of them, to then we
> should review the third usage and at least remove the function and inline
> the code if needed.

Yes, the remaining one is a problem too. If you unsubscribe from your designated trash folder inside your named personal namespace, you get the phantom. I haven't tested, but this should also occur without our patches.

Comment 27

[gene smith]

Attachment: 1428666-review-changes1.patch(gds)

Modifies the previous patch to avoid adding the personal namespace prefix in front of the trash folder path when the trash folder is created since the namespace is already included in the trash_folder_name (actually a path) value configured in Account manager/Server Settings. This avoids "phantom" folder creation such as when the trash folder is unsubscribed or deleted and at other rare occurrences when TB decides the trash folder needs to be re-created as noted in Comment 23.

Also got rid of function CreatePossibleTrashName() and added inline function GetTrashFolderPath() to access member variable m_trashFolderPath that I also renamed from m_trashFolderName.

Tested with dovecot server having 2 personal namespaces. Also retested with other accounts having one namespace and no namespaces. No problems observed.

Comment 28

[Jorg K (CEST = GMT+2)]

Comment on attachment 8942059 [details] [diff] [review]
1428666-review-changes1.patch(gds)

Thanks for the research, let's go with this in conjunction with bug 1335982.

I think having an access function GetTrashFolderPath() is not necessary since this is a private function returning a private member. So I'll take that out.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 1428666-review-changes1.patch (JK)

Removed GetTrashFolderPath(). Tested with the limited accounts I have, works fine.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a554d6a6752d
Follow-up to bug 1335982: Correct trash folder problems with certain imap servers. r=jorgk

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 8942204 [details] [diff] [review]
1428666-review-changes1.patch (JK)

This would need to go with four more bugs and it's not worth it at TB 52.7 when TB 60 ESR is coming out six weeks later.

Comment 32

[gene smith]

Attachment: elmar--confirm-fix-with-tb60.eml

I tested this myself using the reporter's test account and asked the reporter (Elmar) via email to also confirm the fix. The attached email text shows that he is OK with the fix after update to 60.