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.
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.
Alice, do you have time to bisect this for me. That would be much appreciated.
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
(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.
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.
(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.
(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
Thank you so much. How can you do this so fast? Amazing!!
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
Created attachment 8709898 [details] [diff] [review] Backing our rev 15454eb97bbe from bug 1202401 brings the links back. This confirms the regression from bug 1202401.
<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.
Sorry, Henry got NI'ed by mistake.
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' !!
Created attachment 8709927 [details] [diff] [review] 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.
Created attachment 8709936 [details] [diff] [review] This fixes the cut/paste error. Better like this? Version without declaring an nsAutoString.
I'm working on a test so that this doesn't break again. Coming up soon.
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
Magnus, why 64bit? It sseems to be a universal logic error. Jorg, is this ready for check in?
My mouse must have slipped.
(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? ;-)
Test will follow in another bug.
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.
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.
Created attachment 8710309 [details] [diff] [review] **Unsuitable test** (just for reference, see comment #25) For the record, I attach the test I came up with. Sadly no linkifying takes place.
(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.)