Closed Bug 960962 Opened 6 years ago Closed 6 years ago

OS.File normalization and concatenation issues on Windows

Categories

(Toolkit :: OS.File, defect)

All
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox28 --- fixed
firefox29 --- fixed
firefox30 --- fixed
b2g-v1.3 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Whiteboard: [Async:team][qa-])

Attachments

(1 file)

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.
Blocks: 960046, 958899
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"
Attached patch The patchSplinter Review
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8361674 - Flags: review?(dteller)
Comment on attachment 8361674 [details] [diff] [review]
The patch

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

Looks good to me.
Attachment #8361674 - Flags: review?(dteller) → review+
Paolo, are we waiting for something or can we mark this checkin-needed?
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.
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
Pushed with the test updated:

https://hg.mozilla.org/integration/fx-team/rev/3b51568473f2
https://hg.mozilla.org/mozilla-central/rev/3b51568473f2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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.
Attachment #8361674 - Flags: approval-mozilla-beta?
Attachment #8361674 - Flags: approval-mozilla-aurora?
Paolo, I'm starting to wonder: is this bug really a blocker for bug 958899 and bug 960046?
Flags: needinfo?(paolo.mozmail)
(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.
Flags: needinfo?(paolo.mozmail)
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.
Attachment #8361674 - Flags: approval-mozilla-beta?
Attachment #8361674 - Flags: approval-mozilla-beta+
Attachment #8361674 - Flags: approval-mozilla-aurora?
Attachment #8361674 - Flags: approval-mozilla-aurora+
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.
Whiteboard: [Async:team] → [Async:team][qa-]
You need to log in before you can comment on or make changes to this bug.