nsImapProtocol::Discover* does not use the IMAP hierarchy delimiter given by the server

RESOLVED FIXED in Thunderbird 48.0

Status

MailNews Core
Networking: IMAP
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: R.Koerner, Assigned: R.Koerner)

Tracking

unspecified
Thunderbird 48.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8715330 [details] [diff] [review]
Thunderbird_38.5.1_fix_ImapProtocol_DiscoverMailboxes_using_IMAP_delimiter.patch

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160126223146

Steps to reproduce:

The attached file contains a small patch for two file with just a few lines of code that could fix the problem.

The problem: in file "comm-esr38/mailnews/imap/src/nsImapProtocol.cpp":
DiscoverMailboxList() and DiscoverAllAndSubscribedBoxes() both use the string "INBOX." as IMAP namespace with a hard-coded dot "." as delimiter.

code-part:
if (!gHideUnusedNamespaces && *prefix &&
   7133         PL_strcasecmp(prefix, "INBOX."))



Actual results:

Code won't work as intended for IMAP hierarchy delimiters other than a dot ".".


Expected results:

The correct IMAP hierarchy delimiter (provided by the IMAP server) is used.

Comment 1

2 years ago
Comment on attachment 8715330 [details] [diff] [review]
Thunderbird_38.5.1_fix_ImapProtocol_DiscoverMailboxes_using_IMAP_delimiter.patch

Thanks for the patch. I'll try to look at it and comment, but I'm a little behind at the moment so it might be a week or two.
Attachment #8715330 - Flags: review?(rkent)

Updated

2 years ago
Assignee: nobody → r.koerner
Summary: [patch attached] nsImapProtocol::Discover* does not use the IMAP hierarchy delimiter given by the server → nsImapProtocol::Discover* does not use the IMAP hierarchy delimiter given by the server

Updated

2 years ago
Component: Untriaged → Networking: IMAP
Product: Thunderbird → MailNews Core
Version: 38 Branch → unspecified

Comment 2

2 years ago
Comment on attachment 8715330 [details] [diff] [review]
Thunderbird_38.5.1_fix_ImapProtocol_DiscoverMailboxes_using_IMAP_delimiter.patch

OK this makes sense. Sorry it took so long, I'm sort of a reviewer of last resort, so I have to study the issue carefully even for small changes.

It seems to me that the consequences of this bug though would be that an extra LSUB is done on the INBOX so I'm not sure that users would see that. Are you aware of any user-visible effects of this?
Attachment #8715330 - Flags: review?(rkent) → review+

Comment 3

2 years ago
https://hg.mozilla.org/comm-central/rev/c07d429df541
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0

Comment 4

2 years ago
Looks like this might have introduced a mozmill debug failure?

Assertion failure: false (contentPolicyType not supported yet), at c:/builds/moz2_slave/tb-c-cen-w32-d-000000000000000/build/mozilla/dom/security/nsContentSecurityManager.cpp:158
(Assignee)

Comment 5

2 years ago
> Are you aware of any user-visible effects of this?

Sorry, no. I am not aware of any user-visible effect. I came across this when analysing another problem and it seemed to me that the code was definitely wrong since it used a hardcoded mailbox delimiter.

I wanted to know how I could provide a working patch, so I checked some Mozilla coding guidelines and wrote the patch. As far as I remember, I executed a patched version of Thunderbird in such a way that the if condition was once true and once false (for the mailbox name and delimiter), but did not do any further testing.

Comment 6

2 years ago
(In reply to aleth [:aleth] from comment #4)
> Looks like this might have introduced a mozmill debug failure?
> 
> Assertion failure: false (contentPolicyType not supported yet), at
> c:/builds/moz2_slave/tb-c-cen-w32-d-000000000000000/build/mozilla/dom/
> security/nsContentSecurityManager.cpp:158

That is unlikely, as mozmill does not use IMAP message downloads as far as I know. If it does affect mozmill that would be interesting, as I would not expect any UI-visible effect of this patch.

Let me do a try run.

Comment 7

2 years ago
If you check the try server run:

 https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1042a9e054b9ebe331c0a589c238259e88b5d557

then backing out this bug did not get rid of the mozmill failure.
You need to log in before you can comment on or make changes to this bug.