224209 - wrong filename on title when viewing an image from a PHP file (Content-Disposition is not used)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Ido Beeri]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007

I didn't know how to put it so I attached a picture, please take a look.
When choosing "View Image", such as "attachment.php&postid=163252" the title
shows "attachment.php". But if you choose "Save Image As", you get automatically
the real filename in the textbox.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Ido Beeri]

Attachment: explanation of the bug

Comment 2

[Boris Zbarsky [:bzbarsky]]

The title just shows the filename from the URL.  The filepicker takes into
account the content-disposition header.....

I don't know whether we care to use content-disposition to set the title in
nsMediaDocument::UpdateTitleAndCharset.  If we do, we should probably come up
with a new Necko interface that channels that can provide a filename hint should
share; I'm tired of having to check for both http and multipart channels all
over....

Comment 3

[Darin Fisher]

bz: i agree with you... we shouldn't have to replicate that code everywhere.  it
seems like a simple helper function could do the trick... we could add code to
nsNetUtils.h, but i suspect it is a fair amount of code.  another case where a
nsNetUtils helper library would maybe make some sense.  or, we could just have
all the channels implement yet another interface :-/

Comment 4

[Hermann Schwab]

and what about some very long parameters on an URL, like:
attachment.php&postid=163252asfshxbxbnvn-cv.n-dvnccvndgfmjfmncvnmcxvnxcfbmjfhjhk
vmxcbmxcbmxcbmxcbmmbxcbmcbmxcbmsfgjfgkjhfgkghk&user=JohnDoe

I don´t want to see crappy strings like this suggested as title.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hermann, please look at the relevant code before you make comments like that. 
The query string is in fact stripped off the filename we show in the titlebar.

Comment 6

[Ido Beeri]

In addition, if you drag-drop the image on desktop (in opposed to "Save As..."),
it also gets the wrong file name.

Comment 7

[Ido Beeri]

In addition, if you drag-drop the image on desktop (in opposed to "Save As..."),
it also gets the wrong file name.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

drag&drop is a different part of the code... can you file a new bug about that?

Comment 9

[Jungshik Shin]

it should be confirmed at least. I'm changing the component to 'networking' for
now (because most of additional code seem to be written in necko)

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

I want to note: if channels want to expose the filename, the answer these days
would not be an additional interface but just another property exposed via
nsIPropertyBag2.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Sounds good to me.  I think we want to expose both the content-disposition and
the content-disposition-filename.  The former would be the actual disposition
method; the latter would be the filename param.

Alternately, we could just expose the header and let all callers parse it as
needed... I think we may need to expose the entire header for imagelib (since it
promises to return it in some cases).

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

not all channels may _have_ an entire content-disposition... I can easily
imagine a protocol that provides a filename but not a content-disposition header.

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK.  So how about we just expose the filename and the disposition type, and
change the imagelib api accordingly?

Comment 14

[Ido Beeri]

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

Comment 15

[Philip Withnall (unavailable)]

*** Bug 339938 has been marked as a duplicate of this bug. ***

Comment 17

[Boris Zbarsky [:bzbarsky]]

Jason, this is the bug where the discussion happened about the sort of API we want for content-disposition in imagelib.  You might be interested in this insofar as it affects bug 474536.

Comment 18

[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 the title would be "cfdesktop.jpg" when content disposition is used, attachment.php otherwise.

Comment 19

[Lukáš Chmela]

This bug is still present in Firefox 31.0.
Eleven years (!) after the original bug report.
Please, could someone take care of it?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Channels now expose a filename: See nsIChannel.contentDispositionFilename

So what this bug would need is just changes to MediaDocument::GetFileName to consider the channel's filename.  Happy to mentor anyone who wants to take this on.

Comment 21

[Akshendra Pratap Singh]

Attachment: 224209.patch

Its not working yet. But I would like to know am I on the right path?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8511602 [details] [diff] [review]
224209.patch

>+    nsCOMPtr<nsIChannel> channel = do_QueryInterface(imageRequest);

imageRequest is not a channel here, so I expect this to return null.  Just pass mChannel instead.

In fact, I bet you could just examine mChannel inside UpdateTitleAndCharset.

Apart from that, and the fact that you're truncating aResult after setting it to the content-disposition filename, this looks resonable.

Comment 23

[Akshendra Pratap Singh]

Attachment: 224209.patch

This one is working.

Comment 24

[Akshendra Pratap Singh]

Attachment: Screenshot from 2014-10-29 20:04:19.png

Image showing working patch

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8513501 [details] [diff] [review]
224209.patch

>+  MediaDocument::UpdateTitleAndCharset(typeStr, mChannel, formatNames,
>+                                         mImageWidth, mImageHeight, status);  

Please fix the indent.

>+  nsCOMPtr<nsIChannel> channel = do_QueryInterface(aChannel);   

aChannel is already an nsIChannel; why is this QI needed?

>+  if (aResult.IsEmpty()) {

This should be checked where we do the GetContentDispositionFilename, with a return if not, right?

Comment 26

[Akshendra Pratap Singh]

> >+  nsCOMPtr<nsIChannel> channel = do_QueryInterface(aChannel);   
> 
> aChannel is already an nsIChannel; why is this QI needed?
> 

I forgot to qref.

I would also like to ask about the charset stuff that is there in the function tail. Because since I return early with the filename(if available) from the channel. The charset is not taken care of. Is it alright? (I got the punctuation correct here :O)

Comment 27

[Boris Zbarsky [:bzbarsky]]

The charset stuff is needed because the GetFilename on the URI returns (possibly-escaped) bytes, not characters.

GetContentDispositionFilename handles all the charset bits and returns characters, so doesn't need any extra work here.

Comment 28

[Akshendra Pratap Singh]

Attachment: 224209.patch

Please can you assign that to me. Thanks!!

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8513707 [details] [diff] [review]
224209.patch

r=me

Do you need this pushed to try, or can you do that?

Comment 30

[Akshendra Pratap Singh]

I don't have a Mozilla hg account. So I cannot push it.

Comment 31

[Boris Zbarsky [:bzbarsky]]

OK.

I just tried doing that, but it looks like you need to rebase across the patches that were checked in for bug 946065 several days ago.

Also, add a commit message?

Comment 32

[Akshendra Pratap Singh]

Attachment: 224209.patch

Added the commit message

Comment 33

[Boris Zbarsky [:bzbarsky]]

You still need to rebase to tip, right?

Comment 34

[Akshendra Pratap Singh]

Does that mean I have to repull the repo and do the changes again? Or is there a better way.

Comment 35

[Boris Zbarsky [:bzbarsky]]

You have to pull the updated repo.  In terms of updating your changes, there's a sed script at https://gist.github.com/poiru/b266b75473a8d9f71d33 that you can use, or just manually edit the file paths in the patch.... or manually do the changes in the new place, yes.

Comment 36

[Akshendra Pratap Singh]

The files this patch is affection have not been moved yet. I updated the repo and tried a build as well.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Sure they have.  What upstream revision is your repo at?  Where exactly are you pulling from?

Comment 38

[Akshendra Pratap Singh]

I used "hg pull" and that pulled from "https://hg.mozilla.org/mozilla-central" according to the log.

Comment 39

[Boris Zbarsky [:bzbarsky]]

hg pull doesn't update your local checked out copy.  It just pulls the changesets into your local version of the repo.  It's the equivalent of "git fetch", not "git pull".

You want "hg pull -u" or once you've done the pull "hg up -r default".

Comment 40

[Akshendra Pratap Singh]

Attachment: 224209_wrongFilenameOnTitleForAttachment.patch

Did the changes at new places.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8514234 [details] [diff] [review]
224209_wrongFilenameOnTitleForAttachment.patch

Thanks.  Pushed https://tbpl.mozilla.org/?tree=Try&rev=741e628dafad

Comment 42

[Boris Zbarsky [:bzbarsky]]

Looks nice and green.

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a8f290db545

Comment 44

[Boris Zbarsky [:bzbarsky]]

Akshendra Pratap Singh, thank you again for the patch!

Comment 45

[Akshendra Pratap Singh]

Thanks for the help

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a8f290db545

Comment 47

[Ido Beeri]

Hip, hip, hooray.