Closed Bug 1050840 Opened 5 years ago Closed 5 years ago

"Maximum number of server connections to cache" advanced account setting defaults to 1 for new accounts

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set

Tracking

(thunderbird36 fixed)

RESOLVED FIXED
Thunderbird 36.0
Tracking Status
thunderbird36 --- fixed

People

(Reporter: wsmwk, Assigned: aceman)

References

Details

(Keywords: perf, regression, Whiteboard: [regression:TB24])

Attachments

(1 file, 2 obsolete files)

I tested both trunk and TB31.0. "Maximum number of server connections to cache" advanced account setting defaults to 1 for new accounts. Perf won't be helped.

I don't have time to dumpster dive for the regression range
But prefs.js has a value of 5, which is correct.  WTH
So the pref value is not propagated to the UI?
IF you then click OK in the advanced settings, will the value of 1 be stored ?
Assignee: nobody → acelists
Product: Thunderbird → MailNews Core
This seems to be an issue the first time a new profile runs.

Connecting to this instance (Thunderbird 31.0) using remote debugging, there a bunch of errors visible:

Error in setting AccountCentral Items: TypeError: server is null
 msgAccountCentral.js:206
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgAccountManager.defaultAccount]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/mailInstrumentation.js :: minst_postStateObject :: line 101"  data: no] errUtils.js:96
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgAccountManager.defaultAccount]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/mailInstrumentation.js :: minst_postStateObject :: line 101"  data: no] errUtils.js:35
The config file XML does not contain an email account configuration. errUtils.js:96
The config file XML does not contain an email account configuration. errUtils.js:35
 errUtils.js:96
 errUtils.js:35
this._persistOpenMap[mode] is undefined folderPane.js:1295
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgAccountManager.defaultAccount]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/mailInstrumentation.js :: minst_postStateObject :: line 101"  data: no] errUtils.js:96
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgAccountManager.defaultAccount]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/mailInstrumentation.js :: minst_postStateObject :: line 101"  data: no] errUtils.js:35
_smtpServerAdded is not defined mailInstrumentation.js:68
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFolder.getStringProperty]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://messenger/content/folderPane.js :: getSmartFolderName :: line 2654"  data: no] folderPane.js:2656

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFolder.getStringProperty]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://messenger/content/folderPane.js :: getSmartFolderName :: line 2654"  data: no] folderPane.js:2656

Likely one of these errors or states causes a script to fail so the scripts for initializing the advanced server settings dialog doesn't run.

By the way, I don't see these errors in the Error Console, is there a bug for this?
This seems to be a timing/caching issue. If you wait some time like 10 minutes or more after setting up the account, the setting will be correct. Related to this, the setting in the server settings pane for the Trash folder shows "Choose Folder" if this issue occurs and the Trash folder if the issue is gone. (The folder pane shows only Inbox when it occurs and Trash and Drafts later.)
(In reply to Archaeopteryx [:aryx] from comment #3)
> By the way, I don't see these errors in the Error Console, is there a bug
> for this?

I think I have seen some of those in the error console and tried to fix some in bug 1015775.
Could you try it in TB32+?
Also tested with Daily 34.0a1 20140723 and filed:

Bug 1052464 for the mailInstrumentation issue, patch attached
Bug 1052396 for the server is null issue
Bug 1052062 for error console not showing everything.

Bug 615272 is about getSmartFolderName

Back on topic:
If I set a breakpoint on the first statement in am-server.js' OnInit function (had to open the preferences once before to see that in the remote debugger), it doesn't break there, so seems like a script breaks earlier.
Keywords: perf
Severity: normal → minor
The timing analysis seems correct. When I delete the max_cached_connections pref from prefs.js the value is considered to be 0. In nsImapIncomingServer::GetImapConnection if 0 is found, it gets initialized to 5. However, if you open the account manager, advanced settings before that, the input field of the connections number imposes a minimum of 1. If you OK that dialog, that value gets stored now and is not subject of automatic initialization to 5. Seems to be regressed by my bug 810680. Before the bug, the UI probably showed a value of 0, which was later automatically initialized to 5. I'll see if I can somehow initialize it immediately so that 0 is never seen in practice.
Status: NEW → ASSIGNED
Whiteboard: [regession:TB??] → [regression:TB24]
Attached patch patch (obsolete) — Splinter Review
This seems to work for me. I move the setting of the default value into the GetMaximumConnectionsNumber as that is also called from the account manager. Other than that GetMaximumConnectionsNumber seems to only be called from Get*Connection so moving the code block from the latter to the former should not add any overhead.

I tested this by removing mail.server.server*.max_cached_connections from prefs.js (for an imap and news account). Testing with a new account would be useful too.

Josiah, the patch contains some tweaks for the ugliness of the labels around the "maximum number of server connections" field so please look at that part.
Attachment #8491827 - Flags: ui-review?(josiah)
Attachment #8491827 - Flags: feedback?(archaeopteryx)
Severity: minor → normal
Keywords: perf
OS: Windows 7 → All
Hardware: x86 → All
Comment on attachment 8491827 [details] [diff] [review]
patch

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

Creating a profile and adding an IMAP account in a build with the patch applied shows "5" as connection number in the settings for the account. Migrating a profile from 31 to that nightly build also shows 5 (IMAP) or 2 (news) for the connections. Removing the prefs storing these values from prefs.js, launching the profile, checking for messages and then searching about:config for them lists the values as restored with the correct values.
Attachment #8491827 - Flags: feedback?(archaeopteryx) → feedback+
(In reply to Archaeopteryx [:aryx] from comment #9)
> Creating a profile and adding an IMAP account in a build with the patch
> applied shows "5" as connection number in the settings for the account.
> Migrating a profile from 31 to that nightly build also shows 5 (IMAP) or 2
> (news) for the connections.

Great, thanks for testing. I just do no understand this part. What values were in the profile before migration? Why would migrating the profile change the values?
They didn't change, I simply tested existing profiles (and migration) by using one from Tb 31.
Attachment #8491827 - Flags: review?(neil)
Attachment #8491827 - Flags: review?(Pidgeot18)
Comment on attachment 8491827 [details] [diff] [review]
patch

Nits:

>diff --git a/mailnews/base/prefs/content/am-server-advanced.xul b/mailnews/base/prefs/content/am-server-advanced.xul
Irrelevant changes.

>+  if (NS_SUCCEEDED(rv) && (*maxConnections > 0))
Extra parentheses.

>+    return rv;
>+
>+  if (*maxConnections == 0)
>+  {
>+    *maxConnections = 5;
>+  } else
>+  {
>+    *maxConnections = 1;
>+  }
Very inconsistent bracing.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8491827 - Attachment is obsolete: true
Attachment #8491827 - Flags: ui-review?(josiah)
Attachment #8491827 - Flags: review?(neil)
Attachment #8491827 - Flags: review?(Pidgeot18)
Attachment #8492471 - Flags: review?(neil)
Attachment #8492471 - Flags: review?(Pidgeot18)
Comment on attachment 8492471 [details] [diff] [review]
patch v2

>+nsImapIncomingServer::GetMaximumConnectionsNumber(int32_t *maxConnections)
Nit: aMaxConnections

>+  nsresult rv = GetIntValue("max_cached_connections", maxConnections);
>+  // Get our maximum connection count. We need at least 1. If the value is 0,
>+  // we use the default of 5. If it's negative, we treat that as 1.
>+  if (NS_SUCCEEDED(rv) && *maxConnections > 0)
>+    return rv;
>+
>+  if (*maxConnections == 0)
You don't want to do this if NS_FAILED(rv).

>+  }
>+  else
>+  {
[Boy, these braces are ugly. Any chance you could use a ?: instead?]

>-  int32_t maxConnections = 5; // default to be five
>-  rv = GetMaximumConnectionsNumber(&maxConnections);
>-  if (NS_FAILED(rv) || maxConnections == 0)
>-  {
>-    maxConnections = 5;
>-    rv = SetMaximumConnectionsNumber(maxConnections);
>-  }
>-  else if (maxConnections < 1)
>-  { // forced to use at least 1
>-    maxConnections = 1;
>-    rv = SetMaximumConnectionsNumber(maxConnections);
>-  }
>+  int32_t maxConnections;
>+  (void)GetMaximumConnectionsNumber(&maxConnections);
Not sure whether it's safe to ignore failure here, though if you restore the default it should work, I think.

>+  nsresult rv;
Please move this to where it is used.
Attachment #8492471 - Flags: review?(neil) → review-
Comment on attachment 8492471 [details] [diff] [review]
patch v2

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

This code isn't that hard to add tests for. :-)

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +257,5 @@
> +  // we use the default of 5. If it's negative, we treat that as 1.
> +  if (NS_SUCCEEDED(rv) && *maxConnections > 0)
> +    return rv;
> +
> +  if (*maxConnections == 0)

*maxConnections <= 0

::: mailnews/news/src/nsNntpIncomingServer.cpp
@@ +445,5 @@
>  
>    return NS_OK;
>  }
>  
> +/* By default, allow the user to open at most this many connections to one news host */

Might as well make this be a C++-style comment.

Actually, no need to make a define for a single use. Just move the C++'d comment to the use.

@@ +457,5 @@
> +  // we use the default. If it's negative, we treat that as 1.
> +  if (NS_SUCCEEDED(rv) && *maxConnections > 0)
> +    return rv;
> +
> +  if (*maxConnections == 0)

*maxConnections <= 0
Attachment #8492471 - Flags: review?(Pidgeot18) → review-
(In reply to neil@parkwaycc.co.uk from comment #14)
> >-  int32_t maxConnections = 5; // default to be five
> >-  rv = GetMaximumConnectionsNumber(&maxConnections);
> >-  if (NS_FAILED(rv) || maxConnections == 0)
> >-  {
> >-    maxConnections = 5;
> >-    rv = SetMaximumConnectionsNumber(maxConnections);
> >-  }
> >-  else if (maxConnections < 1)
> >-  { // forced to use at least 1
> >-    maxConnections = 1;
> >-    rv = SetMaximumConnectionsNumber(maxConnections);
> >-  }
> >+  int32_t maxConnections;
> >+  (void)GetMaximumConnectionsNumber(&maxConnections);
> Not sure whether it's safe to ignore failure here, though if you restore the
> default it should work, I think.

The idea is that GetMaximumConnectionsNumber can't fail anymore. If it fails internally, it returns the default value.
(In reply to Joshua Cranmer [:jcranmer] from comment #15)
> Comment on attachment 8492471 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8492471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This code isn't that hard to add tests for. :-)
> 
> @@ +457,5 @@
> > +  // we use the default. If it's negative, we treat that as 1.
> > +  if (NS_SUCCEEDED(rv) && *maxConnections > 0)
> > +    return rv;
> > +
> > +  if (*maxConnections == 0)
> 
> *maxConnections <= 0

This would change the logic so that we never settle on 1 if the value is negative. Do you want to force the default value of 5 resp. 2 in all cases?
(In reply to Joshua Cranmer [:jcranmer] from comment #15)
> > +/* By default, allow the user to open at most this many connections to one news host */
> 
> Might as well make this be a C++-style comment.
> 
> Actually, no need to make a define for a single use. Just move the C++'d
> comment to the use.
I just moved this comment and constant. But yes, the number of uses is now reduced to 1 so it is probably unneeded.
Attached patch patch v3Splinter Review
Now with test :)
Attachment #8492471 - Attachment is obsolete: true
Attachment #8493158 - Flags: review?(neil)
Attachment #8493158 - Flags: review?(Pidgeot18)
Attachment #8493158 - Flags: review?(neil) → review+
Attachment #8493158 - Flags: review?(Pidgeot18) → review+
Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f63ed161fb1c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
Flags: in-testsuite+
The default value of 5 causes many problems in particular with respect to IDLE and CONDSTORE. A known workaround is to set it to 1.
 
Why is "Maximum number of server connections to cache" is set to 5 by default?
No idea, but this bug did not change that. Please open a new bug for the discussion of changing the default.
The goal of posting here is that somebody in the CC list is likely to know the answer.
Bug open at Bug 1237656
You need to log in before you can comment on or make changes to this bug.