1034989 - Tests for File -> Save As and files with a leading period

Status: NEW

(Firefox :: File Handling, defect)

Description

[Awad Mackie]

Following on from bug 1022061 and bug 346994, it seems File->Save As will still allow saving a file with a leading period.

Comment 1

[Awad Mackie]

Attachment: leading_period.patch

Patch to make saving with leading period consistent with expected behaviour from bug 346994.

Steps to Test:

1. Navigate to file understood by the browser (https://ftp.mozilla.org/pub/mozilla.org/firefox/releases/0.8/MD5SUMS)
2. Select File->Save As from menu
3. Set the file name to name with a leading period (.bug1034989)
4. Check on disk file, it should be saved without the leading period (.bug1034989)

Currently, the file is saved with the leading period.

Note: Because of the readonly file on the FilePicker, I'm not sure if validateFileName was being run correctly earlier.

Comment 2

[:Paolo Amadini]

Comment on attachment 8451370 [details] [diff] [review]
leading_period.patch

(In reply to Awad Mackie from comment #1)
> Note: Because of the readonly file on the FilePicker, I'm not sure if
> validateFileName was being run correctly earlier.

Can you please verify if this is the case, manually at first? We should understand if we should include the call to validateFileName at all. I'm not exactly sure of what the other code path does, but I don't think it replaces special characters in the file name the user typed. There's a function called validateFileName there, but, confusingly, it does a different thing :-(

This change will need an automated test, a browser-chrome type test in particular. You can check out how these existing tests work:

./mach test toolkit/content/tests/browser/browser_save_resend_postdata.js
./mach test toolkit/content/tests/browser/browser_default_image_filename.js

They use a "mock file picker" to observe the suggested file name and/or change it, and a "mock transfer" (nsITransfer) to observe when the file finished downloading.

These tests are not straightforward, let me know if you have any questions.

We'll need a new test (to be added to the browser.ini manifest) that:
- Sets up a mock file picker that provides a name starting with a dot
- Sets up a mock transfer to wait for the end of the download
- Calls saveDocument (that is what File -> Save As does)
- Checks that the file created on disk does not start with a dot

Comment 3

[Awad Mackie]

Attachment: leading_period.patch

New patch with test case

(In reply to :Paolo Amadini from comment #2)
> Can you please verify if this is the case, manually at first? We should
> understand if we should include the call to validateFileName at all. I'm not
> exactly sure of what the other code path does, but I don't think it replaces
> special characters in the file name the user typed. There's a function
> called validateFileName there, but, confusingly, it does a different thing
> :-(

Yep, readonly prevents the file name from being changed, effectively validateFileName wasn't being run at all. I'm on Linux and my native file picker is catching the only case I fall into, but the other checks seem rather important?

Comment 4

[:Paolo Amadini]

Comment on attachment 8454169 [details] [diff] [review]
leading_period.patch

(In reply to Awad Mackie from comment #3)
> Yep, readonly prevents the file name from being changed, effectively
> validateFileName wasn't being run at all. I'm on Linux and my native file
> picker is catching the only case I fall into, but the other checks seem
> rather important?

Hm, this means that adding this character replacement now will prevent some users to save files with certain chosen names, but I think we can try this and, if it turns out to limit some real use cases, we can figure out how to proceed and fix the issue (probably by doing the replacement before the file selection).

The patch and test case are excellent! I might be able to do a more thorough review next week.

Comment 5

[Awad Mackie]

Comment on attachment 8454169 [details] [diff] [review]
leading_period.patch

(In reply to :Paolo Amadini from comment #4)
> Hm, this means that adding this character replacement now will prevent some
> users to save files with certain chosen names, but I think we can try this
> and, if it turns out to limit some real use cases, we can figure out how to
> proceed and fix the issue (probably by doing the replacement before the file
> selection).

Sounds good, I'd imagine we'd need some feedback for leading period behaviour as well if we wanted to keep completely disallowing it.

> The patch and test case are excellent! I might be able to do a more thorough
> review next week.

That's fine, thanks!

Comment 6

[:Paolo Amadini]

Comment on attachment 8454169 [details] [diff] [review]
leading_period.patch

Review of attachment 8454169 [details] [diff] [review]:
-----------------------------------------------------------------

One important factor that just occurred to me is that we risk overwriting an existing file without prompt in case the original file with the dot does not exist, but the file without the dot exists. In normal cases, the file picker asks the user whether they want to overwrite or not. So, in the end we may need to validate and change the file name before showing the file picker, and keep the exact target selected by the user.

I have a couple of comments on the test code, that in general is very clean and easy to read. I really appreciate your work here!

::: toolkit/content/tests/browser/browser_save_leading_period.js
@@ +26,5 @@
> +  var randomNum = Math.floor(Math.random() * 10000);
> +  chosenFile.append("." + randomNum);
> +  expectedFile.append(randomNum);
> +
> +  gBrowser.loadURI("data:text/plain;charset=US-ASCII,bug1034989");

Technically, we should set up the event listener before we start loading the page, so this line should be moved near the bottom of the function.

In practice it makes little difference, because this call is processed asynchronously, thus you might see tests that do the calls in the wrong order. But we should still write this test correctly, to facilitate things in case the behavior changes in the future.

@@ +46,5 @@
> +      try {
> +        // In case wrong file was created
> +        expectedFile.remove(false);
> +        chosenFile.remove(false);
> +      } catch (e) { };

nit: It makes little difference, but you might want to use two separate try-catch blocks.

nit: "catch (ex)" instead of "catch (e)"

@@ +57,5 @@
> +  function onTransferComplete(downloadSuccess) {
> +    ok(downloadSuccess, "The file was saved succesfully");
> +
> +    // Check if file with that name was not saved
> +    ok(tmpDir.contains(expectedFile), "File was not saved with leading period");

The "contains" method only checks the file path and does not access the file system. The MDN documentation was unclear, I've just updated it accordingly.

You can check expectedFile.exists(), and also !chosenFile.exists() while you're here.

Looking forward to the next patch!

Comment 7

[Awad Mackie]

Attachment: leading_period.patch

(In reply to :Paolo Amadini from comment #6)
> One important factor that just occurred to me is that we risk overwriting an
> existing file without prompt in case the original file with the dot does not
> exist, but the file without the dot exists. In normal cases, the file picker
> asks the user whether they want to overwrite or not. So, in the end we may
> need to validate and change the file name before showing the file picker,
> and keep the exact target selected by the user.

That's a fair point. What nsHelperAppDlg does in this situation is try and find a non-colliding name. There's a similar function here, which I've tried, is this a good idea, or should I revert to the behaviour above?

Fixed nits in test, thanks! Have moved things around a little with the new testing functionality as well.

Comment 8

[:Paolo Amadini]

Comment on attachment 8457636 [details] [diff] [review]
leading_period.patch

(In reply to Awad Mackie from comment #7)
> That's a fair point. What nsHelperAppDlg does in this situation is try and
> find a non-colliding name. There's a similar function here, which I've
> tried, is this a good idea, or should I revert to the behaviour above?

I think the most correct thing to do for both code paths would be to do any sanitization of the target name before displaying the file picker, and then use the chosen target file and overwrite it, since the file picker already asks for confirmation.

This is a less important issue with nsHelperAppDlg because, with default preferences, no file picker is displayed at all, and an unambiguous name is chosen automatically. This code path instead displays a file picker by default in Firefox, this means it will be normally hit by much more users. Other browsers like SeaMonkey use the unambiguous name by default instead.

Comment 9

[:Paolo Amadini]

This has been fixed by bug 1402279, but I'm keeping the bug open to add more specific regression tests as I suggested earlier.