1274602 - When inserting a link into HTML composition, the text part sometimes gets linkified upon send resulting in a double link. Regression from bug 964024.

Status: RESOLVED FIXED

Description

[Jorg K (GMT+2)]

This bug started originally as bug 1274242 were we discovered that linkify stops at the first | it finds.

Hoever, that was not what the reporter of bug 1274242 had originally reported. He said that after upgrading to TB 45, the following problem occurs:

STR:
- enter
    http://www.test.com&tag=a||b
  into the body of an email.
- highlight the link and click Insert ==> Link (<ctrl> L).
- The long link turns all blue.
- Send the email (or just sent to outbox, <ctrl><shift><enter>).

Works in TB 31, but not TB 38 and TB 45.

Alice, could you please find the regression. Thanking you in advance.

Comment 1

[Jorg K (GMT+2)]

Sorry, I got confused. Test case is: http://www.test.com&tag=//a||b.

Comment 2

[Jorg K (GMT+2)]

This is about the weirdest thing I've seen in a long time. I tried:
http://www.test.com?tag=//a|b and sometimes it works in TB 38, sometimes it doesn't.

What always fails in TB 38 is:
http://www.mappingsupport.com/p/gmap4.php?t=aerial&label=on&tilt=off&markers=//Not||description=plm2

Alice, when testing this, please make sure that the e-mail gets sent in HTML format since the auto-downgrade to plain text causes more confusion.

Comment 3

[Joseph]

Here is a full link that opens the map.  I adjusted the t parameter.
This will not make any difference for purpose of testing TB.
But I did want to provide a working link.

http://www.mappingsupport.com/p/gmap4.php?t=s&label=on&tilt=off&markers=//Not_a_survey___Coordinates_are_approximate||description=plm2||line=on||44.810123,-62.871949^1||44.807591,-62.874422^2||44.806832,-62.874392^3||44.807001,-62.874934^4||44.807466,-62.874531^5||44.807781,-62.875128^6||44.80779,-62.875145^7||44.807846,-62.875091^8||44.810321,-62.872686^9

Comment 4

[Jorg K (GMT+2)]

Something has gone terribly wrong here.

TB 31 inserts this:
<a href="http://www.mappingsupport.com/p/gmap4.php?t=aerial&amp;label=on&amp;tilt=off&amp;markers=//Not%7C%7Cdescription=plm2">
http://www.mappingsupport.com/p/gmap4.php?t=aerial&amp;label=on&amp;tilt=off&amp;markers=//Not||description=plm2</a>

TB 38 inserts:
<a href="http://www.mappingsupport.com/p/gmap4.php?t=aerial&amp;label=on&amp;tilt=off&amp;markers=//Not%7C%7Cdescription=plm2">
<a class="moz-txt-link-freetext"
href="http://www.mappingsupport.com/p/gmap4.php?t=aerial&amp;label=on&amp;tilt=off&amp;markers=//Not">
http://www.mappingsupport.com/p/gmap4.php?t=aerial&amp;label=on&amp;tilt=off&amp;markers=//Not</a>||description=plm2</a>

Take a good look:
The original link is there and complete. But Linkify was run on the text part of the link producing yet another link and some link text. Both the inner link and its text are wrong.

Let me restate it in some other way:
TB 31 leaves the original link alone: <a href="http://www.example.com">http://www.example.com</a>

From TB 38 we linkify the text as well, and the result is:
<a href="http://www.example.com"><a href="http://www.example.com">http://www.example.com</a></a>
Note that this example doesn't trigger the bug.

Since the linkify stops at the first "|" due to bug 1274242, we get bad inner links and bad text.

So I'll change the bug summary.

Comment 5

[Jorg K (GMT+2)]

Joseph, I hope you follow. Your report detected two problems:
Bug 1274242: Linkify stops at "|" where it shouldn't. That never worked from day one.
This bug here: Text of links inserted manually gets linkified where it shouldn't thus exposing the faulty behaviour in the other bug. This linkifying of link text is wrong.
No erroneous linkifying took place in TB 31, but later versions were affected.

Comment 6

[Joseph]

Yes, I follow.

Since the map links I send to clients are complex I have always used insert ==> link.
It is possible I tried to rely on linkify way back when, saw that it did not work, and didn't care (and didn't report a bug) since I was willing to do insert ==> link.

Linkify needs to ease off and not mess with stuff that is already a link.

Comment 7

[Jorg K (GMT+2)]

(In reply to Joseph from comment #6)
> Linkify needs to ease off and not mess with stuff that is already a link.
That is exactly the point. But we need to find where that changed somewhere between TB 31 and TB 38.

The thing is, that this was reported before in bug 1240418, but we just brushed it off. With the observation that it works in TB 31 and not TB 38 we can get to the cause of the problem quicker, I hope.

Comment 9

[Jorg K (GMT+2)]

OK, looking at the log of the source file in question:
https://hg.mozilla.org/mozilla-central/log/tip/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp

That code is almost static with only a bit of refactoring taking place, however, bug 964024 landed there, and it landed right on TB 38, meaning that TB 38 would have been the first broken version.

That also modified the link processing <a> so I put my bet on that ;-)
https://hg.mozilla.org/mozilla-central/rev/a8c47d8abcba

Comment 10

[Alice0775 White]

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c2359a6a6958&tochange=940118b1adcd
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=338986d73c02&tochange=f0194350ebf3

Suspect : https://hg.mozilla.org/mozilla-central/rev/a8c47d8abcba

Comment 11

[Jorg K (GMT+2)]

Thanks, Alice. We both came to the same result.
Magnus, you broke that somehow. I'm staring at it, but I can't see it:

-      uint32_t start = uint32_t(i);
-      if (nsCRT::ToLower((char)aInString[uint32_t(i) + 1]) == 'a')
-           // if a tag, skip until </a>
+      int32_t start = i;
+      if (Substring(aInString, i + 1, 2).LowerCaseEqualsASCII("a "))
+           // if a tag, skip until </a>.
+           // Make sure there's a space after, not to match "abbr".

OK, found it!!

The HTML gets wrapped (used ThunderHTMLedit to see it):
    <p><a
href="http://www.mappingsupport.com/p/gmap4.php?t=aerial&amp;label=on&amp;tilt=off&amp;markers=//Not%7C%7Cdescription=plm2"><tt>http://www.mappingsupport.com/p/gmap4.php?t=aerial&amp;label=on&amp;tilt=off&amp;markers=//Not||description=plm2</tt></a><br>

There is NO space behind the "<a" !!!

Comment 12

[Jorg K (GMT+2)]

Created attachment 8754967 [details] [diff] [review]
Fixing the regression from bug 964024: Not only test for space after <a

Comment 13

[Magnus Melin]

Great detective work!

Comment 14

[Joseph]

Wowie zowie - I am amazed at the light speed response.
Many thanks!

Comment 15

[Jorg K (GMT+2)]

I don't like regressions. Other changes take longer ;-)

Comment 16

[Jorg K (GMT+2)]

Created attachment 8755001 [details] [diff] [review]
Fixing the regression from bug 964024: Not only test for space after <a (v2).

This is a little more elegant.

Comment 17

[Honza Bambas (:mayhemer)]

Please explain the patch in detail.

Comment 18

[Jorg K (GMT+2)]

Sure.

Before bug 964024
https://hg.mozilla.org/mozilla-central/rev/a8c47d8abcba
we simply checked for "<a" to detect a link.

In bug 964024 that was changed to checking for "<a ".

In serialised DOM (converted to HTML as text) this fails if the text presented is not
<a href=some site ...>
but instead
<a                         <=== line break after the a
href=usually some very long link>.

See sample link at the bottom of comment #11.

So instead of checking for "<a " I now check for "<" followed by "a" followed by space, \n or \r.

Comment 19

[Magnus Melin]

Comment on attachment 8755001 [details] [diff] [review]
Fixing the regression from bug 964024: Not only test for space after <a (v2).

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

Could also (in theory) be \t or \f - https://www.w3.org/TR/html5/infrastructure.html#space-character

Could you also fix the similar "<head " case?

Comment 20

[Jorg K (GMT+2)]

Created attachment 8755141 [details] [diff] [review]
Fixing the regression from bug 964024: Not only test for space after <a (v3).

More comprehensive fix repairing the other tests as well.

Comment 21

[Jorg K (GMT+2)]

(In reply to Honza Bambas (:mayhemer) from comment #17)
> Please explain the patch in detail.
Since I made more changes, here another explanation.

Before the code tested the following strings to detect a start tag:
|<a |
|<style |
|<style>|
|<script |
|<script>|
|<head |
|<head>|

Since we had cases of |<a\r|, we decided to unify the approach and now detect
|<a_|
|<style_|
|<script_|
|<head_|
where _ can be any of space, \f, \n, \r, \t or >.

There is a text for this:
./mach xpcshell-test --verbose netwerk/test/unit/test_mozTXTToHTMLConv.js

I haven't added more test cases, but I can, if you wish.

Comment 22

[Honza Bambas (:mayhemer)]

Jorg, before I start reviewing this, would you be willing to rewrite the parsing code of this class using mozilla::Tokenizer?  It's a lexical-analyzer-like kind of tokenizer, customizable, and designed to also be derived from (when suitable) to impl a parser of any level of complexity.  It would be a bit larger task, but would make this class much more maintainable.  At least just the ScanHTML method would be enough, as it's more sensitive than just text walk through.

If not, I will jump on the review.

Comment 23

[Jorg K (GMT+2)]

We need this bug fixed quickly since it is a clear regression of bug 964024 and we have users who can't include the links they need into their messages.

As you can see here
https://hg.mozilla.org/mozilla-central/log/tip/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
this code is rather dormant.

The only use of it I could find in M-C is in the spell checker
https://dxr.mozilla.org/comm-central/source/mozilla/extensions/spellcheck/src/mozEnglishWordUtils.cpp#177
and then it's also part of Necko (but I don't know how much it's used there). I've actually played with the idea to move the code to comm-central, but since it's used in M-C that's out of the question.

So what I'm trying to say is that in the short term we need to land this small fix. In the long term we can think about rewriting it. However, I'm a Thunderbird developer (compose peer), so my efforts need to go into TB.

So please proceed with the review.

Comment 24

[Honza Bambas (:mayhemer)]

Good rational, thanks.  Going to review.

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8755141 [details] [diff] [review]
Fixing the regression from bug 964024: Not only test for space after <a (v3).

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

unhappily giving r+

::: netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
@@ +1234,5 @@
>    {
>      if (aInString[i] == '<')  // html tag
>      {
>        int32_t start = i;
> +      if (i+2 < lengthOfInString &&

i + 2 (spaces around signs, applies on more places too)

can overflow when MAX_UINT32 - 2 < i, also applies to other places...  exactly what Tokenizer takes care of, between else..

@@ +1236,5 @@
>      {
>        int32_t start = i;
> +      if (i+2 < lengthOfInString &&
> +          nsCRT::ToLower(aInString[i+1]) == 'a' &&
> +          canFollow.FindChar(aInString[i+2]) >= 0)

!= kNotFound


so, let's assume a string "<a":
- i == 0
- lengthOfInString == 2
- i + 2 == 2
- i + 2 < lengthOfInString --> 2 < 2 == false

so, we are safe


personally I hate these maths.. but comment 23 has good arguments to leave it in this horrible form.

Comment 26

[Jorg K (GMT+2)]

Thanks for the quick review. Since I'm not at my desk right now, I'll fix the review issues tomorrow.

(In reply to Honza Bambas (:mayhemer) from comment #25)
> personally I hate these maths.. 
There really are no maths involved. If we want to access at index (i+k) then i+k simply needs to be < length ;-)

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b05086812022

Comment 28

[Jorg K (GMT+2)]

Created attachment 8756317 [details] [diff] [review]
Fixing regression from bug 964024: Test for white-space after <a and other tags (v4)

Fixed review issues.

Carrying forward Honza Bambas' r+

Comment 29

[Jorg K (GMT+2)]

Dear Sheriff,
I know that you'd like to see a successful try run, but I don't have one.
This code is tested via
./mach xpcshell-test --verbose netwerk/test/unit/test_mozTXTToHTMLConv.js
which I have run locally. Of course I made sure it compiles, too ;-)
Please land the patch.

Comment 30

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/b05086812022

Comment 31

[Jorg K (GMT+2)]

Kent, please include into TB 45.x at your earliest convenience. This is a regression and our users can't include the links they want into their e-mail even if they add the link manually. There is one duplicate of this. The change is harmless, I just changed the comparison a little.

Three people (including myself) looked at it and there is an automated test showing that we didn't break anything.

Comment 32

[Kent James (:rkent)]

Pushed to a release branch on mozilla-beta for Thunderbird 47.0b1

Comment 33

[Kent James (:rkent)]

Pushed http://hg.mozilla.org/releases/mozilla-esr45/rev/c18c3446bd34 to THUNDERBIRD_45_VERBRANCH

Comment 34

[Jorg K (GMT+2)]

Pushed to a release branch on mozilla-beta for Thunderbird 48.0b1
https://hg.mozilla.org/releases/mozilla-beta/rev/f4165cb03763

Comment 35

[Jorg K (GMT+2)]

Pushed to a release branch on mozilla-beta for Thunderbird 48.0b1 - take 2:
https://hg.mozilla.org/releases/mozilla-beta/rev/69f8c8df54e4

Comment 36

[Joseph]

Thanks to all involved for fixing this bug.  I am the reporter.
I am now using Tunderbird 45.2.0 and I can email my clients links like the following:

https://mappingsupport.com/p/gmap4.php?t=h&label=on&tilt=off&markers=//Not_a_survey___Coordinates_are_approximate||description=plm2||label=on||line=on||37.320377,-122.267308^1||37.320395,-122.266774^2||37.320126,-122.266792^3||37.320122,-122.26672^4||37.320173,-122.266717^5||37.320019,-122.266137^6||37.319689,-122.26643^7||37.319923,-122.266733^8||37.320377,-122.267308||line=on||37.320122,-122.26672||37.319923,-122.266733

I am manually making this into a link when I compose my email and linkify no longer breaks the link.

I note that Gmail also allows me to use links like the above.  However I prefer to use Thunderbird when communicating with clients.

Joseph Elfelt
https://MappingSupport.com

Comment 37

[Jorg K (GMT+2)]

If you like the service, please consider a donation ;-)
https://donate.mozilla.org/en-US/thunderbird/about/

Comment 38

[Joseph]

Donation sent.
Thanks again!

Joseph