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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

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/

Comment 1

[Peiyong Lin[:lpy](UTC+8)]

I take this bug.

Comment 2

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug967507.patch

Comment 3

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

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.

Comment 4

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug967507-V2.patch

I will wait until the patch is good enough to add the tests.

Comment 5

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

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.

Comment 6

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug967507-V3.patch

Thank you

Comment 7

[Peiyong Lin[:lpy](UTC+8)]

I will push to Try when I got r+

Comment 8

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

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.

Comment 9

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug967507-V4.patch

Comment 10

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug967507-V4.patch

only in OSError, the field is |path|, otherwise it is |_path|

Comment 11

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

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.

Comment 12

[Peiyong Lin[:lpy](UTC+8)]

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.

Comment 13

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug967507-V5.patch

push to Try
https://tbpl.mozilla.org/?tree=Try&rev=15351b9d66a3

Comment 14

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug967507-V6.patch

push to Try
https://tbpl.mozilla.org/?tree=Try&rev=a983dd67623c

Comment 15

[Peiyong Lin[:lpy](UTC+8)]

Try looks good.

Comment 16

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/integration/fx-team/rev/6021d523a810

Comment 17

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/6021d523a810