creationDate is wrong

RESOLVED FIXED in mozilla19

Status

()

Toolkit
OS.File
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: rnewman, Assigned: Yoric)

Tracking

unspecified
mozilla19
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Blocks: 807842
I can already confirm the "this almost always just prints the promise to my console", which is very weird.
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.
Assignee: nobody → dteller
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.
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.
Created attachment 677751 [details] [diff] [review]
Implementing creationDate for Unix

Attaching an implementation of creationDate. This is a little awkward because it is not reliable on all filesystems or OSes.
Attachment #677751 - Flags: review?(nfroyd)
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...?)
Attachment #677751 - Flags: review?(nfroyd) → review-
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|.
(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.)
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.
By the way, according to MSDN, creation date should work also on fat32 under Windows.
(Reporter)

Comment 11

6 years ago
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.
(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?
(Reporter)

Updated

6 years ago
Blocks: 808321
Created attachment 678287 [details] [diff] [review]
Implementing winCreationDate/macCreationDate (v2)

Here's v2, what do you think?
Attachment #677751 - Attachment is obsolete: true
Attachment #678287 - Flags: review?(nfroyd)
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.
Attachment #678287 - Flags: superreview?(dtownsend+bugmail)
Attachment #678287 - Flags: review?(nfroyd)
Attachment #678287 - Flags: review+
Created attachment 679621 [details] [diff] [review]
Implementing winCreationDate/macCreationDate (v3)

Same patch, with a trivial typo fix.
Attachment #678287 - Attachment is obsolete: true
Attachment #678287 - Flags: superreview?(dtownsend+bugmail)
Attachment #679621 - Flags: superreview?(dtownsend+bugmail)
Attachment #679621 - Flags: review+
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.
Marking bsmedberg as sr?, as I understand that this patch needs to be uplifted quickly to 18 for rnewman.
Attachment #679621 - Flags: superreview?(dtownsend+bugmail) → superreview?(benjamin)

Updated

6 years ago
Attachment #679621 - Flags: superreview?(benjamin) → superreview+
Thanks for the sr+.
Keywords: checkin-needed
This doesn't apply cleanly to inbound. Please rebase. Also, has this been through Try?
Keywords: checkin-needed
(Reporter)

Comment 20

6 years ago
Created attachment 681811 [details] [diff] [review]
Un-bitrotted patch

Pushing this through try now.
(Reporter)

Comment 21

6 years ago
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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/fb65f083690f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Comment 23

5 years ago
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/rnewman@mozilla.com-3434bc76f5ba
You need to log in before you can comment on or make changes to this bug.