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"

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.

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.

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

Borked URL handling is never good.

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.

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

(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.

See bug #89939 for a related issue.

“ 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 ”.

(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

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

(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.

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.

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.

Ellermann's draft RFC is now official as RFC 5538 at <http://www.rfc-editor.org/rfc/rfc5538.txt>.

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.

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.

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.

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.

Well, except for the message ID, but we take care of that in...

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

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.

This is a further extension of part 4, again in its own patch to try to keep patches smaller.

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.

This makes nntp: URLs in particular works, and it also adds tests for other bugs fixed somewhere in this pile of patches.

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.

Wrong patch... That's what I get for naming them morefix and parsefix...

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 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

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".

(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.

(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 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)

Any reason you haven't landed the patches that have reviews?

(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.

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.

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.

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.

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

last time I tried, I couldn't get the patch for part 4 to apply by itself.

Turns out I need to post an updated version of this patch to get everyone else to apply correctly.

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;

(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.

> IBTD: Erm, actually, I agree. :-/

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

Updating patch 6, carrying sr+ from bienvenu.

... and part 8.

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

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

Carrying forward sr+ from bienvenu.

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).

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 on attachment 533343 [details] [diff] [review] Part 10: Make command-line news handling work this no longer applies cleanly - can you refresh? Thx!

This is the latest patch I have applied...

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?

(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 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.

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

Updating patch to tip; also, pinging Neil for sr.

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.

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.

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

The final patch has been checked in as changeset e57a9c880238; time to mark this bug as closed.

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.

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

(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.

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)

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

(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+.

(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.

(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.

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.

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

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.

Oh maybe critical.

(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.

(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