816028 - LIST (SUBSCRIBED) command to Zimbra server with shared folders causes crash in Zimbra and results in disconnection from server

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Ludovic Hirlimann [:Usul]]

I''ve added the following shared folders in zimbra this morning :
Infra/*


Since I've done that Thunderbirdget's disconnected from zimbra.

Nothing suspicious in Thunderbird's imap log.

Comment 1

[Ludovic Hirlimann [:Usul]]

It seems to fail when new folders are discovered.

Comment 2

[Ludovic Hirlimann [:Usul]]

Hum Thunderbird 8 doesn't chook - so it could also be a Thunderbird bug.

Comment 3

[Ludovic Hirlimann [:Usul]]

This works in 17 but is broken in 20

Comment 4

[Ludovic Hirlimann [:Usul]]

I can't use zimbra in tos conditions.

Comment 5

[David Lechner (:dlech)]

See bug 495318 comment 51 and 52. I think Zimbra may be crashing because of LIST (SUBSCRIBED). Do you see LIST (SUBSCRIBED) in your imap log?

Comment 6

[Ludovic Hirlimann [:Usul]]

Attachment: Imap log

(In reply to David Lechner (:dlech) from comment #5)
> See bug 495318 comment 51 and 52. I think Zimbra may be crashing because of
> LIST (SUBSCRIBED). Do you see LIST (SUBSCRIBED) in your imap log?

Unfortunately not.

Comment 7

[Dave Miller [:justdave]]

Comment on attachment 686465 [details]
Imap log

Yes, you do.

>616042496[10a9ab1b0]: 16ceb800:mail.mozilla.com:A:SendData: 6 list (subscribed) "" "*"
>616042496[10a9ab1b0]: 16ceb800:mail.mozilla.com:A:CreateNewLineFromSocket: * BYE zmmbox2.mail.corp.phx1.mozilla.com Zimbra IMAP4rev1 server closing connection

Comment 8

[Dave Miller [:justdave]]

So, as mentioned in bug 495318 comment 52, The potential action here is to version-check Zimbra servers and disable this feature if it's an older version than the one Zimbra gets this fixed in.

Comment 9

[Magnus Melin [:mkmelin]]

Well, there's not a fix for any zimbra version yet so... Really sounds like something that should be fixed on the server.

Comment 10

[Ludovic Hirlimann [:Usul]]

How difficult would it be to sniff zimbra until they fix it ?

Comment 11

[David Lechner (:dlech)]

(In reply to Ludovic Hirlimann [:Usul] from comment #10)
> How difficult would it be to sniff zimbra until they fix it ?

As a workaround, you can turn off subscribed folders. In Account Settings for your Zimbra account, select Server Settings and then click the Advanced... button. Uncheck 'Show only subscribed folders'.

As far as sniffing goes, it looks like at least your Zimbra server supports the ID extension, so we have easy access to server name and version 
> * ID ("NAME" "Zimbra" "VERSION" "7.2.1_GA_2790" "RELEASE" "20120815212257" "USER" "[e-mail]" "SERVER" "721cdaaa-da23-48ed-ab5b-6d4d2be8576f")

Comment 12

[David Lechner (:dlech)]

Ludovic, I just realized that I pasted your email address in my last comment. Sorry about that.

Comment 13

[Ludovic Hirlimann [:Usul]]

Works for me Whoo hoo !!

Comment 14

[Dave Miller [:justdave]]

A fix for this has been committed to Zimbra in both the 7.x and 8.x branches.  I should be fixed in both Zimbra 7.2.2 and 8.0.2 (neither of which are released yet, and may not be for a while).  Even when it is released, hosts that use Zimbra as a mail server are notoriously slow at upgrading (there's a lot of people using 6.x out there, on which this bug will never be fixed), so it's probably worth version-checking for it anyway.  I noticed in the transaction logs that Thunderbird is already doing an ID check on the server anyway so it's probably just a matter of looking at it.  So list (subscribed) should probably be avoided on any server identifying as Zimbra with a version number less than 7.2.2, and also avoided on versions 8.0 (8.0.0?) and 8.0.1.

Comment 15

[Dave Miller [:justdave]]

Er, didn't mean to change those fields, not sure why my browser did that...

Comment 16

[Jake Maul [:jakem]]

Just to nail down the version of TB that is affected, I have no problems with Thunderbird 18.0 Beta or Earlybird 18.0a2, but do have this problem (or at least a very similar one... haven't examined logs) in Earlybird 19.0a2. So a potential client-side sniffing/workaround would need to make its way into current Aurora/Earlybird channel. :)

Comment 17

[David Lechner (:dlech)]

Attachment: workaround for zimbra server crashes

I have implemented a workaround. I don't think it is very pretty, but it works. 

Also, in the tests I moved the appInfo code to IMAPPump.js so that we don't have to copy the code every time we write an IMAP test that uses ID extension

Comment 18

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

Comment on attachment 689551 [details] [diff] [review]
workaround for zimbra server crashes

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +7587,5 @@
> +  if (GetServerStateParser().GetServerID().Find("\"NAME\" \"Zimbra\"", CaseInsensitiveCompare) != kNotFound) {
> +    nsCString serverID(GetServerStateParser().GetServerID());
> +    int start = serverID.Find("\"VERSION\" \"", CaseInsensitiveCompare) + 11;
> +    int length = serverID.Find("\" ", start, CaseInsensitiveCompare);
> +    const nsDependentCSubstring version = Substring(serverID, start, length);

Could you use https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIVersionComparator to do the version comparison instead of re-implementing?

@@ +7613,5 @@
> +      }
> +    }
> +  }
> +  if ((GetServerStateParser().GetCapabilityFlag() & kHasListExtendedCapability) &&
> +      !incompatibleServer)

I would much rather see this done when we initialize the capability flags for the ServerStateParser, somewhere in the neighbourhood of http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapServerResponseParser.cpp#2297, by not setting the nHasListExtendedCapability flag for the broken servers - as long as it's OK for the incompatibility to also affect the other use of the flag at around line 7308

Comment 19

[David Lechner (:dlech)]

(In reply to Irving Reid (:irving) from comment #18)
> Could you use
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIVersionComparator to do the version comparison instead of re-implementing?

I did not know this existed. Will look into it.


> I would much rather see this done when we initialize the capability flags
> for the ServerStateParser, somewhere in the neighbourhood of
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/
> nsImapServerResponseParser.cpp#2297, by not setting the
> nHasListExtendedCapability flag for the broken servers - as long as it's OK
> for the incompatibility to also affect the other use of the flag at around
> line 7308

The nHasListExtendedCapability flag implies more than just the LIST (SUBSCRIBED) command, so I am not sure I like the idea of disabling it completely. Currently, it does not really affect much, but if we implement more of the LIST-EXTENDED extension, then the new functionality would be disabled as well.

I could go either way here. Anyone else have input?

Comment 20

[Magnus Melin [:mkmelin]]

Since it's just dealing with old broken servers i'd just make the code as simple as possible. Having more special things work with those doesn't seem worth it to me.

Comment 21

[Dave Miller [:justdave]]

Update from the Zimbra side...  they apparently had already cut the release branches for 7.2.2 and 8.0.2 when this fix got committed.  The official word is that the Zimbra versions that will contain this fix will be 7.2.3 and 8.0.3.

Comment 22

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

Mark, it would be good to get this into Aurora soon because it's causing Mozilla's Zimbra to fail when people have shared folders.

Do you have an opinion on comment 18 / comment 19? It feels funny to do a complex version check every time we go to issue a list command, but it probably isn't actually enough overhead to make a difference.

Just clearing the feature flag for older versions of Zimbra could affect any future code that comes along trying to use the list extended commands - I don't care a lot about that, but others might.

We could add a whole separate feature flag for this bug; we have similar warts for some other servers.

Comment 23

[David Lechner (:dlech)]

Attachment: patch v2

I have replaced the version checking with the version comparator as you suggested, so have a look if you like. Also updated version based on Dave Miller's last comment. I will wait a bit for more feedback about possibly moving the code to the server response parser.

Comment 24

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

Comment on attachment 691201 [details] [diff] [review]
patch v2

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

Should have caught this in the previous review, but move the "is this a broken server" code out into a separate helper function. Other than that, looks great.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +7591,5 @@
> +    int length = serverID.Find("\" ", start, CaseInsensitiveCompare);
> +    const nsDependentCSubstring serverVersionSubstring = Substring(serverID, start, length);
> +    nsCString serverVersionStr(serverVersionSubstring);
> +    Version serverVersion(serverVersionStr.get());
> +    Version sevenTwoThree("7.2.3_");    

Trailing white space

@@ +7596,5 @@
> +    Version eightZeroZero("8.0.0_");
> +    Version eightZeroThree("8.0.3_");
> +    bool test1 = serverVersion < sevenTwoThree;
> +    bool test2 = serverVersion >= eightZeroZero;
> +    bool test3 = serverVersion < eightZeroThree;

These three lines (bool test...) aren't needed, are they?

@@ +7597,5 @@
> +    Version eightZeroThree("8.0.3_");
> +    bool test1 = serverVersion < sevenTwoThree;
> +    bool test2 = serverVersion >= eightZeroZero;
> +    bool test3 = serverVersion < eightZeroThree;
> +    if ((serverVersion < sevenTwoThree) || 

Trailing white space

Comment 25

[David Lechner (:dlech)]

Attachment: patch v3

Incorporated comments from feedback. Probably doesn't need in depth review again - just would like to make sure I did the helper function satisfactorily.

Comment 26

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

Comment on attachment 694668 [details] [diff] [review]
patch v3

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

Excellent.

Comment 27

[David Lechner (:dlech)]

Comment on attachment 694668 [details] [diff] [review]
patch v3

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

The patch that caused this problem (Bug 495318) landed in TB 19, so this patch should land there as well.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/bd61e0315b04

Comment 29

[Mark Banner (:standard8)]

(In reply to Irving Reid (:irving) from comment #22)
> Do you have an opinion on comment 18 / comment 19? It feels funny to do a
> complex version check every time we go to issue a list command, but it
> probably isn't actually enough overhead to make a difference.

It would have perhaps been nice to cache that per imap protocol or server instance for the session life of Thunderbird, or the login, but I suspect we're not offering the list too frequently for that to matter too much.

> We could add a whole separate feature flag for this bug; we have similar
> warts for some other servers.

I'm willing to consider that for later if we need it, obviously this has landed now.

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 694668 [details] [diff] [review]
patch v3

a=me, lets get this in before the beta cycle.

Comment 31

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/fa823b998517

Comment 32

[Mark Banner (:standard8)]

Unfortunately I had to back this out of aurora due to unit test failures:

https://hg.mozilla.org/releases/comm-aurora/rev/4593a7a89fa5

https://tbpl.mozilla.org/php/getParsedLog.php?id=18427926&tree=Thunderbird-Aurora#error0


TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/mailnews/imap/test/unit/test_listSubscribed.js | true == false - See following stack:
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 460
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: _do_check_eq :: line 554
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: do_check_eq :: line 575
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/tests/mailnews/imap/test/unit/test_listSubscribed.js :: testZimbraServerVersions :: line 110
JS frame :: ../../../resources/asyncTestUtils.js :: _async_driver :: line 156
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: .run :: line 427
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

Are we missing another patch or something?

Comment 33

[David Lechner (:dlech)]

It looks like the missing patch is commit 11620. That commit is only an xpcshell test, so it should be safe to push it to comm-aurora as well, I should think.

Let me know if there is something else I can do.

Comment 34

[David Lechner (:dlech)]

On second thought, that test will probably fail. There is just one line in imapd.js that needs to be changed to get this patch working in aurora. How would I go about doing that?

Comment 35

[David Lechner (:dlech)]

Attachment: patch for comm-aurora

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

fixes broken test. Will need to commit both patches to comm-aurora. Ran tests locally comm-aurora to verify that this does indeed fix the test and does not break others.

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 694668 [details] [diff] [review]
patch v3

[Triage Comment]
aurora has now moved to beta, but we'll land this as soon as the tree is green.

Comment 37

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-beta/rev/b56e8b0b5db1
https://hg.mozilla.org/releases/comm-beta/rev/71c405b77fd1