Closed Bug 1022061 Opened 10 years ago Closed 10 years ago

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

Categories

(Core Graveyard :: File Handling, defect)

29 Branch
x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla33

People

(Reporter: firesock.serwalek, Assigned: firesock.serwalek)

Details

Attachments

(1 file)

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.
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)
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
(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)
(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)
Attachment #8436216 - Flags: review?(paolo.mozmail)
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+
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)
Iteration: --- → 33.2
Flags: firefox-backlog+
Status: NEW → RESOLVED
Closed: 10 years ago
QA Whiteboard: [bugday-20140609][qa+] → [bugday-20140609] [qa+]
Resolution: --- → FIXED
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 → ---
Iteration: 33.2 → ---
https://hg.mozilla.org/mozilla-central/rev/357002b68cb5
Assignee: nobody → firesock.serwalek
Status: REOPENED → RESOLVED
Closed: 10 years ago10 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!]
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.