Last Comment Bug 223681 - [mozTXTToHTMLConv] fails for links with ipv6 addresses
: [mozTXTToHTMLConv] fails for links with ipv6 addresses
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Thunderbird 15.0
Assigned To: Magnus Melin
:
Mentors:
: 409248 719158 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-10-25 12:57 PDT by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2012-05-29 10:21 PDT (History)
10 users (show)
standard8: wanted‑thunderbird+
mkmelin+mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-


Attachments
proposed fix (7.49 KB, patch)
2012-04-26 13:16 PDT, Magnus Melin
no flags Details | Diff | Review
proposed fix v2 (7.28 KB, patch)
2012-05-12 03:38 PDT, Magnus Melin
ben.bucksch: review+
cbiesinger: superreview+
Details | Diff | Review
General URL testcases. Not tests for this bug, but regression tests. (3.78 KB, message/rfc822)
2012-05-21 15:50 PDT, Ben Bucksch (:BenB)
no flags Details

Description Christian :Biesinger (don't email me, ping me on IRC) 2003-10-25 12:57:45 PDT
I got bugmail about an IPv6 bug, it contained an address like this:
http://[2001:200:0:8002:203:47ff:fea5:3085]/

mailnews turned that into a link pointing to:
http://[2001:200/

the link should've pointed to the entire ip address.
Comment 1 Ben Bucksch (:BenB) 2003-10-26 03:57:46 PST
I'm not sure, but I suspect the problem to be in Necko. IIRC, the converter
should make the link text identical to the URL found. So, I guess the difference
comes from the URL parser stripping a part. Somebody would have to confirm that
by adding printf statements or using a debugger (I personally don't care enough).
Comment 2 Ben Bucksch (:BenB) 2003-10-26 04:05:42 PST
Actually, the last ] is not made part of the URL, so that's probably why Necko
fails (assuming it does).

However, ] and [ are disallowed in URLs, IIRC, per RFC2396. So, is this a valid
URL in the first place? If so who says that and how does fit with RFC2396?
Comment 3 Christian :Biesinger (don't email me, ping me on IRC) 2003-10-26 04:15:35 PST
http://www.faqs.org/rfcs/rfc2732.html

"   This document updates the generic syntax for Uniform Resource
   Identifiers defined in RFC 2396 [URL].  It defines a syntax for IPv6
   addresses and allows the use of "[" and "]" within a URI explicitly
   for this reserved purpose."
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2003-10-26 04:21:36 PST
and yes, a missing ] would cause this:
js> uri = ios.newURI("http://[200:1:2:3", "", null);
[xpconnect wrapped nsIURI]
js> uri.spec
http://[200:1/
js> uri = ios.newURI("http://[200:1:2:3]", "", null);
[xpconnect wrapped nsIURI]
js> uri.spec
http://[200:1:2:3]/
Comment 5 Ben Bucksch (:BenB) 2003-10-26 04:28:24 PST
> http://www.faqs.org/rfcs/rfc2732.html

OK, thanks, that explains it.

Is there a place where I can see which RFCs update a certain RFC?

> and yes, a missing ] would cause this

Yes, not Necko's fault.


So, the converter needs an update. Either we allow [] only for this purpose, as
the RFC says, but that could need a chunk of code, or we allow [] generally,
slightly against the RFC.
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2003-10-26 04:48:48 PST
> Is there a place where I can see which RFCs update a certain RFC?

sorry... don't know
Comment 7 John G. Myers 2003-11-12 22:55:37 PST
http://www.ietf.org/iesg/1rfc_index.txt contains information about which RFCs
update other RFCs, but it is incomplete.  I have sent email to the RFC editor
asking for this particular reference to be added to the RFC index.

Comment 8 John G. Myers 2003-11-12 22:57:10 PST
So, this bug should be reassigned to Necko?  Or should a dependent bug be filed?
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2003-11-13 02:41:32 PST
john: as comment 5 says "Yes, not Necko's fault.", necko would be the wrong
component for this bug
Comment 10 Lorenzo Colitti 2004-01-20 06:28:24 PST
Where would this change have to be made?

A simple regex match like \[A-Fa-f0-9:\]{0,40} should catch most cases...
Comment 11 Lorenzo Colitti 2004-01-20 06:43:35 PST
Oops, I thought it was JS code, not C++. Maybe PR_StringToNetAddr could be used
instead...
Comment 12 Mike Cowperthwaite 2004-12-02 11:05:01 PST
Moving a bunch of mozTXTToHTMLConv bugs into Core / MailNews Backend to live 
with their bretheren.
Comment 13 (not reading, please use seth@sspitzer.org instead) 2007-06-21 15:19:15 PDT
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Comment 14 Phil Ringnalda (:philor) 2007-12-20 13:44:08 PST
*** Bug 409248 has been marked as a duplicate of this bug. ***
Comment 15 Ludovic Hirlimann [:Usul] 2012-01-19 01:18:45 PST
*** Bug 719158 has been marked as a duplicate of this bug. ***
Comment 16 Ludovic Hirlimann [:Usul] 2012-01-19 01:21:57 PST
Probably a bit late for TB12, but this year will see the launch of ipv6 (in June) Wondering if we should fix this by then.
Comment 17 Mark Banner (:standard8) 2012-04-03 12:44:45 PDT
Not tracking for 12, though we would like to see this fixed.
Comment 18 Magnus Melin 2012-04-25 11:12:54 PDT
Taking. 

BenB: I have a vague recollection you have some mail where most url variants are shown, kind of a manual test? That could be useful for creating an automated test where most versions would be checked.
Comment 19 Magnus Melin 2012-04-26 13:16:20 PDT
Created attachment 618778 [details] [diff] [review]
proposed fix

Allow IPv6 addresses, add some tests.
Comment 20 Ben Bucksch (:BenB) 2012-05-11 14:25:58 PDT
Magnus,

thanks for the nice patch. I'm mostly OK with it.

2 comments:
- Please avoid making changes that are not needed for the fix, e.g. the "else return false;" and the spaces. These things are there quite intentionally.
- Minor: Please make a comment after "seenOpeningParenthesis" // there is a ( in the URL
  ditto for [ , to make this more readable for non-English speakers like me.

Apart from that, r=BenB
Thanks!
Comment 21 Magnus Melin 2012-05-12 03:38:35 PDT
Created attachment 623411 [details] [diff] [review]
proposed fix v2

Adding the comment.

Well, no else after return is a mozilla coding style rule, i just took the opportunity to fix it since i was touching code next by - i do think overall readability of the method is important.

Re the 3-7 spaces before parenthesis i don't know why they were there, but if it's supposed to be some kind of styling its different from rest of the code as well as code in general.
Comment 22 Ben Bucksch (:BenB) 2012-05-12 06:36:49 PDT
> i do think overall readability of the method is important.

I agree, and I think if (foo) return 1; else return 2; is more readable.
You still haven't removed this change. Please do.
Comment 23 Magnus Melin 2012-05-12 10:26:54 PDT
Ben: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide item #1
"Don't put an else right after a return. Delete the else, it's unnecessary and increases indentation level."

(Yes there are other places where it should be fixed too, but this happened to be in close by, i'm not so keen on making a style cleanup patch for the whole file.)
Comment 24 David :Bienvenu 2012-05-12 10:31:02 PDT
Magnus is right; the style rules are quite clear on that.
Comment 25 Christian :Biesinger (don't email me, ping me on IRC) 2012-05-21 15:18:42 PDT
Comment on attachment 623411 [details] [diff] [review]
proposed fix v2

sorry for the delay!
Comment 26 Ben Bucksch (:BenB) 2012-05-21 15:50:08 PDT
Created attachment 625805 [details]
General URL testcases. Not tests for this bug, but regression tests.

Magnus, in comment 18, you asked for my old testcases. Sorry for the delay. Attached here, as RFC822 message file. I don't expect any problems, but would be nice to 1) check them before commit and 2) add them to the unit test that you added. Thanks for the unit test, BTW!
Comment 28 Ed Morley [:emorley] 2012-05-29 10:21:16 PDT
https://hg.mozilla.org/mozilla-central/rev/58227b467522

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