If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Overwriting files with leading period removes file on disk but doesn't replace

VERIFIED FIXED in mozilla33

Status

Core Graveyard
File Handling
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: Awad Mackie, Assigned: Awad Mackie)

Tracking

29 Branch
mozilla33
x86_64
Linux
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140516085459

Steps to reproduce:

1. Attempt to download file with settings to ask prompt user for file name
2. Set file name to already existing file with leading period and accept the warning prompt


Actual results:

Existing file on disk was removed and new file was saved as name without leading period


Expected results:

According to bug 346994, files with leading period should not be saved, but then the existing file should not be removed.
(Assignee)

Comment 1

3 years ago
Created attachment 8436216 [details] [diff] [review]
leading_period.patch

Carrying on from https://bugzilla.mozilla.org/show_bug.cgi?id=346994#c29

Patch to check if the final name matches and only remove the existing file in that case in nsHelperAppDlg.js

Also, according to the other bug, all leading periods should be removed when saving, but none of the paths in toolkit/content/contentAreaUtills.js seem to do so? So it can save leading period files that way.
Flags: needinfo?(gavin.sharp)

Updated

3 years ago
QA Whiteboard: [bugday-20140609]
Component: Untriaged → File Handling
Product: Firefox → Core
Flags: needinfo?(gavin.sharp) → needinfo?(paolo.mozmail)
Paolo: there's some context in bug 346994 comment 28

Comment 3

3 years ago
(In reply to Awad Mackie from comment #1)
> Patch to check if the final name matches and only remove the existing file
> in that case in nsHelperAppDlg.js

Hi Awad, thank you for the patch! It looks like a good solution to the problem stated in the bug report.

Since we don't have many automated tests for this part of the code tree, can you post in the bug some step-by-step instructions that would allow anyone to verify that the behavior is fixed? Also, instructions to verify that normal cases without leading dot still work would also be appreciated.

You can then ask me for review on the patch, and I'll take a look at the manual test case at the same time.

> Also, according to the other bug, all leading periods should be removed when
> saving, but none of the paths in toolkit/content/contentAreaUtills.js seem
> to do so? So it can save leading period files that way.

That could well be the case. We haven't unified the code paths (though we'd like to!) and this is something that can be improved. I think in the end we might want to do some checks, like the leading dot, before the file picker is shown, as bug 346994 suggests.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(paolo.mozmail) → needinfo?(firesock.serwalek)
(Assignee)

Comment 4

3 years ago
(In reply to :Paolo Amadini from comment #3)
> Since we don't have many automated tests for this part of the code tree, can
> you post in the bug some step-by-step instructions that would allow anyone
> to verify that the behavior is fixed? Also, instructions to verify that
> normal cases without leading dot still work would also be appreciated.

Not sure the kind of detail needed, but here it is:

Leading period correct behaviour:

1. Create an on disk file with leading period (.bug1022061)
2. Ensure Firefox is set to ask user where to save Downloads in Preferences > General
3. Download a file not understood by the browser (Firefox-0.8.zip from https://ftp.mozilla.org/pub/mozilla.org/firefox/releases/0.8/)
4. Select 'Save File' in the opening dialog box
5. Set the file name to the same as created in Step 1
6. Accept the popup, to replace the existing file
7. Check on disk files, file created in step 1 should be intact with a new file named the same without the leading period with the new content (bug1022061)

Standard file correct behaviour:

1. Create an on disk file with no leading period (bug1022061)
2. Ensure Firefox is set to ask user where to save Downloads in Preferences > General
3. Download a file not understood by the browser (Firefox-0.8.zip from https://ftp.mozilla.org/pub/mozilla.org/firefox/releases/0.8/)
4. Select 'Save File' in the opening dialog box
5. Set the file name to the same as created in Step 1
6. Accept the popup, to replace the existing file
7. Check on disk files, file created in step 1 should be replaced with the new content (bug1022061)

> That could well be the case. We haven't unified the code paths (though we'd
> like to!) and this is something that can be improved. I think in the end we
> might want to do some checks, like the leading dot, before the file picker
> is shown, as bug 346994 suggests.

Yep, bug 346994 seems like the right way to go at it, but until then is it worth trying to make the other code paths do the same thing as this one?
Flags: needinfo?(firesock.serwalek)
(Assignee)

Updated

3 years ago
Attachment #8436216 - Flags: review?(paolo.mozmail)

Comment 5

3 years ago
Comment on attachment 8436216 [details] [diff] [review]
leading_period.patch

The steps and the patch look good to me!

(In reply to Awad Mackie from comment #4)
> Yep, bug 346994 seems like the right way to go at it, but until then is it
> worth trying to make the other code paths do the same thing as this one?

I'll be glad to review any changes to that effect :-)
Attachment #8436216 - Flags: review?(paolo.mozmail) → review+

Comment 6

3 years ago
Marco, how can we ensure that this fix is verified by QA in the next iteration?
QA Whiteboard: [bugday-20140609] → [bugday-20140609][qa+]
Flags: needinfo?(mmucci)
(In reply to :Paolo Amadini from comment #6)
> Marco, how can we ensure that this fix is verified by QA in the next
> iteration?

Hi Paolo, it would need to be part of the Backlog and marked as fixed.
Flags: needinfo?(mmucci)

Updated

3 years ago
Iteration: --- → 33.2
Flags: firefox-backlog+

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
QA Whiteboard: [bugday-20140609][qa+] → [bugday-20140609] [qa+]
Resolution: --- → FIXED

Comment 8

3 years ago
This hasn't landed yet, so I guess it shouldn't be marked as fixed.

I've started a tryserver build here:

https://tbpl.mozilla.org/?tree=Try&rev=2c60880cfa08
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

3 years ago
Iteration: 33.2 → ---

Comment 9

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/357002b68cb5
https://hg.mozilla.org/mozilla-central/rev/357002b68cb5
Assignee: nobody → firesock.serwalek
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Hi Florin, can a contact be assigned to this bug for QA verification.
Iteration: --- → 33.2
Flags: needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei)
QA Contact: camelia.badau
Verified fixed on Ubuntu 13.10 32bit using latest Nightly 33.0a1 (buildID: 20140625030206).
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20140609] [qa+] → [bugday-20140609] [qa!]

Comment 13

3 years ago
Thanks Camelia for verifying this, and thanks Awad for the fix!

Awad, if you've seen this bug in other code paths, or you've noticed other download-related bugs, I'll be glad to review any patch you might have to fix them, or provide other assistance.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.