Saving PDF different from viewing PDF

RESOLVED FIXED in mozilla1.5beta

Status

Core Graveyard
File Handling
P2
major
RESOLVED FIXED
15 years ago
2 years ago

People

(Reporter: David G King, Assigned: Biesinger)

Tracking

({fixed1.4.2})

Trunk
mozilla1.5beta
x86
Windows XP
fixed1.4.2

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

15 years ago
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.
"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.
Assignee: blakeross → law
Component: Download Manager → File Handling
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.
Yeah, that's wrong.  We should be calling ValidateFilename() there.
taking
Assignee: law → cbiesinger
Priority: -- → P2
Target Milestone: --- → mozilla1.5beta
Attachment #128525 - Flags: superreview?(bzbarsky)
Attachment #128525 - Flags: review?(jaggernaut)
Status: NEW → ASSIGNED
Comment on attachment 128525 [details] [diff] [review]
patch

sr=bzbarsky
Attachment #128525 - Flags: superreview?(bzbarsky) → superreview+

Comment 7

15 years ago
Comment on attachment 128525 [details] [diff] [review]
patch

r=jag
Attachment #128525 - Flags: review?(jaggernaut) → review+
Attachment #128525 - Flags: approval1.5b?
Attachment #128525 - Flags: approval1.4.x?

Comment 8

15 years ago
Comment on attachment 128525 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to 1.5beta
Attachment #128525 - Flags: approval1.5b? → approval1.5b+
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

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

if fb forked that file, that is their problem, not mine
(Reporter)

Comment 12

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

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

Comment 14

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

15 years ago
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.
okay, sorry about that
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Testing on Windows 2003082404 trunk:

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

Is this correct?
QA Contact: petersen → caillon
(Reporter)

Comment 18

15 years ago
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.
Keywords: 4xp
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.
(Reporter)

Comment 20

15 years ago
OK, I'm convinced....RFC2396 helped as well.

V'ing...
Status: RESOLVED → VERIFIED

Comment 21

15 years ago
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.
Attachment #128525 - Flags: approval1.4.x? → approval1.4.x-
Attachment #128525 - Flags: approval1.4.2?

Comment 22

15 years ago
Comment on attachment 128525 [details] [diff] [review]
patch

a=mkaply for 1.4.2
Attachment #128525 - Flags: approval1.4.2? → approval1.4.2+
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
Keywords: fixed1.4.2

Comment 24

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

Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 25

15 years ago
Created attachment 137348 [details] [diff] [review]
patch for 'catch-clause'

Comment 26

15 years ago
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.
Attachment #137348 - Flags: superreview?(jag)
Attachment #137348 - Flags: review?(cbiesinger)

Comment 27

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

sr=jag
Attachment #137348 - Flags: superreview?(jag) → superreview+
Comment on attachment 137348 [details] [diff] [review]
patch for 'catch-clause'

oops, you're right. r=me on the xpfe/ part of this patch.
Attachment #137348 - Flags: review?(cbiesinger) → review+

Comment 29

15 years ago
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'.
Attachment #137348 - Flags: approval1.6?
Attachment #137348 - Flags: approval1.4.2?

Comment 30

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

a=asa (on behalf of drivers) for checkin to Mozilla 1.6
Attachment #137348 - Flags: approval1.6? → approval1.6+

Comment 31

15 years ago
fix checked into the trunk. marking as fixed now, but I'll wait for a1.4.2
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Comment 32

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

a=mkaply for 1.4.2
Attachment #137348 - Flags: approval1.4.2? → approval1.4.2+

Comment 33

15 years ago
thanks. fix checked into 1.4.2 branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.