Closed Bug 364987 Opened 13 years ago Closed 11 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b1

People

(Reporter: dbaron, Assigned: mkmelin)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

[ 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.
Flags: blocking-thunderbird3.0a1?
Is this bug still valid in recent builds?
Keywords: qawanted
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.
And oh, fresh profile on Mac OS X 10.5 Leopard.
Keywords: qawanted
Yep, linux too. Tried creating a third account, and it does not show up in the tree.
Assignee: mscott → nobody
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.
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");
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.
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3.0a1?
Flags: blocking-thunderbird3.0a1-
(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.
Flags: blocking-thunderbird3? → blocking-thunderbird3+
(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.
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?
(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.
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)
Flags: blocking-thunderbird3+ → wanted-thunderbird3+
Priority: -- → P1
Target Milestone: --- → Thunderbird 3.0b2
(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.
Assignee: nobody → mkmelin+mozilla
Attached patch proposed fix (obsolete) — Splinter Review
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.
Attachment #340820 - Flags: superreview?(neil)
Attachment #340820 - Flags: review?(philringnalda)
Blocks: 313525
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.
(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.
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;
Status: NEW → ASSIGNED
(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 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?
(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...
Attachment #340820 - Attachment is obsolete: true
Attachment #340820 - Flags: superreview?(neil)
Attachment #340820 - Flags: review?(philringnalda)
Attached patch proposed fix, v2 (obsolete) — Splinter Review
Updated patch. Fix comment, return error if we can't create an url also and take care of the incomingServerName mixup also.
Attachment #341143 - Flags: superreview?(neil)
Attachment #341143 - Flags: review?(philringnalda)
(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?
Depends on: 458357
Attachment #341143 - Flags: superreview?(neil)
Comment on attachment 341143 [details] [diff] [review]
proposed fix, v2

Cancelling review in expectation of an unbitrotted patch ;-)
Attachment #341143 - Attachment is obsolete: true
Attachment #341143 - Flags: review?(philringnalda)
Attached patch proposed fix, v3Splinter Review
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().
Attachment #344776 - Flags: superreview?(neil)
Attachment #344776 - Flags: review?(philringnalda)
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 ;-)
Attachment #344776 - Flags: superreview?(neil) → superreview+
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.
Attachment #344776 - Flags: review?(philringnalda) → review+
While you're touching it, the wizardAccountName line has a tab in it.
changeset:   717:4c428a4a8be6
http://hg.mozilla.org/comm-central/rev/4c428a4a8be6

->FIXED (with the tab removed)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Component: RSS → Feed Reader
Flags: blocking-thunderbird3.0a1-
Product: Thunderbird → MailNews Core
You need to log in before you can comment on or make changes to this bug.