Closed Bug 1240903 Opened 4 years ago Closed 4 years ago

Links no longer created in compose in versions 45/46

Categories

(Thunderbird :: Message Compose Window, defect, major)

45 Branch
defect
Not set
major

Tracking

(thunderbird45+ fixed, thunderbird46 fixed)

RESOLVED FIXED
Thunderbird 46.0
Tracking Status
thunderbird45 + fixed
thunderbird46 --- fixed

People

(Reporter: mir4, Assigned: jorgk)

References

Details

(Keywords: regression, Whiteboard: [regression:TB45])

Attachments

(2 files, 2 obsolete files)

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.
Severity: normal → major
Status: UNCONFIRMED → NEW
Component: Untriaged → Message Compose Window
Ever confirmed: true
Flags: needinfo?(neil)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
Alice, do you have time to bisect this for me. That would be much appreciated.
Flags: needinfo?(alice0775)
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.
Flags: needinfo?(alice0775)
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.
Flags: needinfo?(neil)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
(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
This confirms the regression from bug 1202401.
Flags: needinfo?(mkmelin+mozilla)
Blocks: 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.
Flags: needinfo?(hsivonen)
Sorry, Henry got NI'ed by mistake.
Flags: needinfo?(hsivonen)
Sorry, Henri.
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' !!
Attached patch This fixes the cut/paste error. (obsolete) — Splinter Review
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.
Assignee: nobody → mozilla
Attachment #8709898 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #8709927 - Flags: review?(mkmelin+mozilla)
Version without declaring an nsAutoString.
Attachment #8709927 - Attachment is obsolete: true
Attachment #8709927 - Flags: review?(mkmelin+mozilla)
Attachment #8709936 - Flags: review?(mkmelin+mozilla)
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
Attachment #8709936 - Flags: review?(mkmelin+mozilla) → review+
Magnus, why 64bit? It sseems to be a universal logic error. Jorg, is this ready for check in?
My mouse must have slipped.
Keywords: 64bit
(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.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d5c36d588b08
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
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.
Attachment #8709936 - Flags: approval-comm-aurora+
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.
For the record, I attach the test I came up with. Sadly no linkifying takes place.
Whiteboard: [regression:TB45]
(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.)
You need to log in before you can comment on or make changes to this bug.