[mozTXTToHTMLConv] fails for links with ipv6 addresses

RESOLVED FIXED in Thunderbird 15.0


MailNews Core
14 years ago
5 years ago


(Reporter: Biesinger, Assigned: Magnus Melin)


Thunderbird 15.0
Bug Flags:
wanted-thunderbird +
in-testsuite +

Thunderbird Tracking Flags




(2 attachments, 1 obsolete attachment)

I got bugmail about an IPv6 bug, it contained an address like this:

mailnews turned that into a link pointing to:

the link should've pointed to the entire ip address.

Comment 1

14 years ago
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).
Summary: automatic "linkifier" fails for ipv6 addresses → [mozTXTToHTMLConv] fails for ipv6 addresses

Comment 2

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

"   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."
and yes, a missing ] would cause this:
js> uri = ios.newURI("http://[200:1:2:3", "", null);
[xpconnect wrapped nsIURI]
js> uri.spec
js> uri = ios.newURI("http://[200:1:2:3]", "", null);
[xpconnect wrapped nsIURI]
js> uri.spec

Comment 5

14 years ago
> 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.
Summary: [mozTXTToHTMLConv] fails for ipv6 addresses → [mozTXTToHTMLConv] fails for links with ipv6 addresses
> Is there a place where I can see which RFCs update a certain RFC?

sorry... don't know

Comment 7

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

14 years ago
So, this bug should be reassigned to Necko?  Or should a dependent bug be filed?
john: as comment 5 says "Yes, not Necko's fault.", necko would be the wrong
component for this bug

Comment 10

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

14 years ago
Oops, I thought it was JS code, not C++. Maybe PR_StringToNetAddr could be used
Product: Browser → Seamonkey

Comment 12

13 years ago
Moving a bunch of mozTXTToHTMLConv bugs into Core / MailNews Backend to live 
with their bretheren.


13 years ago
Component: MailNews: Main Mail Window → MailNews: Backend
Product: Mozilla Application Suite → Core
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody


10 years ago
OS: Linux → All
QA Contact: esther → backend
Hardware: PC → All
Duplicate of this bug: 409248
Product: Core → MailNews Core
Duplicate of this bug: 719158
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.
tracking-thunderbird12: --- → ?
Not tracking for 12, though we would like to see this fixed.
tracking-thunderbird12: ? → -
Flags: wanted-thunderbird+

Comment 18

5 years ago

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.
Assignee: nobody → mkmelin+mozilla

Comment 19

5 years ago
Created attachment 618778 [details] [diff] [review]
proposed fix

Allow IPv6 addresses, add some tests.
Attachment #618778 - Flags: review?(ben.bucksch)

Comment 20

5 years ago

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

- 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

Comment 21

5 years ago
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.
Attachment #618778 - Attachment is obsolete: true
Attachment #623411 - Flags: review?(ben.bucksch)
Attachment #618778 - Flags: review?(ben.bucksch)


5 years ago
Attachment #623411 - Attachment description: proposed fix v2 (for checkin) → proposed fix v2

Comment 22

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

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

5 years ago
Magnus is right; the style rules are quite clear on that.


5 years ago
Attachment #623411 - Flags: review?(ben.bucksch) → review+


5 years ago
Attachment #623411 - Flags: superreview?(cbiesinger)
Comment on attachment 623411 [details] [diff] [review]
proposed fix v2

sorry for the delay!
Attachment #623411 - Flags: superreview?(cbiesinger) → superreview+

Comment 26

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

5 years ago
Flags: in-testsuite+
Target Milestone: --- → Thunderbird 15.0
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.