Closed
Bug 1356245
Opened 8 years ago
Closed 8 years ago
image restored broken from draft or template when file extension is missing
Categories
(Thunderbird :: Message Compose Window, defect)
Tracking
(thunderbird_esr5253+ fixed, thunderbird53 fixed, thunderbird54 fixed, thunderbird55 fixed)
RESOLVED
FIXED
Thunderbird 55.0
People
(Reporter: kidservice, Assigned: jorgk-bmo)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file)
1.37 KB,
patch
|
mkmelin
:
review+
jorgk-bmo
:
approval-comm-aurora+
jorgk-bmo
:
approval-comm-esr52+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
tracking-thunderbird_esr52:
--- → +
Assignee | ||
Updated•8 years ago
|
Keywords: regression
Assignee | ||
Comment 2•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
Summary: Broken templates when composing new message → Broken templates images when composing new message
Reporter | ||
Comment 3•8 years ago
|
||
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&part=1.2&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
Assignee | ||
Comment 4•8 years ago
|
||
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.
Reporter | ||
Comment 5•8 years ago
|
||
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:
Assignee | ||
Comment 6•8 years ago
|
||
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).
Reporter | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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
Reporter | ||
Comment 9•8 years ago
|
||
I confirm that adding .png extension in the signature (name= ) works.
Thank you for the help!
Assignee | ||
Comment 10•8 years ago
|
||
We'll still fix the bug :-)
Assignee | ||
Comment 11•8 years ago
|
||
Assignee: nobody → jorgk
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8858256 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•8 years ago
|
![]() |
||
Comment 12•8 years ago
|
||
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.
Assignee | ||
Comment 13•8 years ago
|
||
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]
![]() |
||
Comment 14•8 years ago
|
||
Yeah, sorry. Is that mime service under our control to make it better behaved? :)
Assignee | ||
Comment 15•8 years ago
|
||
No. netwerk/mime/nsIMIMEService.idl
![]() |
||
Comment 16•8 years ago
|
||
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.
Assignee | ||
Comment 17•8 years ago
|
||
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
![]() |
||
Comment 18•8 years ago
|
||
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).
Assignee | ||
Comment 19•8 years ago
|
||
(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.
![]() |
||
Comment 20•8 years ago
|
||
(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.
![]() |
||
Comment 21•8 years ago
|
||
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 :)
![]() |
||
Comment 22•8 years ago
|
||
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)
Assignee | ||
Comment 23•8 years ago
|
||
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"
Comment 24•8 years ago
|
||
(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 25•8 years ago
|
||
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+
Assignee | ||
Comment 26•8 years ago
|
||
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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Assignee | ||
Updated•8 years ago
|
Attachment #8858256 -
Flags: approval-comm-esr52?
Attachment #8858256 -
Flags: approval-comm-aurora+
![]() |
||
Comment 27•8 years ago
|
||
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.
Assignee | ||
Comment 28•8 years ago
|
||
What's so hard to understand about comment #26: We should fix losing the type information.
![]() |
||
Comment 29•8 years ago
|
||
Nothing, I'm filing the promised bug now ;)
Assignee | ||
Updated•8 years ago
|
status-thunderbird53:
--- → wontfix
status-thunderbird54:
--- → affected
status-thunderbird55:
--- → fixed
status-thunderbird_esr52:
--- → affected
Assignee | ||
Comment 30•8 years ago
|
||
Assignee | ||
Comment 31•8 years ago
|
||
Assignee | ||
Comment 32•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8858256 -
Flags: approval-comm-esr52? → approval-comm-esr52+
Assignee | ||
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•