960962 - OS.File normalization and concatenation issues on Windows

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[:Paolo Amadini]

Assuming the current directory on drive "C:" is "C:\Current":

let file = new FileUtils.File("C:\\"); file.normalize(); file.path
=> "C:\"
OS.Path.normalize("C:\\")
=> "C:\"

let file = new FileUtils.File("C:"); file.normalize(); file.path
=> "C:\Current"
OS.Path.normalize("C:")
=> "C:"

let file = new FileUtils.File("C:file"); file.normalize(); file.path
=> "C:\Current\file"
OS.Path.normalize("C:file")
=> "C:file"

let file = new FileUtils.File("C:"); file.append("file"); file.path
=> "C:\file"
OS.Path.join("C:", "file");
=> "C:file"

Some of these are incorrect.

Comment 1

[:Paolo Amadini]

Out of curiosity:

NetUtil.newURI("file:///C:file").QueryInterface(Ci.nsIFileURL).file.path
=> "C:file"

NetUtil.newURI("file:///C:/file").QueryInterface(Ci.nsIFileURL).file.path
=> "C:\file"

Comment 2

[:Paolo Amadini]

Attachment: The patch

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8361674 [details] [diff] [review]
The patch

Review of attachment 8361674 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Paolo, are we waiting for something or can we mark this checkin-needed?

Comment 5

[:Paolo Amadini]

There's a failure in test_file_URL_conversion.js (https://tbpl.mozilla.org/?tree=Try&rev=13570576d957).

It's a simple fix, I planned to make the change later today but feel free to go ahead and fix it if you prefer.

Comment 6

[:Paolo Amadini]

It seems that the conversion of nsIFileURL always omits the trailing slash:

NetUtil.newURI("file:///C:/dir/").QueryInterface(Ci.nsIFileURL).file.path
=> "C:\dir"

NetUtil.newURI("file:///C:/").QueryInterface(Ci.nsIFileURL).file.path
=> "C:"

This might have been implemented this way for friendliness towards naive concatenation, for example:

fileUrl.file.path + "\subdirectory"

However, the second case resulting in "C:" is definitely incorrect. The OS.File conversion now results in "C:\" for that case, that I think is better and compatible with "normalize" and "join", though incompatible with naive concatenation.

I think we might file a separate bug to change the nsIFileURL behavior and make it consistent, what do you think?

For now, I've simply excluded the test about this case so that this patch can move forward. Tryseerver build running here:
https://tbpl.mozilla.org/?tree=Try&rev=12ffc86ff779

Comment 7

[:Paolo Amadini]

Pushed with the test updated:

https://hg.mozilla.org/integration/fx-team/rev/3b51568473f2

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b51568473f2

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8361674 [details] [diff] [review]
The patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Since initial landing of OS.File.
User impact if declined: Cannot fix bug 958899, i.e. Windows users cannot download to root directories.
Testing completed (on m-c, etc.): Not enough. We should let this spend a few days on m-c before uplifting.
Risk to taking this patch (and alternatives if risky): This fixes a corner case of OS.Path under Windows. There is a small chance that some add-ons relying on the bug might exist but http://mxr.mozilla.org/addons doesn't list any such add-on.
String or IDL/UUID changes made by this patch: None.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Paolo, I'm starting to wonder: is this bug really a blocker for bug 958899 and bug 960046?

Comment 11

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #10)
> Paolo, I'm starting to wonder: is this bug really a blocker for bug 958899
> and bug 960046?

With the latest incarnation of the patch in bug 960046, this bug is not a blocker anymore for the most common case of downloading a file to the root directory of an external drive, while the current directory of the Firefox process is set to the "C:" drive.

In the rare case of the preferred download directory being set to the root directory of the same drive as the one containing the current directory, this patch will ensure that the concatenation of "C:" and "filename.txt" will write the file to the root directory instead of the current directory on the drive.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8361674 [details] [diff] [review]
The patch

As discussed in private with David, comment #11 does not change the uplift request because it still fixes some bugs.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/279f0dc7ee42
https://hg.mozilla.org/releases/mozilla-beta/rev/b37b9d564062

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/b37b9d564062

Comment 15

[u279076]

Marking as [qa-] given in-testsuite coverage. If this needs regression testing before we release please let me know so I can flag it for more focused testing.