Closed Bug 1402279 Opened 7 years ago Closed 7 years ago

Move the DownloadPaths module to the jsdownloads folder and unify its usage

Categories

(Toolkit :: Downloads API, enhancement, P1)

Product:
Toolkit
Component:
Downloads API
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- verified
firefox59 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1402279 - Part 1 - Move the DownloadPaths module to the jsdownloads folder.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1402279 - Part 2 - Unify the usage of the DownloadPaths module.
59 bytes, text/x-review-board-request
mak
: review+
aswan
: review+
Details 
Now that the concerns around changing the structure of internal files due to add-on compatibility are dispelled, we can finally start to unify the code paths used for assigning local file names. This will result in the assigned names being consistent across different ways of starting the downloads from the user interface.
This unification should help with avoiding files with a leading period in all cases, which is on file as bug 1034989.

As noted there, in cases where we show a file picker it's still possible that the name will be changed and we'll overwrite an existing file, but this is an existing issue with the current code as well. This is still better than creating a hidden file, as noted in bug 753267 comment 7.
Blocks: 1034989
Comment on attachment 8911238 [details]
Bug 1402279 - Part 2 - Unify the usage of the DownloadPaths module.

Adding Andrew since I changed some code to call the new DownloadPaths method instead of throwing an exception.
Attachment #8911238 - Flags: review?(aswan)
Comment on attachment 8911238 [details]
Bug 1402279 - Part 2 - Unify the usage of the DownloadPaths module.

https://reviewboard.mozilla.org/r/182732/#review188090

We try to avoid changing the behavior of webextensions apis without a compelling reason.  It was a conscious decision for download() to fail if given a bad path rather than silently rewriting it.  Can you elaborate on why you want to change the behavior?  I can imagine various reasons and I don't want to guess about what the actual reason is...
Attachment #8911238 - Flags: review?(aswan)
Comment on attachment 8911238 [details]
Bug 1402279 - Part 2 - Unify the usage of the DownloadPaths module.

(In reply to Andrew Swan [:aswan] from comment #6)
> We try to avoid changing the behavior of webextensions apis without a
> compelling reason.  It was a conscious decision for download() to fail if
> given a bad path rather than silently rewriting it.  Can you elaborate on
> why you want to change the behavior?  I can imagine various reasons and I
> don't want to guess about what the actual reason is...

The reason is that we are slightly changing the logic here, and the choice is between making more of the existing uses in extensions succeed versus making more of them fail with an exception. To enforce the same file system rules we have in the operating system and in the rest of Firefox, for example to avoid the creation of hidden files as noted in comment 1, we need to be more conservative in the validation, and different classes of mismatch like a leading dot or space will now be considered invalid. In the earlier patch, the extensions would just inherit the unified logic we now have for prettifying some of the invalid characters and sequences.

However, I agree that the API should be consistent with what Chrome does, and if it throws an exception then we should as well. This version of the patch is an alternative that throws if the file name has any character or sequence that is invalid for the current platform. Since this is platfrom-dependent, add-ons that accept user input would likely need to replicate the full sanitization logic in their own code, either the most restrictive or the one based on the current platform, before providing a target file name to the API.
Attachment #8911238 - Flags: review?(aswan)
Priority: -- → P1
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Comment on attachment 8911238 [details]
Bug 1402279 - Part 2 - Unify the usage of the DownloadPaths module.

https://reviewboard.mozilla.org/r/182732/#review188536

The extensions bits look great, thanks!
Attachment #8911238 - Flags: review?(aswan) → review+
Comment on attachment 8911237 [details]
Bug 1402279 - Part 1 - Move the DownloadPaths module to the jsdownloads folder.

https://reviewboard.mozilla.org/r/182730/#review188736

I still think we should flatten jsdownloads/src into jsdownloads (like the most of the other components), and merge jsdownloads/ and downloads/ folders in components/. I'm not sure about the value of keeping toolkit/mozapps/downloads too, I think it would be more valuable to have all the related code in one place. This is going the right direction and that's great!
Attachment #8911237 - Flags: review?(mak77) → review+
Comment on attachment 8911238 [details]
Bug 1402279 - Part 2 - Unify the usage of the DownloadPaths module.

https://reviewboard.mozilla.org/r/182732/#review188756

r=me with a few things addressed/answered.

::: toolkit/components/jsdownloads/src/DownloadPaths.jsm:23
(Diff revision 2)
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
> +                                  "resource://gre/modules/AppConstants.jsm");
> +
> +/**
> + * Platform-dependent regular expression used by the "sanitize" mathod.

typo: mathod

Is there an "official" source for these, where we can look at in the future?
A comment explaining how these values were created would help future contributions.

::: toolkit/components/jsdownloads/src/DownloadPaths.jsm:33
(Diff revision 2)
> +    // On mobile devices, the file system may be very limited in what it
> +    // considers valid characters. To avoid errors, sanitize conservatively.
> +    case "android": return /[\x00-\x1F:*?|"<>;,+=\[\]]+/g;
> +    case "win": return /[\x00-\x1F:*?|]+/g;
> +    case "macosx": return /[\x00-\x1F:]+/g;
> +    default: return /[\x00-\x1F]+/g;

Shouldn't we handle 0x7F and the C1 (0x80-0x9F) control codes as well?

https://en.wikipedia.org/wiki/C0_and_C1_control_codes

I'm not sure whether we are also doing that, but we should also crop the file name at 255 chars, since that's the limit for some common filesystems (FAT, some versions of MacOS fs).

Source: I was looking at links pointed out by this nice module https://www.npmjs.com/package/sanitize-filename. Also note (for testing purposes) https://github.com/minimaxir/big-list-of-naughty-strings (MIT license)

::: toolkit/components/jsdownloads/src/DownloadPaths.jsm:34
(Diff revision 2)
> +    // considers valid characters. To avoid errors, sanitize conservatively.
> +    case "android": return /[\x00-\x1F:*?|"<>;,+=\[\]]+/g;
> +    case "win": return /[\x00-\x1F:*?|]+/g;
> +    case "macosx": return /[\x00-\x1F:]+/g;
> +    default: return /[\x00-\x1F]+/g;
> +  }

nit: I'd prefer the common indentation rules, but no strong feelings.

::: toolkit/content/contentAreaUtils.js:1073
(Diff revision 2)
>        // We purposefully do not use a localized default filename,
>        // which we could have done using
>        // ContentAreaUtils.stringBundle.GetStringFromName("DefaultSaveFileName")
>        // since it may contain invalid characters.
>        var original = processed;
>        processed = "download";

Considered both this code and WebExt provide a default name... I wonder if we shouldn't make the util itself do that, with a second argument

::: toolkit/content/contentAreaUtils.js:1076
(Diff revision 2)
>        // since it may contain invalid characters.
>        var original = processed;
>        processed = "download";
>  
>        // Preserve a suffix, if there is one
>        if (original.indexOf(".") >= 0) {

can this actually happen? if the file was named "invalid_chars.something" wouldn't sanitize return "something"?

What if the extension contains invalid chars? looks like this code will just reappend them, generating an invalid file name.
Attachment #8911238 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #12)
> Is there an "official" source for these, where we can look at in the future?

I kept the logic of filesystem-specific characters from the existing code, and checked it against this non-official source:

https://en.wikipedia.org/wiki/Filename#Comparison_of_filename_limitations

> ::: toolkit/components/jsdownloads/src/DownloadPaths.jsm:33
> (Diff revision 2)
> Shouldn't we handle 0x7F and the C1 (0x80-0x9F) control codes as well?

Good call, they're valid but it's better to convert them to spaces regardless.

> I'm not sure whether we are also doing that, but we should also crop the
> file name at 255 chars, since that's the limit for some common filesystems
> (FAT, some versions of MacOS fs).

Yes, we're already doing that when we hit the file system later, as the final limit depends on the full path. I've updated the comment to specify.

> >        // Preserve a suffix, if there is one
> >        if (original.indexOf(".") >= 0) {
> 
> can this actually happen? if the file was named "invalid_chars.something"
> wouldn't sanitize return "something"?

I think the idea is that if "++++1++++.txt" becomes "1.txt", the file will be named "download.txt". But that's right, "++++++++.txt" would become "txt". Maybe we could handle this case better in a follow-up.

> What if the extension contains invalid chars? looks like this code will just
> reappend them, generating an invalid file name.

This operates on the sanitized name, so there shouldn't be any invalid characters. The code is not written very clearly, but for the moment I just kept it unchanged. We should probably unify Desktop and Android to both use the same logic and a localized default name at some point.

> Considered both this code and WebExt provide a default name... I wonder if
> we shouldn't make the util itself do that, with a second argument

We might have to do that if we want to preserve extensions when the base name is only made of invalid characters. For the first step, I think we can avoid to make the API more complex, and just keep what we already do for validation.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5bc3152714e
Part 1 - Move the DownloadPaths module to the jsdownloads folder. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/41da9c5daa90
Part 2 - Unify the usage of the DownloadPaths module. r=mak,aswan
Backed out for failing devtools' devtools/client/jsonview/test/browser_jsonview_save_json.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d1d138d90dba6ed1111da13f1a0ea1bb5517f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/353347df68d12bccd3874517afa67fa3af253e45

Push which ran failing test: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2da272c1ad8ddd8a731a1ff93e0b7a178e6c9dcb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133363969&repo=mozilla-inbound

[task 2017-09-26T15:52:37.287Z] 15:52:37     INFO - TEST-PASS | devtools/client/jsonview/test/browser_jsonview_save_json.js | Original JSON contents should have been saved. - 
[task 2017-09-26T15:52:37.287Z] 15:52:37     INFO - Selecting tab: 'rawdata'
[task 2017-09-26T15:52:37.288Z] 15:52:37     INFO - Switched to Raw Data tab.
[task 2017-09-26T15:52:37.290Z] 15:52:37     INFO - Clicked Pretty Print button.
[task 2017-09-26T15:52:37.291Z] 15:52:37     INFO - Clicked Save button.
[task 2017-09-26T15:52:37.291Z] 15:52:37     INFO - TEST-PASS | devtools/client/jsonview/test/browser_jsonview_save_json.js | File picker was opened - 
[task 2017-09-26T15:52:37.292Z] 15:52:37     INFO - Buffered messages finished
[task 2017-09-26T15:52:37.294Z] 15:52:37     INFO - TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_save_json.js | File picker should provide the correct default filename. - Got _.json, expected simple_json.json
[task 2017-09-26T15:52:37.295Z] 15:52:37     INFO - Stack trace:
[task 2017-09-26T15:52:37.295Z] 15:52:37     INFO - chrome://mochikit/content/browser-test.js:test_is:1011
[task 2017-09-26T15:52:37.296Z] 15:52:37     INFO - chrome://mochitests/content/browser/devtools/client/jsonview/test/browser_jsonview_save_json.js:awaitFileSave/</MockFilePicker.showCallback:38
[task 2017-09-26T15:52:37.297Z] 15:52:37     INFO - chrome://specialpowers/content/MockFilePicker.jsm:open/</<:275
Flags: needinfo?(paolo.mozmail)
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/976b17e4e111
Part 1 - Move the DownloadPaths module to the jsdownloads folder. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d44cbfb37ff
Part 2 - Unify the usage of the DownloadPaths module. r=mak,aswan
https://hg.mozilla.org/mozilla-central/rev/976b17e4e111
https://hg.mozilla.org/mozilla-central/rev/4d44cbfb37ff
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8911238 [details]
Bug 1402279 - Part 2 - Unify the usage of the DownloadPaths module.

Approval Request Comment
[Feature/Bug causing the regression]: DownloadPaths module
[User impact if declined]: For example, users would still be able to create hidden files accidentally (bug 1034989), and special characters in document titles could be used to create files on the user system which have valid names but are difficult to access.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: Try to save a file by adding a leading dot in the name selection dialog and verify that it is always saved without the dot. As a regression test, try saving documents with different complex titles and verify that the chosen name is still similar, even if not identical, to what was used before this bug was fixed.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Medium risk but one that I think is appropriate for beta
[Why is the change risky/not risky?]: We change the way some file names are prettified by making it consistent across different ways of starting downloads. This means that some documents may be saved with slightly different names than before, for example with an invalid character being replaced by a space instead of an underscore. This doesn't affect anything else in the save functionality.
[String changes made/needed]: None
Flags: needinfo?(paolo.mozmail)
Attachment #8911238 - Flags: approval-mozilla-beta?
> [Feature/Bug causing the regression]: DownloadPaths module
When was it introduced?
From bug 1034989, I can guess that it is at least several years old, correct?

I am not happy to take a refactor in 57beta. This is a too important release to try if I feel lucky or not ;)
Comment on attachment 8911238 [details]
Bug 1402279 - Part 2 - Unify the usage of the DownloadPaths module.

Ok, having this change ride the trains sounds good to me.
Attachment #8911238 - Flags: approval-mozilla-beta?
Flags: qe-verify+
I tested this bug using an older version of Nightly (2017-09-22) and latest Nightly and I got different results:

- on Windows 10 x64:
    - saving a file with the name ".test":
             - older Nightly: file is saved as ".test"
             - latest Nightly: file is saved as ".test"

- on Ubuntu 16.04 x64:
    - saving a file with the name ".test":
             - older Nightly: file is saved as ".test" - the file is hidden
             - latest Nightly: file is saved as ".test" - the file is hidden

- on macOS 10.13:
    - saving a file with the name ".test":
             - older Nightly: it's not allowed that the file is saved as ".test"
             - latest Nightly: it's not allowed that the file is saved as ".test"

As far as I can tell only macOS doesn't allow you to save the files with the name ".test". 
I don't know what the expected result should be. Paolo, can you please have a quick look into this?
Thank you.
Flags: needinfo?(paolo.mozmail)
Oana, sorry for the late reply. There are several code paths that ask for a file name, maybe I missed some of them here, in which case we may need to file a new bug. Note that we should remove the dot both before we suggest a name in the file picker, and after the file name is edited by the user. It seems that the issue you noticed here is related to the latter.

Can you verify the following cases, and see which ones still preserve the starting dot after the file name is edited?
- Clicking a download link after setting the preference to always ask where to save the file
- Right clicking a link and choosing "Save Link As"
- Using the "Save Page As" command
Flags: needinfo?(paolo.mozmail)
 Sorry it took so long to respond, but I was on pto.

 I retested everything using the latest Nightly and beta 58.0b13:

On Windows 10 x64:
- Clicking a download link after setting the preference to always ask where to save the file
               - the file is saved as "test"
- Right clicking a link and choosing "Save Link As"
               - the file is saved as "test"
- Using the "Save Page As" command
               -  the file is saved as ".test"

On Mac OS 10.11:
- Clicking a download link after setting the preference to always ask where to save the file
               - the file is saved as "test"
- Right clicking a link and choosing "Save Link As"
               - the file is saved as "test"
- Using the "Save Page As" command
               -  the file is saved as "test" 

      Even if it says that the file is hidden if the user uses "." in the name, the file is visible and saved with the name "test".


On Ubuntu 16.04 x64:
- Clicking a download link after setting the preference to always ask where to save the file
               - the file is saved as "test"
- Right clicking a link and choosing "Save Link As"
               - the file is saved as "test"
- Using the "Save Page As" command
               -  the file is saved as ".test" - the file is hidden
Flags: needinfo?(paolo.mozmail)
Thank you, that was quite useful. The bug with "Save Page As" is probably an old one, it seems that on Windows and Linux if the name is edited in the file picker we'll always use the specified name for saving, even though the code is supposed to change it.

Can you file a new bug for that specific issue if there isn't a duplicate already? I think this bug instead can be marked as verified if this is the only issue.
Flags: needinfo?(paolo.mozmail) → needinfo?(oana.botisan)
I filled a new bug from that problem, bug 1430029, because I couldn't find another one logged.
According to the comment 25 and comment 26, I will mark this bug as verified fixed.
status-firefox58: fixed → verified
status-firefox59: --- → verified
Flags: needinfo?(oana.botisan)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: