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

VERIFIED FIXED

Status

Core Graveyard
File Handling
P2
normal
VERIFIED FIXED
10 years ago
a year ago

People

(Reporter: 'Ili Butterfield, Assigned: vlad)

Tracking

({regression})

Trunk
PowerPC
Mac OS X
regression
Bug Flags:
blocking1.9 +
in-testsuite +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
That is a pretty recent regression, I think.

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

Comment 2

10 years ago
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
Blocks: 412907
Severity: minor → normal
Status: UNCONFIRMED → NEW
Component: OS Integration → File Handling
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: unspecified → Trunk

Comment 3

10 years ago
fails as well on the 20080126 Camino trunk build running on 10.5.1/Intel.
Flags: blocking1.9?
QA Contact: os.integration → file-handling

Comment 4

10 years ago
--> florian since that looks like his checkin.
Assignee: nobody → florian
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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

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

Comment 7

10 years ago
I've noticed this issue on 2008013100.
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.
Still there in Beta3 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; fr; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3)
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");
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.
Attachment #303780 - Flags: superreview?(jst)
Attachment #303780 - Flags: review?(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 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.
Attachment #303780 - Attachment is obsolete: true
Attachment #303780 - Flags: superreview?(jst)
Attachment #303780 - Flags: review?(jst)
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.
Josh: can you take a look at this? Pretty bad UE regression.
Flags: blocking1.9+

Updated

10 years ago
Flags: tracking1.9+

Comment 16

10 years ago
This hasn't been mentioned yet, saving an html page (html complete) also changes the extension jpg --> jfif
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

9 years ago
(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?
The problem in comment 17 and 18 is bug 401380.
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.
oh, sharedMappings should be (NSURLFileTypeMappings*)sharedMappings, not (id).

Comment 22

9 years ago
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.
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?
Assignee: florian → vladimir
Status: NEW → ASSIGNED
Attachment #310641 - Flags: review?
Comment on attachment 310641 [details] [diff] [review]
don't believe internet config's lies

Let's try dmose for this review.
Attachment #310641 - Flags: review? → review?(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.
Attachment #310641 - Flags: review?(dmose) → review?(stanshebs)

Comment 26

9 years ago
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.
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

9 years ago
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.
Attachment #310641 - Flags: review?(stanshebs) → review+

Comment 29

9 years ago
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.
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.
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.
Attachment #310641 - Attachment is obsolete: true
Attachment #310806 - Flags: review+
Attachment #310806 - Flags: approval1.9b5?
Comment on attachment 310806 [details] [diff] [review]
updated with comments

a=beltzner
Attachment #310806 - Flags: approval1.9b5? → approval1.9b5+
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
Flags: in-litmus?
Created attachment 310819 [details] [diff] [review]
updated with unit test

Same patch with unit test, carrying over r and a+
Attachment #310806 - Attachment is obsolete: true
Attachment #310819 - Flags: review+
Attachment #310819 - Flags: approval1.9b5+
Comment on attachment 310819 [details] [diff] [review]
updated with unit test

sr=shaver, thanks for the test!
Attachment #310819 - Flags: superreview+
Checked in; fix needs to be verified especially on 10.4.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Filed this as apple bug # 5811207

Comment 38

9 years ago
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

9 years ago
Reopening per last comment.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Checked in a fix for that (got rid of releases).
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 41

9 years ago
(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.
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
Status: RESOLVED → VERIFIED
https://litmus.mozilla.org/show_test.cgi?id=5230 added to Litmus.
Flags: in-litmus? → in-litmus+

Comment 44

8 years ago
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]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.