714825 - Attachment error for signatures containg callto:skypeid, tel:nbr type hrefs

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[thomas.vansundert]

Attachment: appstrakt_sig.html

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20100101 Firefox/10.0
Build ID: 20111228055358

Steps to reproduce:

I have an HTML based signature (see attachment) that also contains remotely hosted images. The signature appear correctly when composing a mail.


Actual results:

When I send the mail or when the mail is automatically saved as a draft, some of the images in my signature disappear and the following message appears:

"Sending of message failed.
Attachment error."

I can not sent the mail unless by removing my signature.


Expected results:

The mail should be sent perfectly and the attachment should look fine.

Comment 1

[David :Bienvenu]

Is this a regression from previous versions of Thunderbird?

Comment 2

[thomas.vansundert]

It worked perfectly up to v9. The problem started from v10 beta 1.

Comment 3

[David :Bienvenu]

Yes, this is reproducible - I suspect this is a regression from the ResetEmbeddedURI's changes having to do with saving drafts with embedded images.

Comment 4

[David :Bienvenu]

The issue with the test case seems to be the anchor elements with telephone numbers, e.g.,

										<td><a href="tel:[phone]"><img src="http://www.appstrakt.be/mailsignature/crew/thomas/row4_2_cellphone.png" width="134" height="22" style="display:block" border="0" alt="tel:[phone]" /></a></td>


That's not a valid href as far as the code is concerned, which causes an error in nsMsgComposeAndSend::GetEmbeddedObjectInfo. I'll try to figure out why the older code doesn't care about the error.

Comment 5

[David :Bienvenu]

OK, the change that broke this is in the core necko http code, which is not happy with the url getting passed in. (we stick http:// in front because we recognize that it's not a valid url) I suspect you need to escape the ':' and the spaces to make it happy. My inclination would be to mark this invalid, if Mark agrees.

Comment 6

[Mark Banner (:standard8)]

I checked out the RFC for tel: http://tools.ietf.org/html/rfc3966

It lists these characters as visual separators: - . ( )

It doesn't list space. So the code is now being stricter according to the protocol.

Therefore I agree with the invalid resolution.

Comment 7

[thomas.vansundert]

I have adapted the signature (I've attached the code) and removed the spaces in the URI. I however still get the same result.

I've noticed that if I double click the image, the "link" field contains "tel:[phone]". I guess this is the problem (it's now indeed http://tel:[phone]). If I remove the link completely (for both phone numbers in the signature) the mail is sent correctly

Comment 8

[thomas.vansundert]

Attachment: The reworked attachment

The reworked attachment

Comment 9

[jun]

I've experienced the same problem with  the error message :
"Sending of message failed.
Attachment error."

I have done the following test with Thunderbird 10 beta 4 :
#1 - Signature with an <img> tag and a <a href="callto:skypeid"> : FAILS
#2 - Signature with just an <img> tag : WORKS
#3 - Signature with just the <a href="callto:skypeid"> : FAILS

Variations on #1 using tel: or skype: also fail. 

Alhtough I understand these are not W3C standard protocols (although tel: seems to be). It is quite common to want to facilitate access to various communication software through this type of notation, particularly in signatures.

For now I will remove the special protocol link but assume this might have to be addressed (or at least precised in a clearer error message). 

Best,

Jun.

Comment 10

[jun]

Sorry number #3 WORKS.

Comment 11

[Roberto Ballerini - Automa]

Attachment: Problematic Signature

I have the same problem with the attached signature. No tel: but only skype:

Comment 12

[fglez]

The problem is also present with gtalk:

Comment 13

[david woakes]

I can confirm that messages with a callto:skypeid and an image will fail.

Using callto:skypeid or the image in isolation work.

Using moz-do-not-send="true" for the image doesn't help.

Comment 14

[Arivald]

I can confirm it too.

If image is before skype link, it works.

One way to make it send mail is to add one "/" to skype URL

bad:
<a href="skype:testUsername?call">TEST</a>

working:
<a href="skype:/testUsername?call">TEST</a>
although Skype will try to call user "/testUsername"

Comment 15

[intendentedelleacque]

On Linux I can't see this bug, so I wonder if it depends from some windows-specific change (for example https://bugzilla.mozilla.org/show_bug.cgi?id=22473?)

Comment 16

[intendentedelleacque]

(In reply to intendentedelleacque from comment #15)
> On Linux I can't see this bug, so I wonder if it depends from some
> windows-specific change (for example
> https://bugzilla.mozilla.org/show_bug.cgi?id=22473?)

The bug above is wrong, the right link is https://bugzilla.mozilla.org/show_bug.cgi?id=224733

Comment 17

[Roberto Ballerini - Automa]

(In reply to david woakes from comment #13)
> 
> Using moz-do-not-send="true" for the image doesn't help.

Using moz-do-not-send="true" in the skype: URI DO help. 
The message is correctly managed by TB, but the skype link doesn't work in Gmail (I suppose because it doesn't understand the attribute).

Comment 18

[Andrei Niculae]

You do realize this is a very big bug. Imagine that now all the click to call applications will stop working and everybody will either stick with v9 or use outlook instead. 

A very interesting fact is that I can have a <a href="tel:+123">123</a> but if I also have an <img tag in there, it stops working. 

This looks like the parser is doing something very strange ...

Please solve this.

Original signature (worked in 9):
<html>
<body link="#000000" alink="#ff0000">
-- <br>
<a href="http://www.XXX.com" target="_blank"><img src="file://C:/ProgramData/XXX/BaseImage/obs/XXX.png" alt="XXX" border="0"></a><br>
<font color="#666666" face="Verdana, Arial, Helvetica, sans-serif" size="2">Andrei Niculae | Consultant<br>Phone: <a href="tel:+123">+123</a>| Mobile: <a href="tel:+123">+123</a> 
<br>
<font color="#ff0000">XXX</font> XX<br>
address</font>
<br>
<table>
<tr>
<td><a href="http://www.XXX.com/" target="_blank"><img src="file://C:/ProgramData/XXX/BaseImage/obs/YYY.gif" alt="YYY" width="44" height="28" border="0" align="abscenter"></a></td>
<td><font color="#4B7D42" size="1" face="Verdana, Arial, Helvetica, sans-serif">YYY ZZZ</font></td>
</tr>
</table>
</body>
</html>

Comment 19

[alex mateescu]

"Very big bug" is an understatement. This is one of those bugs our clients would fry us for, should we let it slip through testing. Needless to say many of the diehards that stuck with TB despite company's policy, have now switched to Outlook. Because, you know, some signatures are mandatory and can't be edited as a workaround.

Comment 20

[WADA:World Anti-bad-Duping Agency]

(In reply to David :Bienvenu from comment #5)
> (we stick http:// in front because we recognize that it's not a valid url)

Why should http:// be sticked in front of not-valid url which starts with unknown-schema-name + ":"?
Why should http:// be sticked in front of callto:skypeid(undefined schema name) or tel:+123(defined schema name)?
Why not-valid url is not ignored?
Who is "we"? Tb? Parser? Necko? Feature like "auto-complement of http:// at URLbar" is culprit?

Comment 21

[David :Bienvenu]

(In reply to WADA from comment #20)
> (In reply to David :Bienvenu from comment #5)
> > (we stick http:// in front because we recognize that it's not a valid url)
> 
> Why should http:// be sticked in front of not-valid url which starts with
> unknown-schema-name + ":"?
> Why should http:// be sticked in front of callto:skypeid(undefined schema
> name) or tel:+123(defined schema name)?
> Why not-valid url is not ignored?
> Who is "we"? Tb? Parser? Necko? Feature like "auto-complement of http:// at
> URLbar" is culprit?

See https://bugzilla.mozilla.org/show_bug.cgi?id=714825#c5 - this seems to be a necko change. We can try to ignore the failure, but I'm not sure what will break downstream.

Comment 22

[WADA:World Anti-bad-Duping Agency]

Aha!
(i)  Tb adds "http://" if "Invalid URL" is returned from Necko.
(ii) Old Necko returned "Vaild"   to callto:skypeid or tel:+123.
     New Necko returns  "Invaild" to callto:skypeid or tel:+123.

It looks for me that (i) is to avoid further "invalid URL" errors which occurred in the past.
How about next?
> (a) network.protocol-handler.external.Dummy = false.
> (b) Never pass Dummy: to external handler nor Tb's handler(ignore true).
> (c) Add "Dummy://" internally, if "Invalid URL" is returned from Necko.
>     Don't change URL like string in message source.
I think attachment code can ignore this Dummy://.

Comment 23

[David :Bienvenu]

Attachment: possible fix

This ignores the error creating the url, and handles have a null attachment url a bit further downstream. With this fix, I can save as draft and send the sig that Thomas attached to this bug. I've requested a try server build as well, so that others can try this and verify it fixes their cases as well. I'll post a link when the try server build starts completing.

Comment 24

[David :Bienvenu]

I think we should track this for TB 11 and 12 as well. I think the fix is *relatively* safe and I'd like to know if it fixes the real world cases, which means getting it into alpha and beta builds.

Comment 25

[David :Bienvenu]

try server builds here:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-52111db5538f/

If this bug is affecting you, and you'd like to see a fix landed sooner rather than later, trying out the build and letting us know if it fixes the issues you're seeing would be very helpful, thx!

Comment 26

[Roberto Ballerini - Automa]

Hi David.
I made a test and it sent a message that was blocked on TB10.X.
Thanks

(In reply to David :Bienvenu from comment #25)
> try server builds here:
> 
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/
> [e-mail]-52111db5538f/
> 
> If this bug is affecting you, and you'd like to see a fix landed sooner
> rather than later, trying out the build and letting us know if it fixes the
> issues you're seeing would be very helpful, thx!

Comment 28

[David :Bienvenu]

Thx Roberto. And I guess your case is different from the bug reporter's.

Comment 29

[fglez]

I tried this build and I was able to send messages with signature containing a gtalk: URL, which was not possible since TB 10.0.0

Thank you, David.

(In reply to David :Bienvenu from comment #25)
> try server builds here:
> 
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/
> [e-mail]-52111db5538f/
> 
> If this bug is affecting you, and you'd like to see a fix landed sooner
> rather than later, trying out the build and letting us know if it fixes the
> issues you're seeing would be very helpful, thx!

Comment 30

[Roberto Ballerini - Automa]

Hi David. My signature is the third attachment of this ticket.
Basically it contains a skype: link and various remote images.
(In reply to David :Bienvenu from comment #28)
> Thx Roberto. And I guess your case is different from the bug reporter's.

Comment 31

[leonardtheron]

I can confirm I'm pretty sure it's the image embedded in the signature because when I suppress it, the mail can be sent!

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 599251 [details] [diff] [review]
possible fix

Ok, this seems to be fine. I looked through the code and it appears we have the right checks in place already for a null attachment->m_url, so I think this is safe.

Comment 33

[David :Bienvenu]

fixed on trunk - http://hg.mozilla.org/comm-central/rev/d5a16ea57ebb

Comment 34

[David :Bienvenu]

Comment on attachment 599251 [details] [diff] [review]
possible fix

I also had checked other users of attachment->m_url, and fixed the one place we deref it w/o checking.

Comment 35

[alex mateescu]

Is there any chance to see the fix ported to TB10?

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 599251 [details] [diff] [review]
possible fix

[Triage Comment]
Opps, meant to set approval-comm-aurora+ rather than approval-comm-release+

Comment 37

[Mark Banner (:standard8)]

Landed on aurora/beta branches:

http://hg.mozilla.org/releases/comm-aurora/rev/80a80cd502a7
http://hg.mozilla.org/releases/comm-beta/rev/5e996adb1ff5

Comment 38

[Mark Banner (:standard8)]

Landed for esr 10.0.3: http://hg.mozilla.org/releases/comm-esr10/rev/348b51b0728c