Implement the JS File API backend. A few considerations: - Linux/Android and BSD/MacOS differ on a number of subtle-yet-interesting points (e.g. Linux/Android has a number of `openat`-style system calls optimized for opened directories, BSD/MacOS has optimized tree walking, additional `fstat` properties and `open` flags, etc.); - this back-end needs to handle Unix-style permissions; - Unix-style file properties is rather simple, as `fstat` extracts plenty of meaningful information in one operation; - in some contexts, information may be obtained through less-expensive operations, e.g. determining whether a file is a directory while walking a directory.

Still very much a work in progress. Since previous preview: - introduce a "native path" type, as suggested – as a typedef, though, I believe people would lynch me if I intend to add a new hierarchy of strings; - removed a few functions/methods that had become useless by the way of this change; - implemented a public |fstat| API; - removed methods for reading a complete file, as expected; - renamed |Rmdir| => |Remove|, |Rmcontents| => |Clear|; - collapsed the variants of |Move|. Mike, do you think that this |Resolve|/|Unresolve| API is meaningful? Or should we assume that a directory is always |Resolved| and should therefore be closed as early as possible?

Lets get moving on landing this(ie switch to r?). Is it feasible to land something before we branch on Dec 20?

(In reply to Taras Glek (:taras) from comment #2) > Lets get moving on landing this(ie switch to r?). Is it feasible to land > something before we branch on Dec 20? I can prioritize this, but the Windows version is lagging.

(In reply to David Rajchenbach Teller [:Yoric] from comment #3) > (In reply to Taras Glek (:taras) from comment #2) > > Lets get moving on landing this(ie switch to r?). Is it feasible to land > > something before we branch on Dec 20? > > I can prioritize this, but the Windows version is lagging. please do

I attach a version of the Unix back-end. The architecture of RawFile should not change much. I still need to implement the following features: - changing attributes (chown, chmod, chgrp); - truncate; - dup (should be useful for clean multi-threaded operations); - in the future, play nicely with AIO. By opposition, RawDirectory is still at the stage of untested, early work in progress. The architecture of RawDirectory remains a big question. I am very interested in feedback about |Resolve|/|Unresolve|. The main reason for including these functions are for users who could be tempted to cache many directories for later use – something that we do with |nsIFile| atm. However, at JS-level, we have a data structure to represent paths. I tend to believe that we should recommend that users should never hold onto a RawDirectory and simply |Resolve| all directories on platforms for which it matters. To discourage this, at JS-level, it would be quite easy to display warnings if a directory (or a file) remains open for more than, say, 30 minutes. Finally, we are missing: - creation of temporary files; - |copy|, |touch| a file in a RawDirectory; - |move|, |copy|, |rm|, |touch| an arbitrary file in the fs; - getting the contents of a directory (with platform-specific info).

Comment on attachment 581986 [details] [diff] [review] Prev 3. Unix back-end. Break this up atleast into 3 patches: c/c++ bits basic js bits more js bitss

Here's the same patch, minus the first few lines, which had been included by error. Sorry about that.

Comment on attachment 582523 [details] [diff] [review] Prev 4. Unix back-end. This seems to be doing a lot for a thin layer. I think we should be passing all of the flags from JS. Forcing flags to be defined in C++ makes it impossible for addons to utilize a flag that we didn't provide.

Comment on attachment 581988 [details] [diff] [review] Prev 3. Back-end common files. -ing since i -sed part4. This is not minimal enough.

(In reply to Taras Glek (:taras) from comment #9) > Comment on attachment 582523 [details] [diff] [review] > Prev 4. Unix back-end. > > This seems to be doing a lot for a thin layer. I am not really sure how I could do less while still giving access to OS-accelerated features. > I think we should be passing > all of the flags from JS. Forcing flags to be defined in C++ makes it > impossible for addons to utilize a flag that we didn't provide. Ok, I will refactor |OpenFlags| to allow that. (In reply to Taras Glek (:taras) from comment #10) > Comment on attachment 581988 [details] [diff] [review] > Prev 3. Back-end common files. > > -ing since i -sed part4. This is not minimal enough. Just to be sure I understand: you want me to split the patch, is that it?

(In reply to David Rajchenbach Teller [:Yoric] from comment #11) > (In reply to Taras Glek (:taras) from comment #9) > > Comment on attachment 582523 [details] [diff] [review] > > Prev 4. Unix back-end. > > > > This seems to be doing a lot for a thin layer. > > I am not really sure how I could do less while still giving access to > OS-accelerated features. I think the api shouldn't provide much more than a syscall wrapper. Then the js side should figure out which syscall to use and what parameters to pass. > > > I think we should be passing > > all of the flags from JS. Forcing flags to be defined in C++ makes it > > impossible for addons to utilize a flag that we didn't provide. > > Ok, I will refactor |OpenFlags| to allow that. > > (In reply to Taras Glek (:taras) from comment #10) > > Comment on attachment 581988 [details] [diff] [review] > > Prev 3. Back-end common files. > > > > -ing since i -sed part4. This is not minimal enough. > > Just to be sure I understand: you want me to split the patch, is that it? yes. I like patches split up to be <15KB each(when reasonable).

(In reply to Taras Glek (:taras) from comment #12) > (In reply to David Rajchenbach Teller [:Yoric] from comment #11) > > (In reply to Taras Glek (:taras) from comment #9) > > > Comment on attachment 582523 [details] [diff] [review] > > > Prev 4. Unix back-end. > > > > > > This seems to be doing a lot for a thin layer. > > > > I am not really sure how I could do less while still giving access to > > OS-accelerated features. > > I think the api shouldn't provide much more than a syscall wrapper. Then the > js side should figure out which syscall to use and what parameters to pass. We could put all the logics into JS. I am not quite clear what we would gain, but I see a number of potential issues, which have made me choose this slightly thicker back-end: - the resulting API is usable only by JS, not by C++; - we lose performance, both in the implementation itself, and at boundaries (e.g. many Windows data structures are 64 bits or 128 bits, so we have to represent them as arrays in JS); - we make it much easier to have resource leaks - we lose some useful compile-time checking. If you strongly believe that I should go for the syscall wrapper, I will go that way, but I believe that the current approach is better. > yes. I like patches split up to be <15KB each(when reasonable). Ok, will do.

Just to be clear: I strongly believe that we should go for the syscall wrapper approach.

Sure. Let me just finish cleaning up my patch queue and I'll start this new strategy.

Attaching a first draft. This module exports a global object |OS.Constants.libc|, with many (all?) of the important libc constants. I envision that this module can be later extended with additional constants as needed.

Attaching a fully revamped version of the Unix back-end. As discussed with Taras, this back-end is synchronous for the moment, and designed to be used only from a chrome worker thread. Once we have reached the stage at which this synchronous back-end works to satisfaction, we will build the asynchronous back-end on top of it. And only once the asynchronous back-end works will we start working first on the Windows back-end, then on the actual user-facing API.

Comment on attachment 609850 [details] [diff] [review] Unix synchronous back-end Nice. I just can't f- any js-ctypes patch!! Wouldn't it be easier to use `new Type` instead of this 3th levels indirection? declareType > aDeclareType > gDeclareType > new Type() Especially when, at the end, we do want a Type instance at the end. (It would simplify code comprehension and allow removing init arguments that aren't used) Similar comment apply to declareFFI, we might call gDeclareFFI directly. Otherwise it looks really nice, js-ctypes burden doesn't look that bad in this library! We are trying to provide some js-ctypes helpers in order to help addon developers start using it. And I think that your experience around js-ctype is really valuable. We may just take your helper methods and offer them in a ctype helper jetpack module! What's the plan next? Would you build some cross platform library on top of that? I'm saying that because Jetpack FS API is cross platform, so that we would have to to this job, if you do not plan to do it. Finally, what about CPU/memory performances? Especially compared to current xpcom components. I can't wait to get other OSes and asynchronous support :)

(In reply to Alexandre Poirot (:ochameau) from comment #19) > Comment on attachment 609850 [details] [diff] [review] > Unix synchronous back-end > > Nice. I just can't f- any js-ctypes patch!! > > Wouldn't it be easier to use `new Type` instead of this 3th levels > indirection? > declareType > aDeclareType > gDeclareType > new Type() > Especially when, at the end, we do want a Type instance at the end. > (It would simplify code comprehension and allow removing init arguments that > aren't used) For Type, you are most likely right. > Similar comment apply to declareFFI, we might call gDeclareFFI directly. For declareFFI, it is a little more complicated. But I'm cheating, because I already have the implementation of the next bug at hand, and this argument serves for the threaded back-end. The main thread uses its own |declareFFI| to declare a function that checks and serializes arguments, deserializes the result and uses a Promise to synchronize, while the worker thread uses its own |declareFFI| to declare a function that deserializes arguments, performs the ffi call, serializes the result and posts it back to the main thread. > Otherwise it looks really nice, js-ctypes burden doesn't look that bad in > this library! > We are trying to provide some js-ctypes helpers in order to help addon > developers start using it. And I think that your experience around js-ctype > is really valuable. We may just take your helper methods and offer them in a > ctype helper jetpack module! This would be with pleasure. Do you want to open a bug on extracting the relevant part and making them usable by Jetpack? > What's the plan next? Would you build some cross platform library on top of > that? I'm saying that because Jetpack FS API is cross platform, so that we > would have to to this job, if you do not plan to do it. Definitely planned. I used to have one, but it was scrapped in favor of this design, which is expected to be more hackable. > Finally, what about CPU/memory performances? Especially compared to current > xpcom components. Untested yet. I suspect CPU/memory will be somewhat worse due to all conversions, but there should be less system calls, which we consider more important atm. > I can't wait to get other OSes and asynchronous support :) Thanks :)

Comment on attachment 609850 [details] [diff] [review] Unix synchronous back-end Review of attachment 609850 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/osfile_unix.jsm @@ +536,5 @@ > + /*dest*/ Types.string, > + /*flag*/ Types.int); > + > + UnixFile.lockf = > + declareFFI("lockf", ctypes.default_abi, I'm not convinced it's a good idea to expose file locking functions. @@ +555,5 @@ > + /*return*/ Types.string, > + /*template*/Types.string); > + > + UnixFile.mkstmp = > + declareFFI("mkstmp", ctypes.default_abi, didn't you mean mkstemp ?

(In reply to David Rajchenbach Teller [:Yoric] from comment #20) > Untested yet. I suspect CPU/memory will be somewhat worse due to all > conversions, but there should be less system calls, which we consider more > important atm. > I expect this to be faster because we do a lot of conversions in xpconnect.

(In reply to Taras Glek (:taras) from comment #22) > (In reply to David Rajchenbach Teller [:Yoric] from comment #20) > > Untested yet. I suspect CPU/memory will be somewhat worse due to all > > conversions, but there should be less system calls, which we consider more > > important atm. > > > > I expect this to be faster because we do a lot of conversions in xpconnect. There is only one way to settle this: Benchmark Kombat!

Comment on attachment 609850 [details] [diff] [review] Unix synchronous back-end Please uses spaces around your operators. I'm not sure what the benefit of wrapping FILE* functions is. I think wrapping basic fd-functions is sufficient for a low level api.

(In reply to David Rajchenbach Teller [:Yoric] from comment #23) > (In reply to Taras Glek (:taras) from comment #22) > > (In reply to David Rajchenbach Teller [:Yoric] from comment #20) > > > Untested yet. I suspect CPU/memory will be somewhat worse due to all > > > conversions, but there should be less system calls, which we consider more > > > important atm. > > > > > > > I expect this to be faster because we do a lot of conversions in xpconnect. > > There is only one way to settle this: Benchmark Kombat! It's true. Another thing to keep in mind that JIT guys have a hope in hell in optimizing ctypes code at runtime, not such hopes for layering-heavy xpconnect :)

(In reply to Taras Glek (:taras) from comment #24) > Comment on attachment 609850 [details] [diff] [review] > Unix synchronous back-end > > Please uses spaces around your operators. > > I'm not sure what the benefit of wrapping FILE* functions is. I think > wrapping basic fd-functions is sufficient for a low level api. I should clarify this. I think we should add functions as we need them instead of aggressively mirroring libc from getgo. The big benefit of an extensible ctypes-based api like this is that if we miss an api that a user might need, they can add it themselves.

Ok, I have removed some functions that are probably not going to be useful in the near future, fixed a few trivial issues, added a little documentation, removed |gDeclareType| et al and resolved the scope of |export| (thanks, ochameau).

Extended the test suite, reworked it a little so that it has chances of working with Android. Additional tests: access, read, write, unlink.

Comment on attachment 612541 [details] [diff] [review] Unix synchronous back-end r+, but please get rid of *at functions until we need them. A real tookit reviewer needs to review this before this lands.

Comment on attachment 612560 [details] [diff] [review] Companion testsuite looks good

Comment on attachment 612546 [details] [diff] [review] Companion makefile I don't review makefiles unless I wrote them

Ok, I have removed *at, as requested by Taras, and I have taken the opportunity to divide the patch in two. First part of the patch contains the Unix specific bits, while the second part of the patch contains bits that are shared between Unix and Windows implem.

Comment on attachment 612832 [details] [diff] [review] Unix synchronous back-end + let libc_candidates = ["libSystem.dylib", + "libc.so.6", + "libc.so"]; + for (let i = 0; i < libc_candidates.length; ++i) { use for each I think it's better to use todo instead of "note".

Comment on attachment 612837 [details] [diff] [review] Non-Unix specific functions of the back-end this seems ok. Nit: + let key = ":"+type.name; put spaces around your operators, elsewhere too.

Comment on attachment 612837 [details] [diff] [review] Non-Unix specific functions of the back-end Review of attachment 612837 [details] [diff] [review]: ----------------------------------------------------------------- fixed up the ws on checkin. r+ if you have answers for my questions. ::: toolkit/components/osfile/osfile_shared.jsm @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +{ > + if (typeof Components != "undefined") { Is this really the right way to test that we are running this jsm on the right thread? @@ +11,5 @@ > + > + throw new Error("osfile_shared.jsm cannot be used from the main thread yet"); > + } > + > + (function(exports) { Why not do something like: if (exports.OS) return; exports.OS = {}; exports.OS.Shared = {} ... @@ +64,5 @@ > + this.name = name; > + this.implementation = implementation; > + if (convert_from_c) { > + this.convert_from_c = convert_from_c; > + } else {// Optimization: Ensure harmony of shapes what is harmony of shapes? @@ +65,5 @@ > + this.implementation = implementation; > + if (convert_from_c) { > + this.convert_from_c = convert_from_c; > + } else {// Optimization: Ensure harmony of shapes > + this.convert_from_c = Type.prototype.convert_from_c; this.convert_from_c = convert_from_c || Type.prototype.convert_from_c; ? @@ +91,5 @@ > + */ > + Types.int = > + new Type("int", > + ctypes.int); > + Types.int32_t = missing white space after the "int" type @@ +110,5 @@ > + ctypes.int); > + Types.off_t = > + new Type("off_t", > + ctypes.int); > + Types.size_t = Missing white space after the off_t type @@ +113,5 @@ > + ctypes.int); > + Types.size_t = > + new Type("size_t", > + ctypes.size_t); > + Types.ssize_t = same @@ +116,5 @@ > + ctypes.size_t); > + Types.ssize_t = > + new Type("ssize_t", > + ctypes.ssize_t); > + Types.DWORD = same @@ +185,5 @@ > + */// Note: Future versions will use a different implementation of this > + // function on the main thread, osfile worker thread and regular worker > + // thread > + let declareFFI = function declareFFI(lib, symbol, abi, > + returnType /*, argTypes ...*/) { another ws nit. line up returnType with lib @@ +190,5 @@ > + LOG("Attempting to declare FFI ", symbol); > + // We guard agressively, to avoid any late surprise > + if (typeof symbol != "string") { > + if (symbol instanceof String) { > + symbol = symbol.toString(); is there ever a |symbol| that is a typeof string, but isn't a instance of String?

(In reply to Doug Turner (:dougt) from comment #38) > Comment on attachment 612837 [details] [diff] [review] > Non-Unix specific functions of the back-end > > Review of attachment 612837 [details] [diff] [review]: > ----------------------------------------------------------------- > > fixed up the ws on checkin. r+ if you have answers for my questions. > > ::: toolkit/components/osfile/osfile_shared.jsm > @@ +2,5 @@ > > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > + > > +{ > > + if (typeof Components != "undefined") { > > Is this really the right way to test that we are running this jsm on the > right thread? This is the only technique I know of, but I can investigate further with the JS team. > > + (function(exports) { > > Why not do something like: > > if (exports.OS) > return; > > exports.OS = {}; > exports.OS.Shared = {} Not 100% sure I understand your question. I am actually building |OS.| in several layers, so I need some more careful checking. > what is harmony of shapes? I want to ensure that the VM representation of the object always has the exact same Shape (as in "these fields are defined in the immediate object, not in a prototype"). I have not benchmarked this, so this may well be premature optimization. > > @@ +65,5 @@ > > + this.implementation = implementation; > > + if (convert_from_c) { > > + this.convert_from_c = convert_from_c; > > + } else {// Optimization: Ensure harmony of shapes > > + this.convert_from_c = Type.prototype.convert_from_c; > > this.convert_from_c = convert_from_c || Type.prototype.convert_from_c; ? ok. > > @@ +91,5 @@ > > + */ > > + Types.int = > > + new Type("int", > > + ctypes.int); > > + Types.int32_t = > > missing white space after the "int" type Not sure I understand. Do you want a new line? > > + */// Note: Future versions will use a different implementation of this > > + // function on the main thread, osfile worker thread and regular worker > > + // thread > > + let declareFFI = function declareFFI(lib, symbol, abi, > > + returnType /*, argTypes ...*/) { > > another ws nit. line up returnType with lib ok > > @@ +190,5 @@ > > + LOG("Attempting to declare FFI ", symbol); > > + // We guard agressively, to avoid any late surprise > > + if (typeof symbol != "string") { > > + if (symbol instanceof String) { > > + symbol = symbol.toString(); > > is there ever a |symbol| that is a typeof string, but isn't a instance of > String? I had the case in a previous version of this code, but I probably do not need that check anymore. Do you want me to remove it?

> Not sure I understand. Do you want a new line? Yes, basically if you look at all of those assignments, most have one line of white space between them. Just a nit for us to be consistent. > Do you want me to remove it? Yes, or add a comment as to why this case could exist. Otherwise great stuff!

Thanks. By the way, I have had confirmation from Jorendorff (and #developers silence): detecting the presence of |Components| seems to be the state of the art for detecting whether JS code is executed in the main thread or in a worker.

(In reply to Taras Glek (:taras) from comment #36) > Comment on attachment 612832 [details] [diff] [review] > Unix synchronous back-end > > + let libc_candidates = ["libSystem.dylib", > + "libc.so.6", > + "libc.so"]; > + for (let i = 0; i < libc_candidates.length; ++i) { > > use for each In another bug, Gavin recommended not using for each on arrays, as this can be fooled if the array has additional properties. While this is quite unlikely in a JS module, chrome workers make no such guarantee, so I would rather not take the chance. > I think it's better to use todo instead of "note". Well, the note is actually more an explanation of why things are designed like this, rather than a TODO.

A few minor updates to the various patches, reflecting current work on the front-end. I promise I will now stop updating anything other than bugs :)

Comment on attachment 618690 [details] [diff] [review] Unix synchronous back-end + UnixFile.close = function(fd) { + fd.dispose(); + if (ctypes.errno) { + throw new OSUnix.Error(ctypes.errno, "close"); + } + }; + same as in the windows bug, please do not modify semantics of low level functions, instead directly call close(fd) and bypass the finalizer. (Sorry if I missed this before)

(In reply to Taras Glek (:taras) from comment #47) > Comment on attachment 618690 [details] [diff] [review] > Unix synchronous back-end > > + UnixFile.close = function(fd) { > + fd.dispose(); > + if (ctypes.errno) { > + throw new OSUnix.Error(ctypes.errno, "close"); > + } > + }; > + > > same as in the windows bug, please do not modify semantics of low level > functions, instead directly call close(fd) and bypass the finalizer. (Sorry > if I missed this before) Ok, I will remove all the exceptions everywhere. Can I at least return an error data structure instead of -1 / 0 / NULL / INVALID_HANDLE / ...?

Same code, without exceptions and with simpler |close|.

Propagated the changes to the test suite.

Same patch with a little more polish. Added a hack to ensure that we can check whether |open| returns -1, removed redundant |_strerror| and |OS.Unix.Error|.

Comment on attachment 619056 [details] [diff] [review] Unix synchronous back-end + UnixFile.close = function close(fd) { + return fd.dispose(); + }; I think should should say return _close(fd.forget()).

(In reply to David Rajchenbach Teller [:Yoric] from comment #48) > Ok, I will remove all the exceptions everywhere. Can I at least return an > error data structure instead of -1 / 0 / NULL / INVALID_HANDLE / ...? I think wrappers should be wrappers, we can introduce diversions from the platform at higher levels.

Comment on attachment 618986 [details] [diff] [review] Non-Unix specific functions of the back-end I think that's ok.

(In reply to Taras Glek (:taras) from comment #53) > Comment on attachment 619056 [details] [diff] [review] > Unix synchronous back-end > > + UnixFile.close = function close(fd) { > + return fd.dispose(); > + }; > > I think should should say > > return _close(fd.forget()). Well, both have the exact same semantics, but I would prefer keeping the |dispose| version, if you do not mind, as it is faster, shorter and (I believe) simpler.

Minor fix wrt the return of |OS.File.Unix.pipe| in case of error. Also, see my comment above regarding |dispose|.

Comment on attachment 619541 [details] [diff] [review] Unix synchronous back-end + + UnixFile.pipe = function pipe(array) { + if (_pipe(_pipebuf) == -1) { + return -1; + } + array[0] = ctypes.CDataFinalizer(_pipebuf[0], _close); + array[1] = ctypes.CDataFinalizer(_pipebuf[1], _close); + return 0; + }; nit: I would prefer to do ret = _pipe...; if (ret == -1) return ret; ..... return ret. You can keep your dispose call, but please comment that you are relying on the finalizer calling close and dispose returning proper success/error code

Fixed a discrepancy between Linux and BSD (behavior of |getwd| is slightly different).

Fixed an issue with 32-bits vs. 64-bits.

Fixed a bug that caused off_t to have the wrong size on some configurations.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bb11da7be6 https://hg.mozilla.org/integration/mozilla-inbound/rev/378acab7d10c https://hg.mozilla.org/integration/mozilla-inbound/rev/87174904a926 https://hg.mozilla.org/integration/mozilla-inbound/rev/43b7500f5954 Sorry for the delay...

https://hg.mozilla.org/mozilla-central/rev/f7bb11da7be6 https://hg.mozilla.org/mozilla-central/rev/378acab7d10c https://hg.mozilla.org/mozilla-central/rev/87174904a926 https://hg.mozilla.org/mozilla-central/rev/43b7500f5954