download saveas loses file extension if basename modified

VERIFIED FIXED in Firefox 61

Status

defect
P3
normal
VERIFIED FIXED
2 years ago
7 months ago

People

(Reporter: michel.gutierrez, Assigned: zombie)

Tracking

57 Branch
mozilla61

Firefox Tracking Flags

(firefox61 verified)

Details

(Whiteboard: downloads)

Attachments

(2 attachments)

Reporter

Description

2 years ago
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.
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
:zombie any chance you can look at this?
Flags: needinfo?(tomica)
Summary: [WebExtensions] On Windows, download saveas loses file extension if basename modified → download saveas loses file extension if basename modified
Whiteboard: downloads
Assignee

Comment 2

2 years ago
Sure, will investigate.
Assignee: nobody → tomica
Flags: needinfo?(tomica)

Updated

a year ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Comment hidden (mozreview-request)
Assignee

Comment 4

a year ago
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.
Assignee

Updated

a year ago
Attachment #8963710 - Flags: review?(aswan) → review?(lgreco)

Comment 5

a year ago
mozreview-review
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;
```
Attachment #8963710 - Flags: review?(lgreco) → review+
Comment hidden (mozreview-request)

Comment 7

a year ago
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7f91a8b447b5
Add default extension to downloads saveAs dialogs, r=rpl

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7f91a8b447b5
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Comment 9

a year ago
Could you please provide a link or xpi for your extension in order to test it?
Flags: needinfo?(tomica)
Assignee

Comment 10

a year ago
Just the code from comment 0 in the background script should be enough.
Flags: needinfo?(tomica)

Comment 11

a year ago
Posted image 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

Updated

a year ago
Status: RESOLVED → VERIFIED

Updated

11 months ago
Product: Toolkit → WebExtensions
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.