807875 - creationDate is wrong

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Richard Newman [:rnewman]]

const {utils: Cu, classes: Cc, interfaces: Ci} = Components;
Cu.import("resource://gre/modules/osfile.jsm")

// Finder reports this file as both created and modified:
//   27 Jan 2004 2:37am
let path = "/Users/rnewman/Library/Application Support/Firefox/Profiles/zqpyta06.default/cookperm.txt";
let promise = OS.File.stat(path);
promise.then(
  function onSuccess(info) {
    print(info.creationDate);
  },
  function onError(reason) {
    throw reason;
  });

----

This almost always just prints the promise to my console. Very occasionally I can get it to actually run, in which case it prints:

Tue Mar 08 2011 14:56:17 GMT-0800 (PST)

which is totally wrong.

Mac 10.6.8, HFS+ journaled.

ls output:

$ ls -lTUt cookperm.txt
-rw-r--r--  1 rnewman  rnewman  222 Jan 27 02:37:28 2004 cookperm.txt
$ ls -lTut cookperm.txt
-rw-r--r--  1 rnewman  rnewman  222 Nov  1 17:57:37 2012 cookperm.txt
$ ls -lTt cookperm.txt
-rw-r--r--  1 rnewman  rnewman  222 Jan 27 02:37:28 2004 cookperm.txt

Comment 1

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

I can already confirm the "this almost always just prints the promise to my console", which is very weird.

Comment 2

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

Interestingly,

let promise = OS.File.stat(path);
promise.then(
  function onSuccess(info) {
    console.log("info", info.creationDate);
  },
  function onError(reason) {
    throw reason;
  });

prints the promise, but

let promise = OS.File.stat(path);
promise.then(
  function onSuccess(info) {
    print("info", info, info.creationDate);
  },
  function onError(reason) {
    throw reason;
  });

prints a date, which is surprising.

Comment 3

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

Ah, my bad, my test protocol was wrong. The console was just printing the result of |promise.then|, which is a promise once again. So, as far as I can tell, the printing is correct. To check this, just add "promise = null;", or anything trivial, at the end of your snippet.

On the other hand, I confirm that creationDate is actually the last status change date. This needs some fixing.

Comment 4

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

By the way, note that file creation date is something that is not present on all file systems (e.g. 32 bit HFS) or on all operating systems (e.g. Linux). You may wish to not to rely on it too much.

Comment 5

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

Attachment: Implementing creationDate for Unix

Attaching an implementation of creationDate. This is a little awkward because it is not reliable on all filesystems or OSes.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 677751 [details] [diff] [review]
Implementing creationDate for Unix

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

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +785,5 @@
> +         let time;
> +         if ("OSFILE_OFFSETOF_STAT_ST_BIRTHTIME" in OS.Constants.libc) {
> +           time = this._st_birthtime;
> +         } else {
> +           time = this._st_ctime;

Ew.  Shoulda caught this at initial review.

I am tempted to say that we should just throw if there's no _st_birthtime and update the docs accordingly.  I don't think it's worth lying to the user.

Or, if we must lie, we should *really* lie and return something that's not going to change, like 0.  Just like (presumably) _st_birthtime.

@@ +817,5 @@
>         },
>         /**
> +        * Return the date at which the status of this file was last modified
> +        * (this is the date of the latest write/renaming/mode change/...
> +        * of the file)

My stat(2) man page on Linux says: "The field st_ctime is changed by writing or by setting inode information  (i.e., owner, group, link count, mode, etc.)."  I don't think that's consistent with what you say here.  (Specifically, I don't think writing data to the file necessarily changes the ctime--and possibly not at all...?)

Comment 7

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

Let's review the combinations:
- on Windows + NTFS, creationDate works;
- on Windows + FAT32, I do not know;
- on MacOS X with _DARWIN_FEATURE_64_BIT_INODE (I think that's Mac OSX 10.5.5+), on HFS +, birthtime works;
- on MacOS X with _DARWIN_FEATURE_64_BIT_INODE (I think that's Mac OSX 10.5.5+), on other file systems, birthtime may default to ctime (!);
- on older MacOS X, there is no birthtime;
- on Linux, there is no birthtime.

So, we could do the following:
- on Windows and platforms with |birthtime|, return whatever is in |birthtime| (or the Windows equivalent);
- on other platforms, return the epoch;
- document that the result may be inadequate, and the detailed behavior.

I don't think we should throw an error, as this can break existing code.

> 
> @@ +817,5 @@
> >         },
> >         /**
> > +        * Return the date at which the status of this file was last modified
> > +        * (this is the date of the latest write/renaming/mode change/...
> > +        * of the file)
> 
> My stat(2) man page on Linux says: "The field st_ctime is changed by writing
> or by setting inode information  (i.e., owner, group, link count, mode,
> etc.)."  I don't think that's consistent with what you say here. 
> (Specifically, I don't think writing data to the file necessarily changes
> the ctime--and possibly not at all...?)

Oh, great, it's OS-specific. On MacOS X, |write| changes |st_ctime|.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> Let's review the combinations:
> - on Windows + NTFS, creationDate works;
> - on Windows + FAT32, I do not know;
> - on MacOS X with _DARWIN_FEATURE_64_BIT_INODE (I think that's Mac OSX
> 10.5.5+), on HFS +, birthtime works;
> - on MacOS X with _DARWIN_FEATURE_64_BIT_INODE (I think that's Mac OSX
> 10.5.5+), on other file systems, birthtime may default to ctime (!);
> - on older MacOS X, there is no birthtime;
> - on Linux, there is no birthtime.
> 
> So, we could do the following:
> - on Windows and platforms with |birthtime|, return whatever is in
> |birthtime| (or the Windows equivalent);

Can you find out what FAT32 does here?  We should think about handling that specially.

> - on other platforms, return the epoch;
> - document that the result may be inadequate, and the detailed behavior.
> 
> I don't think we should throw an error, as this can break existing code.

OK, I'm fine with that.  Probably wouldn't be a bad idea to ask Mossop for sr+ on this patch.

> Oh, great, it's OS-specific. On MacOS X, |write| changes |st_ctime|.

I don't think that's necessarily the end of the world.  It can be documented as such and we can throw our hands up at bringing consistency to the wide variety of Unix platforms.  (It's also possible that the Linux man page is just wrong or poorly worded.  Should be pretty easy to verify.)

Comment 9

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

Variant strategy:
- introduce |winCreationDate| and |macCreationDate|, documenting the quirks;
- deprecate |creationDate|, but still ensure that on Mac, it aliases to |macCreationDate|;
- introduce |unixLastStatusChangeDate|, documenting the quirks as best as possible.

This ensures that we have stuff that works, without breaking anything.

Comment 10

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

By the way, according to MSDN, creation date should work also on fat32 under Windows.

Comment 11

[Richard Newman [:rnewman]]

Something I'd love to see: a way to ask OS.File whether the filesystem that contains a particular file supports creation time -- in other words, if the info returned by stat() not only gave access to `creationDate`, but also whether or not it is an actual creation time.

This will give callers a way to fall back to other mechanisms without writing their own OS/FS-sniffing code.

Comment 12

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

(In reply to Richard Newman [:rnewman] from comment #11)
> Something I'd love to see: a way to ask OS.File whether the filesystem that
> contains a particular file supports creation time -- in other words, if the
> info returned by stat() not only gave access to `creationDate`, but also
> whether or not it is an actual creation time.
> 
> This will give callers a way to fall back to other mechanisms without
> writing their own OS/FS-sniffing code.

This looks a little tricky. Could you file a bug on the topic?

Comment 13

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

Attachment: Implementing winCreationDate/macCreationDate (v2)

Here's v2, what do you think?

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 678287 [details] [diff] [review]
Implementing winCreationDate/macCreationDate (v2)

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

WFM!  Going to ask Mossop for sr+ on this one, especially given the discussion in bug 808321.

Comment 15

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

Attachment: Implementing winCreationDate/macCreationDate (v3)

Same patch, with a trivial typo fix.

Comment 16

[Dave Townsend [:mossop]]

FYI I'm out till next week so if you need a faster turn-around then someone else, bsmedberg maybe, might get to if faster.

Comment 17

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

Marking bsmedberg as sr?, as I understand that this patch needs to be uplifted quickly to 18 for rnewman.

Comment 18

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

Thanks for the sr+.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to inbound. Please rebase. Also, has this been through Try?

Comment 20

[Richard Newman [:rnewman]]

Attachment: Un-bitrotted patch

Pushing this through try now.

Comment 21

[Richard Newman [:rnewman]]

Try build:

https://tbpl.mozilla.org/?tree=Try&rev=3434bc76f5ba

Looks pretty clean, so:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb65f083690f

Let's give this a while to bake, then I'll request Aurora uplift.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb65f083690f

Comment 23

[Mozilla RelEng Bot]

Try run for 3434bc76f5ba is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3434bc76f5ba
Results (out of 313 total builds):
    success: 295
    warnings: 16
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3434bc76f5ba