1383262 - browser.downloads.download fails if filename contains ":"

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[kernp25]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170720030203



Actual results:

The following code fails:
browser.downloads.download({
    url: downloadUrl,
    filename: "Template:DailyDilbert.pdf",
    saveAs: true
  });

Comment 1

[kernp25]

The error message will be:
Error: An unexpected error occurred

Comment 2

[kernp25]

If you replace the ":" with "_" or " " then it works.

Comment 3

[kernp25]

I got this from validateFileName function from contentAreaUtils.js [1]

[1] http://searchfox.org/mozilla-central/source/toolkit/content/contentAreaUtils.js

Comment 4

[Kris Maglione [:kmag]]

That's because ":" is an illegal character in filenames on Windows.

Although I suppose we should handle it with a better error.

Comment 5

[kernp25]

browser.downloads.download should call validateFileName i think.

Comment 6

[kernp25]

It also fails for this url:
https://www.waxmann.com/?eID=texte&pdf=2814Volltext.pdf&typ=zusatztext

browser.downloads.download({
  url: "https://www.waxmann.com/?eID=texte&pdf=2814Volltext.pdf&typ=zusatztext",
  saveAs: true
});

Note: There is no filename now

The error message in console:
Error: Incorrect use of option |from|: C:\Users\user is not a descendant of C:\Users\user\Downloads  osfile_shared_front.jsm:546
	postMessage resource://gre/modules/PromiseWorker.jsm:321:17
	throw self-hosted:1200:9
Error: An unexpected error occurred

Comment 7

[Andrew Swan [:aswan]]

The last issue is separate.  The real underlying issue is bug 1254327 but in the mean time there's a bug with downloading a url with an empty filename.  zombie, it looks like the bug 1280044 inadvertently broke the logic that created the target file is the filename in the original url was empty.  That logic wasn't covered by a unit test though. :(

Comment 8

[Tomislav Jovanovic :zombie]

Yeah, that was dumb https://reviewboard.mozilla.org/r/77916/#comment216544

Comment 9

[Tomislav Jovanovic :zombie]

Attachment: Bug 1383262 - Better download() error message and default filename

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

Comment 10

[Tomislav Jovanovic :zombie]

Comment on attachment 8891705 [details]
Bug 1383262 - Better download() error message and default filename

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

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8891705 [details]
Bug 1383262 - Better download() error message and default filename

https://reviewboard.mozilla.org/r/162766/#review168364

The default filename bit looks good, if you want to split it out we can land it right now.  But I'd like to find a better way to handle filename validation.

::: toolkit/components/extensions/ext-downloads.js:421
(Diff revision 2)
> +            if (AppConstants.platform === "win" && /[|"*?:<>]/.test(filename)) {
> +              return Promise.reject({message: "filename must not contain illegal characters"});
> +            }

For downloads initiated from content, we have this:
http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/content/contentAreaUtils.js#1073

That's probably not directly usable here but do you think we could find a way to just write that code once and share it between contentAreaUtils and here?

Comment 12

[Tomislav Jovanovic :zombie]

Comment on attachment 8891705 [details]
Bug 1383262 - Better download() error message and default filename

https://reviewboard.mozilla.org/r/162766/#review168364

> For downloads initiated from content, we have this:
> http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/content/contentAreaUtils.js#1073
> 
> That's probably not directly usable here but do you think we could find a way to just write that code once and share it between contentAreaUtils and here?

As per our discussion over irc, this bug is about a better error message, not about actually validating and/or sanitizing the filename on different platforms (which is probably not even possible, or at least an open-ended task), since the download will fail anyway with an invalid filename.

I tried to catch the error message from the OS, but it seems platform specific ("Win error 123 during operation open on file") and not even uniform on just Windows, but depends on the illegal character used.

So I think this approach to just cover the most common cases (on Windows, since virtually all characters are legal on *nix) is good enough, and anything more involved would be over-engineering.

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8891705 [details]
Bug 1383262 - Better download() error message and default filename

https://reviewboard.mozilla.org/r/162766/#review169046

Okay thanks for digging in and sorry for the wild goose chase...

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df3624361dc6
Better download() error message and default filename  r=aswan

Comment 15

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

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