414201 - JPEG images dragged to the Finder have their file extensions changed to .jfif

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

['Ili Butterfield]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012504 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012504 Minefield/3.0b3pre

Whenever I load a JPEG image directly in a browser window or embedded in a web page and drag it to the Finder, the file extension is changed to .jfif, no matter what the original file extension of the image is. The file name itself remains unchanged from the name on the server; it's only the file extension that is altered.

Reproducible: Always

Steps to Reproduce:
1. Load a JPEG image.
2. Drag it to the Desktop or a Finder window.
Actual Results:  
The image is saved with the proper file name, except with the file extension always .jfif.

Expected Results:  
The file extension should remain the same as the name on the server: image.jpg should be saved as image.jpg, image.jpeg as image.jpeg, etc.

Image that are saved through the save dialog keep the chosen file extension with no problems, be it by right-clicking the image and choosing "Save Image As...", entering an image URL into the URL field and option-clicking the green arrow, and so on.

I've found this image mentioned in a couple of other places, like <http://forums.mozillazine.org/viewtopic.php?p=3231348#3231348> and <http://groups.google.com/group/mozilla.feedback/browse_thread/thread/faa9bb30d6f8db6d>. Might just be Mac specific, but that's based just on a sample of four people.

Comment 1

[philippe (part-time)]

That is a pretty recent regression, I think.

That used to happen in the past, see bug 163254.

Comment 2

[philippe (part-time)]

regressed between 20080118_1054 and 20080118_1139 (based on builds from hourly archive: http://hourly-archive.localgho.st/mac.html)
Tested on ppc 10.4

bonsai
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1200682440&maxdate=1200685139

--> bug 412907

Comment 3

[philippe (part-time)]

fails as well on the 20080126 Camino trunk build running on 10.5.1/Intel.

Comment 4

[Mike Schroepfer]

--> florian since that looks like his checkin.

Comment 5

[Florian Quèze [:florian]]

Yes it's my checkin/patch but this was a one line trivial patch to fix an obvious bug.

I guess this code relies on the bug, as it has been there for years.

I suspect .jfif is chosen because it's the first valid extension for image/jpeg when sorted in alphabetical order.  Will need to understand why the code changes the extension even though the orignal one was valid...

Comment 6

[Patrick]

FWIW, i'm not seeing this on Thunderbird version 3.0a1pre (2008012903) with Leopard on PPC.

Comment 7

[Steven Hollingsworth]

I've noticed this issue on 2008013100.

Comment 8

[Deb Richardson [:dria] (plz NEEDINFO)]

Just noticed this on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008020104 Minefield/3.0b3pre.

Comment 9

[Pierre de La Morinerie]

Still there in Beta3 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; fr; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3)

Comment 10

[Florian Quèze [:florian]]

I just spent some time looking more at this bug.  The real problem is that this evaluates to false on Mac:

Components.classes["@mozilla.org/mime;1"]
          .getService(Components.interfaces.nsIMIMEService)
          .getFromTypeAndExtension("image/jpeg", null)
          .extensionExists("jpg");

Comment 11

[Florian Quèze [:florian]]

Attachment: patch v1

This patch does not fix the issue mentioned in comment 10, but fixes the issue of comment 0.

If we want to fix the issue I mentioned in comment 10, I can file another bug for it.

Comment 12

[Johnny Stenback (:jst)]

So the most important issue here is to make sure this doesn't re-introduce bug 279945 (security bug, though opened by now). Passing the extension in scares me a bit, as that's sort of the whole point of bug 279945. We only ever want to save files with an extension that matches the mimetype of what we're saving, and this seems to be moving a step away from that. I'd like to understand this better before I review...

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 303780 [details] [diff] [review]
patch v1

Err... you are totally right, this would regress bug 279945.

So I guess we can either reassign this bug to someone who would know how to fix the issue I described in comment 10, or maybe use this fix as a temporary workaround with an ifdef XP_MACOS.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

FWIW, you're going to get a lot of people (like me!) saying they can't reproduce this bug, probably because of the fact that I don't have any registered handlers for .jfif (if comment #5 is correct) or something.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Josh: can you take a look at this? Pretty bad UE regression.

Comment 16

[philippe (part-time)]

This hasn't been mentioned yet, saving an html page (html complete) also changes the extension jpg --> jfif

Comment 17

[Dan Mills [:thunder]]

One additional data point:  previously, dragging an image to the desktop saved it near the pointer where you dropped it.  Now the jfif file is saved on the right, as if I'd dropped it on the "Desktop" folder in my home directory.

Is that a different bug, or is it a side-effect of this one?

Comment 18

[Louise]

(In reply to comment #17)
> One additional data point:  previously, dragging an image to the desktop saved
> it near the pointer where you dropped it.  Now the jfif file is saved on the
> right, as if I'd dropped it on the "Desktop" folder in my home directory.
> 
> Is that a different bug, or is it a side-effect of this one?
> 
I filed Bug 420663 for that one although it's unconfirmed yet.  So this was a regression?

Comment 19

[philippe (part-time)]

The problem in comment 17 and 18 is bug 401380.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

There's a NSURLFileTypeMappings service available; should be present on at least 10.4 and 10.5, roughly:

00009 @class NSURLFileTypeMappingsInternal;
00010 
00011 @interface NSURLFileTypeMappings : NSObject
00012 {
00013     NSURLFileTypeMappingsInternal *_internal;
00014 }
00015 
00016 + (id)sharedMappings;
00017 - (id)_init;
00018 - (id)_UTIextensionForMIMEType:(id)fp8;
00019 - (id)_UTIMIMETypeForExtension:(id)fp8;
00020 - (NSString*)MIMETypeForExtension:(NSString*)fp8;
00021 - (NSString*)preferredExtensionForMIMEType:(NSString*)fp8;
00022 - (NSArray*)extensionsForMIMEType:(NSString*)fp8;
00023 
00024 @end

Usage:
[[NSURLFileTypeMappings sharedMappings] preferredExtensionForMIMEType:"image/jpeg"] -> "jpg"

[[NSURLFileTypeMappings sharedMappings] extensionsForMIMEType:"image/jpeg"] -> ("jpg", "jpeg", "jpe")

that gives the right extension here; I have no idea why the internet config stuff gives a different one.

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

oh, sharedMappings should be (NSURLFileTypeMappings*)sharedMappings, not (id).

Comment 22

[SpaceCat85]

This bug also appears when I do a "Save As" on the webpage in both Firefox 3.0b4 and Camino trunk nightlies. All of the linked images in the resulting "[webpage name]_files" folder have "jfif" at the end.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: don't believe internet config's lies

Here's a fix.

I hate the entire internet config service, and this entire file; this really needs to die and/or be rewritten to use Launch Services instead of IC.

I, uh, have no idea who should review this.  gavin?

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 310641 [details] [diff] [review]
don't believe internet config's lies

Let's try dmose for this review.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 310641 [details] [diff] [review]
don't believe internet config's lies

shebs is going to be a better reviewer than me here.

Comment 26

[Stan Shebs]

As the recent fiasco with Leopard for building demonstrates (Bug 423672), undocumented API can migrate in a way that breaks things only at runtime. I want to do a bit more research (read: grep in WebKit sources) first.

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Not sure what you mean.. this is an ObjC API that's preset on both 10.4 and 10.5, which is all we care about at the moment (and in the same framework on both, though I don't think that matters for ObjC?).  In any case, this is hidden behind a private WK* method (WKGetPreferredExtensionForMIMEType).

Comment 28

[Stan Shebs]

Comment on attachment 310641 [details] [diff] [review]
don't believe internet config's lies

r+ with the addition of the NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT wrapper, just as the function above does.

Comment 29

[Stan Shebs]

I can't help thinking that we're going to too much trouble here - I understand about the spoofing problem, but if the file's extension matches what is in the list of allowed extensions for its mime type, why not just accept that extension? I think it's less surprising to validate the file extension and retain it, rather than ask Apple what it thinks should be preferred.

Comment 30

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I think the problem is that there is no way (from internet config) to get the full list of extensions.. and the code here is generic, it doesn't know which extension is being validated.  That would have to be fixed further up the call chain from this point.

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated with comments

Updated with objc exception macros; also expanded on the comment explaining why we're doing this.  Carrying review over, requesting 1.9b5 approval.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310806 [details] [diff] [review]
updated with comments

a=beltzner

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

For litmus..:
 - OSX-only
 - go to http://www.mozilla.com/img/home/main-feature.jpg
 - drag image to desktop
 - verify that main-feature.jpg exists on desktop as JPEG file

Comment 34

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated with unit test

Same patch with unit test, carrying over r and a+

Comment 35

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 310819 [details] [diff] [review]
updated with unit test

sr=shaver, thanks for the test!

Comment 36

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in; fix needs to be verified especially on 10.4.

Comment 37

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Filed this as apple bug # 5811207

Comment 38

[Håkan Waara]

Comment on attachment 310819 [details] [diff] [review]
updated with unit test

>+    NSURLFileTypeMappings *map = [NSURLFileTypeMappings sharedMappings];
>+    NSString *mimeStr = [NSString stringWithCString:mimetype.get() encoding:NSASCIIStringEncoding];
>+    NSString *realExtension = map ? [map preferredExtensionForMIMEType:mimeStr] : NULL;
>+    [mimeStr release];

mimeStr is autoreleased, you shouldn't manually release it. It will be done by the autorelease pool. I'm surprised this doesn't crash -- or does it?

>+
>+    if (realExtension) {
>+      temp.Assign([realExtension cStringUsingEncoding:NSASCIIStringEncoding]);
>+
>+      info->AppendExtension(temp);
>+
>+      [realExtension release];

Same with realExtension. 

You should only ever release an object that *you* called |alloc| or |new| on -- others are released by the autorelease pool, which is usually after one spin in the event loop (not sure what the nearest autorelease pool is in Mozilla's event system).

Comment 39

[Håkan Waara]

Reopening per last comment.

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in a fix for that (got rid of releases).

Comment 41

['Ili Butterfield]

(In reply to comment #36)
> Checked in; fix needs to be verified especially on 10.4.
> 

Bit late with this, but confirming the fix works on 10.4/PPC.

Comment 42

[philippe (part-time)]

based on comment 41 (10.4.11) and my testing (10.5.2) with
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032519 Minefield/3.0b5pre
--> verified

Comment 43

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5230 added to Litmus.

Comment 44

[David Dahl :ddahl]

while working on an unrelated bug this test failed:

TEST-UNEXPECTED-FAIL | c:\moz\mozilla-central\obj-i686-pc-mingw32-dbg\_tests\xpcshell\test_docshell\unit\test_bug414201_
jfif.js | test failed (with xpcshell return code: 3), see following log:
  >>>>>>>
  ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to c:\users\ddahl\appdata\local\temp\runxpcshelltests_leaks.log
TEST-INFO | (xpcshell/head.js) | test 1 pending
uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJS
CID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: c:\moz\mozilla-centr
al\mozilla\testing\xpcshell\head.js :: <TOP_LEVEL> :: line 69"  data: no]