Last Comment Bug 226890 - Thunderbird doesn't handle news: and nntp: URLs properly (RFC 5538)
: Thunderbird doesn't handle news: and nntp: URLs properly (RFC 5538)
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: NNTP (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: Thunderbird 11.0
Assigned To: Joshua Cranmer [:jcranmer]
:
Mentors:
http://tools.ietf.org/search/rfc5538
: 175292 224335 601564 603671 607116 (view as bug list)
Depends on: 705471
Blocks: 108107 108948 37465 403242 498321 694915
  Show dependency treegraph
 
Reported: 2003-11-26 17:06 PST by Robert Marshall
Modified: 2015-12-08 12:23 PST (History)
26 users (show)
mscott: blocking‑aviary1.5-
Pidgeot18: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Part 1: Clean up some unimplemented junk in NNTP code [Checked in] (15.08 KB, patch)
2011-01-05 12:25 PST, Joshua Cranmer [:jcranmer]
mozilla: review+
Details | Diff | Splinter Review
Part 2: Implement news URI tests [Checked in] (17.09 KB, patch)
2011-01-05 12:28 PST, Joshua Cranmer [:jcranmer]
standard8: review+
Details | Diff | Splinter Review
Part 3: Introduce nsCString to the URL parser (21.06 KB, patch)
2011-01-05 12:29 PST, Joshua Cranmer [:jcranmer]
mozilla: review+
Details | Diff | Splinter Review
Part 4: Move the parsing code to nsNntpUrl (35.66 KB, patch)
2011-01-05 12:31 PST, Joshua Cranmer [:jcranmer]
mozilla: review-
Details | Diff | Splinter Review
Part 5: Test the URI parser [checked in] (2.26 KB, patch)
2011-01-05 12:33 PST, Joshua Cranmer [:jcranmer]
standard8: review+
Details | Diff | Splinter Review
Part 6: Move the message key to nsNntpUrl as well (20.43 KB, patch)
2011-01-05 12:35 PST, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Splinter Review
Part 7: simplify nsParseNewsMessageURI (3.04 KB, patch)
2011-01-05 12:36 PST, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Splinter Review
Part 8: Only initialize m_nntpServer once (9.63 KB, patch)
2011-01-05 12:40 PST, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Splinter Review
Part 9: Parser fixups and more testing (3.04 KB, patch)
2011-01-05 12:43 PST, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Splinter Review
Part 10: Make command-line news handling work (11.89 KB, patch)
2011-01-05 12:50 PST, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Splinter Review
Part 9: Parser fixups and more testing [Checked in] (15.46 KB, patch)
2011-01-05 12:56 PST, Joshua Cranmer [:jcranmer]
mozilla: review+
Details | Diff | Splinter Review
Part 4: Move the parsing code to nsNntpUrl [Checked in] (35.78 KB, patch)
2011-02-07 06:33 PST, Joshua Cranmer [:jcranmer]
mozilla: review+
standard8: superreview+
Details | Diff | Splinter Review
Part 6: Move the message key to nsNntpUrl as well (20.40 KB, patch)
2011-02-07 06:41 PST, Joshua Cranmer [:jcranmer]
mozilla: superreview+
Details | Diff | Splinter Review
Part 3: Introduce nsCString to the URL parser [checked in] (21.14 KB, patch)
2011-02-16 14:26 PST, Joshua Cranmer [:jcranmer]
Pidgeot18: review+
Details | Diff | Splinter Review
Part 6: Move the message key to nsNntpUrl as well (20.39 KB, patch)
2011-04-08 10:46 PDT, Joshua Cranmer [:jcranmer]
Pidgeot18: superreview+
Details | Diff | Splinter Review
Part 8: Only initialize m_nntpServer once (9.63 KB, patch)
2011-04-08 10:48 PDT, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Splinter Review
Part 4.5: CStringify m_searchData [Checked in] (8.00 KB, patch)
2011-05-04 11:10 PDT, Joshua Cranmer [:jcranmer]
standard8: review+
Details | Diff | Splinter Review
Part 6: Move the message key to nsNntpUrl as well [Checked-in] (20.35 KB, patch)
2011-05-05 05:15 PDT, Joshua Cranmer [:jcranmer]
standard8: review+
Pidgeot18: superreview+
Details | Diff | Splinter Review
Part 7: simplify nsParseNewsMessageURI [Checked in] (3.04 KB, patch)
2011-05-05 05:17 PDT, Joshua Cranmer [:jcranmer]
standard8: review+
Details | Diff | Splinter Review
Part 8: Only initialize m_nntpServer once [Checked in] (9.64 KB, patch)
2011-05-05 05:18 PDT, Joshua Cranmer [:jcranmer]
standard8: review+
Details | Diff | Splinter Review
Part 10: Make command-line news handling work (12.03 KB, patch)
2011-05-18 11:39 PDT, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Splinter Review
Part 10: Make command-line news handling work (11.85 KB, patch)
2011-06-24 17:26 PDT, Joshua Cranmer [:jcranmer]
mozilla: review+
Details | Diff | Splinter Review
Part 10: Make command-line news handling work (12.00 KB, patch)
2011-11-04 20:33 PDT, Joshua Cranmer [:jcranmer]
Pidgeot18: review+
neil: superreview+
Details | Diff | Splinter Review

Description Robert Marshall 2003-11-26 17:06:50 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031126 Firebird/0.7+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031126 Firebird/0.7+

When trying to click on a "news:" URL Thunderbird will launch, however, the
newsgroup will not open up to read.

Reproducible: Always

Steps to Reproduce:
1. Open http://mcadams.posc.mu.edu/news.htm
2. Click on link that says "Read the group from your default news server."
3. The URL is "news:alt.assasination.jfk"

Actual Results:  
Thunderbird launches and nothing happens. Also, Mail notifier in System Tray
displays telling me "robertjm has 1 new message"
NOTE: robertjm is one of my email accounts, and NOT the default one and there
are no new messages in either the robertjm account, nor the default account I
have set up.

Expected Results:  
After clicking on the URL, Thunderbird should launch. You will be asked whether
you want to subscribe to the newsgroup, and then you will be prompted for your
username and password for the server. If you are already a member of that group
then it should simply launch into that group.

Was able to reproduce this using either Internet Explorer or Mozilla Firebird.

Thunderbird build is: "Mozilla Thunderbird 0.4a (20031119)"

The correct behaviour worked in a version of Netscape 7.02 that I have installed
from earlier this year:

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.2) Gecko/20030208
Netscape/7.02

Tested only in W2k Professional with latest SP and incremental updates.
Also, didn't know which Component to specify, so chose "Preferences"
Comment 1 Tom Metro 2004-05-20 18:47:04 PDT
I observed the same results when accessing a URL like
news://news.gmane.org./gmane.mail.im2000 in Firefox on my Windows NT system. It
launched Thunderbird, but then did nothing.

Trying the URL in IE also launched Thunderbird, and then produced an error
dialog saying: "Cannot find the file 'news://news.gmane.org./gmane.mail.im2000' ..."

Then I realized that my registry keys were obsolete (I installed the current
version from a zip file; apparently the app. doesn't automatically update these
registry keys) and it had launched an older 0.4a release of Thunderbird.
Correcting the keys to point to the current installation (0.6+ 20040518), fixed
the problem. Now, clicking on the URL launches Thunderbird and prompts with a
dialog asking "Do you want to subscribe to gmane.mail.im2000?"

Trying from IE still produces the same error, but that's likely due to some
broken registry key and not the fault of Thunderbird.

So the originally reported bug is probably fixed in the latest builds.

However, there is still some strange behavior. 

1. After clicking on Yes on the above dialog, it launched a new Thunderbird main
window (still only one instance, though), and then ran the new newsgroup account
wizard over that. Launching the second window is probably a bug.

2. By this time I had already setup an account (using 0.4a) for the
news.gmane.org server, and subscribed to the specified group, yet this wasn't
detected, and I was prompted to create a new account, if I wished to subscribe
to the group. So now I have two accounts for news.gmane.org (with a user name
appended to the one created in 0.6). But now that the account has been setup in
0.6, launching the URL does do the right thing and detects that the account for
that server already exists, so it just opens that group.

3. Requiring the user to setup a new account to view a group specified via a URL
seems to be a "heavy weight" solution and diminishes the usual convenience of
accessing random bits of data via URLs. Could the account be automatically
generated on the fly, using general news account defaults (for things like the
user's email address)? Or does this require more radical restructuring? If RSS
reading is folded into Thunderbird, it'll probably require similar restructuring
to make things more light weight, as it'll be common for people to subscribe to
dozens of RSS "news servers," and little benefit to each being viewed as a
separate account.

I'll split off these three items into their own bug reports if they seem like
items worth tracking.
Comment 2 Greg K. 2004-05-27 13:51:39 PDT
Thunderbird-Mac also fails to properly handle news: URLs; for example,
<news://news.mozdev.org:119/c94bvc$28cg$1@mozdev.mozdev.org>.

Also, TBird needs an Open URL... menu item for news and IMAP URLs.
Comment 3 mathew 2004-06-10 15:26:42 PDT
Thunderbird also generates invalid news: URLs when you ask it to copy the URL
for a newsgroup.

It places something looking like   news://servername.example.com/news.group   on
the clipboard.

This is syntactically invalid, as per RFC 1738 section 3.6.  It should either be

news:news.group

or

nntp://servername.example.com/news.group
Comment 4 Doug Wright 2005-01-15 12:25:23 PST
Borked URL handling is never good.
Comment 5 Owen Duffy 2005-07-02 17:03:37 PDT
Some cases of a valid news URL in an incoming plain text message are recognised
by Thunderbird, but it creates an incorrect hot link URL. It is the handling of
news:<msg-id> that is the problem.
For example incoming plain text is "at
news:hpcsb15obme7k39l5pmbq8sc0b30eqe2e3@4ax.com ." The hot link URL that
Thunderbird creates is news://hpcsb15obme7k39l5pmbq8sc0b30eqe2e3@4ax.com , it
should not have the // prepended to the msgid.
Comment 6 alta88 2006-11-02 09:51:03 PST
news: protocol subscribing from web page link still broken in Tb2.0b1.

clicking on a news: link creates as many as 2 additional Tb windows if Tb is already running, and also brings up an unnecessary Account subscription wizard.  throbber also continues to run after a group has been subscribed newly or exists already.

each of these must be handled:

Tb running:
- communicate with existing process, do not start another!
- no account, no group (add acct, add group, show Wizard based on optional pref; do not want to see this and can create posting acct later.
- account exists, no group (show subscribed successfully msg)
- account exists, group subscribed (show already subscribed msg)
Tb not runnning:
- no account, no group (start Tb, add acct, add group, optional Wizard
- account exists, no group (start Tb, show subscribed successfully msg)
- account exists, group subscribed (start Tb, show already subscribed msg)
 
in all cases there is no more than one mailnews window.  perhaps the news: protocol can be renovated the way feed: is being done in newsblogs.js...

Comment 7 Mike Cowperthwaite 2007-02-22 11:23:11 PST
(In reply to comment #6)
> news: protocol subscribing from web page link still broken in Tb2.0b1.

That's true: xref bug 327885 for one of the worst symptoms (which includes the Wizard problem, sort of).

 
> clicking on a news: link creates as many as 2 additional Tb windows if Tb is
> already running

Bug 355633 has implemented a minor mitigation to this which ensures that only one new 3pane window is opened.
Comment 8 David E. Ross 2007-08-15 11:21:42 PDT
See bug #89939 for a related issue.  
Comment 9 Jeff Relf 2009-09-26 02:31:57 PDT
“ 3.6. NEWS

   The news URL scheme is used to refer to either news groups or
   individual articles of USENET news, as specified in RFC 1036.

   A news URL takes one of two forms:

     news:<newsgroup-name>
     news:<message-id> ”.

Quoting the URI “Uniform Resource Identifiers” spec ( RFC-3986 ):
  www.Tools.ietf.ORG/html/rfc3986

“ 1.1.2.  Examples

   The following example URIs illustrate several URI schemes and
   variations in their common syntax components:

      ftp://ftp.is.co.za/rfc/rfc1808.txt

      http://www.ietf.org/rfc/rfc2396.txt

      ldap://[2001:db8::7]/c=GB?objectClass?one

      mailto:John.Doe@example.com

      news:comp.infosystems.www.servers.unix ”.
Comment 10 Jeff Relf 2009-09-26 02:36:17 PDT
(In reply to comment #9)
> “ 3.6. NEWS
> 
>    The news URL scheme is used to refer to either news groups or
>    individual articles of USENET news, as specified in RFC 1036.
> 
>    A news URL takes one of two forms:
> 
>      news:<newsgroup-name>
>      news:<message-id> ”.

I was quoting the URL “Uniform Resource Locators” spec ( RFC-1738 ):
  www.Tools.ietf.ORG/html/rfc1738
Comment 11 David E. Ross 2009-09-26 10:51:20 PDT
RFC 1738 is no longer current.  In the RFC index, it is listed as "Obsolete".
Comment 12 Jeff Relf 2009-09-26 11:22:30 PDT
(In reply to comment #11)
> RFC 1738 is no longer current.  In the RFC index, it is listed as "Obsolete".

I need a link for that; IETF.ORG is ·Thee· authority:
  www.Tools.ietf.ORG/html/rfc1738

The examples here ( RFC 3986 ):
  www.Tools.ietf.ORG/html/rfc3986

“ news:Message-ID ” is a valid URI (URL),
one that MicroSoft was been using for decades,
yet ThunderBird goes haywire when you click it.

It'll never get fixed, you can be sure.
Comment 13 David E. Ross 2009-09-30 18:38:41 PDT
Re: Comment #12.  

If you are asking for a reference to RFC #1738 being obsolete, go to <http://www.rfc-editor.org/rfcsearch.html>.  On the left side, enter 1783 and "Search for" should be "Number".  On the right side, select the RFC radio button for "Search".  Then select the "SEARCH" button on the left.  You will see that RFC #1738 was "Obsoleted by RFC2348".  

If you then select "RFC2348", you will get an RFC that seems to have nothing to do with URIs.  

Yes, something is fouled up.
Comment 14 Joshua Cranmer [:jcranmer] 2009-09-30 18:49:20 PDT
The correct reference for news: and nntp: URIs is
<http://tools.ietf.org/html/draft-ellermann-news-nntp-uri-11>.

Now stop arguing about it here.
Comment 15 David E. Ross 2010-05-24 08:16:55 PDT
Ellermann's draft RFC is now official as RFC 5538 at <http://www.rfc-editor.org/rfc/rfc5538.txt>.
Comment 16 Joshua Cranmer [:jcranmer] 2010-10-04 04:52:26 PDT
*** Bug 601564 has been marked as a duplicate of this bug. ***
Comment 17 Nikolay Shopik 2010-10-12 11:27:51 PDT
*** Bug 603671 has been marked as a duplicate of this bug. ***
Comment 18 Mike Lykov 2010-10-14 05:22:06 PDT
In Knode (for example) when i click this link it opens a linked article in a
separate window. I can read that article and close it.
In thunderbird, when i click a link like this .. nothing happens.
Thunderbird 3 may open news articles in tabs. I suggest to open clicked links
in other tabs.
I use public server news.kraft-s.ru and group kraft.test for testing. All links
usually a local for server.

Instead of waiting for FULL implementing a RFC 5538 at once (with all url schemas, subcribing via link, etc) maybe implementing a part will be better and faster?

In my case all links are "2.2.  'news' URIs", and no need for subscribing to a new group via click a link.
Comment 19 Nikolay Shopik 2010-10-14 08:09:05 PDT
*** Bug 175292 has been marked as a duplicate of this bug. ***
Comment 20 Joshua Cranmer [:jcranmer] 2010-10-16 11:21:13 PDT
I don't have a patch for this yet (I'm only up through centralizing parsing in nsNntpUrl), but I'll still marked myself as assigned anyways.
Comment 21 Joshua Cranmer [:jcranmer] 2010-10-25 17:54:33 PDT
*** Bug 607116 has been marked as a duplicate of this bug. ***
Comment 22 Joshua Cranmer [:jcranmer] 2011-01-05 12:25:58 PST
Created attachment 501393 [details] [diff] [review]
Part 1: Clean up some unimplemented junk in NNTP code [Checked in]

While not strictly needed for anything else, this essentially removes unimplemented news URL types so I don't have to worry about them later. I probably could go on a rampage and remove all of the #ifdef'd non-functional code in the state machine, but that is way too out of scope for now.
Comment 23 Joshua Cranmer [:jcranmer] 2011-01-05 12:28:03 PST
Created attachment 501396 [details] [diff] [review]
Part 2: Implement news URI tests [Checked in]

This is the comprehensive set of tests that check that all of the URIs we create work.

Also included are some fixes for leaking tests.
Comment 24 Joshua Cranmer [:jcranmer] 2011-01-05 12:29:22 PST
Created attachment 501399 [details] [diff] [review]
Part 3: Introduce nsCString to the URL parser

Well, except for the message ID, but we take care of that in...
Comment 25 Joshua Cranmer [:jcranmer] 2011-01-05 12:31:48 PST
Created attachment 501400 [details] [diff] [review]
Part 4: Move the parsing code to nsNntpUrl

... this patch. We also move most of the parser action over to nsNntpUrl, and use the nsNewsAction bit to communicate what needs to be done to nsNNTPProtocol.

By happenstance, this also fixes bug 403242, but the test to make sure of that will be coming in a later part.
Comment 26 Joshua Cranmer [:jcranmer] 2011-01-05 12:33:08 PST
Created attachment 501401 [details] [diff] [review]
Part 5: Test the URI parser [checked in]

The last patch was so bug, I moved the tests for the parser over to this patch. Yet more tests come after some more fixes.
Comment 27 Joshua Cranmer [:jcranmer] 2011-01-05 12:35:44 PST
Created attachment 501402 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well

This is a further extension of part 4, again in its own patch to try to keep patches smaller.
Comment 28 Joshua Cranmer [:jcranmer] 2011-01-05 12:36:54 PST
Created attachment 501404 [details] [diff] [review]
Part 7: simplify nsParseNewsMessageURI
Comment 29 Joshua Cranmer [:jcranmer] 2011-01-05 12:40:30 PST
Created attachment 501406 [details] [diff] [review]
Part 8: Only initialize m_nntpServer once

This is particularly useful for when we start supporting no-authority URIs, and it also brings some sanity into the parsing, since it means we can grab the folder from the server as opposed to looking up the folder via the account manager.
Comment 30 Joshua Cranmer [:jcranmer] 2011-01-05 12:43:47 PST
Created attachment 501407 [details] [diff] [review]
Part 9: Parser fixups and more testing

This makes nntp: URLs in particular works, and it also adds tests for other bugs fixed somewhere in this pile of patches.
Comment 31 Joshua Cranmer [:jcranmer] 2011-01-05 12:50:22 PST
Created attachment 501410 [details] [diff] [review]
Part 10: Make command-line news handling work

With this patch, the following things work:

With Thunderbird open:
thunderbird news://host/group
thunderbird news://host/msgid
thunderbird nntp://host/group/key
thunderbird nntp://host/group

With Thunderbird not open:
thunderbird news://host/group
thunderbird nntp://host/group

With SeaMonkey open or not open:
seamonkey news://host/group
seamonkey news://host/msgid
seamonkey nntp://host/group/key
seamonkey nntp://host/group

The cache service has issues opening up Thunderbird, due to timing issues, and I've not thought up the best way to counteract this. As long as Thunderbird is open when you click on a news link, it shouldn't matter.

I would write tests for all of these, but I would need bug 540330 to be finished first.

And this patch completes the massive newsuri- patch queue... for this bug.
Comment 32 Joshua Cranmer [:jcranmer] 2011-01-05 12:56:00 PST
Created attachment 501412 [details] [diff] [review]
Part 9: Parser fixups and more testing [Checked in]

Wrong patch... That's what I get for naming them morefix and parsefix...
Comment 33 Mark Banner (:standard8) 2011-01-07 06:08:42 PST
Comment on attachment 501396 [details] [diff] [review]
Part 2: Implement news URI tests [Checked in]

on file: mailnews/news/test/unit/test_internalUris.js line 82
>   let nntpService = Cc["@mozilla.org/messenger/nntpservice;1"]

Given you use nntpService in a few functions, its probably just worth
splitting it out to a global.
Comment 34 David :Bienvenu 2011-01-07 16:29:00 PST
Comment on attachment 501399 [details] [diff] [review]
Part 3: Introduce nsCString to the URL parser

don't need space before '(':

+  NS_ASSERTION (!message_id.Length() || message_id != aGroup, "something not null");

m_searchData seems like it could be an nsCString as well
Comment 35 Ludovic Hirlimann [:Usul] 2011-01-10 05:42:09 PST
*** Bug 224335 has been marked as a duplicate of this bug. ***
Comment 36 David E. Ross 2011-01-10 07:53:36 PST
Re: comment #31

Per RFC 5538, testing on the news: protocol should also include cases without the host: 
  news: group
  news: msgid
where the appropriate host will be selected from an account already setup.  By "appropriate host", I mean the host for which "group" is already subscribed or the host that contains the message indicated by "msgid".
Comment 37 Joshua Cranmer [:jcranmer] 2011-01-10 08:16:58 PST
(In reply to comment #36)
> Re: comment #31
> 
> Per RFC 5538, testing on the news: protocol should also include cases without
> the host: 
>   news: group
>   news: msgid
> where the appropriate host will be selected from an account already setup.  By
> "appropriate host", I mean the host for which "group" is already subscribed or
> the host that contains the message indicated by "msgid".

The patches through what I have posted here do not attempt to handle no-authority news URIs by themselves, although they probably do fix parsing. I do have patches in the works to fix this, which will be posted on more appropriate bugs when they have finished, and they will include tests to make sure they work.

If you paid attention to the implementation of the patches, you would notice that these cases do not work as stands, hence why I didn't test them (I tested them on the later patch where they do work ^_^).
Comment 38 Mark Banner (:standard8) 2011-01-10 11:21:14 PST
Comment on attachment 501401 [details] [diff] [review]
Part 5: Test the URI parser [checked in]

>diff --git a/mailnews/news/test/unit/test_uriParser.js b/mailnews/news/test/unit/test_uriParser.js

>+    for (let prop in test) {
>+      if (prop == "uri") continue;

Nit: Please put the continue on the next line.
Comment 39 Joshua Cranmer [:jcranmer] 2011-01-10 17:33:33 PST
(In reply to comment #34)
> m_searchData seems like it could be an nsCString as well

I could have sworn I saw a PR_strtok on m_searchData, which is why I didn't change that. Since I'm planning on changing the XPAT and URI code for bug 530193 anyways, I'll cstringify-it then.
Comment 40 David :Bienvenu 2011-01-11 15:34:17 PST
Comment on attachment 501400 [details] [diff] [review]
Part 4: Move the parsing code to nsNntpUrl

Looks generally ok...

+  {
+    NS_ASSERTION(0, "Should not get here!");
+    rv = NS_ERROR_FAILURE;
+  }

should be NS_ERROR...

this usage occurs several places:
+  if (aMessageID.Length()) {

this usage is potentially faster:
if (!aMessageID.IsEmpty()) {

+  NS_ENSURE_SUCCESS(rv, rv);
+
+  if (scheme.EqualsLiteral("news") || scheme.EqualsLiteral("snews"))
+    rv = ParseNewsURL();
+  NS_ENSURE_SUCCESS(rv, rv);

should use braces and include NS_ENSURE_SUCCESS in the braces, since you checked rv above.

Shouldn't you use some BeinsWith variant here? Neil could say for sure...
+  else if (query.Find("search") == 0)
Comment 41 David :Bienvenu 2011-01-12 12:34:03 PST
Any reason you haven't landed the patches that have reviews?
Comment 42 Joshua Cranmer [:jcranmer] 2011-01-12 12:57:01 PST
(In reply to comment #40)
> this usage occurs several places:
> +  if (aMessageID.Length()) {
> 
> this usage is potentially faster:
> if (!aMessageID.IsEmpty()) {

I thought I had fixed all of those, looks like I didn't (must be the fault of trying to look back on a patch when 8 others are on top of it).

> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (scheme.EqualsLiteral("news") || scheme.EqualsLiteral("snews"))
> +    rv = ParseNewsURL();
> +  NS_ENSURE_SUCCESS(rv, rv);
> 
> should use braces and include NS_ENSURE_SUCCESS in the braces, since you
> checked rv above.

The NS_ENSURE_SUCCESS(rv, rv); is there because it will be used when I add the later schemes a few patches down the line (part 9 in particular, I think).


(In reply to comment #41)
> Any reason you haven't landed the patches that have reviews?

Some of the patches might regress slightly-working functionality (particularly command-line handling, which I didn't test until the end of part 10 since it was so broken for me), so I didn't want to land them pre-alpha 2.
Comment 43 Joe Sabash [:JoeS1] 2011-01-22 10:34:13 PST
Testing with you tryserver build:
Using this link
news://news.annexcafe.com:119/1295017202_82439@pegasus.annex.net
Works fine

Now with:
news:1295017202_82439@pegasus.annex.net
I get a "self signed certificate " warning , then TB hangs. (no crash report)
Looks like it's trying to use secure port 563 by default.
Comment 44 Joshua Cranmer [:jcranmer] 2011-01-22 15:01:51 PST
Joe: the no-authority news URIs are not in the patch queue on this bug and will not be placed in this patch queue, but rather in the more appropriate open bug.
Comment 45 timeless 2011-01-25 05:57:18 PST
fwiw, we have NS_NOTREACHED which is more appropriate than NS_ERROR. In theory someone might use Coverity or similar with special magic to try to detect when it can reach a state you don't want it to reach.
Comment 46 Joshua Cranmer [:jcranmer] 2011-02-07 06:07:29 PST
Parts 1 and 2 checked in:
<http://hg.mozilla.org/comm-central/pushloghtml?changeset=78440aa35ff6>.
Comment 47 Joshua Cranmer [:jcranmer] 2011-02-07 06:33:02 PST
Created attachment 510264 [details] [diff] [review]
Part 4: Move the parsing code to nsNntpUrl [Checked in]
Comment 48 Joshua Cranmer [:jcranmer] 2011-02-07 06:41:55 PST
Created attachment 510265 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well
Comment 49 David :Bienvenu 2011-02-16 13:22:07 PST
last time I tried, I couldn't get the patch for part 4 to apply by itself.
Comment 50 Joshua Cranmer [:jcranmer] 2011-02-16 14:26:10 PST
Created attachment 512922 [details] [diff] [review]
Part 3: Introduce nsCString to the URL parser [checked in]

Turns out I need to post an updated version of this patch to get everyone else to apply correctly.
Comment 51 David :Bienvenu 2011-02-22 09:32:06 PST
Comment on attachment 510264 [details] [diff] [review]
Part 4: Move the parsing code to nsNntpUrl [Checked in]

If we're going to have goto's, it would be better to have them on their own line:

+    if (!m_newsFolder) goto FAIL;
Comment 52 Karsten Düsterloh 2011-02-22 10:45:43 PST
(In reply to comment #51)
> If we're going to have goto's, it would be better to have them on their own
> line:
> 
> +    if (!m_newsFolder) goto FAIL;

IBTD: that'd make use of an usual ("line") debugger unnecessarily complicated.
Comment 53 Karsten Düsterloh 2011-02-22 10:46:23 PST
> IBTD: 

Erm, actually, I agree. :-/
Comment 54 David :Bienvenu 2011-02-24 13:07:08 PST
Comment on attachment 510265 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well

sr=me, with some nits:

I think you can move the nsresult rv to the line it's first used on here:

+  nsresult rv;
 
-   nsCOMPtr <nsINntpService> nntpService = do_GetService(NS_NNTPSERVICE_CONTRACTID, &rv);
-   NS_ENSURE_SUCCESS(rv,rv);
+  nsCOMPtr<nsIMsgIncomingServer> server;
+  rv = GetServer(getter_AddRefs(server));
+  NS_ENSURE_SUCCESS(rv, rv);


This seems unlikely :-)

+      m_key = nsMsgKey_None;
+      m_key = keyStr.ToInteger(&rv, 10);
Comment 55 Mark Banner (:standard8) 2011-03-16 02:41:53 PDT
Comment on attachment 501410 [details] [diff] [review]
Part 10: Make command-line news handling work

Unfortunately I think that parts 6 and 8 have bitrotted. Once they are fixed, re-request review and I'll get to this straight away.
Comment 56 Joshua Cranmer [:jcranmer] 2011-04-08 10:46:37 PDT
Created attachment 524676 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well

Updating patch 6, carrying sr+ from bienvenu.
Comment 57 Joshua Cranmer [:jcranmer] 2011-04-08 10:48:13 PDT
Created attachment 524678 [details] [diff] [review]
Part 8: Only initialize m_nntpServer once

... and part 8.
Comment 58 Mark Banner (:standard8) 2011-05-04 04:11:59 PDT
Comment on attachment 510264 [details] [diff] [review]
Part 4: Move the parsing code to nsNntpUrl [Checked in]

Review of attachment 510264 [details] [diff] [review]:

This is sr=me as long as you make sure you fix the memory leak (and other nits) before you land this.

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +1046,5 @@
+    nsCOMPtr<nsIURL> url = do_QueryInterface(m_runningURL);
+    rv = url->GetQuery(commandSpecificData);
+    NS_ENSURE_SUCCESS(rv, rv);
+    MsgUnescapeString(commandSpecificData, 0, unescapedCommandSpecificData);
+    m_searchData = ToNewCString(unescapedCommandSpecificData);

This is actually a problem in the part 3 patch, but you're creating a memory leak here - m_searchData never gets freed afaict.

Could it just be changed to an nsCString?

::: mailnews/news/src/nsNntpUrl.cpp
@@ +141,5 @@
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  // Drop the potential beginning from the path
+  if (path.Length() > 0 && path[0] == '/')

nit: if (path.Length() && path[0] == '/')

@@ +170,3 @@
     return NS_OK;
   }
+  else if (query.EqualsLiteral("list-ids"))

nit: normally we don't have else after return (several places in this function).
Comment 59 Mark Banner (:standard8) 2011-05-04 06:20:52 PDT
Comment on attachment 524676 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well

Review of attachment 524676 [details] [diff] [review]:

This is looking good with a few minor comments. However, the test doesn't apply cleanly at the moment (on top of parts 3 - 5), so I can't do a full review yet.

::: mailnews/news/src/nsNntpUrl.cpp
@@ +119,5 @@
   if (scheme.EqualsLiteral("news") || scheme.EqualsLiteral("snews"))
     rv = ParseNewsURL();
+  else if (scheme.EqualsLiteral("news-message"))
+  {
+    nsCString spec;

Might as well use nsCAutoString here.

@@ +159,5 @@
     MsgUnescapeString(path, 0, m_messageID);
+
+    // Set group, key for ?group=foo&key=123 uris
+    nsCAutoString spec;
+    GetSpec(spec);

Would getting the path be more efficient than the spec?

@@ +166,5 @@
+    if (groupPos != kNotFound && keyPos != kNotFound)
+    {
+      // get group name and message key
+      m_group = Substring(spec, groupPos + kNewsURIGroupQueryLen,
+                            keyPos - groupPos - kNewsURIGroupQueryLen);

nit: start of keyPos should line up with the s of spec on the line above.

@@ +168,5 @@
+      // get group name and message key
+      m_group = Substring(spec, groupPos + kNewsURIGroupQueryLen,
+                            keyPos - groupPos - kNewsURIGroupQueryLen);
+      nsCAutoString keyStr;
+      keyStr = Substring(spec, keyPos + kNewsURIKeyQueryLen);

nit: you can do initialisation all on one line.

@@ +170,5 @@
+                            keyPos - groupPos - kNewsURIGroupQueryLen);
+      nsCAutoString keyStr;
+      keyStr = Substring(spec, keyPos + kNewsURIKeyQueryLen);
+      m_key = nsMsgKey_None;
+      m_key = keyStr.ToInteger(&rv, 10);

ToInteger returns zero on failure. So assigning to nsMsgKey_None first isn't going to do much.

@@ +430,5 @@
+  // either one. We'll assume it's an internal one first, though.
+  PRBool isNews = scheme.EqualsLiteral("news") || scheme.EqualsLiteral("snews");
+  PRBool isNntp = scheme.EqualsLiteral("nntp") || scheme.EqualsLiteral("nntps");
+  
+  PRBool tryReal = isNntp;

You don't seem to change isNntp, so I don't see the need for tryReal.
Comment 60 Mark Banner (:standard8) 2011-05-04 08:32:58 PDT
Comment on attachment 501404 [details] [diff] [review]
Part 7: simplify nsParseNewsMessageURI

Review of attachment 501404 [details] [diff] [review]:

This looks fine, but I'll need the other patches to be able to test it fully.
Comment 61 Mark Banner (:standard8) 2011-05-04 08:42:49 PDT
Comment on attachment 524678 [details] [diff] [review]
Part 8: Only initialize m_nntpServer once

Review of attachment 524678 [details] [diff] [review]:

One nit, but this is generally looking fine. I'll take another look once the rest of the patches are updated.

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +477,5 @@
 
     rv = server->GetRealHostName(hostName);
     NS_ENSURE_SUCCESS(rv, rv);
 
+    PR_LOG(NNTP,PR_LOG_ALWAYS,("(%p) opening connection to %s on port %d",this, hostName.get(), port));

nit: spaces after commas, and please wrap this a bit.
Comment 62 Joshua Cranmer [:jcranmer] 2011-05-04 11:10:40 PDT
Created attachment 530086 [details] [diff] [review]
Part 4.5: CStringify m_searchData [Checked in]

Well, this is theoretically an addendum to part 3, but I put it on top of part 4, so I'll call it 4.5. Although, if it were on 3, then I could call it part π ;-)
Comment 63 Joshua Cranmer [:jcranmer] 2011-05-05 05:07:21 PDT
Comment on attachment 501401 [details] [diff] [review]
Part 5: Test the URI parser [checked in]

Checked in 3, 4, 4.5, and 5: <http://hg.mozilla.org/comm-central/rev/d9b0c5b283f4>.
Comment 64 Joshua Cranmer [:jcranmer] 2011-05-05 05:15:44 PDT
Created attachment 530278 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well [Checked-in]

Carrying forward sr+ from bienvenu.
Comment 65 Joshua Cranmer [:jcranmer] 2011-05-05 05:17:57 PDT
Created attachment 530279 [details] [diff] [review]
Part 7: simplify nsParseNewsMessageURI [Checked in]
Comment 66 Joshua Cranmer [:jcranmer] 2011-05-05 05:18:55 PDT
Created attachment 530281 [details] [diff] [review]
Part 8: Only initialize m_nntpServer once [Checked in]
Comment 67 Mark Banner (:standard8) 2011-05-06 04:38:25 PDT
Comment on attachment 530278 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well [Checked-in]

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

::: mailnews/news/src/nsNntpUrl.cpp
@@ +167,5 @@
> +    {
> +      // get group name and message key
> +      m_group = Substring(spec, groupPos + kNewsURIGroupQueryLen,
> +                          keyPos - groupPos - kNewsURIGroupQueryLen);
> +      nsCAutoString keyStr = Substring(spec, keyPos + kNewsURIKeyQueryLen);

I wasn't quite right here, this does need to be on two lines (I get a compiler failure on Mac otherwise).
Comment 68 Joshua Cranmer [:jcranmer] 2011-05-18 11:39:35 PDT
Created attachment 533343 [details] [diff] [review]
Part 10: Make command-line news handling work

I guess I forgot to request review on this earlier. While it doesn't strictly need two reviewers on this, as this is the culmination of the entire bug and difficult/impossible to test with the current frameworks, I'd feel more comfortable if I had more people look at it.
Comment 69 David :Bienvenu 2011-05-23 09:38:04 PDT
Comment on attachment 533343 [details] [diff] [review]
Part 10: Make command-line news handling work

this no longer applies cleanly - can you refresh? Thx!
Comment 70 Joshua Cranmer [:jcranmer] 2011-06-24 17:26:37 PDT
Created attachment 541868 [details] [diff] [review]
Part 10: Make command-line news handling work

This is the latest patch I have applied...
Comment 71 neil@parkwaycc.co.uk 2011-06-25 16:31:23 PDT
Comment on attachment 541868 [details] [diff] [review]
Part 10: Make command-line news handling work

I don't build Thunderbird. Would I be expected to notice any effect of this patch on SeaMonkey, and if so, what?
Comment 72 Joshua Cranmer [:jcranmer] 2011-06-25 16:42:31 PDT
(In reply to comment #71)
> Comment on attachment 541868 [details] [diff] [review] [review]
> Part 10: Make command-line news handling work
> 
> I don't build Thunderbird. Would I be expected to notice any effect of this
> patch on SeaMonkey, and if so, what?

In theory, it should allow seamonkey news://news.mozilla.org/mozilla.dev.planning to open up the mail window with the newsgroup in question, or a corresponding message-ID API to do the same. In other words, if seamonkey is the default news handler, all news links (except for no-authority links) should just work.
Comment 73 David :Bienvenu 2011-07-05 12:12:52 PDT
Comment on attachment 541868 [details] [diff] [review]
Part 10: Make command-line news handling work

last chunk of nsNNTPService part of patch doesn't apply for me, so clearing review request.
Comment 74 David :Bienvenu 2011-07-06 11:48:43 PDT
When I try clicking on a news: url in the browser, with TB running, I get the following error:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004003: file c:/builds
/tbirdhq/objdir-tb/mailnews/news/src/../../../../mailnews/news/src/nsNntpService
.cpp, line 1729
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POI
NTER) [nsIMsgMessageUrl.messageHeader]"  nsresult: "0x80004003 (NS_ERROR_INVALID
_POINTER)"  location: "JS frame :: file:///C:/builds/tbirdhq/objdir-tb/mozilla/d
ist/bin/components/nsMailNewsCommandLineHandler.js :: nsMailNewsCommandLineHandl
er_handle :: line 117"  data: no]
************************************************************

which seems to mean that nsMailNewsCommandLineHandler needs to be taught about urls like news://news.mozilla.org/mozilla.dev.apps.thunderbird, in particular, they don't have a message header.
Comment 75 David :Bienvenu 2011-07-06 15:06:29 PDT
Comment on attachment 541868 [details] [diff] [review]
Part 10: Make command-line news handling work

so, modulo the actual command line parsing not working, this patch does seem to work if the link is in an e-mail message...

this part might have unforeseen consequences:

+        // This helps when running URLs from the command line
+        rv = pump->SetLoadGroup(m_loadGroup);
+        NS_ENSURE_SUCCESS(rv, rv);
+

so we'll have to look out for them.
Comment 76 Mark Banner (:standard8) 2011-08-03 04:26:57 PDT
Comment on attachment 541868 [details] [diff] [review]
Part 10: Make command-line news handling work

You've got David's review on this, so I don't think you need mine as well. You probably will want Neil's for the SM side though I guess.
Comment 77 Joshua Cranmer [:jcranmer] 2011-11-04 20:33:58 PDT
Created attachment 572163 [details] [diff] [review]
Part 10: Make command-line news handling work

Updating patch to tip; also, pinging Neil for sr.
Comment 78 neil@parkwaycc.co.uk 2011-11-17 16:38:31 PST
Comment on attachment 572163 [details] [diff] [review]
Part 10: Make command-line news handling work

We open a browser window for all external URLs anyway (I wonder whether there is a way to tell the browser window that it can close again like it does for downloads/helper applications) but this does fix newsgroup links.
Comment 79 Joe Sabash [:JoeS1] 2011-11-20 12:44:47 PST
Joshua,
For purposes of testing the fixes here (before this checks in)
I would appreciate a refresher on how these links are formed, especially in the case of UI methods to form them I.E. (Right click >>copy message location)
news://news.mozilla.org:119/4YidnViddtJz5z7RnZ2dnUVZ_qudnZ2d@mozilla.org

I don't see a way to easily form url's such as you reference here:
With Thunderbird open:
thunderbird news://host/group
thunderbird news://host/msgid
thunderbird nntp://host/group/key
thunderbird nntp://host/group

With Thunderbird not open:
thunderbird news://host/group
thunderbird nntp://host/group

With SeaMonkey open or not open:
seamonkey news://host/group
seamonkey news://host/msgid
seamonkey nntp://host/group/key
seamonkey nntp://host/group

I imagine the news://host/msgid will be the most common use case.(for links within a common subscribed newsgroup.)

news://host/group/key for instance is a mystery to me.
Comment 80 Joshua Cranmer [:jcranmer] 2011-11-20 16:08:22 PST
It's easiest to compose most of these links by hand; not all of them are synthesizable via the UI. Getting the message-ID is most easily done by looking at the message source and hunting for the Message-ID; the message key can often be similarly gotten via reading the Xref header or by looking at the Order Received column
Comment 81 Joshua Cranmer [:jcranmer] 2011-11-21 16:46:51 PST
The final patch has been checked in as changeset e57a9c880238; time to mark this bug as closed.
Comment 82 Joe Sabash [:JoeS1] 2011-11-22 17:33:41 PST
I'm seeing some problems testing with:
Mozilla/5.0 (Windows NT 5.0; rv:11.0a1) Gecko/20111122 Thunderbird/11.0a1 ID:20111122030022

Things that worked in your original tryserver build no longer work.
Example:
news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org
worked on the original tryserver build, It now opens a compose window with the contents of the messsage as an html attachment.
Thats using winxp, for win2k I get a message..
cannot locate MSVCR80D.DLL when I click on that link. (although that could be my problem, I keep win2k to test minimal usage requirements)

Maybe we should test on a post to MDAT


That link was tested as email content as well as a link in a newsgroup.
Comment 83 David E. Ross 2011-11-23 11:31:21 PST
Someone please tell me what are the other bugs cited in comment #37 and comment #44.
Comment 84 neil@parkwaycc.co.uk 2011-11-27 07:13:27 PST
(In reply to Joe Sabash from comment #82)
> for win2k I get a message..
> cannot locate MSVCR80D.DLL when I click on that link. (although that could
> be my problem, I keep win2k to test minimal usage requirements)
Sounds like you're trying to run a debug build on a machine that doesn't have any version of VC2005 and/or the Vista SDK compiler installed on it.
Comment 85 Joe Sabash [:JoeS1] 2011-11-27 07:29:32 PST
That missing DLL message is _not_ a result of this bugfix.
I get the same error with other TB versions as well. (current Earlybird and Betas)
But only when I click on a news url.
I have tried to run debug builds that I accidentally downloaded from the tryserver.

I'm thinking this is result of those attempts, somehow.
Comment 86 neil@parkwaycc.co.uk 2011-11-27 07:32:07 PST
Comment on attachment 530278 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well [Checked-in]

[Original request... 30 weeks ago. Oops!]

>+    m_group = Substring(m_group, m_group.RFind("/") + 1);
[m_group.Cut(0, m_group.RFind("/") + 1); avoids an allocation.]
Comment 87 neil@parkwaycc.co.uk 2011-11-27 07:43:40 PST
Comment on attachment 530279 [details] [diff] [review]
Part 7: simplify nsParseNewsMessageURI [Checked in]

>+    group = StringHead(uriStr, keySeparator);
>+    PRInt32 groupSeparator = group.RFind("/");
>+    if (groupSeparator == -1)
>+      return NS_ERROR_FAILURE;
>+    group = Substring(group, groupSeparator + 1);
Is it helpful to set group when returning failure?
Assuming it isn't, this might help avoid copies/allocations:
PRInt32 groupSeparator = MsgRFindChar(uriStr, '/', keySeparator);
if (groupSeparator == -1)
  return NS_ERROR_FAILURE;
group = Substring(uriStr, groupSeparator + 1,
                          keySeparator - groupSeparator - 1);
// or Substring(StringHead(uriStr, keySeparator), groupSeparator + 1)
Comment 88 John David Galt 2012-01-05 12:12:16 PST
This fix appears to have caused bug 702038 (which see).
Comment 89 Jim Porter (:squib) 2012-01-05 12:30:39 PST
(In reply to John David Galt from comment #88)
> This fix appears to have caused bug 702038 (which see).

That bug is filed against version 8.0. This bug is only fixed in 11.0+.
Comment 90 Jim Porter (:squib) 2012-01-05 12:40:07 PST
(In reply to Jim Porter (:squib) from comment #89)
> (In reply to John David Galt from comment #88)
> > This fix appears to have caused bug 702038 (which see).
> 
> That bug is filed against version 8.0. This bug is only fixed in 11.0+.

Actually, some of the parts appear to have been checked in prior to 11.0, so we should probably narrow things down to the individual parts that could affect that bug.
Comment 91 Ache 2012-03-21 15:14:00 PDT
(In reply to Jim Porter (:squib) from comment #90)
> > That bug is filed against version 8.0. This bug is only fixed in 11.0+.
> 
> Actually, some of the parts appear to have been checked in prior to 11.0, so
> we should probably narrow things down to the individual parts that could
> affect that bug.

Under Win7 32bit TB 11.0 mentioned just brings its window in front (and may additionally start itself, if not started yet), but _nothing_ happens afterwards on both news: and nntp: URLs. When called manually
TB.exe -osint -mail News_URI
(as nntp: and news: protocol handler says) the same thing happens. If I call it as
TB.exe -news News_URI
URLs subscription works as supposed, but additional TB copy is started instead using already running one.
Comment 92 David E. Ross 2012-04-27 14:04:20 PDT
The failure to fix bug #617287 makes the fix under this bug generally useless.  URIs of the form <news:rec.gardens> are interpreted as <news:///rec.gardens> in violation of RFC 5538.
Comment 93 Lu Wei 2015-12-08 02:57:23 PST
This bug is NOT fixed. What's the relation of this bug to bug 617287?
Comment 94 Lu Wei 2015-12-08 03:08:11 PST
Another bug situation: news:Message-id
like this:
news:mailman.8161.1449216174.18044.support-thunderbird@lists.mozilla.org
Click the link will crash TB. So I think the importance should be raised to major.
Comment 95 Lu Wei 2015-12-08 03:10:27 PST
Oh maybe critical.
Comment 96 Ache 2015-12-08 04:07:36 PST
(In reply to Lu Wei from comment #94)
> Another bug situation: news:Message-id
> like this:
> news:mailman.8161.1449216174.18044.support-thunderbird@lists.mozilla.org
> Click the link will crash TB. So I think the importance should be raised to
> major.

Under Win10 64bit TB 38.4.0 I have no crash clicking that, but exact as I already describe before for Win7 32bit and TB 11: TB just brings its window in front (and may additionally start itself, if not started yet), but _nothing_ happens afterwards.
Comment 97 Kent James (:rkent) 2015-12-08 11:20:09 PST
A bug like that that had a lot of stuff landed years ago is not a good location for new comments. Please open a new bug if there are still issues with these same symptoms.
Comment 98 Ache 2015-12-08 12:23:27 PST
(In reply to Kent James (:rkent) from comment #97)
> A bug like that that had a lot of stuff landed years ago is not a good
> location for new comments. Please open a new bug if there are still issues
> with these same symptoms.

Already open, see https://bugzilla.mozilla.org/show_bug.cgi?id=745856

Note You need to log in before you can comment on or make changes to this bug.