Closed Bug 967507 Opened 6 years ago Closed 6 years ago

[OS.File] Instances of OS.File.Error should come with a filename

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: Yoric, Assigned: lpy)

Details

(Whiteboard: [Async:ready][mentor=Yoric][lang=js])

Attachments

(1 file, 6 obsolete files)

At the moment, when we throw an OS.File.Error, nothing mentions the file name. Experience shows that it would often be very useful to have the file name.

This requires:
- patching the definitions of OSError to have an additional field |path| and to use this field in toString() (in files osfile_shared_allthreads.jsm, osfile_unix_allthreads.jsm, osfile_win_allthreads.jsm );
- patching the uses of OSError in all the files of http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/
I take this bug.
Assignee: nobody → pylaurent1314
Attached patch bug967507.patch (obsolete) — Splinter Review
Attachment #8372202 - Flags: review?(dteller)
Comment on attachment 8372202 [details] [diff] [review]
bug967507.patch

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

Good start.
You should also adapt the methods of File.prototype (which will require storing the path as a field _path). Some of these methods are defined in osfile_{unix, win}_allthreads, some in osfile_shared_front.
Also, I'd like tests for some of the functions.

::: toolkit/components/osfile/modules/osfile_unix_allthreads.jsm
@@ +86,4 @@
>   * failed.
>   * @param {number=} lastError The OS-specific constant detailing the
>   * reason of the error. If unspecified, this is fetched from the system
>   * status.

Please document |path|.

@@ +100,5 @@
>  OSError.prototype = Object.create(SharedAll.OSError.prototype);
>  OSError.prototype.toString = function toString() {
>    return "Unix error " + this.unixErrno +
>      " during operation " + this.operation +
> +    " on file " + this.path +

We should handle a little bit more nicely the case in which path is not defined (e.g. don't display |" on file " + this.path| if we don't know the path).

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +938,5 @@
>       // Utility functions
>  
>       /**
>        * Turn the result of |open| into an Error or a File
>        */

Now that there are two arguments, please document |maybe| and |path|.

@@ +947,5 @@
>         return new File(maybe);
>       }
>  
>       /**
>        * Utility function to sort errors represented as "-1" from successes.

This function needs to be adapted, too.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +1007,4 @@
>         }
>         return new File(maybe);
>       }
>       function throw_on_zero(operation, result) {

This function needs to be adapted, too.
Attachment #8372202 - Flags: review?(dteller) → feedback+
Attached patch bug967507-V2.patch (obsolete) — Splinter Review
I will wait until the patch is good enough to add the tests.
Attachment #8372202 - Attachment is obsolete: true
Attachment #8373968 - Flags: feedback?(dteller)
Comment on attachment 8373968 [details] [diff] [review]
bug967507-V2.patch

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

Looks mostly good.
In osfile_shared_front.jsm, you missed a few instances of throwing in OS.File.read and OS.File.writeAtomic.
Also, I'd like a few tests.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +661,5 @@
>      ).then(
>      function onSuccess(msg) {
>        return {
>          path: msg.path,
> +        file: new File(msg.file, path)

Currently, you don't use this argument anywhere.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +450,5 @@
>             flags |= Const.COPYFILE_EXCL;
>           }
>           throw_on_negative("copy",
> +           UnixFile.copyfile(sourcePath, destPath, null, flags),
> +           this._path

There's no this._path in this context.

@@ +645,5 @@
>         // or if the error is EXDEV and we have passed an option
>         // that prevents us from crossing devices, throw the
>         // error.
>         if (ctypes.errno != Const.EXDEV || options.noCopy) {
> +         throw new File.Error("move", ctypes.errno, destPath);

I believe that this should be the source path.

@@ +752,5 @@
>       /**
>        * Return directory as |File|
>        */
>       File.DirectoryIterator.prototype.unixAsFile = function unixAsFile() {
> +       if (!this._dir) throw File.Error.closed("UnixAsFile", this._path);

lowercase "unixAsFile"

@@ +915,5 @@
>        */
>       File.getCurrentDirectory = function getCurrentDirectory() {
>         let path = UnixFile.get_current_dir_name?UnixFile.get_current_dir_name():
>           UnixFile.getwd_auto(null);
> +       throw_on_null("getCurrentDirectory", path, path);

No, here, we throw if |path| is |null|. So passing |path| as a third argument is useless.

@@ +981,5 @@
> +      *
> +      * @param {string=} operation The name of the operation. If unspecified,
> +      * the name of the caller function.
> +      * @param {number} result The result of the operation that may
> +      * represent either an error or a success. If -1, this function raises

No, the result is a pointer, not a number.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +504,5 @@
>       */
>       File.copy = function copy(sourcePath, destPath, options = {}) {
>         throw_on_zero("copy",
> +         WinFile.CopyFile(sourcePath, destPath, options.noOverwrite || false),
> +         destPath

I'd prefer sourcePath.

@@ +544,5 @@
>           flags = flags | Const.MOVEFILE_REPLACE_EXISTING;
>         }
>         throw_on_zero("move",
> +         WinFile.MoveFileEx(sourcePath, destPath, flags),
> +         destPath

sourcePath

@@ +1070,5 @@
> +      * @param {string=} operation The name of the operation. If unspecified,
> +      * the name of the caller function.
> +      * @param {number} result The result of the operation that may
> +      * represent either an error or a success. If -1, this function raises
> +      * an error holding ctypes.errno, otherwise it returns |result|.

It's a pointer.
Attachment #8373968 - Flags: feedback?(dteller) → feedback+
Attached patch bug967507-V3.patch (obsolete) — Splinter Review
Thank you
Attachment #8373968 - Attachment is obsolete: true
Attachment #8374825 - Flags: feedback?(dteller)
I will push to Try when I got r+
Comment on attachment 8374825 [details] [diff] [review]
bug967507-V3.patch

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

You'll need to double-check:
- this.path vs. this._path;
- that you're never using this.path (or this._path) in a context in which it doesn't exist;
- ctypes.errno (for Unix) vs. ctypes.winLastError (for Windows).

::: toolkit/components/osfile/modules/osfile_shared_allthreads.jsm
@@ +1237,5 @@
>   * details about an error, you should use the platform-specific error
>   * codes provided by subclasses of |OS.Shared.Error|.
>   *
>   * @param {string} operation The operation that failed.
> + * @param {string} path The path of the file that failed.

@param {string=} path The path of the file on which the operation failed, or nothing if there was no file involved in the failure.

@@ +1242,4 @@
>   *
>   * @constructor
>   */
> +function OSError(operation, path) {

Make it a default value
function OSError(operation, path = "") {

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +24,5 @@
>  /**
>   * Code shared by implementations of File.
>   *
>   * @param {*} fd An OS-specific file handle.
> + * @param {string} path File path of the file handle.

"(used for error-reporting)"

@@ +32,2 @@
>    this._fd = fd;
> +  this._path = path;

Just to avoid regressions, can you throw a TypeError if |path| is |undefined|?

::: toolkit/components/osfile/modules/osfile_unix_allthreads.jsm
@@ +72,5 @@
>   * failed.
>   * @param {number=} lastError The OS-specific constant detailing the
>   * reason of the error. If unspecified, this is fetched from the system
>   * status.
> + * @param {string=} path The file path that manipulated. If unspecified,

Just "The file path, if any."

@@ +78,5 @@
>   *
>   * @constructor
>   * @extends {OS.Shared.Error}
>   */
> +let OSError = function OSError(operation, errno, path) {

Here also, define argument |path| as |path = ""|.

@@ +85,2 @@
>    this.unixErrno = errno || ctypes.errno;
> +  this.path = path || "";

You don't need this line. The constructor of |SharedAll.OSError|, which you have modified in osfile_shared_allthreads.jsm will set |this.path|.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +38,5 @@
>        *
>        * You generally do not need to call this constructor yourself. Rather,
>        * to open a file, use function |OS.File.open|.
>        *
>        * @param fd A OS-specific file descriptor.

Please document @param {string} path

@@ +69,5 @@
>           if (typeof fd == "object" && "forget" in fd) {
>             fd.forget();
>           }
>           if (result == -1) {
> +           this._closeResult = new File.Error("close", ctypes.errno, this._path);

In osfile_shared_front.jsm, you have defined it as |this.path|, not |this._path|, I believe. Please pick one or the other.

@@ +915,5 @@
>        */
>       File.getCurrentDirectory = function getCurrentDirectory() {
>         let path = UnixFile.get_current_dir_name?UnixFile.get_current_dir_name():
>           UnixFile.getwd_auto(null);
> +       throw_on_null("getCurrentDirectory", path);

No path here.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +87,5 @@
>           if (typeof fd == "object" && "forget" in fd) {
>             fd.forget();
>           }
>           if (result == -1) {
> +           this._closeResult = new File.Error("close", ctypes.errno, this._path);

Under Windows, you should use ctypes.winLastError instead of ctypes.errno.

@@ +842,5 @@
>       File.Info = function Info(stat) {
>         let isDir = !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_DIRECTORY);
>         let isSymLink = !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_REPARSE_POINT);
>         
> +       let winBirthDate = FILETIME_to_Date(stat.ftCreationTime, this._path);

File.Info is a constructor. There is no this._path in this context. Rather, you should add an argument |path| to |File.Info| and use it here for error reporting.

@@ +977,5 @@
>         while (true) {
>           let array = new (ctypes.ArrayType(ctypes.jschar, buffer_size))();
>           let expected_size = throw_on_zero("getCurrentDirectory",
> +           WinFile.GetCurrentDirectory(buffer_size, array),
> +           this._path

We don't have a path here.
Attachment #8374825 - Flags: feedback?(dteller) → feedback+
Attached patch bug967507-V4.patch (obsolete) — Splinter Review
Attachment #8376261 - Flags: feedback?
Attached patch bug967507-V4.patch (obsolete) — Splinter Review
only in OSError, the field is |path|, otherwise it is |_path|
Attachment #8374825 - Attachment is obsolete: true
Attachment #8376261 - Attachment is obsolete: true
Attachment #8376261 - Flags: feedback?
Attachment #8376263 - Flags: feedback?(dteller)
Comment on attachment 8376263 [details] [diff] [review]
bug967507-V4.patch

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

Looks good to me, with a few minor changes.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +32,2 @@
>    this._fd = fd;
> +  this._path = path;

Can you throw a TypeError if |path| is undefined?

@@ +42,5 @@
>    get fd() {
>      if (this._fd) {
>        return this._fd;
>      }
> +    throw OS.File.Error.closed("get file descriptor", this._path);

Nit: Let's change the message to "accessing file".

::: toolkit/components/osfile/modules/osfile_unix_allthreads.jsm
@@ +78,5 @@
>   *
>   * @constructor
>   * @extends {OS.Shared.Error}
>   */
> +let OSError = function OSError(operation, errno, path = "") {

Let's take the opportunity to make this |OSError(operation = "unknown operation, errno = ctypes.errno, path = "")|.
Same thing for Windows, with winLastError.

@@ +168,3 @@
>                                           lastModificationDate, unixLastStatusChangeDate,
>                                           unixOwner, unixGroup, unixMode) {
> +  this._path = path;

I don't think you ever use the path of an AbstractInfo or Info for error-reporting. If I'm right, please get rid of the changes to AbstractInfo and Info.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +39,5 @@
>        * You generally do not need to call this constructor yourself. Rather,
>        * to open a file, use function |OS.File.open|.
>        *
>        * @param fd A OS-specific file descriptor.
> +      * @param {string} path File path of the file handle, used for error-reporting

Nit: "used for error-reporting." (with a final dot)

@@ +650,5 @@
>           let fd = UnixFile.open(destPath, Const.O_RDONLY, 0);
>           if (fd != -1) {
>             fd.dispose();
>             // The file exists and we have access
> +           throw new File.Error("move", Const.EEXIST, destPath);

Let's use srcPath here and in the next branch. Slightly less useful, but also slightly less confusing if the error message always mentions the source path.
Attachment #8376263 - Flags: feedback?(dteller) → review+
For File.Info in osfile_win_front.jsm, it calls |FILETIME_to_Date|, in which |throw_on_zero| and it needs |path| for error report.
Attached patch bug967507-V5.patch (obsolete) — Splinter Review
push to Try
https://tbpl.mozilla.org/?tree=Try&rev=15351b9d66a3
Attachment #8376263 - Attachment is obsolete: true
Attachment #8378226 - Flags: review+
push to Try
https://tbpl.mozilla.org/?tree=Try&rev=a983dd67623c
Attachment #8378226 - Attachment is obsolete: true
Attachment #8379709 - Flags: review+
Try looks good.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/6021d523a810
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [Async:ready][mentor=Yoric][lang=js] → [Async:ready][mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6021d523a810
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Async:ready][mentor=Yoric][lang=js][fixed-in-fx-team] → [Async:ready][mentor=Yoric][lang=js]
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.