There are a number of bugs that are a result of the LSUB command not returning the same flags as the LIST command. According to IMAP4rev1 RFC, this is allowable.

Pending review of Bug 495318, the issues in all of the blocked bugs should be fixed for any server that supports LIST-EXTENDED imap extension. For other servers, I have come up with a couple options. 1. After issuing LSUB "" "*", do a LIST "" "folder" for each folder returned by LSUB. This should be fairly simple to implement, but I think there would be serious performance issues for users with a large number of folders. 2. After (or before) issuing LSUB "" "*", issue LIST "" "*" and meld the two results together. This is a more difficult change to implement, but I think I could do it.

Apparently if you stay up all night staring at this, it gets easier for some reason... This patch causes TB to issue a LIST command immediately before calling LSUB using the same arguments. The mailbox names and flags are stored from the LIST response. In the LSUB response, we add those flags back to the mailboxes as if LSUB had returned them in its response. We also add a \Noselect flag for folders that are subscribed but don't exist (weren't returned by LIST)

test to go with the patch. I have confirmed that test fails without patch and succeeds with patch. I have also run all tests in mailnews/imap/test to make sure patch did not break anything else.

Comment on attachment 669881 [details] [diff] [review] adds call to LIST before call to LSUB to get mailbox flags I forgot to mention that these patches build on top of the patches in bug 495318, so apply those first and then the patches for this bug.

Comment on attachment 669881 [details] [diff] [review] adds call to LIST before call to LSUB to get mailbox flags Review of attachment 669881 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/imap/src/nsImapProtocol.cpp @@ +4840,5 @@ > + if (m_standardListMailboxes.Count() > 0) > + { > + int32_t hashValue = 0; > + nsCString strHashKey(adoptedBoxSpec->mAllocatedPathName); > + if (m_standardListMailboxes.Get(strHashKey, &hashValue)) Trailing white space

Comment on attachment 669882 [details] [diff] [review] companion test Review of attachment 669882 [details] [diff] [review]: ----------------------------------------------------------------- After updating the patch for bitrot, the tests pass, so r=me with the updates. ::: mailnews/imap/test/unit/test_listSubscribed.js @@ -48,3 @@ > { > - dump("HERE I AM!"); > - Patch needs to be updated; this hunk did not apply cleanly ::: mailnews/imap/test/unit/xpcshell.ini @@ +42,4 @@ > [test_largeOfflineStore.js] > skip-if = os == 'mac' > [test_listClosesDB.js] > +[test_listSubscribed.js] This hunk did not apply cleanly either

both patches need to be checked in. r=irving

https://hg.mozilla.org/comm-central/rev/b8f425be58ed https://hg.mozilla.org/comm-central/rev/efbc490f42c9

This fix for this bug seems to be causing other problems (bug 859269 and bug 858062 in particular). I thing that we should roll back the patch until we can figure out a better fix. I think that rolling it back and starting from scratch would be better than starting a new bug and trying to patch up this bug. Is there a proper procedure for requesting a rollback of a commit?

Probably create a backout patch and have it reviewed as that.

FYI. (In reply to David Lechner (:dlech) from comment #12) > This fix for this bug seems to be causing other problems (bug 859269 and bug 858062 in particular). (A) Bug 859269 is: (1) Server is not correctly configured. To LIST "*" and LIST "%", server returns current directory as Mbox of IMAP server, and when LIST "*", server returns all sub directories and files of current directory as Mbox of server. (2) Needless to say, bug 317597 comment #22 occurs, because this is not-well-configured UW-IMAP server. i.e. "Change by this bug is needed" case. (3) To avoid problem of (1), user utilized "Show only subscribed folders" as effective workaround of problem by (1). (4) Before patch of this bug. Because LIST "MboxName/%" and LIST "MboxName/%/%" is used by Tb upon folder re-discovery, LIST "*", LIST "%", LIST "%/%" was never used by Tb when "Show only subscribed folders" is checked. => "Show only subscribed folders" worked as workaround of (1). (5) After patch of this bug. "Show only subscribed folders" is Checked : Because LIST "*" is issued just before LSUB "*", problem of (1) always occurs. "Show only subscribed folders" is Unchecked : Because LIST "%" and "LIST %/%" is issued by Tb, problem of (1) still always occurs. (B) Bug 858062 is perhaps: (1) User uses Tb with "Show only subscribed folders" checked. (2) Server has problem of bug 317597 comment #22 in LSUB response. (\Noselect is not set in LSUB response by server) i.e. "Change by this bug is needed" case. (3) This problem of (2) was bypassed by unchecking "Server supports folders that contains sub-folders and messages". (4) Before patch of this bug. Because of (1), \Noselect flag in LIST response was never used by Tb to determine Mbox's attribute. So, "randomly set \Noselect, \Noinferiors etc. in LIST response" didn't produce problem if "Show only subscribed folders" is checked. Then, user never corrected the bad configuration. (5) After patch of this bug. LIST "*" is issued just before LSUB "*" by Tb, and "randomly set \Noselect, \Noinferiors etc. in LIST response" is used by Tb to determine Mbox's attribute. If \Noselect is unfortunately set in a LIST response, the Mbox is shown as "Mbox of \Noselect" at folder pane by Tb. i.e. This bug's patch worked pretty well as designed/implemented. I believe patch of this bug does do essentially correct thing. To reduce problem like (A) due to bad server configuration, 'avoid LIST "*" use which is sometimes dangerous when badly configured server" is perhaps needed. A possible solution. Issue LSUB "*" first, without folder re-discovery process, for each RootLevelMbox in LSUB response, LIST "RootLevelMbox/*", Use flags in LIST response, and do LSUB "*" + folder re-discovery. Note: Reason why LIST "%", LIST "%/%" is used, reason why LIST "*" is not used, reason why changed from LIST "*" use to LIST "%" and LIST "%/%" use, when "Show only subscribed folders" is unchecked, is: Performance reason. If very many and deep Mbox is defined at server, getting all sub folders by LIST "*" takes long. To avoid problem like (B) due to bad server configuration, 'making behavior by patch of this bug optional" may be needed.

I was not able to make time to find a solution to make my previous patch work before ESR 24 is released. I think is is important that we roll back so that this bug does not get released in ESR 24 otherwise there will be some folks rather unhappy that we broke their Thunderbird. [Approval Request Comment] Regression caused by (bug #): This bug User impact if declined: Users (UW-IMAP in particular) could have their accounts rendered unusable. See previous comments for related bugs that were caused by this bug landing. Testing completed (on c-c, etc.): Ran all IMAP tests on local machine (Win7) Risk to taking this patch (and alternatives if risky): None that I see. There is no other related code that depends on this that I know of.

David, which bug(s) do you think backing this out will fix, and are you suggesting to just back this out from 24 and not 25/26 for now?

(In reply to Mark Banner (:standard8) from comment #16) > David, which bug(s) do you think backing this out will fix, bug 859269 and bug 858062 > and are you > suggesting to just back this out from 24 and not 25/26 for now? 24 for sure. The rest don't really matter to me. We could back it out everywhere for constancy or you could leave it and then I will *have* to fix it :p. Whichever you think is easier or makes the most sense.

Ok, we'll back this out for 24 beta 2, and keep it on other branches for now.

I'll get to this before the next beta.

Comment on attachment 789086 [details] [diff] [review] rollback I've checked this by examination that it does indeed do the required backout.

Backout patch landed in comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/67c13f67bda9

Unfortunately, I had to backout the backout due to test failures. I tried to reproduce them locally, so I'm puzzled at the moment. https://tbpl.mozilla.org/php/getParsedLog.php?id=27121858&tree=Thunderbird-Beta#error0 TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/mailnews/imap/test/unit/test_dontStatNoSelect.js | test failed (with xpcshell return code: 0), see following log: >>>>>>> TEST-INFO | (xpcshell/head.js) | test MAIN run_test pending (1) AUTH PLAIN line -AHVzZXIAcGFzc3dvcmQ=- authorize-id: --, username: -user-, password: -password- TEST-INFO | (xpcshell/head.js) | test pending (2) TEST-INFO | (xpcshell/head.js) | test _async_driver pending (3) TEST-INFO | (xpcshell/head.js) | test MAIN run_test finished (3) TEST-INFO | (xpcshell/head.js) | running event loop 2013-08-28 08:46:30 test.test INFO [Context: test.test:1 state: started] Starting test: checkStatSelect TEST-INFO | (xpcshell/head.js) | test _async_driver pending (3) TEST-INFO | (xpcshell/head.js) | test _async_driver finished (3) 2013-08-28 08:46:30 test.test INFO [Context: test.test:1 state: finished] Finished test: checkStatSelect 2013-08-28 08:46:30 test.test INFO [Context: test.test:2 state: started] Starting test: checkStatNoSelect TEST-INFO | (xpcshell/head.js) | test _async_driver pending (3) TEST-INFO | (xpcshell/head.js) | test _async_driver finished (3) TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/mailnews/imap/test/unit/test_dontStatNoSelect.js | 0 == 1 - See following stack: JS frame :: /builds/slave/test/build/xpcshell/tests/mailnews/imap/test/unit/test_dontStatNoSelect.js :: checkStatNoSelect :: line 90 JS frame :: ../../../resources/asyncTestUtils.js :: _async_driver :: line 156 JS frame :: /builds/slave/test/build/xpcshell/head.js :: do_execute_soon/<.run :: line 444 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-INFO | (xpcshell/head.js) | exiting test ******************************************* Generator explosion! Unhappiness at: undefined:undefined Because: 2147500036

I missed a change in imapd.js. I think that it was the culprit for the failing test. It is strange that the test does not fail when running locally though.

I pushed the rollback from try, and it was coming up with this: TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/mailnews/imap/test/unit/test_listSubscribed.js | true == false - See following stack: JS frame :: /builds/slave/test/build/xpcshell/tests/mailnews/imap/test/unit/test_listSubscribed.js :: testZimbraServerVersions :: line 108 JS frame :: ../../../resources/asyncTestUtils.js :: _async_driver :: line 156 JS frame :: /builds/slave/test/build/xpcshell/head.js :: do_execute_soon/<.run :: line 444 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 This was added in bug 816028, and if I understand it right, due to the backout of this bug, we're safe to disable this part of the test.

So what are the affects if this doesn't land in time for Gecko24, go to build for TB24 is scheduled for early-week.

(In reply to Mark Banner (:standard8) from comment #24) > Created attachment 798778 [details] [diff] [review] > Fix test_listSubscribed.js > This was added in bug 816028, and if I understand it right, due to the > backout of this bug, we're safe to disable this part of the test. Yes. (In reply to Justin Wood (:Callek) from comment #25) > So what are the affects if this doesn't land in time for Gecko24, go to > build for TB24 is scheduled for early-week. There will be some unhappy UW-IMAP users that have to deal with bug 859269 and bug 858062. Essentially, it could break subscribed folders for them.

https://hg.mozilla.org/releases/comm-beta/rev/119a80893dab https://hg.mozilla.org/releases/comm-beta/rev/340434e8f9e7

(In reply to Mark Banner (:standard8) from comment #27) FYI. After check-in of backout, bug 916630 has been reported for Dovecot server who surely produces problem such as Bug 317597 / Bug 534021 without patch, but who never produces problem such as bug 859269 / bug 897854 with patch. (This is : regression on Bug 317597 / Bug 534021 over Tb 17 by backout of effective patch of this bug) I think following is better. (1) Make feature by this bug's patch optional. (1-1) If server doesn't have problem of Bug 317597 / Bug 534021, there is no need to enable patch. (1-2) If server has problem of Bug 317597 / Bug 534021 but doesn't have problem of bug 859269 / bug 897854, - if user wants to bypass Bug 317597 / Bug 534021, use can enable patch. - if there is no need to bypass Bug 317597 / Bug 534021, there is no need to enable patch. (reporter of bug 916630) (1-3) Even If server has problem of Bug 317597 / Bug 534021 and also has problem of bug 859269 / bug 897854, if "Show only subscribed folder"/"Server supports folder..." without patch is usable, there is no need to enable patch. (reporters of bug 859269 / bug 897854) (1-4) If server has problem of Bug 317597 / Bug 534021 and also has problem of bug 859269 / bug 897854, and if "Show only subscribed folder" without patch is not usable, (bypasss of Bug 317597 / Bug 534021 is neeeded) wait for new version of patch of this bug(new version of solution for Bug 317597, Bug 534021 etc.) (2) For unforunate (1-4) users, implement new version(Dont' use LIST "*").

I'd prefer that we just spend time on fully fixing this bug, rather than discussing and coming up with numerous work arounds.

So, the way I understand things... the original patch made subscribed folders work properly with Dovecot and others that don't return flags for folders in the LSUB response (which is proper behavior according to the spec). That patch was backed out so that subscribed folders would work with a broken server (UW-IMAP--misbehaves on LIST * (?)). But in doing so, now Dovecot et al. are broken?

Comment on attachment 797478 [details] [diff] [review] rollback v2 Cancelling old approval request for tracking purposes, this patch landed ages ago where it was meant to.

So apparently this is worse in 31 vs. 24?

We rolled back the patch from this bug in TB 24 just before TB 24 was released because it was causing the issues we are seeing in TB 31. We left the patch applied in comm-central though and so it was included in TB31.

David, I'm tempted to say we should back this out from 31, and maybe also from the rest of the branches, until we can attempt a proper fix, what do you think?

Yes, I would say applying the same backout patch to TB31 as we did on TB24 is the prudent thing to do. I had an idea on how to fix this as was falling asleep last night, so I am trying it out to day, so let's hold off on the others for now.

there may be bigger issues, but why isn't it just as simple as making sure to pass the folder prefix to the list command?

@dlech, along Mark's line of thought, if it doesn't cause undue problems I'd like to suggest this also be backed off trunk. By doing so, those of us using trunk will get a better picture of the changes and impact of the "real" patch when it comes out.

I suppose you could do that if you think it is useful. However, the "real" patch is not going to behave any different from the existing patch, other that it should not cause a crash when connecting to servers that use the users home dir. As Joe said, it should be a simple a passing the folder prefix to the list command.

OK. Now that I have may head back into this (mostly) here is what I think we should do: This bug can be closed after we decide whether to apply the backout patch to TB31 or not. The patch for this bug does not need to be rewritten from scratch or anyting. The recent new bugs against TB31 are really duplicates of bug 859269 rather than this one. That bug is just caused by a defect in the code from this bug. So I think it make sense for any further patches to be handled in other bugs.

This combines the previous two attachments and brings them up to date with the 32 branch. I'm proposing that we land these on 32 beta, and release in the next beta, and then do the backout on 31 before the next release. The other bugs can then be fixed separately.

Sounds like a good plan to me.

@dlech, is bug 885162 related fallout? I think that is what I am seeing. This might also explain some slowness reported in TB24, but at the time of release last year we didn't correlate the performance to this bug.

I don't think bug 885162 is related to this one. It has to do with LIST (SUBSCRIBED). The patches from this bug only take effect if the server does not support LIST-EXTENDED (i.e. the server can't do LIST (SUBSCRIBED)).

Landed the backout on beta: https://hg.mozilla.org/releases/comm-beta/rev/4e6b8ab772be

https://hg.mozilla.org/releases/comm-esr31/rev/2c2355a79a0a David, should we close this bug now in preference to the other bugs?

Yes.