1240903 - Links no longer created in compose in versions 45/46

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[MirandaGoshawk]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160115055328

Steps to reproduce:

Typed link ie http://www.google.com in compose/or via email link in Firefox and sent it to myself. 


Actual results:

When email is sent it is not possible to immediately click on the link. 


Expected results:

In all previous versions it possible to click the link once sent in the sent folder and as a recipient.

Comment 1

[Jorg K (CEST = GMT+2)]

Thanks for reporting this. I've noticed this myself, but didn't pay much attention to it.

I've just tried in TB 38 and TB 45.

TB 38:
Plain: this is a link www.google.com
HTML: this is a link <a class="moz-txt-link-abbreviated" href="http://www.google.com">www.google.com</a><br>

TB 45:
Plain: this is a link www.google.com
HTML: this is a link www.google.com<br>

This is pretty bad, when did that break? Where do we process those links? Sorry about the NI-SPAM.

Comment 2

[Jorg K (CEST = GMT+2)]

Alice, do you have time to bisect this for me. That would be much appreciated.

Comment 3

[Joe Sabash [:JoeS1]]

A couple observations:
1 Works in TB44
2 If you view>>plain text the link works
3 If you have links in a pre-formatted signature they work fine

Comment 4

[Alice0775 White]

(In reply to Jorg K (GMT+1) from comment #2)
> Alice, do you have time to bisect this for me. That would be much
> appreciated.

I tried, but it works...
Perhaps, I would not read your STR correctly.

Comment 5

[Jorg K (CEST = GMT+2)]

STR are very simple:
Write an e-maik, type:
  Link www.google.com
Set delivery option to HTML or plain+HTML.
Send the e-mail, maybe just to your outbox (control shift enter).
The sent message doesn't not contain a link.

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #1)
> This is pretty bad, when did that break? Where do we process those links?

A search for scanTXT and scanHTML should get you started. Don't recall further details off-hand.

Comment 7

[Alice0775 White]

(In reply to Jorg K (GMT+1) from comment #5)
> STR are very simple:
> Write an e-maik, type:
>   Link www.google.com
> Set delivery option to HTML or plain+HTML.
> Send the e-mail, maybe just to your outbox (control shift enter).
> The sent message doesn't not contain a link.

Ok, I can reproduced

Last Good:
https://hg.mozilla.org/mozilla-central/rev/202b199b9fcf37a687bef882f2513e191f079622
https://hg.mozilla.org/comm-central/rev/5c1bc9144b1fa10837fc034d11be60df1092ef7e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.0a1 ID:20151114030209

First Bad:
https://hg.mozilla.org/mozilla-central/rev/51fa3e0d4f7bb2bf3457261091b1cb7a75e1255d
https://hg.mozilla.org/comm-central/rev/884430e0888d72741dbc005a20bc5e9f0c88e2eb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.0a1 ID:20151115030207

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=202b199b9fcf37a687bef882f2513e191f079622&tochange=51fa3e0d4f7bb2bf3457261091b1cb7a75e1255d
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=5c1bc9144b1fa10837fc034d11be60df1092ef7e&tochange=884430e0888d72741dbc005a20bc5e9f0c88e2eb

Comment 8

[Jorg K (CEST = GMT+2)]

Thank you so much. How can you do this so fast? Amazing!!

Comment 9

[Jorg K (CEST = GMT+2)]

Just confirming:
Good: 45.0a1 (2015-11-14)
Bad: 45.0a1 (2015-11-15)

I don't want to point fingers too early but I can't see any M-C change that could have caused this. The processing is done in mozTXTToHTMLConv.cpp and I don't see any change there:
https://hg.mozilla.org/mozilla-central/log/tip/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp

The likely candidate is this ... maybe, perhaps?
https://hg.mozilla.org/comm-central/rev/15454eb97bbe

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Backing our rev 15454eb97bbe from bug 1202401 brings the links back.

This confirms the regression from bug 1202401.

Comment 11

[Jorg K (CEST = GMT+2)]

<off-topic>
Of course the backout brings nsMsgI18NSaveAsCharset() back to life which uses nsISaveAsCharset which was supposed to be removed in Q4 of 2015 (bug 1202362). Well, it's still there despite bug 1214619. I noticed that the interface has changed in https://hg.mozilla.org/mozilla-central/rev/6644bf5d558c:
-  NS_IMETHOD Init(const char *charset, uint32_t attr, uint32_t entityVersion) override;
+  NS_IMETHOD Init(const nsACString& aCharset, uint32_t aIgnored, uint32_t aAlsoIgnored) override;
-  NS_IMETHOD Convert(const char16_t *inString, char **_retval) override;
+  NS_IMETHOD Convert(const nsAString& ain, nsACString& aOut) override;
-  NS_IMETHODIMP GetCharset(char * *aCharset) override;
+  NS_IMETHODIMP GetCharset(nsACString& aCharset) override;
which my backout patch takes into account.
</off-topic>

Now that we know what caused it, and backing this out is not an option, we need a way forward.

With the backout applied I see:
mozTXTToHTMLConv::CheckURLAndCreateHTML() Line 425	C++
mozTXTToHTMLConv::FindURL() Line 564	C++
mozTXTToHTMLConv::ScanTXT() Line 1186	C++
mozTXTToHTMLConv::ScanHTML() Line 1308	C++
mozTXTToHTMLConv::ScanHTML() Line 1404	C++
nsMsgComposeAndSend::GetBodyFromEditor() Line 1579	C++
nsMsgComposeAndSend::Init() Line 3154	C++
nsMsgComposeAndSend::CreateAndSendMessage() Line 4052	C++
nsMsgCompose::SendMsgToServer(int deliverMode, nsIMsgIdentity * identity, const char * accountKey) Line 1134	C++
nsMsgCompose::SendMsg() Line 1343	C++

Without the backout it still does the right thing.

I think the logic got confused here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#1637

There were changes to this part in
https://hg.mozilla.org/comm-central/diff/15454eb97bbe/mailnews/compose/src/nsMsgSend.cpp
when shuffling around the bodies before and after the link conversion.

Comment 12

[Jorg K (CEST = GMT+2)]

Sorry, Henry got NI'ed by mistake.

Comment 13

[Jorg K (CEST = GMT+2)]

Sorry, Henri.

Comment 14

[Jorg K (CEST = GMT+2)]

Found the problem. Patch coming.
Copy/paste error:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#1598
rv = nsMsgI18NConvertFromUnicode(aCharset, bodyStr, outCString, false, true);
Must use the body with the links inserted in 'bodyText' !!

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: This fixes the cut/paste error.

I've been struggling with Mozilla strings for about a year now. So if this isn't the best way, please let me know. Magnus, you may want to take over since it's no point telling me in a comment how to best fix one line.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: This fixes the cut/paste error. Better like this?

Version without declaring an nsAutoString.

Comment 17

[Jorg K (CEST = GMT+2)]

I'm working on a test so that this doesn't break again. Coming up soon.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 8709936 [details] [diff] [review]
This fixes the cut/paste error. Better like this?

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

Thx for tracking it down, LGTM! r=mkmelin

Comment 19

[:aceman]

Magnus, why 64bit? It sseems to be a universal logic error. Jorg, is this ready for check in?

Comment 20

[Magnus Melin [:mkmelin]]

My mouse must have slipped.

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #19)
> Jorg, is this ready for check in?
Yes, but still fighting with a test. You want to help? ;-)

Comment 22

[Jorg K (CEST = GMT+2)]

Test will follow in another bug.

Comment 23

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/d5c36d588b08

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8709936 [details] [diff] [review]
This fixes the cut/paste error. Better like this?

[Approval Request Comment]
Regression caused by (bug #): 1202401
User impact if declined: Major: Links in HTML messages not linkified.
Risk to taking this patch (and alternatives if risky):
No risk, this is fixing a cut/paste error from bug 1202401.

Comment 25

[Jorg K (CEST = GMT+2)]

Some notes regarding the test. I finally got an xpcshell test going, based on test_messageHeaders.js/test_longLines.js. I managed to send a message to the Outbox using Ci.nsIMsgSend.nsMsgQueueForLater and I was able to retrieve it from there using:
+  let folderName = "Outbox";
+  let folder = localAccountUtils.rootFolder.getChildNamed(folderName);
+  let msgData = mailTestUtils
+    .loadMessageToString(folder,mailTestUtils.firstMsgHdr(folder));

Sadly it turns out that the "linkifying" is not run in the xpcshell test. It happens in nsMsgComposeAndSend::GetBodyFromEditor() (see comment #11) and this is not run since during the xpcshell test no editor is present. Instead the body is passed in and is sent unmodified.

So a Mozmill test is necessary. This will be done in another bug.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: **Unsuitable test** (just for reference, see comment #25)

For the record, I attach the test I came up with. Sadly no linkifying takes place.

Comment 27

[Jorg K (CEST = GMT+2)]

Aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/a17d1490f020

Comment 28

[[e-mail]]

(In reply to Jorg K (GMT+1) from comment #16)
> Version without declaring an nsAutoString.

Indeed, the same thing is done 27 lines later. (But bodyText ought to be turned into an nsAutoString at some point anyway.)