Last Comment Bug 414201 - JPEG images dragged to the Finder have their file extensions changed to .jfif
: JPEG images dragged to the Finder have their file extensions changed to .jfif
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: File Handling (show other bugs)
: Trunk
: PowerPC Mac OS X
: P2 normal with 5 votes (vote)
: ---
Assigned To: Vladimir Vukicevic [:vlad] [:vladv]
:
Mentors:
http://www.mozilla.com/img/home/main-...
Depends on:
Blocks: 412907
  Show dependency treegraph
 
Reported: 2008-01-27 04:54 PST by 'Ili Butterfield
Modified: 2009-09-15 16:08 PDT (History)
26 users (show)
mbeltzner: blocking1.9+
vladimir: in‑testsuite+
mozillamarcia.knous: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (1.15 KB, patch)
2008-02-16 15:20 PST, Florian Quèze [:florian] [:flo]
no flags Details | Diff | Review
don't believe internet config's lies (2.26 KB, patch)
2008-03-19 17:02 PDT, Vladimir Vukicevic [:vlad] [:vladv]
stanshebs: review+
Details | Diff | Review
updated with comments (3.27 KB, patch)
2008-03-20 11:38 PDT, Vladimir Vukicevic [:vlad] [:vladv]
vladimir: review+
mbeltzner: approval1.9b5+
Details | Diff | Review
updated with unit test (5.80 KB, patch)
2008-03-20 13:01 PDT, Vladimir Vukicevic [:vlad] [:vladv]
vladimir: review+
shaver: superreview+
vladimir: approval1.9b5+
Details | Diff | Review

Description 'Ili Butterfield 2008-01-27 04:54:31 PST
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) 2008-01-27 17:41:43 PST
That is a pretty recent regression, I think.

That used to happen in the past, see bug 163254.
Comment 2 philippe (part-time) 2008-01-27 18:14:08 PST
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) 2008-01-27 18:16:19 PST
fails as well on the 20080126 Camino trunk build running on 10.5.1/Intel.
Comment 4 Mike Schroepfer 2008-01-29 22:18:12 PST
--> florian since that looks like his checkin.
Comment 5 Florian Quèze [:florian] [:flo] 2008-01-30 07:38:21 PST
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 2008-01-30 08:58:39 PST
FWIW, i'm not seeing this on Thunderbird version 3.0a1pre (2008012903) with Leopard on PPC.
Comment 7 Steven Hollingsworth 2008-01-31 18:26:03 PST
I've noticed this issue on 2008013100.
Comment 8 Deb Richardson [:dria] (plz NEEDINFO) 2008-02-01 19:27:51 PST
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 2008-02-16 06:52:40 PST
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] [:flo] 2008-02-16 13:54:23 PST
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] [:flo] 2008-02-16 15:20:16 PST
Created attachment 303780 [details] [diff] [review]
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, jst@mozilla.com) 2008-02-19 14:00:07 PST
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] [:flo] 2008-02-19 15:26:23 PST
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] 2008-02-26 13:07:48 PST
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] 2008-02-29 11:43:23 PST
Josh: can you take a look at this? Pretty bad UE regression.
Comment 16 philippe (part-time) 2008-02-29 19:28:00 PST
This hasn't been mentioned yet, saving an html page (html complete) also changes the extension jpg --> jfif
Comment 17 Dan Mills [:thunder] 2008-03-03 17:28:01 PST
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 2008-03-03 18:02:10 PST
(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) 2008-03-03 18:08:25 PST
The problem in comment 17 and 18 is bug 401380.
Comment 20 Vladimir Vukicevic [:vlad] [:vladv] 2008-03-08 12:02:40 PST
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] 2008-03-08 12:05:07 PST
oh, sharedMappings should be (NSURLFileTypeMappings*)sharedMappings, not (id).
Comment 22 SpaceCat85 2008-03-17 19:15:44 PDT
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] 2008-03-19 17:02:02 PDT
Created attachment 310641 [details] [diff] [review]
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] 2008-03-19 17:04:40 PDT
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 (:dmose) 2008-03-19 17:29:48 PDT
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 2008-03-19 18:23:05 PDT
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] 2008-03-19 19:17:49 PDT
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 2008-03-20 11:15:33 PDT
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 2008-03-20 11:19:47 PDT
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] 2008-03-20 11:34:00 PDT
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] 2008-03-20 11:38:14 PDT
Created attachment 310806 [details] [diff] [review]
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] 2008-03-20 11:45:18 PDT
Comment on attachment 310806 [details] [diff] [review]
updated with comments

a=beltzner
Comment 33 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-20 11:47:56 PDT
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] 2008-03-20 13:01:46 PDT
Created attachment 310819 [details] [diff] [review]
updated with unit test

Same patch with unit test, carrying over r and a+
Comment 35 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-20 14:25:31 PDT
Comment on attachment 310819 [details] [diff] [review]
updated with unit test

sr=shaver, thanks for the test!
Comment 36 Vladimir Vukicevic [:vlad] [:vladv] 2008-03-20 14:39:59 PDT
Checked in; fix needs to be verified especially on 10.4.
Comment 37 Vladimir Vukicevic [:vlad] [:vladv] 2008-03-20 14:51:30 PDT
Filed this as apple bug # 5811207
Comment 38 Håkan Waara 2008-03-20 15:52:09 PDT
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 2008-03-20 16:11:35 PDT
Reopening per last comment.
Comment 40 Vladimir Vukicevic [:vlad] [:vladv] 2008-03-20 16:24:26 PDT
Checked in a fix for that (got rid of releases).
Comment 41 'Ili Butterfield 2008-03-25 22:04:21 PDT
(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) 2008-03-25 22:58:49 PDT
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 - use ni] 2008-04-01 13:07:52 PDT
https://litmus.mozilla.org/show_test.cgi?id=5230 added to Litmus.
Comment 44 David Dahl :ddahl 2009-09-15 16:08:02 PDT
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]

Note You need to log in before you can comment on or make changes to this bug.