1280044 - browser.downloads.download failed with subdirectories in filename

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[mail]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.84 Safari/537.36

Steps to reproduce:

Run next code in Web Extension:

var blob = new Blob(['text'], {
    type: 'text/plain'
});
var url = URL.createObjectURL(blob);
browser.downloads.download({
    url: url,
    filename: 'one/file.txt'
});



Actual results:

Download has failed.


Expected results:

Under the default downloads folder (from browser settings) Firefox should create directory "one" and file in it "file.txt" with content from blob object. This is how extensions work in chrome: https://developer.chrome.com/extensions/downloads#method-download (see filename property description).

Comment 1

[Tomislav Jovanovic :zombie]

Attachment: bug 1280044 - handle subdirs in browser.downloads filenames,

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

Comment 2

[Andrew Swan [:aswan]]

Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review76402

Thanks for working on this!  A couple of things below and this will be ready to go.
Also please do a try run on all platforms.

::: toolkit/components/extensions/ext-downloads.js:420
(Diff revision 1)
>            // TODO
>            // if (options.saveAs) { }
>  
>            let target;
>            if (options.filename) {
> -            target = OS.Path.join(downloadsDir, options.filename);
> +            // can't use OS.Path.split(), but explicitly split on "/" instead

What's the purpose of this change?  And why wouldn't OS.Path.split() be suitable?

::: toolkit/components/extensions/ext-downloads.js:437
(Diff revision 1)
>  
> +          // handle filenames with subdirs
> +          const dir = OS.Path.dirname(target);
> +          return OS.File.exists(dir).then(dirExists => {
> +            if (!dirExists) {
> +              return OS.File.makeDir(dir, {from: downloadsDir})

lets not return early here, if somebody wants to add logic below later this will trip them up.  just chain the remaining logic into another then handler.

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js:123
(Diff revision 1)
>    function testDownload(options, localFile, expectedSize, description) {
>      return download(options).then(msg => {
>        equal(msg.status, "success", `downloads.download() works with ${description}`);
>        return waitForDownloads();
>      }).then(() => {
> +      let parts = localFile.split("/");

Lets just pass the array of parts in here instead of splitting the string.  but keep localFile working as a string, i.e. something like:
```
parts = Array.isArrary(localFile) ? localFile : [localFile];
```

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js:152
(Diff revision 1)
> +  // Call download() with a filename with subdirs.
> +  yield testDownload({
> +    url: FILE_URL,
> +    filename: "sub/dir/file",
> +  }, "sub/dir/file", FILE_LEN, "source and filename with subdirs");
> +

can you add a test hitting sub/dir/file2 here to make sure everything works when the target is an existing subdirectory.
To do that, you'll need to take the logic for removing the subdirectory from testDownload() and manually do `file.remove("sub")` after the second test.

Comment 3

[Tomislav Jovanovic :zombie]

Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

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

Comment 4

[Tomislav Jovanovic :zombie]

Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review76402

> What's the purpose of this change?  And why wouldn't OS.Path.split() be suitable?

running OS.Path.split() on Windows splits on backslash "\\", so the example doesn't work.  slashes "/" are the cross-platform way to write paths in js extensions (as well as node.js, etc), and I even thought that using a backslash wouldn't work. I just tested, and it seems to work, so I'm gonna try a different approach.

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review76696

::: toolkit/components/extensions/ext-downloads.js:396
(Diff revision 2)
>    let {extension} = context;
>    return {
>      downloads: {
>        download(options) {
> -        if (options.filename != null) {
> -          if (options.filename.length == 0) {
> +        let filename = options.filename;
> +        if (filename && PlatformInfo.os === "win") {

Okay, I see what you're trying to do here.  Can you verify what Chrome does in this case?  (i.e., this means that it would not be possible to create a file with a / in the name on windows, which is an otherwise valid thing to do).  Also, this code will need a test (i.e., test that both types of relative paths work on windows)

::: toolkit/components/extensions/ext-downloads.js:427
(Diff revision 2)
> -            target = OS.Path.join(downloadsDir, options.filename);
> +            const path = OS.Path.split(filename);
> +            target = OS.Path.join(downloadsDir, ...path.components);

Why is this change necessary?

::: toolkit/components/extensions/ext-downloads.js:432
(Diff revision 2)
> -            target = OS.Path.join(downloadsDir, options.filename);
> +            const path = OS.Path.split(filename);
> +            target = OS.Path.join(downloadsDir, ...path.components);
>            } else {
>              let uri = NetUtil.newURI(options.url);
>  
>              let filename;

this is now shadowing filename from the enclosing scope, I think the simplest thing is to just remove this line.

::: toolkit/components/extensions/ext-downloads.js:439
(Diff revision 2)
>                filename = uri.fileName;
>              }
>              target = OS.Path.join(downloadsDir, filename || "download");
>            }
>  
> +          // handle filenames with subdirs

Please start the comment with a capital letter and end with a period, and make this more clear (e.g., "Create any needed intermediate directories if the target is in a subdirectory")

Comment 6

[Tomislav Jovanovic :zombie]

Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

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

Comment 7

[Tomislav Jovanovic :zombie]

Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review76696

> Why is this change necessary?

it was in the previous patch, reverted.

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review76696

> this is now shadowing filename from the enclosing scope, I think the simplest thing is to just remove this line.

I dislike modifying the variable from the outher scope even more. renamed instead.

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review77334

Nice, thank you!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c22083b56078
handle subdirs in browser.downloads filenames, r=aswan

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/c22083b56078

Comment 12

[Tomislav Jovanovic :zombie]

Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review167414

::: toolkit/components/extensions/ext-downloads.js:431
(Diff revision 3)
> -            let filename;
> +            let remote = "download";
>              if (uri instanceof Ci.nsIURL) {
> -              filename = uri.fileName;
> +              remote = uri.fileName;
>              }
> -            target = OS.Path.join(downloadsDir, filename || "download");
> +            target = OS.Path.join(downloadsDir, remote);

This change is wrong, filename will still be empty if the URL doesn't include one.