Saving images via drag-and-drop loses filename extension

VERIFIED FIXED in Firefox 58

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: abr, Assigned: mrbkap)

Tracking

({regression})

57 Branch
Firefox 59
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox56 wontfix, firefox57- wontfix, firefox58 verified, firefox59 verified)

Details

Attachments

(4 attachments)

Under OS X, dragging an image from the browser onto the desktop creates a file with no filename extension. This results in a file that the OS does not know how to handle, until it is manually renamed to add an extension that matches its type.

Filenames are handled just fine if the image is saved via other means (e.g., right click -> "Save Image As...")

This is a regression.
possible d&d regression on osx?
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to Adam Roach [:abr] from comment #0)
> Under OS X, dragging an image from the browser onto the desktop creates a
> file with no filename extension. This results in a file that the OS does not
> know how to handle, until it is manually renamed to add an extension that
> matches its type.
> 
> Filenames are handled just fine if the image is saved via other means (e.g.,
> right click -> "Save Image As...")
> 
> This is a regression.

I'm unable to reproduce this with either Firefox 55.0.2 or current mozilla-central. Is there a particular website and/or image that reproduces this? What version of Firefox is affected by this?
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(adam)
(In reply to Stephen A Pohl [:spohl] from comment #2)
> I'm unable to reproduce this with either Firefox 55.0.2 or current
> mozilla-central. Is there a particular website and/or image that reproduces
> this? What version of Firefox is affected by this?

I'm getting this on Nightly, downloaded from the Nightly channel (e.g., not locally built). It impacts every image, not just select ones. For example, right now, I can drag your "inverted user" icon to my desktop, and it has no extension.

I've checked in a new profile, and everything works fine, so I'm left believing it is something in my profile. I'll poke around and see if I can isolate it. Leaving n-i set so I remember to get back to this (it may be a few days before I can devote enough time to track this down).
Okay, I've restarted in safe mode, and the problem persists in my normal profile. I double-checked all my non-default prefs, and can't see one that's obviously the problem. I can't spend time on this today, but I'll see if I can isolate it to a pref when I have a block of time.
mystor, could you test this (I think you use a mac).
Flags: needinfo?(michael)
(In reply to Olli Pettay [:smaug] (Currently reviews only for patches which should land to FF57 [2017-09-21]) from comment #5)
> mystor, could you test this (I think you use a mac).

I don't use a mac, but I borrowed one in the office and tested with it & the latest nightly.

As :abr mentions in comment 4, this problem appears to be with a specific profile, and I can't reproduce it on the machine. Dragging images (including the "inverted user" icon used by :sphol) correctly adds an extension to the file.
Flags: needinfo?(michael)
Duplicate of this bug: 1393773
(In reply to Tooru Fujisawa [:arai] from comment #3)
- bug: 1393773

This problem happens in a Firefox 57.0a1 (2017-09-15) (64-bit), macOS 10.12.6. But since the first 57 version this problem come happens.

In my tests when multi process is on the problem occurs  (GIF: https://imgur.com/R5YNfjM) but when is off don't (GIF: https://imgur.com/HaNFpa8). 

I remember old version of Firefox Nightly (maybe 52~53, i cant remember exactly) this same problem have been resolved, but the new version of Firefox Nightly  happens again.
Could you run mozregression[1] to see when this started happening again?

[1] http://mozilla.github.io/mozregression/
Flags: needinfo?(knotvoid)
As a follow-up (to save steps for anyone trying to figure out STR): I blew away my prefs.js entirely, and the problem persisted. So whatever part of my profile causing this issue isn't in prefs.js.
Flags: needinfo?(adam)
Can confirm this issue. Looks like it appeared with Firefox 57, don't think it was there before. Hope it will be fixed soon actually.
Can you run mozregression to see when this started (see comment 9)?
Flags: needinfo?(jbeylovesyou)
This is the output to install mozregression. You only need to post the final result (a regression range) from mozregression once you've been able to run mozregression. It looks like you're having trouble installing mozregression however. You should try to manually uninstall six-1.4.1 and install mozregression again.
Flags: needinfo?(jbeylovesyou)
Well, that was a wild ride. The change in behavior appears to be quite old -- so old, in fact, that there are no taskcluster binaries to track it down to. Nightly bisection at least gives us a date range:

 7:54.33 INFO: Last good revision: a523d4c7efe2f43dd6b25a176c07b729918d550f (2015-11-19)
 7:54.33 INFO: First bad revision: 3835b568092ae3b71adc931d24928670ad7141a7 (2015-11-20)

I didn't start seeing this until sometime in the 57 nightly cycle, so I posit that it consists of two bugs: one, introduced in late 2015, that does The Wrong Thing if some value in the profile takes an unexpected value, and another one, introduced sometime during the 57 development cycle, that causes that unexpected value in the profile to be set.
Flags: needinfo?(knotvoid)
Flags: needinfo?(jbeylovesyou)
I dug into quite a few of the changesets here, and can't find anything obvious:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a523d4c7efe2f43dd6b25a176c07b729918d550f&tochange=3835b568092ae3b71adc931d24928670ad7141a7

I'm back to waiting until I have enough time to yank various files out of my profile and see which one is triggering this...
It would be great to hear if others are getting the same result as in comment 15.
Flags: needinfo?(jbeylovesyou)
Okay, on a more rigorous bisection of files in my profile, I tracked it down to handlers.json; and, in particular, to the presence of this attribute in it (which I have beautified here to make it easier to read):

    "image/jpeg": {
      "action": 4,
      "ask": true,
      "extensions": [
        "jpg",
        "jpeg",
        "jpe"
      ]
    },

If I remove that one entry, drag and drop works just fine.
I might suspect that this has something to do with the e10s rewrite that landed as part of Bug 1190018, but that's just outside the regression range I tracked this to. I wonder if mozregression might lose some degree of precision when working back that far?
(In reply to Adam Roach [:abr] from comment #20)
> Okay, on a more rigorous bisection of files in my profile, I tracked it down
> to handlers.json; and, in particular, to the presence of this attribute in
> it (which I have beautified here to make it easier to read):
> 
>     "image/jpeg": {
>       "action": 4,
>       "ask": true,
>       "extensions": [
>         "jpg",
>         "jpeg",
>         "jpe"
>       ]
>     },
> 
> If I remove that one entry, drag and drop works just fine.

This is great! I'm still sidetracked with a number of macOS 10.13 regressions, but this should help to reproduce locally and start figuring out what's going on.
Flags: needinfo?(jbeylovesyou)
I forgot to ask: does adding this to handlers.json in a new profile reproduce the issue?
Flags: needinfo?(adam)
(In reply to Stephen A Pohl [:spohl] from comment #23)
> I forgot to ask: does adding this to handlers.json in a new profile
> reproduce the issue?

Yes, it does. If you copy this handlers.json file (attached) into any profile, it appears to be sufficient to cause the problem to arise.
Flags: needinfo?(adam)
Perfect! Stefan, I remember you did some work with file extensions. Is this something that you might be interested in looking into? Fyi, there is a sample project[1] from Apple to see what's currently on the clipboard, which might be helpful here.

[1] https://developer.apple.com/library/content/samplecode/ClipboardViewer/Introduction/Intro.html
Flags: needinfo?(stefanh)
Removing the regression window wanted flag due to comment 16 and comment 20
(In reply to Adam Roach [:abr] from comment #20)
> Okay, on a more rigorous bisection of files in my profile, I tracked it down
> to handlers.json; and, in particular, to the presence of this attribute in
> it (which I have beautified here to make it easier to read):
> 
>     "image/jpeg": {
>       "action": 4,
>       "ask": true,
>       "extensions": [
>         "jpg",
>         "jpeg",
>         "jpe"
>       ]
>     },
> 
> If I remove that one entry, drag and drop works just fine.

I have the exact same problem (works fine in a fresh profile, doesn't work in existing profile) on Mac Nightly.

Replicating your solution also allows me to drag-n-drop an image with file extension intact.

I had the same issue with PNG file, removed the image/png entry from handlers.json and that fixed it too.

I wonder what's causing this and I'm surprised there hasn't been more reports.
Keeping the ni? until I've had some time to look at this.
FWIW, I think what we're seeing is the interaction of this code:

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentAreaDragDrop.cpp#604

...with this code:

https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/HandlerServiceParent.cpp#190

You'll see that the drag-and-drop "change the filename extension" code only executes when a MIME handler is set (presumably by handlers.json). I don't know the architecture here very well, but I presume drag-and-drop runs in the parent process, and ends up hitting the proxy code in ProxyMimeInfo, which is just stubbed out with a "not implemented" error.

I haven't tried it myself, but I think you could band-aid this by performing error checking at the calling site and skipping the SetFileExtension() call if the call to GetPrimaryExtension() fails. The correct solution would be finishing the ProxyMIMEInfo class, at least for those methods that get used in the parent process -- but I suspect this would take a fair bit of work.

I would encourage testing the band-aid I propose above and uplifting it to 57: as comment 27 correctly points out, it's surprising that we haven't seen more reports of this. I suspect that a lot of mac users will be surprised when 57 rolls out to release and forces e10s on.
Okay, I'm apparently half-right. We're definitely hitting the "return NS_ERROR_NOT_IMPLEMENTED;" in ProxyMIMEInfo::AppendExtension -- but it's being called from here: 

https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsHandlerService-json.js#405

The good news is that I've verified the root cause (the proxy object never got fully implemented). The bad news is that I don't see a simple wallpaper solution here -- but someone who understands nsHandlerService-json.js might.

I'm tagging :paolo, as he seems to have been in here recently and might be able to see a quick fix.
Flags: needinfo?(paolo.mozmail)
[Tracking Requested - why for this release]:
Sounds like a rather broken dnd handling when this occurs, though based on comment 6 this doesn't happen always.
(In reply to Adam Roach [:abr] from comment #30)
> The good news is that I've verified the root cause (the proxy object never
> got fully implemented). The bad news is that I don't see a simple wallpaper
> solution here -- but someone who understands nsHandlerService-json.js might.
> I'm tagging :paolo, as he seems to have been in here recently and might be
> able to see a quick fix.

Eek, I've just looked at this code after reading your comments, and it looks really broken :-( I've never fully understood this IPC architecture, but that seems to be because all of it is was implemented as a simple wallpaper solution. This is understandable as this module has not had an owner for a long time. Bug 1382323 was another e10s issue caused by some portions of this not being implemented.

I can't think of an easier solution off the top of my head, other than trying to change the drag-and-drop code so that it does whatever it's doing for the case where "handlers.json" has no entry in it, but I don't know what the side effects could be. What I can say is that in most cases we can trust the operating system with providing the right type/extension associations without having to specify them in "handlers.json", but we didn't stop storing this information for backwards compatiblity, as changing this may also have unintended side effects.

I'm not sure if any change in behavior would be upliftable to 57 at this point.

In the future we should rewrite most of this to get rid of sync IPC eniterly, but this is dependent on making handler choice asynchronous in the networking code.
Flags: needinfo?(paolo.mozmail)
After I deleted handlers.json, Firefox Nightly regenerated a default one when it ran again. So far drag-n-drop has been working as expected but I haven't done any testing with different file types.
(In reply to Stephen A Pohl [:spohl] from comment #25)
> Perfect! Stefan, I remember you did some work with file extensions. Is this
> something that you might be interested in looking into?

Reading the latest comments, I don't think I'm the right person.
Flags: needinfo?(stefanh)
Moving this to a (hopefully) more appropriate component.
Component: Drag and Drop → IPC
Component: IPC → File Handling
Product: Core → Firefox
How is this an IPC bug?
This is some issue with handlers.json (which I have no idea what it is about). 
I guess the relevant code landed in bug 1287660 or bug 1287658.
(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #36)
> How is this an IPC bug?

Comment 32 referred to IPC quite a bit and I may have misunderstood. Thanks for moving to the right component.
Priority: -- → P2
(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #36)
> I guess the relevant code landed in bug 1287660 or bug 1287658.

The bug exhibits as far back as 2015 (see the regression range above) -- so, if anything, the code in those bugs was a reimplementation of this flaw. After my digging, I'm fairly certain that the roots for this issue landed in Bug 1190018. From that point forward, it was a ticking time bomb waiting for e10s to be forced on for all users.
I have this issue beginning in FF 56.0 (64-bit) on macOS High Sierra.

Previous versions I was able to drag an image from a webpage to the desktop, but now it drags the image without a file extension. I must then add the extension manually (and sleuth the file type!) to open it in an image editor.
I also have this issue in FF 57.0b10 on macOS High Sierra (10.13).
[Tracking Requested - why for this release]:
Sounds like this issue is happening rather often.
(see comment 38)
Flags: needinfo?(lassey)
I don't think Brad is going to be interested in working on this bug. He left Mozilla a while ago.
(In reply to Markus Stange [:mstange] from comment #43)
> I don't think Brad is going to be interested in working on this bug. He left
> Mozilla a while ago.

Ok, maybe Blake knows who could take a look here (see comment 38)?
Flags: needinfo?(lassey) → needinfo?(mrbkap)
Duplicate of this bug: 1411206
I'm looking into this.
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
Windows 10 is also affected by this, so presumably this is simply a univeral issue for all used - testing using image sharing sites like imgur.com and image subreddits on v57 and v58 shows that files with the .jpg extension lose their extension, while .gif and .png files are named correctly when dragged from the browser onto an OS-managed location (desktop, folder, etc).
(In reply to Mike "Pomax" Kamermans [:pomax] from comment #47)
> Windows 10 is also affected by this...

Thanks -- I was worried that might be the case. I've updated the 'Platform' field accordingly.

> ...files with the .jpg extension
> lose their extension, while .gif and .png files are named correctly when
> dragged from the browser onto an OS-managed location (desktop, folder, etc).

To be clear, all image types can be affected -- all it takes is a handler entry for the corresponding mime type. See comment 27, for example, for someone who has this issue with .png files.
OS: Mac OS X → All
Hardware: x86 → All
Duplicate of this bug: 1401977
Duplicate of this bug: 1412335
This really should be p1, just based on the number of duplicates.
Priority: P2 → P1
I think I have a patch for this but need to test it. I'm hoping to have a full patch (with a mochitest) by Monday.
Also, we really need to figure out how to de-sync-ify all of these APIs. I'll file a followup bug once this lands to look into what's required there.
Tracking - based on the won't fix resolution. I see this bug on 10.13 when dragging and dropping images, but as noted earlier it does work using the "save as" menu.
I've given up on trying to write a mochitest for this for the time being.
(In reply to Blake Kaplan (:mrbkap) from comment #56)
> I've given up on trying to write a mochitest for this for the time being.

Makes sense to me, as there isn't a lot of coverage to begin with. It's better to spend time building proper test coverage when we develop the asynchronous version of the handler service API.
Comment on attachment 8927523 [details]
Bug 1389836 - Push extensions from MIME info objects to the child.

https://reviewboard.mozilla.org/r/197912/#review203896


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: uriloader/exthandler/HandlerServiceParent.cpp:164
(Diff revision 1)
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIMIMEINFO
>    NS_FORWARD_NSIHANDLERINFO(mProxyHandlerInfo->);
>  
> -  explicit ProxyMIMEInfo(HandlerInfo aHandlerInfo) : mProxyHandlerInfo(new ProxyHandlerInfo(aHandlerInfo)) {}
> +  explicit ProxyMIMEInfo(HandlerInfo aHandlerInfo)

Warning: The parameter 'ahandlerinfo' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

  explicit ProxyMIMEInfo(HandlerInfo aHandlerInfo)
                                     ^
                         const      &
Comment on attachment 8927522 [details]
Bug 1389836 - Put HandlerServiceChild in the proper namespace.

https://reviewboard.mozilla.org/r/197910/#review204238

::: commit-message-933f9:1
(Diff revision 1)
> +Bug 1389836 - Put this class in the proper namespace. r=bz

Please say what class, in the commit message, not "this class".

::: commit-message-933f9:3
(Diff revision 1)
> +Bug 1389836 - Put this class in the proper namespace. r=bz
> +
> +Non ns-prefixed classes are supposed to be in the mozilla::dom namespace. This

Or the "mozilla" namespace, or "mozilla::css" namespace, or whatnot.

Implementations of DOM things are supposed to be in the "mozilla::dom" namespace.

I would really rather we put this in "mozilla", not "mozilla::dom".
Attachment #8927522 - Flags: review?(bzbarsky) → review-
Comment on attachment 8927522 [details]
Bug 1389836 - Put HandlerServiceChild in the proper namespace.

https://reviewboard.mozilla.org/r/197910/#review204306
Attachment #8927522 - Flags: review?(bzbarsky) → review+
Comment on attachment 8927523 [details]
Bug 1389836 - Push extensions from MIME info objects to the child.

https://reviewboard.mozilla.org/r/197912/#review204308

::: uriloader/exthandler/ContentHandlerService.cpp:90
(Diff revision 2)
>    aInfo->GetPreferredAction(&action);
> -  HandlerInfo info(type, isMIMEInfo, description, alwaysAskBeforeHandling, happ, happs, action);
> +  HandlerInfo info(type,
> +                   isMIMEInfo,
> +                   description,
> +                   alwaysAskBeforeHandling,
> +                   extensions,

Wan to Move(extensions) here too, so we can avoid copying it?
Attachment #8927523 - Flags: review?(bzbarsky) → review+
Comment on attachment 8928331 [details]
Bug 1389836 - Don't recreate mHandlerInfo for every little change.

https://reviewboard.mozilla.org/r/199538/#review204704

r=me.  Nice catch!
Attachment #8928331 - Flags: review?(bzbarsky) → review+
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a89b42b23213
Put HandlerServiceChild in the proper namespace. r=bz
https://hg.mozilla.org/integration/autoland/rev/469978a83f5c
Push extensions from MIME info objects to the child. r=bz
https://hg.mozilla.org/integration/autoland/rev/43b13dc4ab4c
Don't recreate mHandlerInfo for every little change. r=bz
Issue was resolved in version prior to the update on the 14th November 2017 which once again introduced the problem on Mac.
Images not saving with extension when drag and dropped onto desktop.

Issue in version: 57.0 (64-bit) Firefox Quantum (Mac)
Please request Beta approval on this when you're comfortable doing so.
Flags: needinfo?(mrbkap)
(In reply to u605854 from comment #71)
> Issue was resolved in version prior to the update on the 14th November 2017
> which once again introduced the problem on Mac.
> Images not saving with extension when drag and dropped onto desktop.
> 
> Issue in version: 57.0 (64-bit) Firefox Quantum (Mac)

Ditto - Firefox 57 auto-installed for me (against my preferences, which is a whole other can of worms), and immediately drag-and-drop no longer saves the extension. Checked with some other users and they had the same issue.

Issue in version: 57.0 (64-bit) Firefox Quantum 

Running on OS: macOS Sierra v. 10.12.6
Duplicate of this bug: 1417584
Comment on attachment 8927522 [details]
Bug 1389836 - Put HandlerServiceChild in the proper namespace.

Approval Request Comment
[Feature/Bug causing the regression]: e10s
[User impact if declined]: Dragging certain images out of Firefox will save them with the wrong extension.
[Is this code covered by automated tests?]: No :-(
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The change is just passing more data around over IPC.
[String changes made/needed]: n/a
Flags: needinfo?(mrbkap)
Attachment #8927522 - Flags: approval-mozilla-beta?
Attachment #8927523 - Flags: approval-mozilla-beta?
Attachment #8928331 - Flags: approval-mozilla-beta?
Hi Adam,
Can you help check if this issue was fixed in the latest nightly?
Flags: needinfo?(adam)
(In reply to Gerry Chang [:gchang] from comment #78)
> Can you help check if this issue was fixed in the latest nightly?

Yes. I checked 59.0a1 (2017-11-23), and this problem is fixed. Thanks!
Flags: needinfo?(adam)
Comment on attachment 8927522 [details]
Bug 1389836 - Put HandlerServiceChild in the proper namespace.

Fix an issue that dragging certain images out of Firefox will be created with wrong extension. Beta58+.
Attachment #8927522 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8927523 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8928331 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Duplicate of this bug: 1418073
I have reproduced this issue using Firefox  57.0b3 (2017.09.25) on Mac OS X 10.13.2.
I can confirm this issue is fixed, I verified using Firefox 58.0b10 on  Mac OS X 10.13.2.
Duplicate of this bug: 1430026
You need to log in before you can comment on or make changes to this bug.