291837 - Content-Disposition is not used when drag&dropping an image from a PHP file

Status: NEW

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P5)

Description

[Ido Beeri]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

See Bug 224209

When you "Save As..." an image from a php file (i.e.
"attachment.php&postid=163252") it shows you the real filename (i.e.
"mypicture.gif"). That's good.
But when you're dragging the image to desktop you get the filename "attachment.gif".

Reproducible: Always

Steps to Reproduce:

Comment 1

[Boris Zbarsky [:bzbarsky]]

Marking dependent on bug 224209 for now, since the arch work there may make this
easier...

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Not just PHP, of course, but any sort of "dynamic image-serving scripts" (Bugzilla attachment CGIs)....

Comment 3

[Chris]

This behaviour is still occuring.

Would a bugfix for this need intricate knowledge of firefox internals or is this something a person with a couple years of general C++ experience could fix in a few days?

Comment 4

[ffux]

I would have thought someone with that experience would be able to answer the question by having a quick dig into the source. My guess is that it would be fairly simple by comparing what Save As does to get the correct filename.

Comment 5

[Xavier Robin]

A good testcase can be found here: http://forum.magicball.net/attachment.php?s=&postid=163252

It has:
> Content-disposition: inline; filename*=utf-8''cfdesktop.jpg

So it is named "cfdesktop.jpg" when content disposition is used, attachment.php otherwise.

Comment 6

[u212413]

Bug 440930 and bug 449588 are dupes of this bug, as I wrote there. Would
somebody mark them as dupes, please?

Comment 9

[Xavier Robin]

As I said in Comment #4 of bug 449588, it isn't a dupe because solving this bug is only one of the possibilities to fix bug 449588. The other being *not* to use content-disposition in File > Save as. This other solution might be easier to implement and the inconsistency could be fixed faster. It will always be possible to revert once this bug is solved.

Please feel free to reopen it if you agree.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

We're not going to stop using content-disposition in File > Save as.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Draft Patch

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 414866 [details] [diff] [review]
Draft Patch

Patch itself is fairly straightforward, though the code is mostly duped from helper functions that go along with nsExternalAppHelperService.  It would be cool if we could somehow expose those functions more widely, maybe in nsNetUtil?  I also can't see an easy way to test this.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Any chance for some mochitests?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 414866 [details] [diff] [review]
Draft Patch

>diff --git a/content/base/src/nsContentAreaDragDrop.cpp b/content/base/src/nsContentAreaDragDrop.cpp
>--- a/content/base/src/nsContentAreaDragDrop.cpp
>+++ b/content/base/src/nsContentAreaDragDrop.cpp
>@@ -86,17 +86,19 @@
> #include "nsIPresShell.h"
> #include "nsPresContext.h"
> #include "nsIDocShellTreeItem.h"
> #include "nsIFrame.h"
> #include "nsRange.h"
> #include "nsIWebBrowserPersist.h"
> #include "nsEscape.h"
> #include "nsContentUtils.h"
>+#include "nsIMIMEHeaderParam.h"
> #include "nsIMIMEService.h"
>+#include "imgICache.h"
> #include "imgIContainer.h"
> #include "imgIRequest.h"
> #include "nsContentCID.h"
> #include "nsDOMDataTransfer.h"
> #include "nsISelectionController.h"
> #include "nsFrameSelection.h"
> #include "nsIDOMEventTarget.h"
> #include "nsWidgetsCID.h"
>@@ -816,16 +818,17 @@ nsTransferableFactory::GetNodeString(nsI
> 
> 
> nsresult
> nsTransferableFactory::Produce(nsDOMDataTransfer* aDataTransfer,
>                                PRBool* aCanDrag,
>                                PRBool* aDragSelection,
>                                nsIContent** aDragNode)
> {
>+  nsresult rv;
>   NS_PRECONDITION(aCanDrag && aDragSelection && aDataTransfer && aDragNode,
>                   "null pointer passed to Produce");
>   NS_ASSERTION(mWindow, "window not set");
>   NS_ASSERTION(mSelectionTargetNode, "selection target node should have been set");
> 
>   *aDragNode = nsnull;
> 
>   nsIContent* dragNode = nsnull;
>@@ -974,22 +977,66 @@ nsTransferableFactory::Produce(nsDOMData
> 
>         // grab the image data, and its request.
>         nsCOMPtr<imgIContainer> img =
>           nsContentUtils::GetImageFromContent(image,
>                                               getter_AddRefs(imgRequest));
> 
>         nsCOMPtr<nsIMIMEService> mimeService =
>           do_GetService("@mozilla.org/mime;1");
Do you actually need this if content-disposition is used?


>+              if (NS_SUCCEEDED(rv) && !mImageDestFileName.IsEmpty()) {
>+                mImageDestFileName.ReplaceChar(FILE_PATH_SEPARATOR
>+                                               FILE_ILLEGAL_CHARACTERS, '-');
>+                mImage = img;
>+                imgUri = nsnull;
>+              }
>+            }
>+          }
>+        }
>+        // If we can't use the cache to get the correct filename, at least
>+        // fix up the file's extension
>+        if (imgUri && mimeService) {

Setting imgURI to nsnull is a bit ugly, IMO. Could you just have some PRBool variable to
indicate if file's extension needs to be fixed up?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 414866 [details] [diff] [review]
Draft Patch

So if you could update the patch. I'll re-review

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 414866 [details] [diff] [review]
Draft Patch

Thanks, it'll be a few days before I have time to update the patch.  Any thoughts on avoiding the code duplication? (Most of it is copy pasted from nsExternalAppHelperService.cpp where all of the content-disposition helper functions live)

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Yeah, it would be good to use the same code as what nsExternalAppService uses.
That one seems to handle also multipart channels.

Not sure yet which interface should have the new method.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

nsIMIMEService seems to have similar methods and nsExternalAppService implements
that interface anyway.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

After thinking about this some more, I think the right way to do this is to fix Bug 356086 so that the underlying channel will have a content-disposition-filename that we can pull straight out of imgICache.

Comment 20

[Anonymous]

This bug (first identified ~10 years ago(!)) is still occurring. The old test case posted earlier still shows the problem: http://forum.magicball.net/attachment.php?s=&postid=163252.

Comment 22

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, 4 years untouched DOM/Storage bugs' priority.

If you have reason to believe this is wrong (especially for the severity), please write a comment and ni :jstutte.

Comment 23

[ffux]

Bug 291837 Opened 16 years ago

FFS. Yeah downgrade will really help get it fixed.