Closed Bug 714825 Opened 8 years ago Closed 8 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

10 Branch
x86
Windows 7
defect
Not set

Tracking

(thunderbird11+ fixed, thunderbird12+ fixed, thunderbird-esr1011+ fixed)

RESOLVED FIXED
Thunderbird 13.0
Tracking Status
thunderbird11 + fixed
thunderbird12 + fixed
thunderbird-esr10 11+ fixed

People

(Reporter: thomas.vansundert, Assigned: Bienvenu)

References

()

Details

(Keywords: regression, Whiteboard: [GS])

Attachments

(4 files)

Attached file 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.
Is this a regression from previous versions of Thunderbird?
It worked perfectly up to v9. The problem started from v10 beta 1.
Assignee: nobody → dbienvenu
Yes, this is reproducible - I suspect this is a regression from the ResetEmbeddedURI's changes having to do with saving drafts with embedded images.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
The issue with the test case seems to be the anchor elements with telephone numbers, e.g.,

										<td><a href="tel:+32  32 498 82 16 84"><img src="http://www.appstrakt.be/mailsignature/crew/thomas/row4_2_cellphone.png" width="134" height="22" style="display:block" border="0" alt="tel:+32  32 498 82 16 84" /></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.
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
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:0032-498-82-16-84". I guess this is the problem (it's now indeed http://tel:0032-498-82-16-84). If I remove the link completely (for both phone numbers in the signature) the mail is sent correctly
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
The reworked attachment
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.
Sorry number #3 WORKS.
Attached file Problematic Signature
I have the same problem with the attached signature. No tel: but only skype:
Keywords: qawantedregression
Summary: Attachment error for signature images when sending a mail → Attachment error for signatures containg callto:skypeid, tel:nbr type hrefs
The problem is also present with gtalk:
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.
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"
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?)
(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
(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).
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>
"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.
(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?
(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.
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://.
Attached patch possible fixSplinter Review
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.
Attachment #599251 - Flags: review?(mbanner)
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.
try server builds here:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-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!
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/
> bienvenu@nventure.com-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!
Duplicate of this bug: 729442
Thx Roberto. And I guess your case is different from the bug reporter's.
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/
> bienvenu@nventure.com-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!
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.
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 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.
Attachment #599251 - Flags: review?(mbanner) → review+
fixed on trunk - http://hg.mozilla.org/comm-central/rev/d5a16ea57ebb
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
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.
Attachment #599251 - Flags: approval-comm-release?
Attachment #599251 - Flags: approval-comm-beta?
Attachment #599251 - Flags: approval-comm-release?
Attachment #599251 - Flags: approval-comm-release+
Attachment #599251 - Flags: approval-comm-beta?
Attachment #599251 - Flags: approval-comm-beta+
Is there any chance to see the fix ported to TB10?
Comment on attachment 599251 [details] [diff] [review]
possible fix

[Triage Comment]
Opps, meant to set approval-comm-aurora+ rather than approval-comm-release+
Attachment #599251 - Flags: approval-comm-release+ → approval-comm-aurora+
Blocks: 692875
You need to log in before you can comment on or make changes to this bug.