813929 - Lost the SMTP parameters line in Thunderbird

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[[e-mail]]

Attachment: TBPB.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121024073032

Steps to reproduce:

Nothing. I just try to attempt the SMTP configuration.


Actual results:

The SMTP configuration line in account parameters windows as disappear. Instead of, there is a blank line. When I click on this line, on the right side of the windows, A new view of TB appears.

Comment 1

[[e-mail]]

And the error lines in tools

Horodatage : 06/11/2012 16:33:00
Erreur : NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFolder.prettyName]
Fichier Source : chrome://messenger/content/AccountManager.js
Ligne : 1185

Horodatage : 06/11/2012 16:32:50
Erreur : SyntaxError: syntax error
Fichier Source : chrome://messenger/content/selectionsummaries.js
Ligne : 291, Colonne : 24
Code Source :
let msgContents = <div class="row">

the end :

Comment 2

[:aceman]

Very strange. Can you try in TB safe mode: help -> restart with addons disabled?

Comment 3

[:aceman]

Wayne, have you ever seen this strangeness?

Comment 4

[Vincent (caméléon)]

just wanted to add that the user has posted its question on different French forums (including Geckozone) and on MozillaZine without anybody able to help...

See: 
http://forums.mozillazine.org/viewtopic.php?f=39&t=2607725
http://forums.mozfr.org/viewtopic.php?f=4&t=109139

I had never seen that before.

Comment 5

[:aceman]

This could be an addon problem or gecko problem (like painting old HW accelerated data).

Comment 6

[Ludovic Hirlimann [:Usul]]

Does removing localstore.rdf in your profile helps ?

Comment 7

[[e-mail]]

(In reply to :aceman from comment #2)
> Very strange. Can you try in TB safe mode: help -> restart with addons
> disabled?

Done. Same effect.

Comment 8

[[e-mail]]

(In reply to Ludovic Hirlimann [:Usul] from comment #6)
> Does removing localstore.rdf in your profile helps ?

Can you explain more of this way. What dou you mean ?

Comment 9

[[e-mail]]

(In reply to :aceman from comment #5)
> This could be an addon problem or gecko problem (like painting old HW
> accelerated data).

If a addon is the problem, it is definitive. Le safe mode as the same effect.

Comment 10

[Vincent (caméléon)]

(In reply to [e-mail] from comment #8)
> (In reply to Ludovic Hirlimann [:Usul] from comment #6)
> > Does removing localstore.rdf in your profile helps ?
> 
> Can you explain more of this way. What dou you mean ?
Ludovic demande de supprimer le fichier localstore.rdf présent dans ton profil Thunderbird (consulte cet article pour plus de détails sur le profil: https://support.mozillamessaging.com/fr/kb/profil-utilisateur )

Je pense qu'il est préférable d'effectuer cette manipulation en ayant au préalable fermé Thunderbird

Comment 11

[[e-mail]]

(In reply to Ludovic Hirlimann [:Usul] from comment #6)
> Does removing localstore.rdf in your profile helps ?

No, it does'nt.

Comment 12

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #3)
> Wayne, have you ever seen this strangeness?

nope. well, not for a long time.

Comment 13

[Wayne Mery (:wsmwk)]

(priority is reserved for developers https://bugzilla.mozilla.org/page.cgi?id=fields.html#priority )

Comment 14

[:aceman]

Is this a permanent problem? At each open of the account manager and click on SMTP line?

What about TB 17.0.5?

Comment 15

[[e-mail]]

(In reply to :aceman from comment #14)
> Is this a permanent problem? At each open of the account manager and click
> on SMTP line?
> 
> What about TB 17.0.5?

The problem is permanent. TB 17.0.5 as the same problem. I think that some files have been corrupted at one time, and now, nothing can make the situation back.

Comment 16

[:aceman]

Have you tried downloading the full TB installer for TB 17 ? Do did you just update ?

Comment 17

[[e-mail]]

(In reply to :aceman from comment #16)
> Have you tried downloading the full TB installer for TB 17 ? Do did you just
> update ?

I tried to install the full version of TB 17. Nothing changes. The only thing I have done is to create a new profil. In this case, the problem diseppears.

Comment 18

[:aceman]

(In reply to [e-mail] from comment #1)
> And the error lines in tools
> 
> Horodatage : 06/11/2012 16:33:00
> Erreur : NS_ERROR_FAILURE: Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsIMsgFolder.prettyName]
> Fichier Source : chrome://messenger/content/AccountManager.js
> Ligne : 1185

Can you update this error message? In TB 17, the line number could be different.

Comment 19

[[e-mail]]

(In reply to :aceman from comment #18)
> (In reply to [e-mail] from comment #1)
> > And the error lines in tools
> > 
> > Horodatage : 06/11/2012 16:33:00
> > Erreur : NS_ERROR_FAILURE: Component returned failure code: 0x80004005
> > (NS_ERROR_FAILURE) [nsIMsgFolder.prettyName]
> > Fichier Source : chrome://messenger/content/AccountManager.js
> > Ligne : 1185
> 
> Can you update this error message? In TB 17, the line number could be
> different.

I Don't understand what you want.
The only error I get is this one : 

Horodatage : 18/04/2013 20:50:36
Avertissement : La boîte XUL pour l'élément _moz_generated_content_before contient un enfant #text intégré, forçant tous ses enfants à être englobés dans un bloc.
Fichier Source : chrome://global/content/bindings/toolbar.xml
Ligne : 276

Comment 20

[:aceman]

You posted that error message in comment 1 with TB 16. Do you no longer get that error?

Comment 21

[[e-mail]]

(In reply to :aceman from comment #20)
> You posted that error message in comment 1 with TB 16. Do you no longer get
> that error?

I just clear the history. The only error I get is when I try to attempt to the smtp configuration.

Comment 22

[[e-mail]]

(In reply to [e-mail] from comment #21)
> (In reply to :aceman from comment #20)
> > You posted that error message in comment 1 with TB 16. Do you no longer get
> > that error?
> 
> I just clear the history. The only error I get is when I try to attempt to
> the smtp configuration.

I am sorry. I have two errors but they append not when I try to attempt the smtp configuration.

Horodatage : 18/04/2013 21:11:45
Erreur : NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFolder.abbreviatedName]
Fichier Source : chrome://messenger/content/folderPane.js
Ligne : 1944

and

Horodatage : 18/04/2013 21:11:45
Erreur : [Exception... "Component returned failure code: 0x80550007 [nsIMsgFolder.getStringProperty]"  nsresult: "0x80550007 (<unknown>)"  location: "JS frame :: chrome://messenger/content/folderPane.js :: getSmartFolderName :: line 2436"  data: no]
Fichier Source : chrome://messenger/content/folderPane.js
Ligne : 2438

Comment 23

[:aceman]

Pub, have you tried to set up a Instant messaging account in the past?

Comment 24

[:aceman]

Seems to be reproducible by Dagger by having a somehow broken IM account from the past.

Comment 25

[:aceman]

Attachment: WIP patch

OK, this is a WIP patch. Dagger, what do you think? Does it make AM in your profile work and still make you aware of the broken accounts?

Comment 26

[dagger.bugzilla]

Yeah, that does do the trick. SMTP settings are back. The invalid account shows as "Invalid account null"; if I click it, the right hand side of the dialog is blank. Account Actions -> Remove Account is disabled, so I can't delete the account, but at least it's not breaking AM any more.

I figured out how to reproduce this. Create a chat account, then hunt down the corresponding messenger.account.accountN.prpl pref, set it to some unrecognized protocol, and restart Tb.

Note that if you have one of these broken chat accounts the UI for chat also ends up broken, so we do need to fix the actual bug too, not just cover over it in AM. It might be easiest to just do that (though having AM be resilient to this sort of failure would of course be nice.)

The cause of the breakage is here:

https://mxr.mozilla.org/comm-central/source/chat/components/src/imAccounts.js#131

i.e. for accounts with unknown protocols, fall back to UnknownProtocol -- the problem is that this code block returns, but the account is only added to the internal list of accounts in the call to gAccountsService._keepAccount(), which is just after the return. Florian suggested fixing it by moving the _keepAccount() call further up the function. Since it depends only on this.numericId, it can be moved up to just after where that's defined.

Comment 27

[:aceman]

Thanks for the test. The reasoning of this "papering over" is that even if the rest of the UI is broken we would like at least the AM to be robust and allow e.g. deletion of the broken account. The cause of the broken account may be unknown and some other reason than what you have in your profile. I'll check why deletion is disabled for you, I'd like it enabled.

The code tries to show account name or at least key for the "Invalid account" but yours is so broken that it shows neither of them so the result is "Invalid account null". Actually I can fix this, by fetching account.key first which should succeed with higher probability.

Comment 28

[:aceman]

Comment on attachment 758278 [details] [diff] [review]
WIP patch

IanN, do you agree with the way I am going forward here?

Comment 29

[[e-mail]]

Sorry for my English, but I have not understood all you said. Do I have something to do ?

Comment 30

[:aceman]

Pub, look into file prefs.js and see if there are any prefs like "messenger.accounts" .

Comment 31

[[e-mail]]

It didn't find a occurence of the item messenger.accounts.

Comment 32

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 758278 [details] [diff] [review]
WIP patch

(Dagger already gave feedback…)

Comment 33

[[e-mail]]

Do I have something to do ?

Comment 34

[:aceman]

Let TB update to 24 (it is already released). Please comment if the problem is still there. Paste the error from console again (it may have a different line number now).

Comment 35

[[e-mail]]

Horodatage : 20/09/2013 13:33:26
Erreur : NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFolder.prettyName]
Fichier Source : chrome://messenger/content/AccountManager.js
Ligne : 1192

Comment 36

[:aceman]

OK. I think you have not yet replied to comment 23 whether you have some Instant messaging (IM/chat) accounts set up, or whether you have tried to have some in the past on this TB profile/installation.

Comment 37

[[e-mail]]

No, I don't have a instant messaging account neither I tried to have one.

Comment 38

[:aceman]

Other than this problem in account manager, all your accounts in Thunderbird are working fine and receiving messages?

So I assume you can not apply the patch yourself and build Thunderbird. We will make you a special build for Windows to try running with the patch. It should output some more info to the Error console to help you determine which account is causing the problem. We can then look at that one more closely.

Comment 39

[[e-mail]]

I 'am sorry. For the answer of the 35, it was with TB 17.08. With 24, I don't have any error number but the bug is always present.

Comment 40

[:aceman]

Aryx, could you please make a try-build with this patch for Windows (64bit)?

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed to Thunderbird-Try as https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=bc042bb9d673

Comment 42

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Re-pushed because Thunderbird-Try doesn't support win64 at the moment: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=247748b4b400

Comment 43

[Mike Conley (:mconley) (:⚙️)]

(In reply to dagger.bugzilla from comment #26)
> The cause of the breakage is here:
> 
> https://mxr.mozilla.org/comm-central/source/chat/components/src/imAccounts.
> js#131
> 
> i.e. for accounts with unknown protocols, fall back to UnknownProtocol --
> the problem is that this code block returns, but the account is only added
> to the internal list of accounts in the call to
> gAccountsService._keepAccount(), which is just after the return. Florian
> suggested fixing it by moving the _keepAccount() call further up the
> function. Since it depends only on this.numericId, it can be moved up to
> just after where that's defined.

This should probably get filed separately. I'm also Cc'ing clokep who's probably interested in knowing about this bug.

Comment 44

[dagger.bugzilla]

It was done in bug 861454. (That bug fixed the exception from the chat code I was seeing; the patch in this bug is about making AM handle exceptions in general.)

Comment 45

[Magnus Melin [:mkmelin]]

Comment on attachment 758278 [details] [diff] [review]
WIP patch

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

::: mailnews/base/prefs/content/AccountManager.js
@@ +1447,5 @@
> +    for (let account of accounts) {
> +      let accountName = null;
> +      let server = null;
> +      let accountKey = null;
> +      let AMChrome = null;

lowercase am

@@ +1451,5 @@
> +      let AMChrome = null;
> +      let panelsToKeep = [];
> +      let accountCorrect = true;
> +
> +      try {

+ need indenting
but what are you expecting will throw? better just put *that* part in the try-catch

Comment 46

[:aceman]

The point is we do not know what may throw again. So I try the whole block and then we may analyze the exceptions if any are found in the future.

Comment 47

[Magnus Melin [:mkmelin]]

Yuck:/

Comment 48

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 758278 [details] [diff] [review]
WIP patch

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

Needs some polish in here, but I think the general approach seems OK - though my experience hacking around in the account manager is very limited.

::: mailnews/base/prefs/content/AccountManager.js
@@ +1506,5 @@
> +
> +      } catch(e) {
> +        Components.utils.reportError("Error accessing account " + account + ": " + e);
> +        accountCorrect = false;
> +        accountName = "Invalid account " + accountName || accountKey;

Needs to be localizable.

@@ +1510,5 @@
> +        accountName = "Invalid account " + accountName || accountKey;
> +      }
> +
> +      // Create the top level tree-item
> +      var treeitem = document.createElement("treeitem");

let not var

@@ +1523,1 @@
>        if (panelsToKeep.length > 0) {

Indentation here too, for everything in here.

Or, this could be simplified I believe, as:

if (accountCorrect) {
  treeitem.setAttribute("PageTag", AMChrome);
}
if (panelsToKeep.length > 0) {
  // ...
}

Comment 49

[:aceman]

Attachment: patch v2

OK, what about this?

Comment 50

[:aceman]

Attachment: patch v3

Sorry, one more fix.

Comment 51

[Magnus Melin [:mkmelin]]

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

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

Looks reasonable to me.

::: mailnews/base/prefs/content/AccountManager.js
@@ +1513,5 @@
> +        amChrome = server ? server.accountManagerChrome : "am-main.xul";
> +      } catch(e) {
> +        // Show only a placeholder in the account list saying this account
> +        // is broken, with no child panels.
> +        Components.utils.reportError("Error accessing account " + account + ": " + e);

accountName?

Comment 52

[:aceman]

Attachment: patch v4

OK.
Magnus, do you intentionally do not want to review this? Can you propose another reviewer from TB?

Comment 53

[Magnus Melin [:mkmelin]]

Comment on attachment 821239 [details] [diff] [review]
patch v4

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

I'm not an official reviewer for /mailnews, and there's nothing /mail here.

Comment 54

[:aceman]

Comment on attachment 821239 [details] [diff] [review]
patch v4

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

OK.

Comment 55

[[e-mail]]

Please can you attach a -w version of the diff?

Comment 56

[:aceman]

Attachment: patch v4 (diff -w)

Sure.

Comment 57

[[e-mail]]

Comment on attachment 822367 [details] [diff] [review]
patch v4 (diff -w)

>+  if (!pageId) {
>+    Components.utils.reportError("No pageId to show specified!");
Surely this is expected i.e. you have already reported the error?

>+      let server = null;
>+      let accountKey = null;
>+      let amChrome = "";
>+      let panelsToKeep = [];
>+
>+      // This "try {} catch {}" block is intentionally very long to catch
>+      // unknown exceptions and confine them to this single account.
>+      // This may happen from broken accounts. See e.g. bug 813929.
>+      // Other accounts can still be shown properly if they are valid.
>+      try {
>+        server = account.incomingServer;
You only use server inside the try block.

>+        accountKey = account.key;
But you should fetch the key first as you use it in the catch block. (Maybe fetch the key before the try, even.)

>-      var offline = server.offlineSupportLevel;
>-      var diskspace = server.supportsDiskSpace;
>+        let offline = server.offlineSupportLevel;
>+        let diskspace = server.supportsDiskSpace;
[Silly var/let changes make the diff unnecessarily harder to read.]

>+        amChrome = server ? server.accountManagerChrome : "am-main.xul";
Nit: server can't be null at this point, we have its pretty name.

Comment 58

[Ian Neal]

Comment on attachment 821239 [details] [diff] [review]
patch v4

>+++ b/mailnews/base/prefs/content/AccountManager.js
>+    for (let account of accounts) {
>+      let accountName = null;
>+      let server = null;
As said by Neil, this is only used within the try, so move it into there.
>+      let accountKey = null;
As said by Neil, you should probably set the accountKey here.

>+      let amChrome = "";
Shouldn't this be set to "am-main.xul"?

>+        // Now add our panels
Nit: missing full stop.

>+        // Check offline/diskspace support level
Nit: missing full stop.
>+        let offline = server.offlineSupportLevel;
As this is only used once, it could be inlined.
>+        let diskspace = server.supportsDiskSpace;
>+        if (offline >= 10 && diskspace)
>+          panelsToKeep.push(panels[2]);
>+        else if (diskspace)
>+          panelsToKeep.push(panels[3]);
>+
>+        // extensions
>+        let catMan = Components.classes["@mozilla.org/categorymanager;1"]
>+                             .getService(Ci.nsICategoryManager);
Nit: line this up with the line above.

>+        amChrome = server ? server.accountManagerChrome : "am-main.xul";
As Neil said, we know we have server here, so just set it to server.accountManagerChrome

>       // Create the top level tree-item
Nit: missing full stop.

>     // Now add the outgoing server node
Nit: missing full stop.

r=me with those addressed / fixed.

Comment 59

[:aceman]

Attachment: patch v5

(In reply to Ian Neal from comment #58)
> >+      let amChrome = "";
> Shouldn't this be set to "am-main.xul"?
> 
> >+        amChrome = server ? server.accountManagerChrome : "am-main.xul";
> As Neil said, we know we have server here, so just set it to
> server.accountManagerChrome

No, this would set the chrome to am-main.xul also for accounts that throw. I don't want to display the main pane for them. Who knows how broken they are if they can't even report their name. In that case I wanted to keep amChrome blank so that I can catch it in pageURL and show about:blank. But I can set it to about:blank here directly.

Thanks for the other nits, I have fixed those.

Comment 60

[Ian Neal]

Comment on attachment 8338818 [details] [diff] [review]
patch v5

>+      try {

>+        amChrome = server ? server.accountManagerChrome : "am-main.xul";
As previously said, we already know we have server at this point, so either this statement needs moving after the try/catch (maybe even inline) or you need to split it into the try/catch parts i.e. amChrome = server.accountManagerChrome and amChrome = "am-main.xul"
>+      } catch(e) {
>+        // Show only a placeholder in the account list saying this account
>+        // is broken, with no child panels.
>+        let accountID = (accountName || accountKey);
>+        Components.utils.reportError("Error accessing account " + accountID + ": " + e);
>+        accountName = "Invalid account " + accountID;
>+        panelsToKeep.length = 0;
>+      }
>+
>+      // Create the top level tree-item.
>       var treeitem = document.createElement("treeitem");
>       mainTree.appendChild(treeitem);
>       var treerow = document.createElement("treerow");
>       treeitem.appendChild(treerow);
>       var treecell = document.createElement("treecell");
>       treerow.appendChild(treecell);
>+      treecell.setAttribute("label", accountName);
>+      treeitem.setAttribute("PageTag", amChrome);

r=me with that fixed

Comment 61

[:aceman]

Attachment: patch v6

OK, I see what you are up to. The server must not throw and must not be null at that point we assign amChrome. But then that check was useless even before my patch as a null server would throw much sooner above, at the server.type and other references of its properties.

Comment 62

[:aceman]

Comment on attachment 8345481 [details] [diff] [review]
patch v6

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

OK, thanks. So let's get a full mailnews r+.

Comment 63

[[e-mail]]

Comment on attachment 8345481 [details] [diff] [review]
patch v6

> function pageURL(pageId)
> {
>+  // If we have a special non account manager pane (e.g. about:blank),
>+  // do not translate it into ChromePackageName URL.
>+  if (pageId.substr(0,3) != "am-")
>+    return pageId;
Use .startsWith instead. r=me with that fixed.
[I didn't have a problem with the null pageId special case before, I just didn't see why you needed to report an error when you were expecting it. Also you forgot the space after comma.]

Comment 64

[:aceman]

Attachment: patch v7

Thanks, fixed the nits.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

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