Open Bug 521861 Opened 15 years ago Updated 2 years ago

After creating news account through clicking news URL Account Wizard is broken (skips account type selection, bound to invalid news account)

Categories

(SeaMonkey :: MailNews: Account Configuration, defect)

defect
Not set
major

Tracking

(seamonkey2.39 affected)

ASSIGNED
Tracking Status
seamonkey2.39 --- affected

People

(Reporter: InvisibleSmiley, Assigned: frg, NeedInfo)

References

()

Details

(Keywords: relnote)

Attachments

(4 files, 3 obsolete files)

As Susanne Jaeger reported in the German MailNews newsgroup, after following a news:// URL you'll be unable to select the account type (mail, feeds, news) when opening the Account Wizard (either from the File menu or Account Settings). Instead you'll be taken directly to the Identity wizard page and from there to the Server Information wizard page. At least the Server Information page will show the account data matching the newsgroup subscribed to through the news:// URL. You will not be able to finish the wizard without changing the Newsgroup Server name. However, even finishing the wizard won't solve the problem.

Problem source: The account is declared invalid, i.e. the mail.server.serverX.valid pref is set to false (where X is the matching account).

Root cause: unknown, yet to be determined. Accounts created in the way described above are either missing a validation step or the corresponding function is broken.

Workarounds: Set mail.server.serverX.valid to true or delete invalid news account.

Backtrace:
1. AccountWizard.js: checkForInvalidAccounts() calls GetPageData() which sets gPageData
2. aw-accounttype.js: also calls GetPageData() which returns the created news server's data because gPageData is still pointing to that.
The faulty behaviour in the Account Wizard is similar to bug 521627.
Found this problem myself on SeaMonkey 2.0.1. It is somewhat nasty, because there is no clue about the reason why suddenly the account manager stops working right.

Is there something we (users) could do to help fixing this problem (providing a fixed set of steps, activating mail debugging logs, etc.)?

BTW, this bug affects SeaMonkey 2.0 branch, so maybe the version field should be changed?
(In reply to comment #3)
> Found this problem myself on SeaMonkey 2.0.1.


BTW, in my case clicking in a news:// link didn't CREATE a fake news account; instead, MODIFIED one of my newsgroup accounts, renaming it and changing the From: e-mail address associated to it.
Do you have the prefs.js files before and after this step? I.e. could you please post an anonymized diff?
(In reply to comment #5)
> Do you have the prefs.js files before and after this step? I.e. could you
> please post an anonymized diff?


I'll take a look (I have two synchronized machines through an external hard disk, and it could be that one of them still have the previous version).
Severity: normal → major
Today I took the time to find a regression window for this. Result:
Last known good: 2009-01-09
Broken since: 2009-01-10
Note that "good" here means that "New > Account" fails the first time after trying to subscribe to a newsgroup and canceling in the following dialog, but afterward the account wizard works again, whereas "broken" means that the account wizard stays broken even if you try "New > Account" again.

Probable causing changeset:
http://hg.mozilla.org/comm-central/rev/b78eb7642489 (bug 538414)

My test case was:
1. create a new profile
2. create a Feed account (or any other, but it's fast to do it this way)
3. subscribe to a newsgroup, but cancel the dialog
4. try to create a different account (check the first screen)
5. cancel the dialog and repeat step 4.
Blocks: 538414
Relnoted, see bug 656719 comment 25.
Keywords: relnote
This could be a security issue.

I tried to create a new email account, but could not because it was defaulting to a news account, so I gave up and switched to a standalone copy of Thunderbird to set up the mail. However, I now find that when replying to a Newsgroup item, it has picked up the private email address that I was trying to set up. This has exposed a personal email address that I do not use for newsgroups ( to avoid spam etc).

Since this problem has been known about for nearly three years, it should have been fixed by now.
I'm having this problem with adding news servers, but not through an URL, through the settings in SM 2.8.  I can't add but one news server now, because it's really badly broken now.  Sometimes what I add doesn't show up at all in SM.  Other times it will, but it has changed the original account that I had.  It has given me too many invalid responses to list.  Does no one care that they have rendered a core function of SM unusable?
I've just run into this issue in SM 2.22.1 on OS X 10.7.5. Nasty!
REPRODUCIBLE with  SeaMonkey 2.39a1 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0 from official download area)  Gecko/20100101  Firefox/42.0 (Classic Theme) on German WIN7 64bit

1. Launch SeaMonkey via Profile manager from command line
2. Create blank new profile "Newsgroupstest2" → Launch SeaMonkey →
    confirm default client for all applications → confirm changes requiring 
    WIN admin permissions
3. on <http://www.seamonkey-project.org/dev/get-involved> in left selection pane
   click "Communitiy and support" for <http://www.seamonkey-project.org/community>
4. Click Link Newsgroup: mozilla.support.seamonkey → confirm confirm 'would
   you like to subscribe' with [ok]
   » "Identity" dialog appears instead of account type selection dialog
5. [Cancel] will create account and subscribe "mozilla.support.seamonkey" with 
   invalid identity entries, but postings will be downloaded.

I changed "Version" to ease recognition of appearance

therube asked me to test this one in "Bug 1179680 - Lost ability to add new mail/news accounts". Effects here look similar, but my problem there did not appear in 2009
See Also: → 1179680
Version: Trunk → SeaMonkey 2.0 Branch
Version: SeaMonkey 2.0 Branch → Trunk
@Hb: May be you can explain your Version modification?
While I'm not :Hb, here's the reasoning:
* The 2.0 branch is long gone, it has been discontinued years ago. "SeaMonkey 2.0 Branch" is not including all 2.x versions as you might think, but only the 2.0.x ones.
* Version should show the latest affected version. Until this bug gets fixed, all versions are affected, i.e. Trunk is the correct value for the field for the time being. For the same reason, setting any of the status tracking flags is pointless, too, since you might then as well set those for all past and future versions, too.
See Also: → 1209859
Attached image Clipboard.jpg
I stumbled again over this bug when I tried to add a mail account today. 

I think the problem is the leftover valid setting for the news server. Usually you can see the pref for a short time when you create an account manually. It's the string pref in the lower half of the picture. 

When you cancel the subscription in the way Rainer did it in Comment 15 you end up with a mail.server.serverX.valid boolean pref set to false. This seems to confuse the heck out of the account wizard. The simple solution seems just to delete the pref. The not so simple solution would be to make sure that every mail.server.serverX has such a valid pref regardless of it's setting. This seems to work too.
Simple patch which just deletes the pref during startup. 

I do not think it's reviewable yet because I am not sure it should be in the migrate js. Was just convenient there because all servers are read here. Any comments where it should be or if this is even the right approach?

Works fine btw. Tested it with a private build.

FRG
(In reply to Frank-Rainer Grahl from comment #20)
> Created attachment 8707885 [details]
> Clipboard.jpg
> 
> I stumbled again over this bug when I tried to add a mail account today. 
> 
> I think the problem is the leftover valid setting for the news server.
> Usually you can see the pref for a short time when you create an account
> manually. It's the string pref in the lower half of the picture. 
> 
> When you cancel the subscription in the way Rainer did it in Comment 15 you
> end up with a mail.server.serverX.valid boolean pref set to false. This
> seems to confuse the heck out of the account wizard. The simple solution
> seems just to delete the pref. The not so simple solution would be to make
> sure that every mail.server.serverX has such a valid pref regardless of it's
> setting. This seems to work too.

Let's ask mkmelin

I think one possible place would be nsMsgAccountManager::LoadAccounts()

http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp?rev=1cb65d027ed4&mark=1188-1192#1188
Flags: needinfo?(mkmelin+mozilla)
Yeah that looks like a good spot to me.
Flags: needinfo?(mkmelin+mozilla)
(In reply to Magnus Melin from comment #23)
> Yeah that looks like a good spot to me.
Frg: do you want to try moving your patch to?:

http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp?rev=1cb65d027ed4&mark=1209-1210#1207
Flags: needinfo?(frgrahl)
Ratty,

sure can do. Not this week but with a bug from 2009 it can eventually wait a little longer :)

But not sure if it fixes all the instances. This one might have been a different one:

http://forums.mozillazine.org/viewtopic.php?f=40&t=2995769

Did anyone else who did run into the problem checked if it's caused by the .valid prefs?
Flags: needinfo?(frgrahl)
Attached patch 521861-Account_Creation.patch (obsolete) — Splinter Review
Attachment #8753546 - Flags: feedback?(philip.chee)
Attachment #8753546 - Flags: feedback?(mkmelin+mozilla)
Pressed Enter too fast. I moved the half baked patch to nsMsgAccountManager. 

It works but not sure if I am happy with it so a two questions. 

There seems to be no way to check which mail.server prefs do exists. You can get the accounts list via mail.accountmanager.accounts and the servers via mail.account.accountx.server but if there is a stale server you will not get it. I implemented it by just going over the first 99 server entries which seems to be a bit excessive but sure cleans stuff out. Should this be changed back to go via the accounts?

This is not javascript. Should the pref be checked that it exists before it is to be cleared.

If the valid pref is cleared might this cause problems with invalid server entries?

The root cause why the accounts manager is stuck will not be fixed by this. It's just a workaround so I think the bug should not be closed if the patch is to be accepted.
Comment on attachment 8753546 [details] [diff] [review]
521861-Account_Creation.patch

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

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +367,5 @@
>    }
>  }
>  
> +void
> +nsMsgAccountManager::CleanServerValidState()

nsresult is usually used even if we might not care too much about the return value atm.

@@ +376,5 @@
> +  if (NS_FAILED(rv))
> +    return;
> +
> +  nsCOMPtr<nsIPrefBranch> prefServer;
> +  if (prefService)

you already know you have a prefService since rv succeeded. Or if you're paranoid, check it where you check the rv code

@@ +385,5 @@
> +  }
> +
> +  nsAutoCString serverKey;
> +
> +  // Loop over first 99 mail.server.server* entried and delete .valid entries.

maybe mail.account.lastKey + some?
Attachment #8753546 - Flags: feedback?(mkmelin+mozilla) → feedback+
Comment on attachment 8753546 [details] [diff] [review]
521861-Account_Creation.patch

> +  nsresult rv;
> +  nsCOMPtr<nsIPrefService> prefService(do_GetService(NS_PREFSERVICE_CONTRACTID,
> +                                       &rv));
If you want to wrap at 80cols:

nsCOMPtr<nsIPrefService> prefService =
  do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);

> +  if (NS_FAILED(rv))
> +    return;
> +
> +  nsCOMPtr<nsIPrefBranch> prefServer;
nsCOMPtr<nsIPrefBranch> prefBranch;

> +  if (prefService)
You don't need to check for prefService if you have already done NS_FAILED

> +  nsAutoCString serverKey;
> +
> +  // Loop over first 99 mail.server.server* entried and delete .valid entries.
prefBranch->GetChildList(...)

+    serverKey.AppendInt(lastKey);
+    serverKey.AppendLiteral(".valid");
+    // Just delete the 'valid' pref. Don't check if it exists.
+    prefServer->ClearUserPref(serverKey.get());

StringEndsWith(.. ".valid")

Reference: http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgTagService.cpp?rev=ff603e79f92a#160
Attachment #8753546 - Flags: feedback?(philip.chee) → feedback-
Attached patch 521861-Account_Creation.patch (obsolete) — Splinter Review
Before we celebrate another aniversary :) Workaround patch redone and hopefully all comments addressed. Tested with SeaMonkey 2.53 and compiled with Thunderbird 65.
Attachment #8753546 - Attachment is obsolete: true
Attachment #9027332 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9027332 [details] [diff] [review]
521861-Account_Creation.patch

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

I guess this is ok. r=mkmelin

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +376,5 @@
> +}
> +
> +nsresult
> +nsMsgAccountManager::CleanServerValidState()
> +{

Please add documentation here on what it's solves (why it exists)
Attachment #9027332 - Flags: review?(mkmelin+mozilla) → review+
Attached patch 521861-Account_Creation.patch (obsolete) — Splinter Review
Comment added per review. r+ from mkmelin retained.

Jorg, I wonder if I shouldn't change it one more time and pref it off for compiling with SeaMonkey only. Seems to not add any visible overhead but if it is not also a TB problem might be better to leave it out there.
Assignee: nobody → frgrahl
Attachment #9027332 - Attachment is obsolete: true
Flags: needinfo?(jorgk)
Attachment #9029141 - Flags: review+
Comment on attachment 9029141 [details] [diff] [review]
521861-Account_Creation.patch

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

Well, I'd like this in TB if it fixes a problem in TB. Are there STR to reproduce this in TB? I don't see any server. ... . valid prefs. If it doesn't apply to TB, please use #ifdef MOZ_SUITE.

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +369,5 @@
>  }
>  
> +template<size_t N>
> +static bool
> +StartsWith(const std::string& haystack, const char (&needle)[N])

What is this? Why not use regular StringBeginsWith()?

@@ +378,5 @@
> +/*
> + * Sometimes you are unable to add a new mail account to SeaMonkey. The account
> + * manager will only allow adding a news account.
> + * One reason identified are stale mail.server.serverx.valid prefs. Until the
> + * account manager is fully fixed in Bug 521861 remove them during startup.

I don't understand this comment. This *is* bug 521861, so do you intend to land more fixes?

@@ +389,5 @@
> +    do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIPrefBranch> prefBranch;
> +

Lose empty line.

@@ +405,5 @@
> +  for (uint32_t i = count; i--;) {
> +    // Check if it is a server*.valid key and if yes delete it.
> +    nsDependentCString prefName(prefList[i]);
> +    if (StringEndsWith(prefName, NS_LITERAL_CSTRING(".valid")) &&
> +        StartsWith(prefName.get(), "server")) {

Why is this so inconsistent?
One uses .get(), the other one NS_LITERAL_CSTRING(). Please fix, ah, I see, home-grown function.

@@ +1194,5 @@
>    // It is ok to return null accounts like when we create new profile.
>    m_accountsLoaded = true;
>    m_haveShutdown = false;
>  
> +  // Clean up servers. Necessary for later account creation see bug 511861.

We usually don't mention bug numbers in code. It's already mentioned above in the function (although I don't understand that comment).
Attachment #9029141 - Flags: review-
Aceman, is this problem reproducible in TB?
Flags: needinfo?(jorgk)
This hopefully fixes all the remaining issues. Thanks for pointing out StringBeginsWith(). I actually looked and didn't find it before. Makes for a much cleaner patch.

I didn't find a similar bug report for Thunderbird. Seems to be the account wizard does a better job than the SM account manager so I enclosed it in ifdef suite. Needed in esr60 and also in our next release still based on 52 (sigh).
Attachment #9029141 - Attachment is obsolete: true
Attachment #9032927 - Flags: review?(jorgk)
Attachment #9032927 - Flags: review?(iann_bugzilla)
Attachment #9032927 - Flags: approval-comm-esr60?
Attachment #9032927 - Flags: approval-comm-esr52?
Comment on attachment 9032927 [details] [diff] [review]
521861-Account_Creation.patch

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

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +390,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  uint32_t count;
> +  char **prefList;
> +  rv = prefBranch->GetChildList("", &count, &prefList);

Does prefList need free'ing?

@@ +394,5 @@
> +  rv = prefBranch->GetChildList("", &count, &prefList);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Traverse the list, and look for .valid prefs.
> +  for (uint32_t i = count; i--;) {

This looks really ugly. And it's wrong as well. The look will stop at i==0, so you don't check that pref. Unlikely it will need processing, but for correctness please use:
for (i = count - 1, i >=0; i--).

Oh, you also cause a crash by starting at count, no?

Unless I'm mistaken, that would be an r- :-(

@@ +397,5 @@
> +  // Traverse the list, and look for .valid prefs.
> +  for (uint32_t i = count; i--;) {
> +    // Check if it is a server*.valid key and if yes delete it.
> +    nsAutoCString prefName;
> +    prefName.Assign(prefList[i]);

Won't that crash for i==count?

@@ +1191,5 @@
>    m_haveShutdown = false;
>  
> +#ifdef MOZ_SUITE
> +  // Clean up servers so that account creation always works.
> +  rv = CleanServerValidState();

Why are assigning to rv if you ignore it?
Attachment #9032927 - Flags: review?(jorgk)
Comment on attachment 9032927 [details] [diff] [review]
521861-Account_Creation.patch

>+ * Sometimes you are unable to add a new mail account to SeaMonkey. The
>+ * account manager will only allow adding a news account.
>+ * One reason identified are stale mail.server.serverx.valid prefs. Until the
Nit: is rather than are
>+ * account manager is fully fixed in Bug 521861 remove these prefs during
>+ * startup.

>+  uint32_t count;
Nit: you could define i here too

>+  // Traverse the list, and look for .valid prefs.
>+  for (uint32_t i = count; i--;) {
Any reason this cannot be done as an incremental loop?
for (i = 0; i < count; ++i)

>+    // Check if it is a server*.valid key and if yes delete it.
>+    nsAutoCString prefName;
Move this outside of the for loop

>+    prefName.Assign(prefList[i]);
>+    if (StringEndsWith(prefName, NS_LITERAL_CSTRING(".valid")) &&
>+        StringBeginsWith(prefName, NS_LITERAL_CSTRING("server"))) {
>+      prefBranch->ClearUserPref(prefName.get());
>+    }
>+  }
>+
NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(count, prefList);

>+  return NS_OK;
>+}

> 
>+#ifdef MOZ_SUITE
>+  // Clean up servers so that account creation always works.
>+  rv = CleanServerValidState();
Do we need here:
NS_ENSURE_SUCCESS(rv,rv);

f+ for the moment
Attachment #9032927 - Flags: review?(iann_bugzilla)
Attachment #9032927 - Flags: feedback+
Attachment #9032927 - Flags: approval-comm-esr60?
Attachment #9032927 - Flags: approval-comm-esr52?
Status: NEW → ASSIGNED
(In reply to Rainer Bielefeld from comment #15)
> <http://www.seamonkey-project.org/community>

The link is:
<news://news.mozilla.org/mozilla.support.seamonkey>

> 4. Click Link Newsgroup: mozilla.support.seamonkey → confirm confirm 'would
>    you like to subscribe' with [ok]

(In reply to Jorg K (GMT+1) (urgent reviews and bustage fix only, Dec 22nd to Jan 1st) from comment #35)
> Aceman, is this problem reproducible in TB?

Only till step 4. Noting else happens after that. (see also: Bug 745856).
In the folderpane no new account is shown, but: In "Account Settings" the new account is there. And after a restart of TB it is shown in the folderpane.
(In reply to Alfred Peters from comment #39)
> Only till step 4. Noting else happens after that. (see also: Bug 745856).
> In the folderpane no new account is shown, but: In "Account Settings" the
> new account is there. And after a restart of TB it is shown in the
> folderpane.
So we should take the patch in general without the #ifdef MOZ_SUITE?
(In reply to Jorg K (GMT+1) (urgent reviews and bustage fix only, Dec 22nd to Jan 1st) from comment #40)
>(In reply to Alfred Peters from comment #39)
>> Only till step 4. Noting else happens after that. (see also: Bug 745856).

> So we should take the patch in general without the #ifdef MOZ_SUITE?

I have no idea what the pref is good for in TB.

If I cancel step 4, I get also the pref with value false.
The pref has no obvious consequences in TB. She seams just to stay forever. ;-)

So yes, it makes sense to delete the pref also in TB.
Just let me know the verdict and I will remove the ifdefs. When I find some time I will look over the review comments and put up a new patch. Hopefully the last one needed :)
Sigh, I meant to NI Aceman in comment #35 but didn't :-( - Since he's not CC'ed he won't answer.

Aceman, given comment #39 and comment #41, should we take this in TB?

Which preference are we talking about exactly? Something mail.server.serverNN.valid? I wanted to try it, but I don't even understand the STR (quoting):

===
My test case was:
1. create a new profile
2. create a Feed account (or any other, but it's fast to do it this way)
3. subscribe to a newsgroup, but cancel the dialog
4. try to create a different account (check the first screen)
5. cancel the dialog and repeat step 4.
===

Well, after you added an empty Feed account, you can subscribe to *feeds*, not newsgroups. So is there a step missing where you also create an "Other Account", which is a news account and then try to add the news server, and then cancel? Which panel do cancel exactly? Or do you add the news account and cancel the subscribe panel?

All I tried, I haven't seen any  mail.server.serverNN.valid preference, there are only mail.identity.idNN.valid.
(In reply to Jorg K (GMT+1) (urgent reviews and bustage fix only, Dec 22nd to Jan 1st) from comment #43)
 
> Which preference are we talking about exactly? Something
> mail.server.serverNN.valid?

correct.

>  I wanted to try it, but I don't even understand
> the STR (quoting):
> 
> ===
> My test case was:
> 1. create a new profile
> 2. create a Feed account (or any other, but it's fast to do it this way)

You need to click to:
<news://news.mozilla.org/mozilla.support.seamonkey>
I wrote me a mail with that link and subscribed to that account first.

> 3. subscribe to a newsgroup, but cancel the dialog

No. The account will be created automatically after you click that link.
Then a Dialog asks you, if you want to subscribe to "mozilla.support.seamonkey".
That is "step 4"

If you click OK, you'll get the group after you restart TB.
If you click CANCEL, you'll get only the empty new account with the "mail.server.serverNN.valid" pref.
- I wrote me a mail with that link and subscribed to that account first.

+ I wrote me a mail with that link and subscribed to the mail account (containing these mail) first.
Thanks Alfred, I could reproduce it now easily. I exported that bugmail to the new profile and after clicking and cancelling I get the "mail.server.serverNN.valid" pref. Thanks for your help and all your work in 2018, I hope the excellent cooperation will continue in 2019.

So now the question goes back to Aceman. Do we care about this preference. Again the clarified STR:
1. create a new profile
2. create a Feed account (or any other, but it's fast to do it this way)
3. Create draft or import e-mail with <news://news.mozilla.org/mozilla.support.seamonkey> in the body.
4. Click that link, on the dialogue that asks you to subscribe, click cancel.

mail.server.server3.valid pref is set to false. 1 and 2 are the feed and local folder accounts. The news account is in fact created, even if you cancel, and that pref hangs around without negative side effects as far as I can see.
 The news account is in fact created, even if you
> cancel, and that pref hangs around without negative side effects as far as I
> can see.

As I mentioned in Comment #11 ( some 7 years ago), one side effect that I found at the time was that one of my real email addresses  was picked up in replies to the Newsgroup postings. I have a specific address (fake) used for Newsgroup postings to reduce spam. It appeared that the accounts database had somehow corrupted. I never got to the bottom of why this happened, and I do not know whether subsequent changes would have altered this undesirable side effect. You may want to try creating an email and a NG posting to see if the appropriate addresses are shown.
Flags: needinfo?(acelists)

Yes I think the news account creation is the same in TB and SM so the patch is needed in TB too, as was confirmed by Alfred and Jorg.
But it is an ugly cludge and needs to be marked as such in the code (it seems it is already) so that we remove it later.

We then need to find the real reason why this happens. If the server is actually usable (after removing the .valid pref), then it shouldn't get the .valid=false flag from the wizard. Or if it is not usable (finished seting up), then it should be finished (a news server without subscribed newsgroups) should be fine. Or it should be deleted as a whole upon clicking Cancel.

I think there was a report of an empty identity being attached to the account and then also comment 47, which may actually have the same cause.
So we do not really know if the created server is fully usable, we just hack away the .valid pref.

There are nice STR in comment 45. Do we know which code runs in step 4 and creates the news account automatically? Then, is the subscribe dialog the standalone one, you can open any time to subscribe to more newsgroups? Or is is part of the "other account" creation wizard?

Flags: needinfo?(acelists)

I will see that I update the patch. Sorry for the inactivity here. Mostly did firefighting and trying to get the next release out.

And aceman is right. It is a cludge. Should be reproducible if you just cancel create an account at a certain stage.

Just some ideas after the tracking this issue (with current SeaMonkey code).
 

When a new account is created through the clicking news URL, nsNntpService::CreateNewsAccount is called. Since it does not know what exactly should be set for "FullName" and "Email" in this case, it leaves these parameters undefined, and set the Valid flag to false:

// the identity isn't filled in, so it is not valid.
rv = (*aServer)->SetValid(false);
if (NS_FAILED(rv)) return rv;

Probably it follows some "ancient" logic, when in a case of impossibility to completely fill in all the required data "right now", a certain flag is set in order to complete the creation of this account later.

So on its next run, AccountWizard sees that some account has "false" valid flag and immediately invites the user to complete (or fix) this account. To do this, onAccountWizardLoad() calls checkForInvalidAccounts().

The follow-up process is currently broken.
 

First, user follows "identitypage", "newsserver", "accnamepage" steps. Both "newsserver" and "accnamepage" show the same current value of the correspond news server name, but both do not allow user to go further because of the disabled Next button. The button is disabled, because the code wrongly assumes that such a server and such an account already exist.

The problem is AccountExists() (for "newsserevr") and accountNameExists() (for "accnamepage") ignore the "valid" flag when searching, and interprete the matched data as referring to another valid server.

The following changes fix this:

--- comm/mailnews/base/prefs/content/AccountWizard.js.orig      2021-07-19 01:02:40.194695831 +0300
+++ comm/mailnews/base/prefs/content/AccountWizard.js   2021-07-19 01:03:25.146412465 +0300
@@ -708,7 +708,8 @@ function setDefaultCopiesAndFoldersPrefs

 function AccountExists(userName, hostName, serverType)
 {
-  return MailServices.accounts.findRealServer(userName, hostName, serverType, 0);
+  let server = MailServices.accounts.findRealServer(userName, hostName, serverType, 0);
+  return server && server.valid != false;
 }

 function getFirstInvalidAccount()

and

--- comm/mailnews/base/prefs/content/amUtils.js.orig    2021-07-19 01:05:23.732664849 +0300
+++ comm/mailnews/base/prefs/content/amUtils.js 2021-07-19 01:05:47.846512850 +0300
@@ -198,6 +198,7 @@ function accountNameExists(aAccountName,
                                   Ci.nsIMsgAccount))
   {
     if (account.key != aAccountKey && account.incomingServer &&
+        account.incomingServer.valid != false &&
         aAccountName == account.incomingServer.prettyName) {
       return true;
     }

(It is unlikely that this introduces the ability to create a "duplicate server" somewhere, since it requires the user to run AccountWizard (to manually fill such duplicate data), but then it will first force the user to complete the non-valid account anyway.)
 

Second, when the account data is completed such a way, that account becomes valid, but the valid flag remains unchanged (ie. "false").
To fix this, the following change is needed for FinishAccount() function:

--- comm/mailnews/base/prefs/content/AccountWizard.js.orig      2021-07-19 01:24:03.807566601 +0300
+++ comm/mailnews/base/prefs/content/AccountWizard.js   2021-07-19 01:24:28.850407172 +0300
@@ -178,6 +178,8 @@ function FinishAccount()
     // we might be simply finishing another account
     if (!gCurrentAccount)
       gCurrentAccount = createAccount(accountData);
+    else
+      gCurrentAccount.incomingServer.valid = true;

     // transfer all attributes from the accountdata
     finishAccount(gCurrentAccount, accountData);

(Note, createAccount() normally manipulates the valid flag and finally set it to true as well).

 
Applying these fixes, we restore that "ancient" account completion algorithm. Such news accounts will be created with valid="false" as for now, but the next time the AccountWizard is run, the user will be able to complete them propely, and the flag will disappear.

The same is true for accounts that have ever had this flag set in the past and remains set until now (they will be fixed this way too).

The changes above restore the account completion algorithm.
But in general, the whole path of processing such news links does not look as the best way.

It looks strange a bit to have such a delay between the account creation and its follow-up fixing sometime later in an arbitrary time. It seems that initially such a fixing performed without a delay, but just at the next start of the messenger.

When the messenger starts, say standard SeaMonkey's 3 pane window, OnLoadMessenger() runs verifyAccounts(), which has a code fragment regarding invalid accounts:

var accountCount = accounts.length;
var invalidAccounts = getInvalidAccounts(accounts);
if (invalidAccounts.length > 0 && invalidAccounts.length == accountCount) {
    prefillAccount = invalidAccounts[0];
}

so when ALL the account are invalid, the AccountWizard will be run. (And this happens after the clicking news URL when there are no any accounts yet, ie. new profile etc.).

But a bit below there is a comment:

// prefillAccount is non-null if there is at least one invalid account.

that follows a different logic, namely:

--- accountUtils.js.orig	2021-07-19 02:54:35.037675140 +0300
+++ accountUtils.js	2021-07-19 02:54:40.176742599 +0300
@@ -96,7 +96,7 @@ function verifyAccounts(wizardCallback,
         // as long as we have some accounts, we're fine.
         var accountCount = accounts.length;
         var invalidAccounts = getInvalidAccounts(accounts);
-        if (invalidAccounts.length > 0 && invalidAccounts.length == accountCount) {
+        if (invalidAccounts.length > 0) {
             prefillAccount = invalidAccounts[0];
         }
 

Perhaps sometime in the past, the code was created exactly like this, and the AccountWizard was run whenever there was "at least one invalid account" at the messenger startup.

With such a change, AccountWizard will invite user to fix the invalid account at any messenger start, including the start performed after the clicking on the news URL. This way there will be no any delay, since the user fixes things immediately after the clicking news URL.

OTOH, the logic of the news links clicking implies some easy and fast operations, and to cause the user to completely fill the account data here looks a bit out of place. Besides this, users are already accustomed that this is not required, and things work OK without this (until they try to create a new account).

So the idea to run AccountWizard at any messenger startup when non-valid accounts present is questionable.

A better idea is to not set the false valid flag at all in nsNntpService::CreateNewsAccount().

It will lead to the same situation as after the recommended work-around is done (ie. manually drop "mail.server.serverX.valid" prefs). Things work, but the FullName and Email parameters remains unset. The user will only notice this when will try to "send" something to the newsgroup. But then an email from the first valid account will be chosen by default for the From address.

Some users complain that such address substitution (when an address from another account is substituted instead of an empty one) is inconvenient and fraught with the loss of private data. The user can in a hurry send a message with the private From address, but he would prefer to hide it from spammers in such a public area as newsgroups.

So a slightly better way is to fill some fake data, following the way how it was done for AccountWizard.js:onCancel() -- ie. set FullName just to the empty string, and set Email to the same fake value of "user@domain.invalid".

The change is:

--- comm/mailnews/news/src/nsNntpService.cpp.orig       2020-02-18 02:36:20.000000000 +0300
+++ comm/mailnews/news/src/nsNntpService.cpp    2021-07-19 03:48:05.804759761 +0300
@@ -948,9 +948,12 @@ nsNntpService::CreateNewsAccount(const c
   rv = identity->SetComposeHtml(false);
   NS_ENSURE_SUCCESS(rv,rv);

-  // the identity isn't filled in, so it is not valid.
-  rv = (*aServer)->SetValid(false);
-  if (NS_FAILED(rv)) return rv;
+  // Use fake data for now (user can change it in AccountWizard whenever want)
+  rv = identity->SetEmail(NS_LITERAL_CSTRING("user@domain.invalid"));
+  NS_ENSURE_SUCCESS(rv,rv);
+
+  rv = identity->SetFullName(NS_LITERAL_STRING(""));
+  NS_ENSURE_SUCCESS(rv,rv);

   // hook them together
   rv = account->SetIncomingServer(*aServer);

With this change the false valid flag is not set anymore. (And solution from comment #51 is not even needed if there are no such half-broken accounts in the past).

When the user post something ti the newsgroup, the From field is "user@domain.invalid". Then he/she either send it as is (for rare posting etc.), or choose an email address from another account, or run AccountWizard to fill the email properly in a case of frequent posts.

Personally, I lean towards both of the comment #51 and comment #53 solutions together. I will try it at the downstream level (Fedora Linux).

Certainly it could be fine if anybody more skilled in these areas prepare all the ideas and provide some final patches etc. (for both SeaMonkey and Thunderbird).

Flags: needinfo?(iann_bugzilla)

The change in behaviour described in comment 52 - the wizard only showing up to complete invalid accounts when there are no valid accounts - appears to have been introduced in bug 476135.

Yep, it was bug #476135, and it was somewhat of a kludge. Seeing that something was going wrong, they just postponed its effect for later.

But this means that the initial algorithm ran the AccountWizard whenever any account was found to be not valid. Then user fix (or remove) it. And it seems to be the reason to consider comment 52 to implement (ie. the same as to revert bug #476135).
 

When user is invited to fix non-valid account that way, he sees an "identity" page, and it is not completely obvious for him that it is about an uncompleted news account. He can guess it if the AcountWizard appears right after the clicking on the news URL, but he will be confused, if it appears later (either when add a new account, as for now, or even when start messenger, as with comment 52).

Fortunately, when user clicks Cancel on such "identity" page, onCancel() fills the fake data (comment 53 just took this idea from here exactly) and then run FinishAccounts(), which now works properly since fixed by comment 51. The result will be the same as proposed in comment 53.

Given that, perhaps comment 52 idea is no longer so questionable...

Fedora Linux patch for now, comment 51 plus comment 53 .

See Also: → 1760393
You need to log in before you can comment on or make changes to this bug.