The default bug view has changed. See this FAQ.

Thunderbird doesn't handle news: and nntp: URLs properly (RFC 5538)

RESOLVED FIXED in Thunderbird 11.0

Status

MailNews Core
Networking: NNTP
RESOLVED FIXED
14 years ago
a year ago

People

(Reporter: Robert Marshall, Assigned: jcranmer)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
Thunderbird 11.0
Dependency tree / graph
Bug Flags:
blocking-aviary1.5 -
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(11 attachments, 12 obsolete attachments)

15.08 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
17.09 KB, patch
standard8
: review+
Details | Diff | Splinter Review
2.26 KB, patch
standard8
: review+
Details | Diff | Splinter Review
15.46 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
35.78 KB, patch
Bienvenu
: review+
standard8
: superreview+
Details | Diff | Splinter Review
21.14 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
8.00 KB, patch
standard8
: review+
Details | Diff | Splinter Review
20.35 KB, patch
standard8
: review+
jcranmer
: superreview+
Details | Diff | Splinter Review
3.04 KB, patch
standard8
: review+
Details | Diff | Splinter Review
9.64 KB, patch
standard8
: review+
Details | Diff | Splinter Review
12.00 KB, patch
jcranmer
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
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

13 years ago
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

13 years ago
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.
Component: Preferences → General
Hardware: PC → All
Summary: When clicking on a news: URL Thunderbird is launched, however, the newsgroup is not openned. → Thunderbird doesn't handle news URLs properly

Comment 3

13 years ago
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

12 years ago
Borked URL handling is never good.
Flags: blocking-aviary1.1?

Updated

12 years ago
Flags: blocking-aviary1.1? → blocking-aviary1.1-

Comment 5

12 years ago
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

11 years ago
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

10 years ago
(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.
QA Contact: general

Comment 8

10 years ago
See bug #89939 for a related issue.  

Updated

10 years ago
Assignee: mscott → nobody
Summary: Thunderbird doesn't handle news URLs properly → Thunderbird doesn't handle news: URLs properly

Comment 9

8 years ago
“ 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

8 years ago
(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

8 years ago
RFC 1738 is no longer current.  In the RFC index, it is listed as "Obsolete".

Comment 12

8 years ago
(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

8 years ago
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.
(Assignee)

Comment 14

8 years ago
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

7 years ago
Ellermann's draft RFC is now official as RFC 5538 at <http://www.rfc-editor.org/rfc/rfc5538.txt>.
Component: General → Networking: NNTP
Product: Thunderbird → MailNews Core
QA Contact: general → networking.nntp
(Assignee)

Updated

7 years ago
Duplicate of this bug: 601564

Updated

7 years ago
Summary: Thunderbird doesn't handle news: URLs properly → Thunderbird doesn't handle news: and nttp: URLs properly (RFC 5538)

Updated

7 years ago
Duplicate of this bug: 603671

Comment 18

7 years ago
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.

Updated

7 years ago

Updated

7 years ago
Duplicate of this bug: 175292
(Assignee)

Comment 20

7 years ago
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.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
(Assignee)

Updated

7 years ago
Duplicate of this bug: 607116
(Assignee)

Comment 22

6 years ago
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.
Attachment #501393 - Flags: review?(bienvenu)
(Assignee)

Comment 23

6 years ago
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.
Attachment #501396 - Flags: review?(bugzilla)
(Assignee)

Comment 24

6 years ago
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...
Attachment #501399 - Flags: review?(bienvenu)
(Assignee)

Comment 25

6 years ago
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.
Attachment #501400 - Flags: superreview?(neil)
Attachment #501400 - Flags: review?(bienvenu)
(Assignee)

Comment 26

6 years ago
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.
Attachment #501401 - Flags: review?(bugzilla)
(Assignee)

Comment 27

6 years ago
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.
Attachment #501402 - Flags: superreview?(bienvenu)
Attachment #501402 - Flags: review?(neil)
(Assignee)

Comment 28

6 years ago
Created attachment 501404 [details] [diff] [review]
Part 7: simplify nsParseNewsMessageURI
Attachment #501404 - Flags: review?(neil)
(Assignee)

Comment 29

6 years ago
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.
Attachment #501406 - Flags: review?(neil)
(Assignee)

Comment 30

6 years ago
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.
Attachment #501407 - Flags: review?(bienvenu)
(Assignee)

Comment 31

6 years ago
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.
Attachment #501410 - Flags: review?(bugzilla)
(Assignee)

Comment 32

6 years ago
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...
Attachment #501407 - Attachment is obsolete: true
Attachment #501412 - Flags: review?(bienvenu)
Attachment #501407 - Flags: review?(bienvenu)

Updated

6 years ago
Attachment #501393 - Flags: review?(bienvenu) → review+
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.
Attachment #501396 - Flags: review?(bugzilla) → review+

Comment 34

6 years ago
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
Attachment #501399 - Flags: review?(bienvenu) → review+
Duplicate of this bug: 224335

Comment 36

6 years ago
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".
(Assignee)

Comment 37

6 years ago
(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 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.
Attachment #501401 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 39

6 years ago
(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

6 years ago
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)
Attachment #501400 - Flags: review?(bienvenu) → review-

Comment 41

6 years ago
Any reason you haven't landed the patches that have reviews?
(Assignee)

Comment 42

6 years ago
(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.
Blocks: 108107
(Assignee)

Updated

6 years ago
Blocks: 403242

Comment 43

6 years ago
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.
(Assignee)

Comment 44

6 years ago
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

6 years ago
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.
(Assignee)

Comment 46

6 years ago
Parts 1 and 2 checked in:
<http://hg.mozilla.org/comm-central/pushloghtml?changeset=78440aa35ff6>.
(Assignee)

Updated

6 years ago
Attachment #501393 - Attachment description: Part 1: Clean up some unimplemented junk in NNTP code → Part 1: Clean up some unimplemented junk in NNTP code [Checked in]
(Assignee)

Updated

6 years ago
Attachment #501396 - Attachment description: Part 2: Implement news URI tests → Part 2: Implement news URI tests [Checked in]
(Assignee)

Comment 47

6 years ago
Created attachment 510264 [details] [diff] [review]
Part 4: Move the parsing code to nsNntpUrl [Checked in]
Attachment #501400 - Attachment is obsolete: true
Attachment #510264 - Flags: superreview?(neil)
Attachment #510264 - Flags: review?(bienvenu)
Attachment #501400 - Flags: superreview?(neil)
(Assignee)

Comment 48

6 years ago
Created attachment 510265 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well
Attachment #501402 - Attachment is obsolete: true
Attachment #510265 - Flags: superreview?(bienvenu)
Attachment #510265 - Flags: review?(neil)
Attachment #501402 - Flags: superreview?(bienvenu)
Attachment #501402 - Flags: review?(neil)
(Assignee)

Updated

6 years ago
Blocks: 498321

Comment 49

6 years ago
last time I tried, I couldn't get the patch for part 4 to apply by itself.
(Assignee)

Comment 50

6 years ago
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.
Attachment #501399 - Attachment is obsolete: true
Attachment #512922 - Flags: review+

Comment 51

6 years ago
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;
Attachment #510264 - Flags: review?(bienvenu) → review+

Updated

6 years ago
Attachment #501412 - Flags: review?(bienvenu) → review+

Comment 52

6 years ago
(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

6 years ago
> IBTD: 

Erm, actually, I agree. :-/

Comment 54

6 years ago
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);
Attachment #510265 - Flags: superreview?(bienvenu) → superreview+
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.
Attachment #501410 - Flags: review?(bugzilla)
(Assignee)

Comment 56

6 years ago
Created attachment 524676 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well

Updating patch 6, carrying sr+ from bienvenu.
Attachment #510265 - Attachment is obsolete: true
Attachment #524676 - Flags: superreview+
Attachment #524676 - Flags: review?(neil)
Attachment #510265 - Flags: review?(neil)
(Assignee)

Comment 57

6 years ago
Created attachment 524678 [details] [diff] [review]
Part 8: Only initialize m_nntpServer once

... and part 8.
Attachment #501406 - Attachment is obsolete: true
Attachment #524678 - Flags: review?(neil)
Attachment #501406 - Flags: review?(neil)
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).
Attachment #510264 - Flags: superreview?(neil) → superreview+
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.
Attachment #524676 - Flags: review?(neil)
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.
Attachment #501404 - Flags: review?(neil) → review?(mbanner)
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.
Attachment #524678 - Flags: review?(neil) → review?(mbanner)
(Assignee)

Comment 62

6 years ago
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 π ;-)
Attachment #530086 - Flags: review?(mbanner)
Attachment #530086 - Flags: review?(mbanner) → review+
(Assignee)

Updated

6 years ago
Attachment #512922 - Attachment description: Part 3: Introduce nsCString to the URL parser → Part 3: Introduce nsCString to the URL parser [checked in]
(Assignee)

Comment 63

6 years ago
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>.
Attachment #501401 - Attachment description: Part 5: Test the URI parser → Part 5: Test the URI parser [checked in]
(Assignee)

Updated

6 years ago
Attachment #510264 - Attachment description: Part 4: Move the parsing code to nsNntpUrl → Part 4: Move the parsing code to nsNntpUrl [Checked in]
(Assignee)

Updated

6 years ago
Attachment #530086 - Attachment description: Part 4.5: CStringify m_searchData → Part 4.5: CStringify m_searchData [Checked in]
(Assignee)

Comment 64

6 years ago
Created attachment 530278 [details] [diff] [review]
Part 6: Move the message key to nsNntpUrl as well [Checked-in]

Carrying forward sr+ from bienvenu.
Attachment #524676 - Attachment is obsolete: true
Attachment #530278 - Flags: superreview+
Attachment #530278 - Flags: review?(mbanner)
(Assignee)

Comment 65

6 years ago
Created attachment 530279 [details] [diff] [review]
Part 7: simplify nsParseNewsMessageURI [Checked in]
Attachment #501404 - Attachment is obsolete: true
Attachment #530279 - Flags: review?(mbanner)
Attachment #501404 - Flags: review?(mbanner)
(Assignee)

Comment 66

6 years ago
Created attachment 530281 [details] [diff] [review]
Part 8: Only initialize m_nntpServer once [Checked in]
Attachment #524678 - Attachment is obsolete: true
Attachment #530281 - Flags: review?(mbanner)
Attachment #524678 - Flags: review?(mbanner)
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).
Attachment #530278 - Flags: review?(mbanner) → review+
Version: unspecified → Trunk
Attachment #530279 - Flags: review?(mbanner) → review+
Attachment #530281 - Flags: review?(mbanner) → review+
(Assignee)

Comment 68

6 years ago
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.
Attachment #501410 - Attachment is obsolete: true
Attachment #533343 - Flags: review?(mbanner)
Attachment #533343 - Flags: review?(dbienvenu)

Comment 69

6 years ago
Comment on attachment 533343 [details] [diff] [review]
Part 10: Make command-line news handling work

this no longer applies cleanly - can you refresh? Thx!
Attachment #533343 - Flags: review?(dbienvenu)
(Assignee)

Comment 70

6 years ago
Created attachment 541868 [details] [diff] [review]
Part 10: Make command-line news handling work

This is the latest patch I have applied...
Attachment #533343 - Attachment is obsolete: true
Attachment #541868 - Flags: superreview?(neil)
Attachment #541868 - Flags: review?(mbanner)
Attachment #541868 - Flags: review?(dbienvenu)
Attachment #533343 - Flags: review?(mbanner)
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?
(Assignee)

Comment 72

6 years ago
(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

6 years ago
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.
Attachment #541868 - Flags: review?(dbienvenu)
(Assignee)

Updated

6 years ago
Attachment #530278 - Attachment description: Part 6: Move the message key to nsNntpUrl as well → Part 6: Move the message key to nsNntpUrl as well [Checked-in]
(Assignee)

Updated

6 years ago
Attachment #501412 - Attachment description: Part 9: Parser fixups and more testing → Part 9: Parser fixups and more testing [Checked in]
(Assignee)

Updated

6 years ago
Attachment #530279 - Attachment description: Part 7: simplify nsParseNewsMessageURI → Part 7: simplify nsParseNewsMessageURI [Checked in]
(Assignee)

Updated

6 years ago
Attachment #530281 - Attachment description: Part 8: Only initialize m_nntpServer once → Part 8: Only initialize m_nntpServer once [Checked in]
(Assignee)

Updated

6 years ago
Attachment #541868 - Flags: review?(dbienvenu)

Comment 74

6 years ago
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

6 years ago
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.
Attachment #541868 - Flags: review?(dbienvenu) → review+
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.
Attachment #541868 - Flags: review?(mbanner)

Updated

6 years ago
Blocks: 694915
(Assignee)

Comment 77

6 years ago
Created attachment 572163 [details] [diff] [review]
Part 10: Make command-line news handling work

Updating patch to tip; also, pinging Neil for sr.
Attachment #541868 - Attachment is obsolete: true
Attachment #572163 - Flags: superreview?(neil)
Attachment #572163 - Flags: review+
Attachment #541868 - Flags: superreview?(neil)
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.
Attachment #572163 - Flags: superreview?(neil) → superreview+

Comment 79

5 years ago
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.
(Assignee)

Comment 80

5 years ago
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
(Assignee)

Comment 81

5 years ago
The final patch has been checked in as changeset e57a9c880238; time to mark this bug as closed.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
Summary: Thunderbird doesn't handle news: and nttp: URLs properly (RFC 5538) → Thunderbird doesn't handle news: and nntp: URLs properly (RFC 5538)

Comment 82

5 years ago
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

5 years ago
Someone please tell me what are the other bugs cited in comment #37 and comment #44.

Updated

5 years ago
Depends on: 705471
(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

5 years ago
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 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 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)
(Assignee)

Updated

5 years ago
Blocks: 37465

Comment 88

5 years ago
This fix appears to have caused bug 702038 (which see).

Comment 89

5 years ago
(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

5 years ago
(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

5 years ago
(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

5 years ago
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

a year ago
This bug is NOT fixed. What's the relation of this bug to bug 617287?

Comment 94

a year ago
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

a year ago
Oh maybe critical.

Comment 96

a year ago
(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.
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

a year ago
(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
You need to log in before you can comment on or make changes to this bug.