The default bug view has changed. See this FAQ.

Should use presence of X-GM-EXT-1 capability to identify Gmail IMAP server

RESOLVED FIXED in Thunderbird 20.0

Status

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

People

(Reporter: dlech, Assigned: atuljangra)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 20.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird18 fixed, thunderbird19 fixed, thunderbird-esr1718+ fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Currently, Gmail server is identified by presence of \AllMail special folder returned by XLIST command
https://mxr.mozilla.org/comm-central/ident?i=kImapAllMail&tree=comm-central&filter=nsImapServerResponseParser
https://mxr.mozilla.org/comm-central/ident?i=SetIsGMailServer&tree=comm-central&filter=nsImapIncomingServer

This should be changed for two reasons:
1. Gmail may drop XLIST in favor of LIST-EXTENDED extension in the future, which would break the current implementation.
2. As of bug 721316, we are using features of X-GM-EXT-1, so it makes sense that the isGmail function/attribute should come from this capability.
(Reporter)

Updated

5 years ago
Blocks: 798145
(Reporter)

Comment 1

5 years ago
This is followup bug to item 7 on https://bugzilla.mozilla.org/show_bug.cgi?id=721316#c60
(Reporter)

Comment 2

5 years ago
(In reply to David Lechner (:dlech) from comment #0)
> 1. Gmail may drop XLIST in favor of LIST-EXTENDED extension in the future,

More specifically, RFC 6154
(Assignee)

Comment 3

4 years ago
So you are suggesting that we should use X-GM-EXT-1 instead of XLIST(All Mail). And we should also clear the pref if we are not getting X-GM-EXT-1?
Also, can you point to some code on c-c?
(Assignee)

Comment 4

4 years ago
Confirming the bug as we are sure we want to do this. Otherwise situations like Bug 815087 can arise.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 5

4 years ago
(In reply to Atul Jangra [:atuljangra] from comment #3)
> So you are suggesting that we should use X-GM-EXT-1 instead of XLIST(All
> Mail). 

Yes.

> And we should also clear the pref if we are not getting X-GM-EXT-1?
> Also, can you point to some code on c-c?
Delete this:
https://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapIncomingServer.cpp#1188
Add X-GM-EXT1 capability here:
https://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapCore.h#147
Make an ServerIsGmailServerthat looks like ServerIsAOLServer here:
https://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapServerResponseParser.h#97
Use GetServerStateParser().ServerIsGmailServer() instead of m_isGmailServer here:
https://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#3452

And there is still the business of calling setIsGMailServer() somewhere.
(Reporter)

Updated

4 years ago
Blocks: 815087
(Assignee)

Comment 6

4 years ago
Created attachment 695059 [details] [diff] [review]
Pretty much does the job

Few changes that uses X-GM-EXT-1 as a check for gmail server instead of X-LIST.

Passes the tests locally. Tests specific to gmail: test_gmailAttributes.js and test_gmailMsgOfflineStore.js
Will push to try as soon as possible, or if someone else can push in the meantime then that would be awesome too. :-)
Assignee: nobody → atuljangra66
Status: NEW → ASSIGNED
Attachment #695059 - Flags: review?(mozilla)
Attachment #695059 - Flags: review?(ludovic)
(Assignee)

Comment 7

4 years ago
I believe that this will also solve the problem reported in Bug 815087.
(Reporter)

Comment 8

4 years ago
Comment on attachment 695059 [details] [diff] [review]
Pretty much does the job

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

Just being nosy :)

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +675,5 @@
>      nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server);
>      nsCOMPtr<nsIStreamListener> aRealStreamListener = do_QueryInterface(aConsumer);
>      m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel));
> +    eIMAPCapabilityFlags server_capabilityFlags = GetServerStateParser().GetCapabilityFlag();
> +    m_isGmailServer = ((server_capabilityFlags & kGmailImapCapability) != 0);

You should be able to use the function you created in nsImapServerResponseParser.h like this...

m_isGmailServer = GetServerStateParser().ServerIsGMailServer();

instead of the two lines above (678 and 679).

@@ +676,5 @@
>      nsCOMPtr<nsIStreamListener> aRealStreamListener = do_QueryInterface(aConsumer);
>      m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel));
> +    eIMAPCapabilityFlags server_capabilityFlags = GetServerStateParser().GetCapabilityFlag();
> +    m_isGmailServer = ((server_capabilityFlags & kGmailImapCapability) != 0);
> +    imapServer->SetIsGMailServer(m_isGmailServer);

For the sake of me learning, I am curious to know why you choose to call SetIsGMailServer() here?
(Reporter)

Comment 9

4 years ago
(In reply to Atul Jangra [:atuljangra] from comment #7)
> I believe that this will also solve the problem reported in Bug 815087.

I tried it out, repeating the steps to cause Bug 815087 and the is_gmail preference changes to false as we hoped, so unless you change something, I can say that this patch _definitely_ fixes Bug 815087.
(Assignee)

Comment 10

4 years ago
Created attachment 695157 [details] [diff] [review]
Final Patch

Patch V2.
Addressed Dlech's comments :-)
Attachment #695059 - Attachment is obsolete: true
Attachment #695059 - Flags: review?(mozilla)
Attachment #695059 - Flags: review?(ludovic)
Attachment #695157 - Flags: review?(mozilla)
Attachment #695157 - Flags: review?(ludovic)
(Assignee)

Comment 11

4 years ago
(In reply to David Lechner (:dlech) from comment #8)
> Just being nosy :)
> 
> ::: mailnews/imap/src/nsImapProtocol.cpp
> @@ +675,5 @@
> >      nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server);
> >      nsCOMPtr<nsIStreamListener> aRealStreamListener = do_QueryInterface(aConsumer);
> >      m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel));
> > +    eIMAPCapabilityFlags server_capabilityFlags = GetServerStateParser().GetCapabilityFlag();
> > +    m_isGmailServer = ((server_capabilityFlags & kGmailImapCapability) != 0);
> 
> You should be able to use the function you created in
> nsImapServerResponseParser.h like this...
> 
> m_isGmailServer = GetServerStateParser().ServerIsGMailServer();
> 
> instead of the two lines above (678 and 679).
Done.
> @@ +676,5 @@
> >      nsCOMPtr<nsIStreamListener> aRealStreamListener = do_QueryInterface(aConsumer);
> >      m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel));
> > +    eIMAPCapabilityFlags server_capabilityFlags = GetServerStateParser().GetCapabilityFlag();
> > +    m_isGmailServer = ((server_capabilityFlags & kGmailImapCapability) != 0);
> > +    imapServer->SetIsGMailServer(m_isGmailServer);
> 
> For the sake of me learning, I am curious to know why you choose to call
> SetIsGMailServer() here?
Because this is the code where we are in a state when we have got the capabilities flag and we are going to request headers. You can see a similar pattern with AOLServers in the code.

Comment 12

4 years ago
Seems like a more natural place to do it would be here:

http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapIncomingServer.cpp#2172
(Assignee)

Updated

4 years ago
URL:
(Assignee)

Comment 13

4 years ago
(In reply to David :Bienvenu from comment #12)
> Seems like a more natural place to do it would be here:
> 
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/
> nsImapIncomingServer.cpp#2172

Yes,. I was looking for exactly the same place.  I've updated the patch.

Also, now there is no need of ServerIsGMailServer() anymore as we can check gmail server by 
imapServer->GetIsGMailServer(&m_isGmailServer);
( as we did earlier)
(Assignee)

Updated

4 years ago
URL:
(Assignee)

Comment 14

4 years ago
Created attachment 695269 [details] [diff] [review]
Final Patch

Made the required minimal changes.
Attachment #695157 - Attachment is obsolete: true
Attachment #695157 - Flags: review?(mozilla)
Attachment #695157 - Flags: review?(ludovic)
Attachment #695269 - Flags: review?(mozilla)
Attachment #695269 - Flags: review?(ludovic)

Comment 15

4 years ago
Comment on attachment 695269 [details] [diff] [review]
Final Patch

looks reasonable, thx, except that you might as well use capability instead of m_capability, to be consistent.
Attachment #695269 - Flags: review?(mozilla) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 695374 [details] [diff] [review]
Final Patch

Changes m_capability to capability.
Already got r+. So checkin-needed.
Attachment #695269 - Attachment is obsolete: true
Attachment #695269 - Flags: review?(ludovic)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/ea973e3223b2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
(Assignee)

Comment 18

4 years ago
Comment on attachment 695374 [details] [diff] [review]
Final Patch

Since this caused problems to users namely Bug 815087, I was hoping that we can land this earlier. Thus requesting beta approval.
Attachment #695374 - Flags: approval-comm-beta?
Comment on attachment 695374 [details] [diff] [review]
Final Patch

[Triage Comment]
Ok, this seems reasonably safe and should be obvious if we hit regressions, a=Standard8
Attachment #695374 - Flags: approval-comm-esr17+
Attachment #695374 - Flags: approval-comm-beta?
Attachment #695374 - Flags: approval-comm-beta+
Attachment #695374 - Flags: approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/32e809bfb669
https://hg.mozilla.org/releases/comm-beta/rev/58a404dbb9d1
https://hg.mozilla.org/releases/comm-esr17/rev/1d8bcbd56570
status-thunderbird18: --- → fixed
status-thunderbird19: --- → fixed
status-thunderbird-esr17: --- → fixed
tracking-thunderbird-esr17: --- → 18+
You need to log in before you can comment on or make changes to this bug.