Closed Bug 263697 Opened 20 years ago Closed 20 years ago

[FIXr]Content-Disposition headers no longer looked at when saving documents

Categories

(Core Graveyard :: File Handling, defect, P2)

Product:
Core Graveyard
Component:
File Handling
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.8alpha5

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(1 file)

Patch
(deleted), patch
Biesinger
: review+
jst
: superreview+
asa
: approval1.8a5+
Details | Diff | Splinter Review 
With the checkin for bug 160454, we no longer look at the content-disposition
header when saving files via "save page as".  We should fix that.  We have a
document object, so we should be able to get the content-disposition header off
of it.

jst, I know we've talked about this in the abstract before, but we sort of need
it in the concrete now.  Do you have a preferred way to expose metadata on
documents, short of adding more members to nsIDOMNSDocument?  I was thinking of
adding a getMetaData() method there which would take a string naming the
metadata (and for now the only metadata recognized would be
"content-disposition").  Or is that too free-form?
(In reply to comment #0)
> With the checkin for bug 160454, we no longer look at the content-disposition
> header when saving files via "save page as".  We should fix that.  We have a
> document object, so we should be able to get the content-disposition header off
> of it.
Isn't it a bigger problem when we initiate a "save link target as"? In that case
we don't have a document object.
And if we /do/ already have a document object, then the user can clearly see
(and be aware of) what they are saving.
> In that case we don't have a document object.

True.  So not much we can do.

> then the user can clearly see (and be aware of) what they are saving.

That's not the point.  The point is that the Content-Disposition header can say
what the filename should be when the document is saved.  We used to respect
that.  Now we don't.  This _will_ break some pages, so we need to fix it.
(In reply to comment #2)
> That's not the point.  The point is that the Content-Disposition header can say
> what the filename should be when the document is saved.  We used to respect
> that.  Now we don't.  This _will_ break some pages, so we need to fix it.

Ah yes. rfc2183 states "If the receiving MUA writes the entity to a file, the
suggested filename should be used as a basis for the actual filename, where
possible... important that the receiving MUA not blindly use the suggested
filename.  The suggested filename SHOULD be checked..." etc etc.

That was referring to a MUA, but that also applies to a browser I figure (or
maybe there's a more up-to-date rfc).
Anyhooo... can the content-disposition header be examined in WBP, or has it all
been squirelled away by then (or even discarded)?
wbp is too late, you want it before the file picker is shown
(In reply to comment #4)
> wbp is too late, you want it before the file picker is shown

ah, true... 
Didn't someone say recently that some of this stuff is discarded when the page
is loaded (i.e it isn't even stored with the file in the cache)? So is
content-disposition one of those things? (I can't see the stuff we need in the
cache either in 1.7.3 or 1.8a4).

anyway, my train of thought was following the scenario of saving link targets,
but that is all bogus because we have to pick a filename (and show the
filepicker) before we even start downloading the file :)
the cache does store this (it stores all http headers), but the page may not be
in the cache... I think the document object (or somesuch) should get this header
from the channel and expose it at least to chrome scripts. or maybe it should
just expose the filename.
Note that I'm just waiting on jst's comments on comment 0.  Once we have
agreement on that, I'll expose the content-disposition header on the document.
Adding a getMetaData() method to our document objects scares me a bit due to the
almost inevitable conflict with global function names already used on some
random webpage out there... But I do want to see this functionality exposed...

Do we want to risk sites breaking? Or do we want to push this into
nsIDOMWindowUtils for now?
We could do that, I guess... There are some cases of documents without windows,
though (eg if we ever implement "save as, complete" for "save link").

Perhaps we could call it getGeckoMetaData() or something?  To make the risk of
collisions very unlikely?
What we really need is for the non-existent W3C DOM WG to specify how HTTP
headers etc are exposed on a document and everyone plays happily together. Well,
that ain't going to happen any time soon, so maybe we just need to bite the
bullet, or involve WHATWG?
I'd say bite the bullet ('cause we need to fix this bug for 1.8a5) _and_ involve
WHATWG.
why not create a new, chrome-only interface implemented by our documents?  is
there really need to expose this information via the DOM to unprivileged script?
> why not create a new, chrome-only interface implemented by our documents?

Last I checked, there was no way to keep non-chrome script from getting at such
an interface once the document had been QIed to it by chrome (thanks to dynamic
interface flattening and so forth).
OK, this is making bugzilla a pain to use.  We need some consensus here.  jst,
if you prefer that we use nsIDOMWindowUtils I can do that (with nsIDocument
changes to let the window get the data, I guess).
Severity: normal → major
Keywords: regression
I just figured that on a photo gallery like in
http://inconstruction.kairo.at/?d=g&i=51&m=v (and I have lots of those), when
you "Save image as...", you get a .htm filename suggested for saving though
Mozilla already knows the file is an image and decoded it that way, the image
has a correct image/* MIME type in content type set and has a proposed file name
in content-disposition. That all (at least MIME type and file name) was worth
something until HEAD requests got removed, now we seems to ignore all of that
and users end up saving a file they can't open (as file managers may link
applications by extension by default).
This is clearly a big regression and we shouldn't even ship a beta with that
one, so we should dig up a solution quite soon. I know you're thinking about
that issue, I just wanted to give you a real life case where it has really bad
impact, as we even mess up the file type completely.
Robert, that case is not actually covered by this bug.  At the moment we really
don't have a way to fix that case at all; if you can suggest one, I'd love to
hear it.  Please file a separate bug on it.
a suggestion would be to use
mimeservice.getPrimaryExtension(document.contentType,
document.location.fileExtension)
(pseudocode)

let me note that the content-disposition part kairo mentions _is_ covered by
this bug
biesi, kairo's talking about "save image as", not saving the current document...
 For save image as we don't have access to the header.  For the document we can
have access to it, and should.

I'm really not sure what getPrimaryExtension has to do with that...
(In reply to comment #18)
> biesi, kairo's talking about "save image as", not saving the current document...
>  For save image as we don't have access to the header.  For the document we can
> have access to it, and should.

Unfortunately when the only thing in the browser window is an image (i.e not
part of an html doc), 'save image as' and 'save page as' both do the same thing.
(In reply to comment #18)
> biesi, kairo's talking about "save image as", not saving the current document...
>  For save image as we don't have access to the header.  For the document we can
> have access to it, and should.
> 
> I'm really not sure what getPrimaryExtension has to do with that...

er, yeah. getPrimaryExtension would only be useful to get an extension if we
don't have one from other sources...
OK, as it turns out to be a different issue then, I filed bug 264757 for comment
#15 as another regression caused by the removal of HEAD.
(In reply to comment #14)
> OK, this is making bugzilla a pain to use.  We need some consensus here.  jst,
> if you prefer that we use nsIDOMWindowUtils I can do that (with nsIDocument
> changes to let the window get the data, I guess).

Sorry for not replying sooner. I'd recomment putting this in nsIDOMWindowUtils
for now, and if at some point this needs to migrate to a more standard place
then so be it, but for now let's keep this private...
Attached patch Patch (deleted) — — Splinter Review 

    
    

    
  
Comment on attachment 165929 [details] [diff] [review]
Patch

biesi, jst, would you review?

        Attachment #165929 -
        Flags: superreview?(jst)

        Attachment #165929 -
        Flags: review?(cbiesinger)

    
    

    
  
Comment on attachment 165929 [details] [diff] [review]
Patch


xpfe/communicator/resources/content/contentAreaUtils.js

could pass {} as the out param, in both calls. then you wouldn't need dummy.

you could use "return mhp.getParameter(..);" and avoid the local var
that way, you can even avoid one of the try..catch blocks, I think

r=biesi either way.

        Attachment #165929 -
        Flags: review?(cbiesinger) → review+

    
    

    
  
> you could use "return mhp.getParameter(..);"

No, I couldn't.  It can return an empty string...

    
    

    
  
Comment on attachment 165929 [details] [diff] [review]
Patch

sr=jst

        Attachment #165929 -
        Flags: superreview?(jst) → superreview+

    
    

    
  
I think we should take this regression fix for 1.8a5.  It's pretty noticeable...
Flags: blocking1.8a5?

    
  
Assignee: Time_lord → bzbarsky
Priority: -- → P2
Summary: Content-Disposition headers no longer looked at when saving documents → [FIXr]Content-Disposition headers no longer looked at when saving documents
Target Milestone: --- → mozilla1.8alpha5

    
    

    
  
Comment on attachment 165929 [details] [diff] [review]
Patch

a=asa for 1.8a5 checkin.

        Attachment #165929 -
        Flags: approval1.8a5+

    
  
Flags: blocking1.8a5? → blocking1.8a5+

    
    

    
  
Fixed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED

    
  
No longer blocks: 271981
Depends on: 271981

    
    

    
  
Is it possible to port this for aviary branch?  It would help UMO bug 275900

    
    

    
  
How would porting this to aviary branch help exactly? This bug was a fix to a
Seamonkey-only regression caused by a Seamonkey-only checkin.  Bug 275900
comment 8 is flat out wrong.

    
  
Blocks: 294759

    
    

    
  
While Save Page As works fine, Save Link Target As is still broken with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050701.

Firefox trunk is now affected as well since bug 294759 synced
contextAreaUtils.js. See bug 299372.

    
    

    
  
This bug is about "save page as", so comment 33 is pretty unrelated.
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: