Links no longer created in compose in versions 45/46

RESOLVED FIXED in Thunderbird 46.0

Status

Thunderbird
Message Compose Window
--
major
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: MirandaGoshawk, Assigned: Jorg K (GMT+2))

Tracking

({regression})

45 Branch
Thunderbird 46.0
regression

Thunderbird Tracking Flags

(thunderbird45+ fixed, thunderbird46 fixed)

Details

(Whiteboard: [regression:TB45])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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)
Keywords: regression, regressionwindow-wanted
(Assignee)

Comment 2

a year ago
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

Updated

a year ago
tracking-thunderbird45: --- → ?

Comment 4

a year ago
(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)
(Assignee)

Comment 5

a year ago
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.
tracking-thunderbird45: ? → +

Comment 6

a year ago
(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)

Comment 7

a year ago
(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
(Assignee)

Comment 8

a year ago
Thank you so much. How can you do this so fast? Amazing!!
(Assignee)

Comment 9

a year ago
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
(Assignee)

Comment 10

a year ago
Created attachment 8709898 [details] [diff] [review]
Backing our rev 15454eb97bbe from bug 1202401 brings the links back.

This confirms the regression from bug 1202401.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Updated

a year ago
Blocks: 1202401
(Assignee)

Comment 11

a year ago
<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)
(Assignee)

Comment 12

a year ago
Sorry, Henry got NI'ed by mistake.
Flags: needinfo?(hsivonen)
(Assignee)

Comment 13

a year ago
Sorry, Henri.
(Assignee)

Comment 14

a year ago
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' !!
(Assignee)

Comment 15

a year ago
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.
Assignee: nobody → mozilla
Attachment #8709898 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #8709927 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 16

a year ago
Created attachment 8709936 [details] [diff] [review]
This fixes the cut/paste error. Better like this?

Version without declaring an nsAutoString.
Attachment #8709927 - Attachment is obsolete: true
Attachment #8709927 - Flags: review?(mkmelin+mozilla)
Attachment #8709936 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 17

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

Comment 18

a year ago
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+

Updated

a year ago
Keywords: regressionwindow-wanted → 64bit

Comment 19

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

Comment 20

a year ago
My mouse must have slipped.
Keywords: 64bit
(Assignee)

Comment 21

a year ago
(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? ;-)
(Assignee)

Comment 22

a year ago
Test will follow in another bug.
Keywords: checkin-needed
(Assignee)

Comment 23

a year ago
https://hg.mozilla.org/comm-central/rev/d5c36d588b08
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-thunderbird45: --- → affected
status-thunderbird46: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
(Assignee)

Comment 24

a year ago
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+
(Assignee)

Comment 25

a year ago
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.
(Assignee)

Comment 26

a year ago
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.
Whiteboard: [regression:TB45]
(Assignee)

Comment 27

a year ago
Aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/a17d1490f020
status-thunderbird45: affected → fixed
(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.