364987 - creating second RSS ("News & Blogs") account fails

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

[ not sure whether this belongs in the "RSS" or "Account Manager" component ]

Creating a second RSS ("News & Blogs") account fails -- it behaves as though it worked, but no account is created.

Steps to reproduce:
 1. start thunderbird on a clean profile
 2. when the account manager comes up, create a "News & Blogs" account with all the default settings
 3. Go To File -> New -> Account, and create another such account, accepting all the default settings except for the name

Actual results:  After step (3), the order of the account created in (2) is switched with Local Folders, but no new account is created.   No directory for the new account is visible in the Mail subdirectory of the profile directory either.

Expected results:  A second account is created (both in the folder pane and in the Mail subdirectory of the profile directory).

This is a regression on the trunk relative to the 1.8 and 1.8.0 branches, where it works correctly.  I observed the problem in the Linux 2006-12-25-03-trunk nightly and in a Windows debug build from the evening of 2006-12-24.

Comment 2

[David Ascher (:davida)]

Is this bug still valid in recent builds?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Confirming on a debug Trunk Thunderbird build version 3.0a1pre (2008040301).

//snip of earlier jibberish
--DOMWINDOW == 13 (0x153b6a7c) [serial = 12] [Outer = 0x153b3960]
--DOMWINDOW == 12 (0x153b398c) [serial = 11] [Outer = 0x0]
// here I pressed "Done" to create the second similar account at step 3:
pageData.server = 
pageData.server.smtphostname.value = undefined
am.createIncomingServer(nobody,News & Blogs,rss)
am.createAccount()
Error: No Generic attribute found for: object
srcServer.ServerType-rss = undefined
WARNING: getting z level of unregistered window: file /Users/skywalker/Desktop/Mozilla/cvs/mozilla/xpfe/appshell/src/nsWindowMediator.cpp, line 635
WARNING: getting z level of unregistered window: file /Users/skywalker/Desktop/Mozilla/cvs/mozilla/xpfe/appshell/src/nsWindowMediator.cpp, line 635
--WEBSHELL 0x65f650 == 5

==========

Perhaps the above debug output may help.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

And oh, fresh profile on Mac OS X 10.5 Leopard.

Comment 5

[Magnus Melin [:mkmelin]]

Yep, linux too. Tried creating a third account, and it does not show up in the tree.

Comment 6

[Phil Ringnalda (:philor)]

Disconcertingly, you haven't really wasted your typing for the second account name, since if you create RSS1, fail to create RSS2, remove RSS1, and create RSS3, then mostly (but not always) the account winds up named RSS2 rather than RSS3.

Comment 7

[WADA:World Anti-bad-Duping Agency]

Following is prefs.js entries for RSS after two RSS accounts are defined by Tb trunk 2008/4/02 build(MS Win-XP SP2).
It seems that it's a result of directory creation failure for second RSS(already exists, then no directory-rel entry for second RSS) because of same hostname.

> user_pref("mail.server.server7.check_time", 100);
> user_pref("mail.server.server7.directory", "C:\\Documents and Settings\\wada\\Application Data\\Thunderbird\\Profiles\\qehhzsoa.Trunk-Nightly-Test\\Mail\\News & Blogs");
> user_pref("mail.server.server7.directory-rel", "[ProfD]Mail/News & Blogs");
> user_pref("mail.server.server7.hostname", "News & Blogs");
> user_pref("mail.server.server7.name", "News & Blogs");
> user_pref("mail.server.server7.type", "rss");
> user_pref("mail.server.server7.userName", "nobody");
> user_pref("mail.server.server8.check_time", 100);
> user_pref("mail.server.server8.hostname", "News & Blogs");
> user_pref("mail.server.server8.name", "News & Blogs - 002");
> user_pref("mail.server.server8.type", "rss");
> user_pref("mail.server.server8.userName", "nobody");

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

3.0a1 is intended to be largely time-driven.  Since this seems like a pretty unusual thing to do, it definitely doesn't block 3.0a1.  Nominating for later consideration to block 3.0, but only because this is a regression.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> 3.0a1 is intended to be largely time-driven.  Since this seems like a pretty
> unusual thing to do, it definitely doesn't block 3.0a1.  Nominating for later
> consideration to block 3.0, but only because this is a regression.
> 
I think we need to fix this in some way. I created a second RSS account the other day (MarcoZ had asked about it on IRC). It apparently had no effect, until I restarted at which time the new RSS account appeared and hid the old one - my initial thought was where have all my feeds gone? I quickly realised that the second account had overridden the first, and after deleting it and restarting, got my feeds back again.

Whilst most users probably won't hit this, it is going to be pretty confusing to them if they do (and don't realise what happened like I did), so I think we should fix it.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #9)
> I think we need to fix this in some way. I created a second RSS account the
> other day (MarcoZ had asked about it on IRC). It apparently had no effect,
> until I restarted at which time the new RSS account appeared and hid the old
> one - my initial thought was where have all my feeds gone? I quickly realised
> that the second account had overridden the first, and after deleting it and
> restarting, got my feeds back again.

I just tried, and got the exact same experience as Standard8.

Comment 11

[Wayne Mery (:wsmwk)]

noted in 3.0a2 testing https://litmus.mozilla.org/single_result.cgi?id=146250

Comment 12

[David Ascher (:davida)]

One idea would be to get rid of the concept of accounts for RSS -- the account creation dialog doesn't actually ask for any info, and I can't come up with significant use case for having multiple accounts.  So we could instead auto-create the infrastructure, setup a feeds area in the folder pane, empty to start (or possibly, subscribed to a thunderbird news feed in the future...)

(We could provide UI to hide the feeds folder if people wanted less clutter there)

Does anyone have a good description of why one would want two RSS accounts?

Bryan, your thoughts?

Comment 13

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

(In reply to comment #12)
> Does anyone have a good description of why one would want two RSS accounts?
I have 11 feed accounts. The main reasons are:
- Move all feeds from one account to a local folder so that the loading of remote content needs my permission
- Easier to tell Thunderbird which feeds to update.

Have a look at Forumzilla, it's really handy with update intervals per feed and the option to store the feed items where you want. It lacks support of folder move (the destinations don't get updated), an organizable structure (hierarchical and move up/down) and a "check these feeds now" option.

Comment 14

[Mark Banner (:standard8)]

Retriaging according to new policy for flags.
https://wiki.mozilla.org/Thunderbird:Release_Driving
(bugs marked wanted- don't indicate we wouldn't accept patches, but that they're not going to be the focus for release drivers)

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #13)
> (In reply to comment #12)
> > Does anyone have a good description of why one would want two RSS accounts?
> I have 11 feed accounts. The main reasons are:
> - Move all feeds from one account to a local folder so that the loading of
> remote content needs my permission
> - Easier to tell Thunderbird which feeds to update.

Seems like we could achieve these goals in another way besides having multiple accounts.

Comment 16

[Magnus Melin [:mkmelin]]

Regression window 2006-12-19 -> 2006-12-20 linux builds
<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=ThunderbirdTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-19+03%3A00%3A00&maxdate=2006-12-20+03%3A00%3A00&cvsroot=%2Fcvsroot>

Backing out bug 261425 fixes the problem.

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

The basic issue was that we set hostname to "News & Blogs", but hostnames can't contain space so it was changed in core to return an error. But we didn't rv check...

I'm fixing this by not allowing localization of the rss "hostname", and changing it to be "Feeds". (We seriously have to get a bug file to rename it to RSS/Atom Feeds" in the UI anyways.)
It wasn't shown in the UI, and nor really localizable anyways - bug 313525 - since the filename still might not be allowed for the platform it's used on.

It doesn't make sense to try-catch in AccountExists, if it really throws something we want to know about it. 

Finally, there was a phony news server hostname check that was hidden by the try catch.

Comment 18

[[e-mail]]

Comment on attachment 340820 [details] [diff] [review]
proposed fix

>+            <NC:hostName>Feeds</NC:hostName>
Aren't host names supposed to be in lower case?

>-    var i = 1;
>+    var i = 2;
>     while (parent.AccountExists(userName, hostName, serverType)) 
>     {
>-      // if "News & Blogs" exists, try
>-      // "News & Blogs-1", then "News & Blogs-2", etc.
>+      // If "Feeds" exists, try "Feeds-2", then "Feeds-3", etc.
>       hostName = hostNamePref + "-" + i;
>       i++;
>     }
I remember writing some unreadable code once - it set i to -1 (!) and then used "prefix" + --i to avoid concatenating the "-" ;-)

>-        
>+

>-    var newsServerName = document.getElementById("newsServer");
>-    if (hostnameIsIllegal(newsServerName.value))
>+    var newsServerName = document.getElementById("newsServer").value;
>+    if (hostnameIsIllegal(newsServerName))
Unrelated changes?

>-  aUrl->SetHost(hostname);
>-  aUrl->SetUserPass(username);
>-  aUrl->SetPort(port);
>+  rv = aUrl->SetHost(hostname);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = aUrl->SetUserPass(username);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = aUrl->SetPort(port);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>   FindServerByURI(aUrl, PR_TRUE, aResult);
>+  // FindServerByURI can fail (e.g. for rss accounts). Just ignore it.
>   return NS_OK;
But we don't ignore invalid host/user/port?

I had a patch somewhere to refactor all this ugly code, but I'm glad now I didn't upload it - I hadn't noticed that findRealServer always succeeds.

Comment 19

[Magnus Melin [:mkmelin]]

(In reply to comment #18)
> (From update of attachment 340820 [details] [diff] [review])
> >+            <NC:hostName>Feeds</NC:hostName>
> Aren't host names supposed to be in lower case?

RFC 2396 says host names are case insensitive. (This is what the folder name is called.)

> >-        
> >+
> 
> >-    var newsServerName = document.getElementById("newsServer");
> >-    if (hostnameIsIllegal(newsServerName.value))
> >+    var newsServerName = document.getElementById("newsServer").value;
> >+    if (hostnameIsIllegal(newsServerName))
> Unrelated changes?

No, newsServerName is used a few rows down... as the value. So it tried to validate find a valid account for a host name of [object XULSomething] iirc.

> >-  aUrl->SetHost(hostname);
> >-  aUrl->SetUserPass(username);
> >-  aUrl->SetPort(port);
> >+  rv = aUrl->SetHost(hostname);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  rv = aUrl->SetUserPass(username);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  rv = aUrl->SetPort(port);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >   FindServerByURI(aUrl, PR_TRUE, aResult);
> >+  // FindServerByURI can fail (e.g. for rss accounts). Just ignore it.
> >   return NS_OK;
> But we don't ignore invalid host/user/port?

Lack of ignoring invalid host was what made this proble hard to catch, so yes. I suppose it's not so nice to ignore the other error either, but at least it works.

Comment 20

[Magnus Melin [:mkmelin]]

Actually, that last comment is a bit off. FindServerByURI will always fail if it doesn't find the server. So it should be

  FindServerByURI(aUrl, PR_TRUE, aResult);
  // FindServerByURI will fail if it doesn't find the server. That's ok.
  return NS_OK;

Comment 21

[[e-mail]]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 340820 [details] [diff] [review] [details])
> > >+            <NC:hostName>Feeds</NC:hostName>
> > Aren't host names supposed to be in lower case?
> RFC 2396 says host names are case insensitive.
I was worried that the URL code lowercases the host name, but we compare it insensitively anyway, so that's OK after all.

> > >-    var newsServerName = document.getElementById("newsServer");
> > >-    if (hostnameIsIllegal(newsServerName.value))
> > >+    var newsServerName = document.getElementById("newsServer").value;
> > >+    if (hostnameIsIllegal(newsServerName))
> > Unrelated changes?
> No, newsServerName is used a few rows down... as the value. So it tried to
> validate find a valid account for a host name of [object XULSomething] iirc.
D'oh, my bug, I think... same problem with incomingServerName, no?

Comment 22

[[e-mail]]

Comment on attachment 340820 [details] [diff] [review]
proposed fix

>@@ -1765,22 +1765,29 @@ nsMsgAccountManager::FindRealServer(cons
I'd still rather go with my refactor of FindRealServer not to use FindServerByURI myself though... wanna review?

+              rv = accountManager->FindRealServer(userName, hostName, EmptyCString(),
+                                                  0, getter_AddRefs(incomingServerToUse));
+              NS_ENSURE_SUCCESS(rv, rv);
I thought we agreed that FindRealServer never fails?

Comment 23

[Magnus Melin [:mkmelin]]

(In reply to comment #21)
> D'oh, my bug, I think... same problem with incomingServerName, no?

Looks like it yes...

(In reply to comment #22)
> (From update of attachment 340820 [details] [diff] [review])
> >@@ -1765,22 +1765,29 @@ nsMsgAccountManager::FindRealServer(cons
> I'd still rather go with my refactor of FindRealServer not to use
> FindServerByURI myself though... wanna review?

I think i'd rather not refactor it in this bug.

> I thought we agreed that FindRealServer never fails?

Not normally, but with this patch yes it will, in case the hostname is invalid. 

Should probably change it to return error if it can't create the aUrl too...

Comment 24

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Updated patch. Fix comment, return error if we can't create an url also and take care of the incomingServerName mixup also.

Comment 25

[[e-mail]]

(In reply to comment #23)
> (In reply to comment #21)
> > I thought we agreed that FindRealServer never fails?
> Not normally, but with this patch yes it will
Do you mind if I file my patch and bitrot you rather than reviewing?

Comment 26

[[e-mail]]

Comment on attachment 341143 [details] [diff] [review]
proposed fix, v2

Cancelling review in expectation of an unbitrotted patch ;-)

Comment 27

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

So bug 458357 actually made account creation work. 

I'd still like the rest of the patch to go in; it fixes bug 313525, the serverName value/object confusion, and cleans up AccountExists().

Comment 28

[[e-mail]]

Comment on attachment 344776 [details] [diff] [review]
proposed fix, v3

(In reply to comment #27)
> So bug 458357 actually made account creation work. 
Heh, that wasn't the idea ;-)

Comment 29

[Phil Ringnalda (:philor)]

Comment on attachment 344776 [details] [diff] [review]
proposed fix, v3

Wow. I really wish I would have looked at bug 313525 close enough three years ago to see that the display name and the directory name come from separate fields.

Comment 30

[Phil Ringnalda (:philor)]

While you're touching it, the wizardAccountName line has a tab in it.

Comment 31

[Magnus Melin [:mkmelin]]

changeset:   717:4c428a4a8be6
http://hg.mozilla.org/comm-central/rev/4c428a4a8be6

->FIXED (with the tab removed)