802534 - [OS.File] Main thread versions of OS.File.Info and OS.File.DirectoryIterator.Entry should have nice prototypes

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

On worker threads, we can perform feature detection on OS.File.Info/OS.File.DirectoryIterator.Entry as follows:

if ("unixGroup" in OS.File.Info.prototype) {
  // OS.File.Info can be used to determine the Unix group of a file
} else {
  // Do something else
}

On the main thread, this is not possible yet.

We should do the following:
- in osfile_{win,unix}_allthreads.jsm: define OS.File.AbstractInfo with a prototype that declares placeholders for the fields of OS.File.Info;
- in osfile_{win,unix}_front.jsm: ensure that OS.File.Info inherits OS.File.AbstractInfo;
- in osfile_abstract_front.jsm: ensure that OS.File.Info inherits OS.File.AbstractInfo.

+ the same thing for OS.File.DirectoryIterator.Entry

Comment 1

[Eduard Neculaesi (:eduardn)]

I would like to work on this bug. Can it be assigned to me?

Comment 2

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

With pleasure.

Comment 3

[Eduard Neculaesi (:eduardn)]

Attachment: Rough Patch

Hi! Sorry for the delay. 

I made a first modification to |osfile_win_allthreads| and |osfile_win_front| but I want to know if my approach is good. Please provide feedback to know if I should continue along this path. 

Thanks!

Comment 4

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

Comment on attachment 689133 [details] [diff] [review]
Rough Patch

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

I don't think that this will work. Your constructor |AbstractInfo| and its prototype require a |stat| containing native js-ctypes data structures. Unfortunately, these data structures cannot be transmitted across threads, so |AbstractInfo| cannot be used by osfile_async_front.jsm.

I can see two ways to do this.

Solution 1:
- have |AbstractInfo| be a very dumb constructor that does nothing interesting;
- attach to |AbstractInfo.prototype| very dumb versions of |isDir|, |isSymlink| that throw if they are called;
- keep the real implementation of |isDir|, |isSymlink|, etc. with |Info|

Solution 2:
- have |AbstractInfo| be a not quite so dumb constructor that takes as arguments |isDir|, |isSymlink|, etc. and stores them as |_isDir|, |_isSymlink|, etc.;
- attach to |AbstractInfo.prototype| very dumb versions of |isDir|, |isSymlink| that simply return |this._isDir|, |this._isSymlink|, etc.
- rework constructor |Info| to compute the values of |isDir|, etc. and pass them to |AbstractInfo|.

Solution 2 is probably best.

Comment 5

[Eduard Neculaesi (:eduardn)]

Attachment: Rough Patch (v2)

I applied your feedback but I'm still not sure if I am doing the right thing. I still have a couple of questions.

1. in some of the |gets| there is a call to |Object.defineProperty|. Should I keep that in |AbstractInfo|
2. Where should I create a test to see if the code has the expected behavior.

With the current code (from patch) all the tests pass.

Comment 6

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

Comment on attachment 689695 [details] [diff] [review]
Rough Patch (v2)

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

That looks good.

> 1. in some of the |gets| there is a call to |Object.defineProperty|. Should I keep that in |AbstractInfo|
Indeed, the calls to |Object.defineProperty| do not match this new design, you were right to remove them. They were used to perform lazy conversion of data from native to JavaScript, but this was most likely overkill. We will reintroduce them later, in another bug, if experience shows that it can optimize uses of OS.File.

> 2. Where should I create a test to see if the code has the expected behavior.
A good place for testing would be main_osfile_async.js. You can check, for instance, that under Unix, ("unixGroup" in OS.File.Info.prototype). To determine whether we are under Unix, check whether OS.Constants.Win is defined.

Comment 7

[Eduard Neculaesi (:eduardn)]

Attachment: First patch

I made all the necessary modifications (tested on windows). If everything is ok, could you please run the patch on the try server?

Comment 8

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

Comment on attachment 690107 [details] [diff] [review]
First patch

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

Looks good, with the few changes detailed below. Let's test it.

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +688,5 @@
> +  File.Info.prototype = Object.create(OS.Shared.Win.AbstractInfo.prototype);
> +} else if (OS.Constants.libc) {
> +  File.Info.prototype = Object.create(OS.Shared.Unix.AbstractInfo.prototype);
> +} else {
> +  throw new Error("I am neither under Windows nor under a Posix system");

In practice |OS.Constants.libc| is always defined, since libc is available on all systems, so the second |if| doesn't add any information.

::: toolkit/components/osfile/osfile_unix_allthreads.jsm
@@ +162,5 @@
>  
> +  /**
> +   * Code shared by implementations of File.Info on Unix
> +   *
> +  */

Nit: whitespace.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +701,4 @@
>       let gStatDataPtr = gStatData.address();
>       let MODE_MASK = 4095 /*= 07777*/;
>       File.Info = function Info(stat) {
> +       let isDir = (stat.st_mode & OS.Const.libc.S_IFMT) == OS.Constants.libc.S_IFDIR;

|OS.Const|? This probably doesn't work.

@@ +707,5 @@
> +
> +       let lastAccessDate = new Date(stat.st_atime * 1000);
> +       let lastModificationDate = new Date(stat.st_mtime * 1000);
> +       let unixLastStatusChangeDate = new Date(stat.st_ctime * 1000);
> +       

Nit: Whitespace

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +660,5 @@
>        */
>       File.Info = function Info(stat) {
> +       let isDir = !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_DIRECTORY);
> +       let isSymLink = !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_REPARSE_POINT);
> +       

Nit: Whitespace.

Comment 9

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

(In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> Comment on attachment 690107 [details] [diff] [review]
> First patch
> 
> Review of attachment 690107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, with the few changes detailed below. Let's test it.
> 
> ::: toolkit/components/osfile/osfile_async_front.jsm
> @@ +688,5 @@
> > +  File.Info.prototype = Object.create(OS.Shared.Win.AbstractInfo.prototype);
> > +} else if (OS.Constants.libc) {
> > +  File.Info.prototype = Object.create(OS.Shared.Unix.AbstractInfo.prototype);
> > +} else {
> > +  throw new Error("I am neither under Windows nor under a Posix system");
> 
> In practice |OS.Constants.libc| is always defined, since libc is available
> on all systems, so the second |if| doesn't add any information.
> 

Let me guess. That's copied and pasted from my code, isn't it? In that case, forget about my comment on these lines :)

Comment 10

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=7a3a8e440829

By the way, please don't forget to add « ;r=yoric » at the end of your commit message.

Comment 11

[Eduard Neculaesi (:eduardn)]

> (In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> > Comment on attachment 690107 [details] [diff] [review]
> > First patch
> > 
> > Review of attachment 690107 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good, with the few changes detailed below. Let's test it.
> > 
> > ::: toolkit/components/osfile/osfile_async_front.jsm
> > @@ +688,5 @@
> > > +  File.Info.prototype = Object.create(OS.Shared.Win.AbstractInfo.prototype);
> > > +} else if (OS.Constants.libc) {
> > > +  File.Info.prototype = Object.create(OS.Shared.Unix.AbstractInfo.prototype);
> > > +} else {
> > > +  throw new Error("I am neither under Windows nor under a Posix system");
> > 
> > In practice |OS.Constants.libc| is always defined, since libc is available
> > on all systems, so the second |if| doesn't add any information.
> > 
> 
> Let me guess. That's copied and pasted from my code, isn't it? In that case,
> forget about my comment on these lines :)

Ummm...yes :)
(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=7a3a8e440829
> 
> By the way, please don't forget to add « ;r=yoric » at the end of your
> commit message.
Ok.

I will do those modifications after I investigate the problem on Unix, as the tests seem to fail. Any idea?

Comment 12

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

Let me relaunch the tests in a more verbose mode: https://tbpl.mozilla.org/?tree=Try&rev=ba4d21a9a5e7

Comment 13

[Eduard Neculaesi (:eduardn)]

Attachment: Second patch

Found the error! I was passing an undefined |creationDate| to the |AbstractInfo| constructor which, by the way, I wasn't even using. Is it ok to keep the implementation of |creationDate| and |macBirthDate| on the |File.Info| side?

Comment 14

[Eduard Neculaesi (:eduardn)]

Attachment: bug-802534-fix

Resolved all errors. Everything is green on Linux: https://tbpl.mozilla.org/?tree=Try&rev=1fd79cba8213

Comment 15

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

Comment on attachment 692768 [details] [diff] [review]
bug-802534-fix

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

Looks good, with the following minor changes.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +707,5 @@
> +
> +       let lastAccessDate = new Date(stat.st_atime * 1000);
> +       let lastModificationDate = new Date(stat.st_mtime * 1000);
> +       let unixLastStatusChangeDate = new Date(stat.st_ctime * 1000);
> +       

Nit: Please remove that tab.

@@ +716,3 @@
>         // Some platforms (e.g. MacOS X, some BSDs) store a file creation date
>         if ("OSFILE_OFFSETOF_STAT_ST_BIRTHTIME" in OS.Constants.libc) {
>           this._st_birthtime = stat.st_birthtime;

You should rather define |this.macBirthDate = new Date(...)| here.

Comment 16

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

Any news? We'll need this patch soon.

Comment 17

[Eduard Neculaesi (:eduardn)]

Attachment: bug-802534-fix

Comment 18

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

Comment on attachment 699900 [details] [diff] [review]
bug-802534-fix

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

r=me with the trivial change below, assuming that it passes tests.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +725,5 @@
> +         * information.
> +         *
> +         * @type {Date}
> +         */
> +         Object.defineProperty(this, "macBirthDate", { value: date });

You just need |this.macBirthDate = date|.
Also, I would move this after the call to |AbstractInfo|.

Comment 19

[Eduard Neculaesi (:eduardn)]

Attachment: bug-802534-fix

Did the changes and pushed to try. Once everything is green it should be ready to land.

Try: https://tbpl.mozilla.org/?tree=Try&rev=111d9bbc22cc

Comment 20

[Daniel Holbert [:dholbert]]

Try run looks good, so I pushed to inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/fdfa7133f207

(The latest patch here had an old datestamp encoded in its headers (from mid-December), so I updated that before pushing w/ 'hg qref -D', for sanity's sake.)

Comment 21

[Ed Morley [:emorley]]

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

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

[Ed Morley [:emorley]]

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