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

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Awad Mackie]

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.

Comment 1

[Awad Mackie]

Attachment: 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.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Paolo: there's some context in bug 346994 comment 28

Comment 3

[:Paolo Amadini]

(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.

Comment 4

[Awad Mackie]

(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?

Comment 5

[:Paolo Amadini]

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 :-)

Comment 6

[:Paolo Amadini]

Marco, how can we ensure that this fix is verified by QA in the next iteration?

Comment 7

[Marco Mucci [:MarcoM]]

(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.

Comment 8

[:Paolo Amadini]

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

Comment 9

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/357002b68cb5

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/357002b68cb5

Comment 11

[Marco Mucci [:MarcoM]]

Hi Florin, can a contact be assigned to this bug for QA verification.

Comment 12

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Ubuntu 13.10 32bit using latest Nightly 33.0a1 (buildID: 20140625030206).

Comment 13

[:Paolo Amadini]

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.