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)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla33
People
(Reporter: firesock.serwalek, Assigned: firesock.serwalek)
Details
Attachments
(1 file)
2.09 KB,
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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•10 years ago
|
QA Whiteboard: [bugday-20140609]
Component: Untriaged → File Handling
Product: Firefox → Core
Updated•10 years ago
|
Flags: needinfo?(gavin.sharp) → needinfo?(paolo.mozmail)
Comment 2•10 years ago
|
||
Paolo: there's some context in bug 346994 comment 28
Comment 3•10 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•10 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•10 years ago
|
Attachment #8436216 -
Flags: review?(paolo.mozmail)
Comment 5•10 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•10 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)
Comment 7•10 years ago
|
||
(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•10 years ago
|
Iteration: --- → 33.2
Flags: firefox-backlog+
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
QA Whiteboard: [bugday-20140609][qa+] → [bugday-20140609] [qa+]
Resolution: --- → FIXED
Comment 8•10 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•10 years ago
|
Iteration: 33.2 → ---
Assignee: nobody → firesock.serwalek
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 11•10 years ago
|
||
Hi Florin, can a contact be assigned to this bug for QA verification.
Iteration: --- → 33.2
Flags: needinfo?(florin.mezei)
Updated•10 years ago
|
Flags: needinfo?(florin.mezei)
QA Contact: camelia.badau
Comment 12•10 years ago
|
||
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•10 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.
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•