799821 - Folders misbehave when LSUB does not return mailbox flags

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David Lechner (:dlech)]

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.

Comment 1

[David Lechner (:dlech)]

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.

Comment 2

[David Lechner (:dlech)]

Attachment: adds call to LIST before call to LSUB to get mailbox flags

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)

Comment 3

[David Lechner (:dlech)]

Attachment: companion test

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 4

[David Lechner (:dlech)]

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 5

[:Irving Reid (No longer working on Firefox)]

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 6

[:Irving Reid (No longer working on Firefox)]

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

Comment 7

[David Lechner (:dlech)]

Attachment: fix v2

Comment 8

[David Lechner (:dlech)]

Attachment: fix v2

Comment 9

[David Lechner (:dlech)]

Attachment: test v2

Comment 10

[David Lechner (:dlech)]

both patches need to be checked in. r=irving

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[David Lechner (:dlech)]

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?

Comment 13

[Magnus Melin [:mkmelin]]

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

Comment 14

[WADA:World Anti-bad-Duping Agency]

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.

Comment 15

[David Lechner (:dlech)]

Attachment: rollback

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.

Comment 16

[Mark Banner (:standard8)]

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?

Comment 17

[David Lechner (:dlech)]

(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.

Comment 18

[Mark Banner (:standard8)]

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

Comment 19

[Mark Banner (:standard8)]

I'll get to this before the next beta.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 789086 [details] [diff] [review]
rollback

I've checked this by examination that it does indeed do the required backout.

Comment 21

[Mark Banner (:standard8)]

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

Comment 22

[Mark Banner (:standard8)]

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

Comment 23

[David Lechner (:dlech)]

Attachment: rollback v2

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.

Comment 24

[Mark Banner (:standard8)]

Attachment: Fix test_listSubscribed.js

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.

Comment 25

[Justin Wood (:Callek)]

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

Comment 26

[David Lechner (:dlech)]

(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.

Comment 27

[Mark Banner (:standard8)]

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

Comment 28

[WADA:World Anti-bad-Duping Agency]

(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 "*").

Comment 29

[Mark Banner (:standard8)]

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

Comment 30

[Jeremy]

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 32

[Mark Banner (:standard8)]

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.

Comment 33

[Magnus Melin [:mkmelin]]

So apparently this is worse in 31 vs. 24?

Comment 34

[David Lechner (:dlech)]

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.

Comment 36

[Mark Banner (:standard8)]

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?

Comment 37

[David Lechner (:dlech)]

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.

Comment 38

[Joe Pruett]

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?

Comment 39

[Wayne Mery (:wsmwk)]

@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.

Comment 40

[David Lechner (:dlech)]

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.

Comment 41

[David Lechner (:dlech)]

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.

Comment 42

[Mark Banner (:standard8)]

Attachment: Combined backout patch

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.

Comment 43

[David Lechner (:dlech)]

Sounds like a good plan to me.

Comment 44

[Wayne Mery (:wsmwk)]

@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.

Comment 45

[David Lechner (:dlech)]

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)).

Comment 46

[Mark Banner (:standard8)]

Landed the backout on beta:

https://hg.mozilla.org/releases/comm-beta/rev/4e6b8ab772be

Comment 47

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr31/rev/2c2355a79a0a

David, should we close this bug now in preference to the other bugs?

Comment 48

[David Lechner (:dlech)]

Yes.