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

RESOLVED FIXED in mozilla36



15 years ago
2 years ago


(Reporter: Ido Beeri, Assigned: Akshendra Pratap Singh, Mentored)


(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [lang=c++], URL)


(3 attachments, 4 obsolete attachments)



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

15 years ago
Created attachment 134490 [details]
explanation of the bug
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

Comment 3

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

15 years ago
and what about some very long parameters on an URL, like:

I don´t want to see crappy strings like this suggested as title.
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.
Product: Browser → Seamonkey

Comment 6

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

Comment 7

13 years ago
In addition, if you drag-drop the image on desktop (in opposed to "Save As..."),
it also gets the wrong file name.
drag&drop is a different part of the code... can you file a new bug about that?

Comment 9

13 years ago
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)
Severity: trivial → enhancement
Component: General → Networking
Ever confirmed: true
OS: Windows XP → All
Product: Mozilla Application Suite → Core
Hardware: PC → All
Summary: wrong filename on title when viewing an image from a PHP file → wrong filename on title when viewing an image from a PHP file (Content-Disposition is not used)
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
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).
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.
OK.  So how about we just expose the filename and the disposition type, and
change the imagelib api accordingly?

Comment 14

13 years ago
--> Filed Bug 291837 - "Content-Disposition is not used when drag&dropping an
image from a PHP file"
*** Bug 339938 has been marked as a duplicate of this bug. ***
Duplicate of this bug: 341489
Assignee: general → nobody
QA Contact: general → networking
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.
Blocks: 474536

Comment 18

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

4 years ago
This bug is still present in Firefox 31.0.
Eleven years (!) after the original bug report.
Please, could someone take care of it?
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.
Mentor: bzbarsky@mit.edu
Component: Networking → DOM
Whiteboard: [lang=c++]

Comment 21

3 years ago
Created attachment 8511602 [details] [diff] [review]

Its not working yet. But I would like to know am I on the right path?
Attachment #8511602 - Flags: feedback?(bzbarsky)
Comment on attachment 8511602 [details] [diff] [review]

>+    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.
Attachment #8511602 - Flags: feedback?(bzbarsky) → feedback+

Comment 23

3 years ago
Created attachment 8513501 [details] [diff] [review]

This one is working.
Attachment #8511602 - Attachment is obsolete: true
Attachment #8513501 - Flags: feedback?(bzbarsky)

Comment 24

3 years ago
Created attachment 8513502 [details]
Screenshot from 2014-10-29 20:04:19.png

Image showing working patch
Comment on attachment 8513501 [details] [diff] [review]

>+  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?
Attachment #8513501 - Flags: feedback?(bzbarsky) → feedback+

Comment 26

3 years ago
> >+  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)
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)

Comment 28

3 years ago
Created attachment 8513707 [details] [diff] [review]

Please can you assign that to me. Thanks!!
Attachment #8513501 - Attachment is obsolete: true
Attachment #8513707 - Flags: review?(bzbarsky)
Assignee: nobody → akshendra521994
Comment on attachment 8513707 [details] [diff] [review]


Do you need this pushed to try, or can you do that?
Flags: needinfo?(akshendra521994)
Attachment #8513707 - Flags: review?(bzbarsky) → review+

Comment 30

3 years ago
I don't have a Mozilla hg account. So I cannot push it.
Flags: needinfo?(akshendra521994)

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?


3 years ago

Comment 32

3 years ago
Created attachment 8513738 [details] [diff] [review]

Added the commit message
Attachment #8513707 - Attachment is obsolete: true
You still need to rebase to tip, right?

Comment 34

3 years ago
Does that mean I have to repull the repo and do the changes again? Or is there a better way.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)

Comment 36

3 years ago
The files this patch is affection have not been moved yet. I updated the repo and tried a build as well.
Sure they have.  What upstream revision is your repo at?  Where exactly are you pulling from?

Comment 38

3 years ago
I used "hg pull" and that pulled from "https://hg.mozilla.org/mozilla-central" according to the log.
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

3 years ago
Created attachment 8514234 [details] [diff] [review]

Did the changes at new places.
Attachment #8513738 - Attachment is obsolete: true
Attachment #8514234 - Flags: review?(bzbarsky)
Comment on attachment 8514234 [details] [diff] [review]

Thanks.  Pushed https://tbpl.mozilla.org/?tree=Try&rev=741e628dafad
Attachment #8514234 - Flags: review?(bzbarsky) → review+
Looks nice and green.
Keywords: checkin-needed
Akshendra Pratap Singh, thank you again for the patch!

Comment 45

3 years ago
Thanks for the help
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

Comment 47

3 years ago
Hip, hip, hooray.


2 years ago
Depends on: 1285835
You need to log in before you can comment on or make changes to this bug.