The default bug view has changed. See this FAQ.

Use new nsIMsgIncomingServer.protocolInfo attribute where useful

RESOLVED FIXED in Thunderbird 24.0

Status

MailNews Core
Backend
--
trivial
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: aceman, Assigned: sshagarwal)

Tracking

Trunk
Thunderbird 24.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=aceman][lang=C++ and/or Javascript])

Attachments

(6 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
Bug 832034 added a new attribute to nsIMsgIncomingServer called protocolInfo that returns the specific nsIMsgProtocolInfo service for the server in question.

Use it where appropriate instead of getting the service directly:

Components.classes["@mozilla.org/messenger/protocol/info;1?type=" + server.type].getService(Components.interfaces.nsIMsgProtocolInfo)

This is mostly useful where we already have the server object. This also makes the code more general as it does not need to hardcode the way how the protocol info service for a server type is retrieved.

This needs to be done in the C++ and also Javascript code so can be split in 2 patches. Or more, depending on the number of places where it is used.
FYI. Dump data of nsIMsgIncomingServer.protocolInfo(Gmail IMAP account)
> CurrentServer.protocolInfo : [xpconnect wrapped nsIMsgProtocolInfo]
>   CurrentServer.protocolInfo / Property
>     canDelete = true
>     canDuplicate = true
>     canGetIncomingMessages = true
>     canGetMessages = true
>     canLoginAtStartUp = true
>     defaultDoBiff = true
>     defaultLocalPath = [xpconnect wrapped nsIFile]
>     foldersCreatedAsync = true
>     preflightPrettyNameWithEmailAddress = true
>     requiresUsername = true
>     serverIID = {ea6a0765-07b8-40df-924c-9004ed707251}
>     showComposeMsgLink = true
>   CurrentServer.protocolInfo / Function
>     QueryInterface = QueryInterface()
>     getDefaultServerPort = getDefaultServerPort()
(Reporter)

Comment 2

4 years ago
So for the javascript part, replace code like this:
(server is a nsIMsgIncomingServer object)
...
protocolInfo = Components
.classes["@mozilla.org/messenger/protocol/info;1?type=" + server.type]
.getService(Components.interfaces.nsIMsgProtocolInfo);
var wantedData = protocolInfo.canGetMessages;

with:

var wantedData = server.protocolInfo.canGetMessages;

--------------------------------------------------
For the C++ part:
If you already have a server like this:
nsCOMPtr<nsIMsgIncomingServer> server;
rv = GetServer(getter_AddRefs(server));

nsCString type;
rv = server->GetType(type);

Then replace:
nsCOMPtr<nsIMsgProtocolInfo> protocolInfo = do_GetService("@mozilla.org/messenger/protocol/info;1?type=" + type, &rv);

with:

nsCOMPtr<nsIMsgProtocolInfo> protocolInfo;
rv = server->GetProtocolInfo(getter_AddRefs(protocolInfo));
NS_ENSURE_SUCCESS(rv, rv);

and it may also be possible to eliminate 'type' variable and the GetType() call.

Comment 3

4 years ago
I'd like to take this ...
(Reporter)

Comment 4

4 years ago
I am sorry, Suyash is already working on this, he just didn't yet get to mark it here. So I fix it.

Suyash, please attach the patches soon.
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
Created attachment 748588 [details] [diff] [review]
Patch (JS)

Patch involving the javascript files.
(Assignee)

Comment 6

4 years ago
Created attachment 748589 [details] [diff] [review]
Patch (CPP)

Patch involving the c++ files.
(Reporter)

Comment 7

4 years ago
Created attachment 748590 [details] [diff] [review]
patch for checkUser

One small change to functionality in account manager, that is why I add this as a separate patch.
(Reporter)

Comment 8

4 years ago
Try run with all these patches:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d13fae67e261

The linux failures also happen without the patches. The WinXP debug crashes are strange.
(Reporter)

Updated

4 years ago
Attachment #748590 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 9

4 years ago
Comment on attachment 748588 [details] [diff] [review]
Patch (JS)

I'm happy with this patch.
Attachment #748588 - Flags: review?(mkmelin+mozilla)
Attachment #748588 - Flags: review?(iann_bugzilla)
Attachment #748588 - Flags: feedback+
(Reporter)

Comment 10

4 years ago
Comment on attachment 748589 [details] [diff] [review]
Patch (CPP)

I'm happy with this patch. Thanks Suyash.
Attachment #748589 - Flags: review?(neil)
Attachment #748589 - Flags: feedback+

Comment 11

4 years ago
Comment on attachment 748588 [details] [diff] [review]
Patch (JS)

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

r=mkmelin with a couple of nits and such

::: mailnews/base/content/msgAccountCentral.js
@@ +62,5 @@
> +    try {
> +      protocolInfo = server.protocolInfo;
> +    } catch (e) {
> +      exceptions.push (e);
> +    }

What's all this for? (And if valid, nit about the space after push + you don't seem to use the protocolInfo so it doesn't need to be declared at all)

::: mailnews/base/prefs/content/AccountWizard.js
@@ +450,5 @@
> +      try {
> +        IID = srcServer.protocolInfo.serverIID;
> +      } catch (ex) {
> +        Components.utils.reportError("Could not get IID for " + srcServer.type + ": " + ex);
> +        IID = null;

no need to null IID here
Attachment #748588 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 748589 [details] [diff] [review]
Patch (CPP)

>--- a/mailnews/base/src/nsMsgAccountManagerDS.cpp
>+++ b/mailnews/base/src/nsMsgAccountManagerDS.cpp
>   bool canGetMessages = false;
>-  protocolInfo->GetCanGetMessages(&canGetMessages);
>+  rv = protocolInfo->GetCanGetMessages(&canGetMessages);
>+  NS_ENSURE_SUCCESS(rv, false);
Nit: if you rv-check then you don't need to initialise the bool. (2x)

>--- a/mailnews/base/src/nsMsgPurgeService.cpp
>+++ b/mailnews/base/src/nsMsgPurgeService.cpp
>-        nsAutoCString contractid(NS_MSGPROTOCOLINFO_CONTRACTID_PREFIX);
>-        contractid.Append(type);
>-
>-        nsCOMPtr<nsIMsgProtocolInfo> protocolInfo =
>-          do_GetService(contractid.get(), &rv);
>+        nsCOMPtr<nsIMsgProtocolInfo> protocolInfo;
>+        rv = server->GetProtocolInfo(getter_AddRefs(protocolInfo));
protocolInfo is unused so you might as well remove it completely.

>--- a/mailnews/imap/src/nsImapIncomingServer.cpp
>+++ b/mailnews/imap/src/nsImapIncomingServer.cpp
>-  nsCOMPtr <nsIMsgProtocolInfo> protocolInfo = do_GetService("@mozilla.org/messenger/protocol/info;1?type=imap", &rv);
I'm in two minds about this one - the advantage here is that we know the final contract ID already, which makes it more efficient than going through the generic but more readable version.
Comment on attachment 748588 [details] [diff] [review]
Patch (JS)

>+    if (server.protocolInfo.canDelete)
>       canDelete = true;
>     else
>       canDelete = server.canDelete;
[Perhaps write this as
   canDelete = server.protocolInfo.canDelete || server.canDelete;
 ?]
(Assignee)

Comment 14

4 years ago
Created attachment 749222 [details] [diff] [review]
Patch v2(JS)

Sir,
Thanks for your comments. I have made the changes.
Melin sir, I think protocolInfo is used at lines 76, 83, 90 and few more, so I have left it.
Neil sir, your || operator suggestion was nice thanks.
:aceman sir, please provide the feedback if something needs to be changed.
Carrying over review from mkmelin.
Attachment #748588 - Attachment is obsolete: true
Attachment #748588 - Flags: review?(iann_bugzilla)
Attachment #749222 - Flags: review+
Attachment #749222 - Flags: feedback?(acelists)
(Assignee)

Updated

4 years ago
Attachment #749222 - Flags: review?(iann_bugzilla)

Comment 15

4 years ago
Ah yes protocolInfo was an argument to the function before. But why the try-catch?
(Assignee)

Comment 16

4 years ago
(In reply to Magnus Melin from comment #15)
> Ah yes protocolInfo was an argument to the function before. But why the
> try-catch?
Sir,

It was there before I made these changes so I left it as such. Should I remove it?
(Reporter)

Comment 17

4 years ago
(In reply to Magnus Melin from comment #15)
> But why the try-catch?

Gettting of protocolInfo was in a try catch block previously. Also, the account central is intentionally hardened with try catch blocks where possible against misbehaving servers or undefined attributes. See ArrangeAccountCentralItems. So I would suggest keeping it.
(Reporter)

Comment 18

4 years ago
Comment on attachment 749222 [details] [diff] [review]
Patch v2(JS)

Yes, seems to fix what the reviewers found.
Attachment #749222 - Flags: feedback?(acelists) → feedback+
(Assignee)

Comment 19

4 years ago
Created attachment 749259 [details] [diff] [review]
Patch v2(CPP)

Sir,

I have made the changes as you suggested.
And for the changes in /mailnews/imap/src/nsImapIncomingServer.cpp,
I have preferred efficiency & added a comment for it.
Attachment #748589 - Attachment is obsolete: true
Attachment #748589 - Flags: review?(neil)
Attachment #749259 - Flags: review?(neil)
Attachment #749259 - Flags: feedback?(acelists)
(Reporter)

Comment 20

4 years ago
Comment on attachment 749259 [details] [diff] [review]
Patch v2(CPP)

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

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +2839,1 @@
>    nsCOMPtr <nsIMsgProtocolInfo> protocolInfo = do_GetService("@mozilla.org/messenger/protocol/info;1?type=imap", &rv);

OK, in that case, is it possible to use the NS_IMAPPROTOCOLINFO_CONTRACTID define here? It is defined in nsMsgImapCID.h which is included here...

Comment 21

4 years ago
(In reply to :aceman from comment #17)
> (In reply to Magnus Melin from comment #15)
> > But why the try-catch?
> 
> Gettting of protocolInfo was in a try catch block previously. Also, the
> account central is intentionally hardened with try catch blocks where
> possible against misbehaving servers or undefined attributes. See
> ArrangeAccountCentralItems. So I would suggest keeping it.

Maybe someone else want to weigh in, but it sure looks bogus to me. Surely 3rd party account types have to make sure there's a protocol info for their type. Otherwise things would blow up all over the place.
(Reporter)

Comment 22

4 years ago
Actually protocolInfo is an attribute of nsMsgIncomingServer. Can we assume all descendent server types inherit it automatically? What about javascript ones, like IM? I probably forgot to define it for IM in bug 832034...

Comment 23

4 years ago
syshagarwal had a discussion with me on IRC about comment 20. My take on that is that, whenever possible, it would be best to eliminate specific references to the contractID of protocolInfo for a specific type. The case is not as strong in the context of nsImapIncomingServer.cpp as it would be in UI code, bit still I would prefer that instead of seeing a contractID there, that this line:

>    nsCOMPtr <nsIMsgProtocolInfo> protocolInfo = do_GetService("@mozilla.org/messenger/protocol/info;1?type=imap", &rv);

used GetProtocolInfo instead.
(Assignee)

Comment 24

4 years ago
Neil sir, 

From comment 12, comment 20, comment 23, I am unable to decide whether to keep 
>nsCOMPtr <nsIMsgProtocolInfo> protocolInfo = do_GetService("@mozilla.org/messenger/protocol/info;1?type=imap", &rv);

as is, or use GetProtocolInfo for this. 
Please help me in deciding.
Flags: needinfo?(neil)
Comment on attachment 749259 [details] [diff] [review]
Patch v2(CPP)

Well, I'm happy with this patch. If between the three of you you want to change nsImapIncomingServer.cpp then just file a separate patch.
Attachment #749259 - Flags: review?(neil) → review+
Flags: needinfo?(neil)
(Assignee)

Comment 26

4 years ago
Created attachment 751196 [details] [diff] [review]
Patch v2(CPP) revised

I have added the #define NS_IMAPPROTOCOLINFO_CONTRACTID.
Carrying over review from Neil.
Attachment #749259 - Attachment is obsolete: true
Attachment #749259 - Flags: feedback?(acelists)
(Assignee)

Updated

4 years ago
Attachment #751196 - Flags: review+
(In reply to :aceman from comment #22)
> Actually protocolInfo is an attribute of nsMsgIncomingServer. Can we assume
> all descendent server types inherit it automatically? What about javascript
> ones, like IM? I probably forgot to define it for IM in bug 832034...

Indeed, it looks like you wanted to add something in http://mxr.mozilla.org/comm-central/source/mail/components/im/imIncomingServer.js
(Reporter)

Comment 28

4 years ago
Created attachment 751466 [details] [diff] [review]
add protocolInfo to im
Attachment #751466 - Flags: review?(florian)
Comment on attachment 751466 [details] [diff] [review]
add protocolInfo to im

Thanks!
Attachment #751466 - Flags: review?(florian) → review+

Comment 30

4 years ago
Comment on attachment 748590 [details] [diff] [review]
patch for checkUser

>+++ b/mailnews/base/prefs/content/AccountManager.js
>@@ -486,16 +486,18 @@ function checkUserServerChanges(showAler
>   var accountValues = getValueArrayFor(currentAccount);
>   if (!accountValues)
>     return true;
> 
>   var pageElements = getPageFormElements();
>   if (!pageElements)
>     return true;
> 
>+  let currentServer = currentAccount.incomingServer;
Isn't there certain circumstances where currentAccount is null? (It used to be null checked further down in the code you are replacing)

>+  // There is no username needed for e.g. news so reset it.
>+  if (!currentServer.protocolInfo.requiresUsername) {
Null check currentServer?

>-      if (currentAccount && checkUser) {
>-        filterList = currentAccount.incomingServer.getEditableFilterList(null);
>+      if (checkUser) {
>+        filterList = currentServer.getEditableFilterList(null);
Should the validity of currentServer be checked here?

r=me with those addressed / answered.
Attachment #748590 - Flags: review?(iann_bugzilla) → review+
(Reporter)

Comment 31

4 years ago
Created attachment 752316 [details] [diff] [review]
patch for checkUser v2

Ok, thanks.
Attachment #748590 - Attachment is obsolete: true
Attachment #752316 - Flags: review?(mkmelin+mozilla)

Comment 32

4 years ago
Comment on attachment 752316 [details] [diff] [review]
patch for checkUser v2

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

I'm not a reviewer for this code, but looks ok to me.
Attachment #752316 - Flags: review?(mkmelin+mozilla) → feedback+

Updated

4 years ago
Attachment #749222 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/34e7ff6c48e6
https://hg.mozilla.org/comm-central/rev/bffe2188e9e0
https://hg.mozilla.org/comm-central/rev/4a0ad377d047
https://hg.mozilla.org/comm-central/rev/b63bf044ddcc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
(Reporter)

Comment 34

4 years ago
Created attachment 756155 [details] [diff] [review]
implement requiresUsername in RSS

The patch using protocolInfo.RequiresUsername uncovered that the attribute is unimplemented for RSS server types. That causes an exception (that is hidden) in the account manager now and makes it unable to leave the RSS server settings pane.

So I implement it here to return false. I assume RSS does not use username and there is no field for it in the account manager.
Attachment #756155 - Flags: review?(neil)
Attachment #756155 - Flags: feedback?(alta88)
(In reply to aceman from comment #34)
> The patch using protocolInfo.RequiresUsername uncovered that the attribute
> is unimplemented for RSS server types. That causes an exception (that is
> hidden) in the account manager now and makes it unable to leave the RSS
> server settings pane.
Which patch and why didn't it cause an exception before?
(Reporter)

Comment 36

4 years ago
Patch https://bugzilla.mozilla.org/attachment.cgi?id=752316 and I don't know why the exception is not visible. Something hides it. When I added try {} catch (e) { reportError(e) } around whole code of oAccountManager.js::nAccountTreeSelect it was shown. The STRs are click the RSS account main settings pane. Then click anything else. It will not change to the new pane as the exception is thrown.
(In reply to :aceman from comment #36)
> Patch https://bugzilla.mozilla.org/attachment.cgi?id=752316 and I don't know
> why the exception is not visible. Something hides it. When I added try {}
> catch (e) { reportError(e) } around whole code of
> oAccountManager.js::nAccountTreeSelect it was shown.

Following code produces "TypeError: currentServer.protocolInfo is undefined", when currentServer.protocolInfo is undefined.
> if (currentServer && !currentServer.protocolInfo.requiresUsername)
In Tb 17, when code like above in add-on produces such error, error is reported to Error Console by Tb.
However, in Tb trunk nightly, error is not reported to Error Console by Tb, and JS code execution silently fails.
So, check of my coding error by trunk is hard in some cases, and I must check with Tb 17 when funny phenomenon happens after code change.

Needless to say, above Type Error due to "referred property is undefined" is bypassed by one of followings.
(A) existence check of higher level property in if
> if (currentServer && currentServer.protocolInfo && !currentServer.protocolInfo.requiresUsername)
(B) try/ctach for part of code
> try
> {
>    if (currentServer && !currentServer.protocolInfo.requiresUsername){ ... }
> } catch(e){ report error, and continue execution }

This case?
Created attachment 756242 [details]
HTML with JavaScript produces Type Error, and code to bypass it
Attachment #756242 - Attachment mime type: text/plain → text/html

Comment 39

4 years ago
(In reply to :aceman from comment #34)
> Created attachment 756155 [details] [diff] [review]
> implement requiresUsername in RSS
> 
> The patch using protocolInfo.RequiresUsername uncovered that the attribute
> is unimplemented for RSS server types. That causes an exception (that is
> hidden) in the account manager now and makes it unable to leave the RSS
> server settings pane.
> 
> So I implement it here to return false. I assume RSS does not use username
> and there is no field for it in the account manager.

Currently feeds don't know about username.  Though there have been requests for auth capability for https:// sites and internal feeds.
(Reporter)

Comment 40

4 years ago
WADA, currentServer.protocolInfo should not fail, only currentServer.protocolInfo.requiresUsername, and my patch fixes it. What kind of server are you trying and is your TB updated to trunk?

Why exceptions are not shown globally in the Account manager (at least in this area of code) is a separate problem.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to :aceman from comment #40)
> What kind of server are you trying and is your TB updated to trunk?

That is not actual currentServer.protocolInfo == undefined case. As seen in attached HTML and Javacript, that is simply an example of Type Error in "if(Obj&&!Obj.PropA.PropB)" when Obj.PropA is undefined.

However, "phenomenon of 'currentServer.PropertyX which should not be undefined' is returned as undefined to JavaScript code" is actual phenomenon which occurs on currentServer.trashFolderName, currentServer.isGmail, currentServer.personalNamespace etc.(properties on the nsIImapIncomingServer interface), even when currentServer is actual Gmail IMAP server.
See bug 831664.
Comment on attachment 756155 [details] [diff] [review]
implement requiresUsername in RSS

> NS_IMETHODIMP nsRssService::GetRequiresUsername(bool *aRequiresUsername)
> {
>-    return NS_ERROR_NOT_IMPLEMENTED;
>+    NS_ENSURE_ARG_POINTER(aRequiresUsername);
>+
>+    *aRequiresUsername = false;
>+    return NS_OK;
Nit: file style seems to be to not leave a blank line (compare e.g. GetShowComposeMsgLink). r=me with that fixed.
Attachment #756155 - Flags: review?(neil) → review+
(Reporter)

Comment 43

4 years ago
Created attachment 756963 [details] [diff] [review]
implement requiresUsername in RSS v2 [ready for check-in]

OK, thanks.
Attachment #756155 - Attachment is obsolete: true
Attachment #756155 - Flags: feedback?(alta88)
Attachment #756963 - Flags: review+
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/7ecbbc9bd30d
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

3 years ago
Depends on: 1008835
You need to log in before you can comment on or make changes to this bug.