Closed Bug 813929 Opened 8 years ago Closed 7 years ago

Lost the SMTP parameters line in Thunderbird

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: pub, Assigned: aceman)

References

Details

(Keywords: reproducible)

Attachments

(2 files, 7 obsolete files)

Attached image 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.
Severity: normal → major
Keywords: reproducible
Priority: -- → P1
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 :
Very strange. Can you try in TB safe mode: help -> restart with addons disabled?
Wayne, have you ever seen this strangeness?
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.
This could be an addon problem or gecko problem (like painting old HW accelerated data).
Does removing localstore.rdf in your profile helps ?
(In reply to :aceman from comment #2)
> Very strange. Can you try in TB safe mode: help -> restart with addons
> disabled?

Done. Same effect.
(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 ?
(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.
(In reply to Pub@salard.name 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
(In reply to Ludovic Hirlimann [:Usul] from comment #6)
> Does removing localstore.rdf in your profile helps ?

No, it does'nt.
(In reply to :aceman from comment #3)
> Wayne, have you ever seen this strangeness?

nope. well, not for a long time.
(priority is reserved for developers https://bugzilla.mozilla.org/page.cgi?id=fields.html#priority )
Priority: P1 → --
Is this a permanent problem? At each open of the account manager and click on SMTP line?

What about TB 17.0.5?
(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.
Have you tried downloading the full TB installer for TB 17 ? Do did you just update ?
(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.
(In reply to Pub@salard.name 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.
(In reply to :aceman from comment #18)
> (In reply to Pub@salard.name 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
You posted that error message in comment 1 with TB 16. Do you no longer get that error?
(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.
(In reply to Pub@salard.name 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
Pub, have you tried to set up a Instant messaging account in the past?
Seems to be reproducible by Dagger by having a somehow broken IM account from the past.
Assignee: nobody → acelists
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: Thunderbird → MailNews Core
Attached patch WIP patch (obsolete) — Splinter Review
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?
Attachment #758278 - Flags: feedback?(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.
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 on attachment 758278 [details] [diff] [review]
WIP patch

IanN, do you agree with the way I am going forward here?
Attachment #758278 - Flags: feedback?(iann_bugzilla)
Sorry for my English, but I have not understood all you said. Do I have something to do ?
Pub, look into file prefs.js and see if there are any prefs like "messenger.accounts" .
It didn't find a occurence of the item messenger.accounts.
Status: NEW → ASSIGNED
Comment on attachment 758278 [details] [diff] [review]
WIP patch

(Dagger already gave feedback…)
Attachment #758278 - Flags: feedback?(dagger.bugzilla)
Attachment #758278 - Flags: feedback?(mkmelin+mozilla)
Do I have something to do ?
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).
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
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.
Attachment #758278 - Flags: feedback?(mconley)
No, I don't have a instant messaging account neither I tried to have one.
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.
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.
Aryx, could you please make a try-build with this patch for Windows (64bit)?
(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.
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 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
Attachment #758278 - Flags: feedback?(mkmelin+mozilla) → feedback+
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.
Yuck:/
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) {
  // ...
}
Attachment #758278 - Flags: feedback?(mconley) → feedback+
Attached patch patch v2 (obsolete) — Splinter Review
OK, what about this?
Attachment #758278 - Attachment is obsolete: true
Attachment #758278 - Flags: feedback?(iann_bugzilla)
Attachment #817288 - Flags: review?(mkmelin+mozilla)
Attachment #817288 - Flags: review?(iann_bugzilla)
Attached patch patch v3 (obsolete) — Splinter Review
Sorry, one more fix.
Attachment #817288 - Attachment is obsolete: true
Attachment #817288 - Flags: review?(mkmelin+mozilla)
Attachment #817288 - Flags: review?(iann_bugzilla)
Attachment #817293 - Flags: review?(mkmelin+mozilla)
Attachment #817293 - Flags: review?(iann_bugzilla)
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?
Attachment #817293 - Flags: review?(mkmelin+mozilla) → feedback+
Attached patch patch v4 (obsolete) — Splinter Review
OK.
Magnus, do you intentionally do not want to review this? Can you propose another reviewer from TB?
Attachment #817293 - Attachment is obsolete: true
Attachment #817293 - Flags: review?(iann_bugzilla)
Attachment #821239 - Flags: review?(mkmelin+mozilla)
Attachment #821239 - Flags: review?(iann_bugzilla)
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.
Attachment #821239 - Flags: review?(mkmelin+mozilla)
Comment on attachment 821239 [details] [diff] [review]
patch v4

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

OK.
Attachment #821239 - Flags: review?(neil)
Please can you attach a -w version of the diff?
Attached patch patch v4 (diff -w) (obsolete) — Splinter Review
Sure.
Attachment #822367 - Flags: review?(neil)
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 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.
Attachment #821239 - Flags: review?(iann_bugzilla) → review+
Attached patch patch v5 (obsolete) — Splinter Review
(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.
Attachment #821239 - Attachment is obsolete: true
Attachment #822367 - Attachment is obsolete: true
Attachment #821239 - Flags: review?(neil)
Attachment #822367 - Flags: review?(neil)
Attachment #8338818 - Flags: review?(iann_bugzilla)
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
Attachment #8338818 - Flags: review?(iann_bugzilla) → review+
Attached patch patch v6 (obsolete) — Splinter Review
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.
Attachment #8338818 - Attachment is obsolete: true
Attachment #8345481 - Flags: review+
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+.
Attachment #8345481 - Flags: review+ → review?(neil)
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.]
Attachment #8345481 - Flags: review?(neil) → review+
Attached patch patch v7Splinter Review
Thanks, fixed the nits.
Attachment #8345481 - Attachment is obsolete: true
Attachment #8362663 - Flags: review+
Keywords: checkin-needed
OS: Windows 7 → All
Hardware: x86_64 → All
https://hg.mozilla.org/comm-central/rev/b4cb7f82793b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
Depends on: 1152989
You need to log in before you can comment on or make changes to this bug.