1418616 - download saveas loses file extension if basename modified

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[Michel Gutierrez]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20171003101344

Steps to reproduce:

This problem concerns only Windows (tested on Windows 10).

Requesting the download of a file through the WebExtensions API:
```
	browser.downloads.download({
		url: "http://media.lelombrik.net/t/d69b3fe05ea016c31fa75d4e72a05616/d69b3fe05ea016c31fa75d4e72a05616.mp4",
		saveAs: true
	});
```
A SaveAs dialog appears, with the base filename "d69b3fe05ea016c31fa75d4e72a05616" editable (note that by default on Windows, the filename extension ".mp4" does not appear in the filename edit field.

Just clicking OK, a file d69b3fe05ea016c31fa75d4e72a05616.mp4 is saved in the Downloads folders as expected.

Doing the same, but this time editing the file name in the dialog from "d69b3fe05ea016c31fa75d4e72a05616" to "myvideo". File "myvideo" is saved, without the ".mp4" extension.

If the file name is explicitly entered as "myvideo.mp4" then the correct file name is used, but this is not intuitive because the original editable text did not show the file extension, and the fact this is ".mp4" does not appear anywhere.

In many cases the Video DownloadHelper add-on uses this browser.downloads.download({..., saveAs: true}) feature and for the past 4 days, i've seen at least 100 reports of this issue, generally blaming the add-on with bad reviews because of this.

If the base name of the downloaded file is edited, the system should append back the file extension to the resulting file name.

Comment 1

[Andrew Swan [:aswan]]

:zombie any chance you can look at this?

Comment 2

[Tomislav Jovanovic :zombie]

Sure, will investigate.

Comment 3

[Tomislav Jovanovic :zombie]

Attachment: Bug 1418616 - Add default extension to downloads saveAs dialogs,

Review commit: https://reviewboard.mozilla.org/r/232598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232598/

Comment 4

[Tomislav Jovanovic :zombie]

I couldn't figure out how to test this using the MockFilePicker, so I did the manual testing of all 3 variants:

1) Clicking ok without changing the filename uses the default one
2) Changing the file name without an extension appends the default extension
3) Changing to a file name with an extension preserves it.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8963710 [details]
Bug 1418616 - Add default extension to downloads saveAs dialogs,

https://reviewboard.mozilla.org/r/232598/#review238202

Thanks! looks good to me, it would be nice to have also some kind of test coverage about it, though.
I've been thinking and looking a bit about what could be easily tested without too much hassle and
also protect this fix from some classes of regressions, and I think that we could add at least a couple of additional
assertions to the existent test_chrome_ext_downloads_saveAs.html test file, e.g. something like this
should be do it:

```
diff --git a/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html b/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
--- a/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
+++ b/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
@@ -44,28 +44,35 @@ add_task(async function setup() {
     await SpecialPowers.popPrefEnv();
     pickerDir.remove(true);
     defaultDir.remove(true);
   });
 });
 
 add_task(async function test_downloads_saveAs() {
   const pickerFile = pickerDir.clone();
-  pickerFile.append("file_download.txt");
+  pickerFile.append("file_download.nonext.txt");
 
   const defaultFile = defaultDir.clone();
-  defaultFile.append("file_download.txt");
+  defaultFile.append("file_download.nonext.txt");
 
   const {MockFilePicker} = SpecialPowers;
   MockFilePicker.init(window);
 
   MockFilePicker.showCallback = fp => {
     // On picker 'show' event, choose the filename that was set as the default
     // and append it to the picker's download directory
     let file = pickerDir.clone();
+
+    // Assert that the downloads API configure both the defaultString and the defaultExtension
+    // on the FilePicker, the defaultExtension will be used as a fallback file extension
+    // on some platforms (e.g. Windows), if needed (See Bug 1418616 for a rationale).
+    is(fp.defaultString, "file_download.nonext.txt", "Got the expected FilePicker defaultString");
+    is(fp.defaultExtension, "txt", "Got the expected FilePicker defaultExtension");
+
     file.append(fp.defaultString);
     MockFilePicker.setFiles([file]);
   };
 
   function background() {
     const url = URL.createObjectURL(new Blob(["file content"]));
     browser.test.onMessage.addListener(async (filename, saveAs) => {
       try {
@@ -96,39 +103,39 @@ add_task(async function test_downloads_s
   await extension.startup();
   await extension.awaitMessage("ready");
 
   async function testExpectFilePicker(saveAs) {
     ok(!pickerFile.exists(), "the file should have been cleaned up properly previously");
 
     MockFilePicker.returnValue = MockFilePicker.returnOK;
 
-    extension.sendMessage("file_download.txt", saveAs);
+    extension.sendMessage("file_download.nonext.txt", saveAs);
     let result = await extension.awaitMessage("done");
     ok(result.ok, `downloads.download() works with saveAs=${saveAs}`);
 
     ok(pickerFile.exists(), "the file exists.");
     is(pickerFile.fileSize, 12, "downloaded file is the correct size");
     pickerFile.remove(false);
 
     // Test the user canceling the save dialog.
     MockFilePicker.returnValue = MockFilePicker.returnCancel;
 
-    extension.sendMessage("file_download.txt", saveAs);
+    extension.sendMessage("file_download.nonext.txt", saveAs);
     result = await extension.awaitMessage("done");
 
     ok(!result.ok, "download rejected if the user cancels the dialog");
     is(result.message, "Download canceled by the user", "with the correct message");
     ok(!pickerFile.exists(), "file was not downloaded");
   }
 
   async function testNoFilePicker(saveAs) {
     ok(!defaultFile.exists(), "the file should have been cleaned up properly previously");
 
-    extension.sendMessage("file_download.txt", saveAs);
+    extension.sendMessage("file_download.nonext.txt", saveAs);
     let result = await extension.awaitMessage("done");
     ok(result.ok, `downloads.download() works with saveAs=${saveAs}`);
 
     ok(defaultFile.exists(), "the file exists.");
     is(defaultFile.fileSize, 12, "downloaded file is the correct size");
     defaultFile.remove(false);
   }
```

This way we are just asserting that the downloads WebExtensions API implementation is configuring the FilePicker as expected, then it is the FilePicker job to ensure that the final behavior (the actual filename on disk) is the expected one even when the user change the filename manually and it doesn't include the extension explicitly (and you already tested that manually for now).

How that sounds to you?

::: toolkit/components/extensions/parent/ext-downloads.js:531
(Diff revision 1)
>              const picker = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
> -            const window = Services.wm.getMostRecentWindow("navigator:browser");
>              picker.init(window, null, Ci.nsIFilePicker.modeSave);
>              picker.displayDirectory = new FileUtils.File(dir);
>              picker.appendFilters(Ci.nsIFilePicker.filterAll);
> -            picker.defaultString = OS.Path.basename(target);
> +            picker.defaultExtension = ext && ext[1];

Nit (a totally optional one), maybe it could be reasonable to also leave an inline comment here, to mention explicitly which is the role of defaultExtension (mainly because, as the docs also explains, it is actually used only on some platforms), e.g.:

```
// Configure a default file extension (used on some platform, like windows,
// as a fallback, See Bug 1418616 for a rationale).
picker.defaultExtension = ext && ext[1];
// Set the defaultString (which should also include the file extension, as
// suggested in the documentation for nsIFilePicker.defaultString).
picker.defaultString = basename;
```

Comment 6

[Tomislav Jovanovic :zombie]

Comment on attachment 8963710 [details]
Bug 1418616 - Add default extension to downloads saveAs dialogs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232598/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f91a8b447b5
Add default extension to downloads saveAs dialogs, r=rpl

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7f91a8b447b5

Comment 9

[marius.santa]

Could you please provide a link or xpi for your extension in order to test it?

Comment 10

[Tomislav Jovanovic :zombie]

Just the code from comment 0 in the background script should be enough.

Comment 11

[marius.santa]

Attachment: video extension.gif

Reproduced on Firefox 59.0a1 (20180101220336) Windows 10 x64
Retested and verified on Firefox 61.0a1 (20180427102245) on Windows 10 x64 and MacOS 10.13.3