761051 - Pasting of an image as JPEG inline results in incomplete attachment name

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[rsx11m]

This used to work after bug 578104 derived the attachment name from the MIME type and a random prefix, and it still works fine when pasting an image as PNG but not as JPEG:

1. Copy an image to the clipboard, e.g., a screendump
2. Paste into an HTML message you are composing
3. Send or save as draft, the image receives an attachment name "abcdefgh.png"
4. Change clipboard.paste_image_type from its default 1 to 0 for JPEG
5. Paste again an image into a new message and send or save
6. The image has an attachment name "abcdefgh." without any file extension

As with bug 578104, the images show still up fine in Mozilla applications when displayed, but may cause issues when saving the image or with any other e-mail application that relies on the file extension rather than MIME type to determine how to handle the attachment.

This doesn't occur with drag-and-drop operation of a JPEG file, those will be correctly named as "abcdefgh.jpg" and the reason appears to be in the MIME type:

Pasted test-case image with clipboard.paste_image_type = 0:
> Content-Type: image/jpg;
>  name="eaggcahh."

JPEG image (originally named ".jpg") drag-and-dropped into the message:
> Content-Type: image/jpeg;
>  name="caedgeeg.jpg"

Thus, apparently image/jpeg is recognized by the code added to generate a file name for an inline attachment whereas image/jpg isn't. Observed on current Thunderbird 15.0a1 nightly, 10.0.4 ESR release, and SeaMonkey 2.11a2 aurora nightly builds, all running on Windows 7.

Comment 1

[rsx11m]

Also seen on Linux, and in fact I can reproduce as far back as Mozilla/5.0 (X11; Linux x86_64; rv:6.0a2) Gecko/20110617 Thunderbird/6.0a2 which would suggest that the initial fix for this was incomplete regarding image/jpg to start with (I'm usually pasting as PNG, and the attachments were hidden from display even in plain-text view until bug 674473, thus I didn't notice it earlier).

I'm wondering if in general, to avoid issues with non-registered MIME types, in cases where a file extension cannot be found the part after "image/" is simply used (or the leading 3-4 characters with a potentially leading "x-" stripped) so that there is always a file extension?

Even more general, the image/jpeg and image/jpg MIME types should probably be treated as being equivalent throughout the Mozilla code, but that's more of a core issue.

Comment 2

[rsx11m]

On a side note, RFC 2046 defines image/jpeg rather than image/jpg as the valid MIME content type for JPEG images (editor pastes "data:image/jpg;base64,..." as image location).

Comment 3

[Joe Sabash [:JoeS1]]

Apparently, this only happens when the original extension is unknown. IE screenshots
If you copy/paste a .jpg image things work fine.
In the case of screenshots, if you edit the intermediate state (before save/send)
from data:image/jpg;base64, to data:image/jpeg; then it works as expected.
So the "simple" fix looks to be to use /jpeg as the default.

I know Ehsan wrote that code, but not sure if it resides in core/editor or not.

Comment 4

[(no longer active)]

I don't know where that code lives, but I'm pretty sure somewhere in comm-central.  The core editor code just stores the image as a data URI, without any name (since it may not even have access to a file name.)

Comment 5

[rsx11m]

(In reply to Joe Sabash from comment #3)
> Apparently, this only happens when the original extension is unknown.
> If you copy/paste a .jpg image things work fine.

Are you sure that you are copying the bitmap from the clipboard in this case and not just a reference to the file which Thunderbird resolves then by itself? Note that drag-and-drop works, which would indeed pass just a reference to the file.

Re comment #4, we needed bug 578104 to add a proper file name back to the pasted image given that no such information is provided with the bitmap on the clipboard, Ehsan is certainly correct here. However, the "image/jpg" rather than "image/jpeg" may come from the changes in the editor code (bug 490879) when the former moz-screenshot.jpg file was replaced by the direct encoding in a data URI. That code however accepts the respective kJPEGImageMime only coming from the transferable, thus the issue is introduced earlier already when creating that flavor from the clipboard.

> from data:image/jpg;base64, to data:image/jpeg; then it works as expected.

Manually translating image/jpg to image/jpeg can be done either when constructing the data URI or when creating the attachment name, thus both variants should do as a workaround. Such a workaround is applied in the Windows pasting code already in nsImageFromClipboard::GetEncodedImageStream() at nsImageClipboard.cpp#204.

The greater-scope problem is that kJPEGImageMime defines "image/jpg" rather than the standard "image/jpeg" thus causing the ambiguity, where I don't know when and why it was defined in this way. One approach might be to fix the specific issue now in either the editor or mailnews code with a special-case handling, then to spin off another bug to resolve the problem at the source (with kJPEGImageMime being used in editor and widget code), removing any explicit workarounds in the process.

Comment 6

[(no longer active)]

I'm getting confused here.  Where does the code on the comm-central side of things live?

Comment 7

[rsx11m]

Translating data URIs attachments is msg_pick_real_name() in nsMsgCompUtils.cpp
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompUtils.cpp#1600

Comment 8

[(no longer active)]

Hmm, so is this about the MIME service not being able to understand image/jpg?  Honestly I'm not sure why the kJPEGImageMime is "image/jpg" and not "image/jpeg", but I can easily fix up the editor code to generate data: URIs which use image/jpeg if you think that fixes this issue.  Or, we could change the comm-central code to special case "image/jpg" (but I think generating image/jpeg when pasting is more correct...)

Comment 9

[rsx11m]

If you want to look into modifying the data URI creation, this appears to be the "correct" way to do it and to be in compliance with the RFC 2046 definition.

I've got a bit started already but don't have a trunk build up, and that likely would need some modifications of whatever tests you have for that anyway. The following would be a direct adaption of Kathleen's workaround from bug 444800:

--- a/editor/libeditor/html/nsHTMLDataTransfer.cpp
+++ b/editor/libeditor/html/nsHTMLDataTransfer.cpp
@@ -1261,7 +1261,10 @@ nsresult nsHTMLEditor::InsertObject(cons

     nsAutoString stuffToPaste;
     stuffToPaste.AssignLiteral("<IMG src=\"data:");
-    AppendUTF8toUTF16(type, stuffToPaste);
+    if (strcmp(type, kJPEGImageMime) == 0)
+      stuffToPaste.AppendLiteral("image/jpeg");
+    else
+      AppendUTF8toUTF16(type, stuffToPaste);
     stuffToPaste.AppendLiteral(";base64,");
     AppendUTF8toUTF16(base64, stuffToPaste);
     stuffToPaste.AppendLiteral("\" alt=\"\" >");

Comment 10

[(no longer active)]

Attachment: Patch (v1)

Comment 11

[(no longer active)]

Yeah, my patch does something similar, in a slightly better way.

Comment 12

[rsx11m]

(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> Hmm, so is this about the MIME service not being able to understand image/jpg?

My guess: Looking at nsExternalHelperAppService::GetFromTypeAndExtension(), it's looking up the MIME information with GetMIMEInfoFromOS() first. As far as I can tell, there is no OS association for "image/jpg" but for "image/jpeg" only. Thus, looking up the entry for "image/jpg" yields no result unless it has been defined explicitly in mimeTypes.rdf (which it isn't for me). On the reading end, there are uriloader/content-handler definitions for both variants, thus internally they are still displayed correctly as JPEG images despite the missing association.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems to me that kJPEGImageMime is just wrong and should be "image/jpeg"? Maybe some places that currently use kJPEGImageMime should accept "image/jpg" as well.

Comment 14

[rsx11m]

Yes, but per last paragraph of comment #5 this patch aims for a solution of the immediate issue while keeping the larger-scale fix in mind for a follow-up bug. kJPEGImageMime is used by the editor code as well as in various widget modules, thus may take a bit longer to do it right and to ensure backwards compatibility for any "image/jpg" uses where they still need to be considered.

Comment 15

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Seems to me that kJPEGImageMime is just wrong and should be "image/jpeg"?
> Maybe some places that currently use kJPEGImageMime should accept
> "image/jpg" as well.

Yeah, that could be the case.  But this constant is used when dealing with both the OS clipboard and drag/drop APIs, and it's a little bit scary to change it without extensive testing.  FWIW, there are a few places in the widget code which look for both "image/jpg" and "image/jpeg".  We can consider changing kJPEGImageMime later, but I'm a bit hesitant as it's hard to imaging what kinds of stuff that could break.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> Yeah, that could be the case.  But this constant is used when dealing with
> both the OS clipboard and drag/drop APIs, and it's a little bit scary to
> change it without extensive testing.

If we're going to do it sometime, why not now?

It shouldn't be much work to audit the users of kJPEGImageMime and make sure that everywhere we're checking for kJPEGImageMime, we accept both jpg and jpeg. That should limit the scope of possible regressions.

If there are regressions, fixing them should be relatively easy.

Comment 17

[(no longer active)]

Attachment: Patch (v2)

I took a closer look at this, and this was actually a lot easier to solve than I realized.  In fact I needed to remove a bunch of existing work-arounds for this problem, and everything else seems to be handled correctly.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 630377 [details] [diff] [review]
Patch (v2)

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

Don't we need to change code like
http://dxr.lanedo.com/mozilla-central/widget/gtk2/nsClipboard.cpp.html#l209
and
http://dxr.lanedo.com/mozilla-central/widget/gtk2/nsClipboard.cpp.html#l317
to accept image/jpg as well?

Comment 19

[rsx11m]

Comment on attachment 630377 [details] [diff] [review]
Patch (v2)

> @@ -441,8 +437,7 @@ nsClipboard::HasDataMatchingFlavors(const char** aFlavorList, PRUint32 aLength,
>              if (!strcmp(atom_name, aFlavorList[i]))
>                  *_retval = true;
>  
> -            // X clipboard wants image/jpeg, not image/jpg
> -            if (!strcmp(aFlavorList[i], kJPEGImageMime) && !strcmp(atom_name, "image/jpeg"))
> +            if (!strcmp(aFlavorList[i], kJPEGImageMime))
>                  *_retval = true;

Shouldn't the second part go entirely? This would set *_retval = true whenever image/jpeg is in the flavor list, regardless of the value of atom_name (thus always yield true, e.g., for textHtmlEditorFlavors[]). Now that atom_name of image/jpeg is caught by the first comparison already, which didn't happen before with image/jpg, the second comparison with the special case of image/jpg flavor matching an image/jpeg atom is now redundant.

Comment 20

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> Comment on attachment 630377 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 630377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Don't we need to change code like
> http://dxr.lanedo.com/mozilla-central/widget/gtk2/nsClipboard.cpp.html#l209
> and
> http://dxr.lanedo.com/mozilla-central/widget/gtk2/nsClipboard.cpp.html#l317
> to accept image/jpg as well?

No.  Those flavors come from the transferable object, which now knows only about image/jpeg.

Comment 21

[(no longer active)]

(In reply to rsx11m from comment #19)
> Comment on attachment 630377 [details] [diff] [review]
> Patch (v2)
> 
> > @@ -441,8 +437,7 @@ nsClipboard::HasDataMatchingFlavors(const char** aFlavorList, PRUint32 aLength,
> >              if (!strcmp(atom_name, aFlavorList[i]))
> >                  *_retval = true;
> >  
> > -            // X clipboard wants image/jpeg, not image/jpg
> > -            if (!strcmp(aFlavorList[i], kJPEGImageMime) && !strcmp(atom_name, "image/jpeg"))
> > +            if (!strcmp(aFlavorList[i], kJPEGImageMime))
> >                  *_retval = true;
> 
> Shouldn't the second part go entirely? This would set *_retval = true
> whenever image/jpeg is in the flavor list, regardless of the value of
> atom_name (thus always yield true, e.g., for textHtmlEditorFlavors[]). Now
> that atom_name of image/jpeg is caught by the first comparison already,
> which didn't happen before with image/jpg, the second comparison with the
> special case of image/jpg flavor matching an image/jpeg atom is now
> redundant.

Yeah, good point.  New patch coming up in a sec.

Comment 22

[(no longer active)]

Attachment: Patch (v3)

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> No.  Those flavors come from the transferable object, which now knows only
> about image/jpeg.

Is it not possible for extensions to add data to an nsITransferable with type image/jpg?

Comment 24

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> (In reply to Ehsan Akhgari [:ehsan] from comment #20)
> > No.  Those flavors come from the transferable object, which now knows only
> > about image/jpeg.
> 
> Is it not possible for extensions to add data to an nsITransferable with
> type image/jpg?

Hmm, good point.  Looking through the addons mxr, it seems like some of them actually do this.

In this case I think I'll add a fourth type of image, kJPGImageMime and handle it correctly everywhere.  Do you agree?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes!

Comment 26

[(no longer active)]

Attachment: Patch (v4)

Comment 27

[(no longer active)]

http://tbpl.mozilla.org/?tree=Try&rev=ac1678f79c78

Comment 28

[rsx11m]

Tests for bug 444800 are orange on Linux and Windows:

32666 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/widget/tests/test_bug444800.xul | image/jpg - hasDataMatchingFlavors()
32667 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/widget/tests/test_bug444800.xul | an unexpected uncaught JS exception reported through window.onerror - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITransferable.getAnyTransferData] at chrome://mochitests/content/chrome/widget/tests/test_bug444800.xul:56

Comment 29

[rsx11m]

Changing runImageClipboardTests(aCBSvc, "image/jpg") in test_bug444800.xul line 86 to test "image/jpeg" instead should resolve the test failure, given that this is the flavor on the clipboard and no longer translated by any workaround. If it fails for Mac OSX then, that test would probably need to be platform-specific. At least we'd get an idea where "image/jpg" came from in this case. ;-)

Comment 30

[(no longer active)]

Attachment: Patch (v5)

Turns out those work-arounds were there for a reason ;)

Comment 31

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/125438dda903

Comment 32

[(no longer active)]

Backed out because of Bq bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8af411a52d3f

Comment 33

[(no longer active)]

The build bustage was caused by a missing parenthesis.  Fixed and relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/26e12b58f9a7

Comment 34

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/26e12b58f9a7

(Merged by Ed Morley)

Comment 35

[rsx11m]

Good news, this also fixes the initial problem this bug was opened for, testing Thunderbird 16.0a1 on Linux x86_64. Images are now pasted with "data:image/jpeg" URI and a complete attachment name results on sending:

> Content-Type: image/jpeg;
>  name="hdabdahg.jpeg"

I yet have to check the Windows side, but I'd expect it to work properly there as well now. Thanks!

Comment 36

[(no longer active)]

Thanks for testing it.  :-)