Closed Bug 213337 Opened 21 years ago Closed 21 years ago

Saving PDF different from viewing PDF

Categories

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

Product:
Core Graveyard
Component:
File Handling
Platform:
x86
Windows XP
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.5beta

People

(Reporter: d_king, Assigned: Biesinger)

References

(
URL
)

Details

(Keywords: fixed1.4.2)

Attachments

(2 files)

patch
1.23 KB, patch
jag+mozilla
: review+
bzbarsky
: superreview+
asa
: approval1.4.1-
mkaply
: approval1.4.2+
asa
: approval1.5b+
Details | Diff | Splinter Review
patch for 'catch-clause'
2.29 KB, patch
Biesinger
: review+
jag+mozilla
: superreview+
mkaply
: approval1.4.2+
asa
: approval1.6+
Details | Diff | Splinter Review 
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 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 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
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
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.
Christian: I just ported the fix over to Firebird. Thanks for the patch!
> 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

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
Closed: 21 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
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.
OK, I'm convinced....RFC2396 helped as well.

V'ing...
Status: RESOLVED → VERIFIED
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-
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
I should have caught it. We need to do the same in 'catch' clause. Patching
coming up.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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 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 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 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+
fix checked into the trunk. marking as fixed now, but I'll wait for a1.4.2
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
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+
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.