Closed Bug 1356245 Opened 4 years ago Closed 4 years ago

image restored broken from draft or template when file extension is missing

Categories

(Thunderbird :: Message Compose Window, defect)

52 Branch
defect
Not set
normal

Tracking

(thunderbird_esr5253+ fixed, thunderbird53 fixed, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr52 53+ fixed
thunderbird53 --- fixed
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: kidservice, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170323105023

Steps to reproduce:

Create a new email.
Use an HTML file as a signature.
Save the mail in the model folder.
Open the model to compose a new message


Actual results:

The new message opens from the model, but the HTML signature gets corrupted:
- embedded base64 image disappears
- font, styles and colours are wrong


Expected results:

Message should look like the model
Can you please attach your template or send it to me in a private message.
Summary: Broken models when composing new message → Broken templates when composing new message
Keywords: regression
I tried this with a simple HTML template: Some formatting (bold/red colour) and an embedded image. Saved as a template. Used the template for a new e-mail, all looks the same.
Summary: Broken templates when composing new message → Broken templates images when composing new message
I've done some test and found that the only part that is broken is the embedded image.

I've looked at the source code of the model and of the generated email, and I've noticed this differences:

Original:
<img id="Logo"
          src="cid:part1.C9632A73.B65D7A4F@mydomain.it" alt="Logo" name="Logo">
[..........]
Content-Type: image/png;
 name="Logo_Kube"
Content-Transfer-Encoding: base64
Content-ID: <part1.8CDD2E64.670301E2@kubesistemi.it>
Content-Disposition: inline;
 filename="Logo_Kube"


Generated:
<img id="Logo"
src="mailbox:///D:/Posta/Profile/Mail/mydomain.gmail/Templates?number=74102&amp;part=1.2&amp;filename=Logo"
        alt="Logo" name="Logo" class="loading-internal">



The source of the image changes, and the "Content-Type: image/png;" disappears.
Please tell me if you still need the template, I'll build you a clean one
OK, in the saved template you will see a cid: reference:
<img id="Logo" src="cid:part1.C9632A73.B65D7A4F@mydomain.it" alt="Logo" name="Logo">

When you re-use the template you should *NOT* see a mailbox URL:
<img id="Logo" src="mailbox:///D:/Posta/Profile/Mail/mydomain.gmail ...

What you should see instead is
<img id="Logo" src="data:image/png;filename=Logo;base64,..."

Our code replaces the mailbox URL: with the data: URL.

What I don't understand is that your filename doesn't have a file extension. That is important to detect the type of the file.

Please check |Tools > Developer Tools > Error Console|: Do you see:
"Won't unblock; URL=...".

Please change the template to reference an image with a full file name that includes a file extension.
Here is the log you've asked (italian)
window.controllers è deprecato. Non utilizzarlo per rilevare l’User Agent del browser.  blank
Problema di sicurezza: i contenuti in about:blank non possono caricare o avere link che rimandino a mailbox:///D:/Posta/Profile/Mail/kubesistemi.gmail/Templates?number=74102&part=1.2&filename=Logo_Kube.NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIMIMEService.getTypeFromURI]  MsgComposeCommands.js:5664
Problema di sicurezza: i contenuti in about:blank non possono caricare o avere link che rimandino a mailbox:///D:/Posta/Profile/Mail/kubesistemi.gmail/Templates?number=74102&part=1.2&filename=Logo_Kube.

window.controlles id deprecated. Don't use it to solve...
Security problem: content of about:blank can't load or have links to a mailbox:
Yes, that is expected. And then the image should be converted to a data: URI automatically.

Please instead of using Logo_Kube use Logo_Kube.png.

Also, start TB with all add-ons disabled (see Help menu).
I've tried in safety mode, same result.
I can't rename it to ".png" because it isn't a png file.
It comes from my signature.
The signature is an HTML file that has this section:

<img 
		id="Logo_Kube" 
		src="data:image/png;base64,iVBO [........] g=="
		alt="Logo Kube"
		name="Logo_Kube">

Should I rename the ID/ALT/NAME ?

The content type of that cid is specified in this part:
--------------438D4AFBC8260A0DD028D123
Content-Type: image/png;
 name="Logo_Kube"
Content-Transfer-Encoding: base64
Content-ID: <part1.8CDD2E64.670301E2@kubesistemi.it>
Content-Disposition: inline;
 filename="Logo_Kube"

But this part is generated by TB when I save the mail as a model.
My orignal signature's HTML file does not have this part because the image is embedded.
Simply try name="Logo_Kube.png".

Of if that doesn't work change the signature to
<img id="Logo_Kube" 
src="data:image/png;filename=Logo_Kube.png;base64,iVBO [........] g=="
alt="Logo Kube"
name="Logo_Kube">

I reproduced the problem, it happens when the there is no file extension.
Summary: Broken templates images when composing new message → image restored broken from draft or template when file extension is missing
I confirm that adding .png extension in the signature (name= ) works.
Thank you for the help!
We'll still fix the bug :-)
Assignee: nobody → jorgk
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8858256 - Flags: review?(mkmelin+mozilla)
Depends on: 1322155
Blocks: 1322155
No longer depends on: 1322155
Comment on attachment 8858256 [details] [diff] [review]
1356245-no-extension.patch (v1).

Review of attachment 8858256 [details] [diff] [review]:
-----------------------------------------------------------------

Why should there be a file extension? The template clearly contains the mime type:
Content-Type: image/png;
 name="Logo_Kube"

Why don't we use it?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5672,5 @@
> +      contentType = Components.classes["@mozilla.org/mime;1"]
> +        .getService(Components.interfaces.nsIMIMEService)
> +        .getTypeFromURI(uri);
> +    } catch (ex) {
> +      contentType = "image/png";

Does the mime service really throw if type couldn't be determined? The reporter didn't say such an error is in the console.
Yes, it throws and the reporter reported it in comment #5. Let's read it together ;-)
Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIMIMEService.getTypeFromURI]
Yeah, sorry. Is that mime service under our control to make it better behaved? :)
No. netwerk/mime/nsIMIMEService.idl
Ok, so can you look at the Content-type and only if it is missing consult the mime service?
Also I'm not sure falling back to image/png does any good if the file data isn't really in that format.
There is no content type by the time we get there. We only have the image src URL. We don't communicate with MIME here, we post process the editor content. It was Magnus' idea. I had different plans for replacing mailnews URLs (see bug 1318450).

The fallback to image/png comes from here:
https://hg.mozilla.org/comm-central/rev/bc46559b2c2b
So do you insist on defaulting to image/png even if we don't know what the file is? Why not throw the "won't unblock" and exit?

I think we should better make sure TB never produces these URIs without file extension. What is the bug for that? I think this one can be it. It produces such URI for me with any image (on Linux).
(In reply to :aceman from comment #18)
> So do you insist on defaulting to image/png even if we don't know what the
> file is? Why not throw the "won't unblock" and exit?
Look at bug 1338794 for an answer. The xxx in image/xxx actually doesn't matter. It can be wrong and the image still shows.

> I think we should better make sure TB never produces these URIs without file
> extension. What is the bug for that? I think this one can be it. It produces
> such URI for me with any image (on Linux).
("we had better ..." is the correct term to use).
So, Mr. Plain-Text-Mail, let's forget background images for a while. What about embedded images, don't the store an extension?

We're here under pressure fixing regressions. So I don't really have time right now to drill through all the layers and make sure the extension gets there. We can do this later and the fallback will never be used.
(In reply to Jorg K (GMT+2) from comment #19)
> (In reply to :aceman from comment #18)
> > So do you insist on defaulting to image/png even if we don't know what the
> > file is? Why not throw the "won't unblock" and exit?
> Look at bug 1338794 for an answer. The xxx in image/xxx actually doesn't
> matter. It can be wrong and the image still shows.

That's strange, then it should be allowed to not specify a type if we rely on autodetection. But OK, then your fix can stay.

> > I think we should better make sure TB never produces these URIs without file
> > extension. What is the bug for that? I think this one can be it. It produces
> > such URI for me with any image (on Linux).
> ("we had better ..." is the correct term to use).
> So, Mr. Plain-Text-Mail, let's forget background images for a while. What
> about embedded images, don't the store an extension?

Actually I now tested it and for normal <img> images TB stores the right filename and with an extension.

I used the same file for background and for embedded image and the first one got random filename without extension and the latter got right filename.
OK, if Gecko is fine and autodetects the type even if the data: URI has wrong type, we are lucky.
But when the message is sent out the image is converted to cid, and the cid part has a Content-type declaration. Will that one be our faked image/png too?

I'd like to see that but embedded images work fine and background images that TB creates without extension need bug 1356303 first. I need to make a hand crafted draft :)
So I have played with this and for an image without extension the default of image/png you set will be stored in the final message to be sent out. In this case I put a GIF image as background but TB itself added it without extension (I'll file it as a new bug), and the result is this:

Content-Type: image/png;
 name="clffkhfdgiodfjln."
Content-Transfer-Encoding: base64
Content-ID: <part4.AED8A4DA.E43A3A8F@xxx.xx>
Content-Disposition: inline;
 filename="clffkhfdgiodfjln."

It seems Gecko copes and displays the image fine analysing the content data. So the Content-Type is ignored. But what about other receiving clients? Would they cope? Some strict ones may obey Content-Type. Are we allowed to send out such bogus declarations? Jcranmer, do you know what the spec says?
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(Pidgeot18)
Can we please approve/land this now and worry about the missing extension (on Linux only? (*)) later in the bug you promised to file? Branch day is in two days and we already have less-than-perfect processing here:
https://dxr.mozilla.org/comm-central/rev/1210b0611ecd2db15ca465e903e07da7a31fabe5/mail/components/compose/content/MsgComposeCommands.js#5712

Tested again on a GIF (the roses from the other bug) as a background and the GIF extension didn't get lost, it's in all the (re-opened) drafts and the sent message.

<off-topic>
When I add a background from a file without extension I get:
Content-Type: application/octet-stream;
 name="klenlifaohidnlfj."
Content-Transfer-Encoding: base64
Content-ID: <part1.1B79A259.1974A456@jorgk.com>
Content-Disposition: inline;
 filename="klenlifaohidnlfj."

Opening the draft again loses the background since MIME hands the cid: URL on and we don't replace this with with a data: URL:
https://dxr.mozilla.org/comm-central/rev/1210b0611ecd2db15ca465e903e07da7a31fabe5/mail/components/compose/content/MsgComposeCommands.js#5463

So there certainly is room for improvement in MIME, but not right now ;-)
</off-topic>

At least the patch fixes the problem reported here: A template which has:
Content-Type: image/png;
 name="pc-help-map"
Content-Transfer-Encoding: base64
Content-ID: <part3.99C1A20A.71D6F786@jorgk.com>
Content-Disposition: inline;
 filename="pc-help-map"
(In reply to :aceman from comment #22)
> It seems Gecko copes and displays the image fine analysing the content data.
> So the Content-Type is ignored. But what about other receiving clients?
> Would they cope? Some strict ones may obey Content-Type. Are we allowed to
> send out such bogus declarations? Jcranmer, do you know what the spec says?

Yes there are no guarantees it will work if the content type is wrong. In practice, web browsers implement some fallback logic so it works. Of course we *want* to send out the right type, but this is the case that's not possible to do, so there is little to lose by doing something that will work for close to everyone.
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(Pidgeot18)
Comment on attachment 8858256 [details] [diff] [review]
1356245-no-extension.patch (v1).

Review of attachment 8858256 [details] [diff] [review]:
-----------------------------------------------------------------

Yes this should be fine. r=mkmelin
Attachment #8858256 - Flags: review?(mkmelin+mozilla) → review+
https://hg.mozilla.org/comm-central/rev/a5beafb7d7a4fc3038b485752777884d62d18119

Not a perfect solution, but at least some solution. We can follow up in a new bug to pass out the content type in the mailnews URL when MIME retrieves the attachment. We do that on other occasions, like: ... &filename=A01.JPG&type=image/jpeg.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Attachment #8858256 - Flags: approval-comm-esr52?
Attachment #8858256 - Flags: approval-comm-aurora+
So because we have an internal problem (at one place in code (m-c) we can analyse and render the image, at another place (in c-c editor) we suddenly don't know what type the data is), we punish receiving clients by sending them lies. So let's see how that pans out.
What's so hard to understand about comment #26: We should fix losing the type information.
Nothing, I'm filing the promised bug now ;)
Blocks: 1356918
Attachment #8858256 - Flags: approval-comm-esr52? → approval-comm-esr52+
You need to log in before you can comment on or make changes to this bug.