213337 - Saving PDF different from viewing PDF

Status: RESOLVED FIXED

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

Description

[David G King]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.4) Gecko/20030624

When I select "Save Link Target As..." via a right mouse click on the link, the
"Save As" window comes up with the following "File name" -
DocumentLibrary\HealthReportWinter2003.pdf and Download Manager comes back with
an "Unknown error has occured".

This seems to be caused by /'s and \'s in the URL -
http://www.liveupdater.com/labourparty/DocumentLibrary\HealthReportWinter2003.pdf

However, when I select "Open Link in New Tab", it opens fine and displays the
PDF using Acrobat Reader.

Reproducible: Always

Steps to Reproduce:
1. Go to URL
2. Select Save Link Target
3. Then try Open in New Tab

Actual Results:  
Download Manager breaks due to filename passed to it including a directory that
doesn't exist on my machine (or maybe the directory bit is irrelevant)

Expected Results:  
Download to work exactly the same as Open in New Tab

I seem to recall another bug dealing with /'s and \'s. However, this is
different as there are functionality differances within Mozilla, and I'm not too
keen to say which one is right.

Comment 1

[Boris Zbarsky [:bzbarsky]]

"Save link target as" works fine on that link for me on Linux... but it does
save with the '\' in the filename.  We should be stripping such chars out of
filenames, no?  I recall code to do that...

In any case, wrong component.

Comment 2

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

code to replace that is at:
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/contentAreaUtils.js#818

However, if we use the filename from the url, we do not call validateFilename:
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/contentAreaUtils.js#766
Venkman shows that we do indeed reach that codepath.

that sounds broken to me.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, that's wrong.  We should be calling ValidateFilename() there.

Comment 4

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

taking

Comment 5

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

Attachment: patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128525 [details] [diff] [review]
patch

sr=bzbarsky

Comment 7

[jag (Peter Annema)]

Comment on attachment 128525 [details] [diff] [review]
patch

r=jag

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 128525 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to 1.5beta

Comment 9

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

checked in to trunk:
Checking in contentAreaUtils.js;
/cvsroot/mozilla/xpfe/communicator/resources/content/contentAreaUtils.js,v  <--
 contentAreaUtils.js
new revision: 1.87; previous revision: 1.86
done

leaving open for 1.4 branch checkin

Comment 10

[Jungshik Shin]

can you also check the patch into firebird? it has exactly the same problem in
contentAreaUtils.js

Comment 11

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

if fb forked that file, that is their problem, not mine

Comment 12

[David G King]

I think Firebird uses the Mozilla Trunk in the same way as Thunderbird and
Mozilla itself do. The only branch I know of (that is relevant) is the one for
1.4.x which has been anticipated.

However, if you see this problem with a version of Firebird that includes this
patch (and Mozilla with this patch works fine) then please file a new bug
specific to Firebird.

I'd love to test Firebird myself, but I'm busy rebuilding my Mozilla (et. al.)
environment after an upgrade that broke things badly.

Comment 13

[Blake Ross]

Christian: I just ported the fix over to Firebird. Thanks for the patch!

Comment 14

[Jungshik Shin]

> I think Firebird uses the Mozilla Trunk in the same way as 
> Thunderbird and Mozilla itself do.

  I wish that were the case. However, it's not. With firebird and thunderbird,
sometimes two or three (almost) identical changes have to be made (in xpfe/,
browser/ or toolkits/ and mail/) as shown here and in bug 215493

Comment 15

[Asa Dotzler [:asa]]

please resolve this bug if it's fixed on the trunk. The request for the 1.4
branch isn't going to be seriously considered until this fix is resolved and
verified on the trunk.

Comment 16

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

okay, sorry about that

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Testing on Windows 2003082404 trunk:

I can save the attachment with a default filename of
"DocumentLibrary-HealthReportWinter2003.pdf"

Is this correct?

Comment 18

[David G King]

Re if "DocumentLibrary-HealthReportWinter2003.pdf" is correct default file name.
Well, ignoring the typo (- versus _), I can now download the file and view it
offline, and I can also view it with the Acrobat plug-in. So, the patch fixed
the problem and Saving is now the same as Viewing.

Having said that, the default filename is actually wrong. The filename should be
"HealthReportWinter2003.pdf" as is demonstrated by IE 6.0, Opera 7 and Netscape 4.8.

I'm tempted to reopen this, but would like feedback before I do so.

Comment 19

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

david: no, it shouldn't. \ is not a path separator in URLs - it is part of the
filename. But as windows doesn't allow this character in filenames, we replace
it with a character that is allowed (we use '-').

interesting though that mozilla is the only browser doing that.

caillon: Yes, that is correct.

Comment 20

[David G King]

OK, I'm convinced....RFC2396 helped as well.

V'ing...

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 128525 [details] [diff] [review]
patch

This is not going to make 1.4.1.  Please re-request aproval after 1.4.1 ships
if you'd like to get this in for 1.4.2.

Comment 22

[Mike Kaply [:mkaply]]

Comment on attachment 128525 [details] [diff] [review]
patch

a=mkaply for 1.4.2

Comment 23

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

fixed on 1.4 branch
cvs commit: Examining xpfe/communicator/resources/content
Checking in xpfe/communicator/resources/content/contentAreaUtils.js;
/cvsroot/mozilla/xpfe/communicator/resources/content/contentAreaUtils.js,v  <--
 contentAreaUtils.js
new revision: 1.78.2.2; previous revision: 1.78.2.1
done

Comment 24

[Jungshik Shin]

I should have caught it. We need to do the same in 'catch' clause. Patching
coming up.

Comment 25

[Jungshik Shin]

Attachment: patch for 'catch-clause'

Comment 26

[Jungshik Shin]

Comment on attachment 137348 [details] [diff] [review]
patch for 'catch-clause'

asking for r/sr.

this is just a trivial extension of the original patch.

Comment 27

[jag (Peter Annema)]

Comment on attachment 137348 [details] [diff] [review]
patch for 'catch-clause'

sr=jag

Comment 28

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

Comment on attachment 137348 [details] [diff] [review]
patch for 'catch-clause'

oops, you're right. r=me on the xpfe/ part of this patch.

Comment 29

[Jungshik Shin]

Comment on attachment 137348 [details] [diff] [review]
patch for 'catch-clause'

Thanks for r/sr.

asking for a1.4.2 and a1.6

This is so trivial that I'm just asking for both a's at the same time. Besides,
it's been proven by the corresponding change in 'try-clause'.

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 137348 [details] [diff] [review]
patch for 'catch-clause'

a=asa (on behalf of drivers) for checkin to Mozilla 1.6

Comment 31

[Jungshik Shin]

fix checked into the trunk. marking as fixed now, but I'll wait for a1.4.2

Comment 32

[Mike Kaply [:mkaply]]

Comment on attachment 137348 [details] [diff] [review]
patch for 'catch-clause'

a=mkaply for 1.4.2

Comment 33

[Jungshik Shin]

thanks. fix checked into 1.4.2 branch