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

RESOLVED FIXED in Thunderbird 20.0

Status

MailNews Core
Networking: IMAP
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Usul, Assigned: dlech)

Tracking

Thunderbird 20.0
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird19+ fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
It seems to fail when new folders are discovered.
(Reporter)

Updated

5 years ago
Summary: Since I've subscribed to the infra shared mailboxes I get disconnect from Zimbra → Since I've subscribed to the infra shared mailboxes I get disconnected from Zimbra
(Reporter)

Comment 2

5 years ago
Hum Thunderbird 8 doesn't chook - so it could also be a Thunderbird bug.
(Reporter)

Comment 3

5 years ago
This works in 17 but is broken in 20
Assignee: server-ops → nobody
Component: Server Operations → Networking: IMAP
Product: mozilla.org → MailNews Core
QA Contact: shyam
Version: other → 20
(Reporter)

Comment 4

5 years ago
I can't use zimbra in tos conditions.
(Assignee)

Comment 5

5 years ago
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?
(Reporter)

Comment 6

5 years ago
Created attachment 686465 [details]
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 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
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.
(Assignee)

Updated

5 years ago
Blocks: 495318
OS: Mac OS X → All
Hardware: x86 → All
Summary: Since I've subscribed to the infra shared mailboxes I get disconnected from Zimbra → LIST (SUBSCRIBED) command to Zimbra server with shared folders causes crash in Zimbra and results in disconnection from server
Version: 20 → 19

Comment 9

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

Comment 10

5 years ago
How difficult would it be to sniff zimbra until they fix it ?
(Assignee)

Comment 11

5 years ago
(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" "lhirlimann@mozilla.com" "SERVER" "721cdaaa-da23-48ed-ab5b-6d4d2be8576f")
(Assignee)

Comment 12

5 years ago
Ludovic, I just realized that I pasted your email address in my last comment. Sorry about that.
(Reporter)

Comment 13

5 years ago
Works for me Whoo hoo !!
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.
OS: All → Mac OS X
Hardware: All → x86
Version: 19 → 20
Er, didn't mean to change those fields, not sure why my browser did that...
OS: Mac OS X → All
Hardware: x86 → All
Version: 20 → 19

Comment 16

5 years ago
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. :)
(Assignee)

Comment 17

5 years ago
Created attachment 689551 [details] [diff] [review]
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
Assignee: nobody → david
Status: NEW → ASSIGNED
Attachment #689551 - Flags: review?(irving)
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
Attachment #689551 - Flags: review?(irving) → review-
(Assignee)

Comment 19

5 years ago
(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

5 years ago
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.
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.
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.
Flags: needinfo?(mbanner)
(Assignee)

Comment 23

5 years ago
Created attachment 691201 [details] [diff] [review]
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.
Attachment #691201 - Flags: feedback?(irving)
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
Attachment #691201 - Flags: feedback?(irving) → feedback+
(Assignee)

Comment 25

5 years ago
Created attachment 694668 [details] [diff] [review]
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.
Attachment #689551 - Attachment is obsolete: true
Attachment #691201 - Attachment is obsolete: true
Attachment #694668 - Flags: review?(irving)
Comment on attachment 694668 [details] [diff] [review]
patch v3

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

Excellent.
Attachment #694668 - Flags: review?(irving) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 27

5 years ago
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.
Attachment #694668 - Flags: approval-comm-aurora?
https://hg.mozilla.org/comm-central/rev/bd61e0315b04
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
(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.
Flags: needinfo?(mbanner)
Comment on attachment 694668 [details] [diff] [review]
patch v3

a=me, lets get this in before the beta cycle.
Attachment #694668 - Flags: approval-comm-aurora? → approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/fa823b998517
status-thunderbird19: --- → fixed
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?
status-thunderbird19: fixed → affected
tracking-thunderbird19: --- → +
Attachment #694668 - Flags: approval-comm-aurora+ → approval-comm-aurora-
(Assignee)

Comment 33

4 years ago
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.
(Assignee)

Comment 34

4 years ago
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?
(Assignee)

Comment 35

4 years ago
Created attachment 697992 [details] [diff] [review]
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.
Attachment #697992 - Flags: approval-comm-aurora?
(Assignee)

Updated

4 years ago
Attachment #694668 - Flags: approval-comm-aurora- → approval-comm-aurora?
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.
Attachment #694668 - Flags: approval-comm-aurora? → approval-comm-beta+
Attachment #697992 - Flags: approval-comm-aurora? → approval-comm-aurora+
https://hg.mozilla.org/releases/comm-beta/rev/b56e8b0b5db1
https://hg.mozilla.org/releases/comm-beta/rev/71c405b77fd1
status-thunderbird19: affected → fixed
You need to log in before you can comment on or make changes to this bug.