Closed Bug 1141021 Opened 10 years ago Closed 8 months ago

New API for low level filesystem access

Categories

(Core :: DOM: Core & HTML, defect, P5)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: janjongboom, Unassigned)

References

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 14 obsolete files)

bug1141021-add-raii.diff
2.04 KB, patch
baku
: review+
Details | Diff | Splinter Review
bug1141021_v6.patch
117.52 KB, patch
Details | Diff | Splinter Review
bug1141021-b2g.patch
6.65 KB, patch
Details | Diff | Splinter Review
Mailing list discussion in mozilla.dev.webapi: https://groups.google.com/forum/#!topic/mozilla.dev.webapi/t4Xh2EH0rIo We're needing a new API that allows us to perform lower level filesystem operations to make it easier to write applications that run on embedded systems and dev boards like the Raspberry Pi. One use case is interacting with the GPIO ports on the Rpi. The API should follow the standard C filesystem APIs pretty close, and act as a small wrapper around libc's fopen/fwrite/fread/etc. This means that the API will be blocking, and thus only runnable in a worker. Some more caveats: - Certified only - Paths that you need access to need to be listed in the app manifest - Do a realpath() before any call, and check against the path list. Should add sufficient security. - Close to the metal, model it after C-API. So fclose/fopen/fread/etc. - I think we also need epoll(), for interrupts - I think we also need mmap/msync/munmap From :sicking : If we use syncronous blocking APIs for reading/writing/waiting, which is the case if you want them to map closely to their posix equivalents, then we likely only want to expose them to workers. If we have this API, we can also add a new FS backend in emscripten and cross-compile LLVM code that deals with filesystem to Gecko/FxOS.
realpath() can be tricky. make sure the output buffer is allocated large enough to avoid buffer overruns in all cases, see pathconf(). having a promise interface for I/O events would be nice.
FYI, my working branch is https://github.com/jan-os/gecko-dev/tree/os-module-worker in case people want to keep track.
Attached patch Creating new Worker API (obsolete) — — Splinter Review
:baku, according to :ehsan you were willing to give some feedback on my initial work. Never wrote a Gecko API from scratch in C++.
Attachment #8583964 - Flags: feedback?(amarchesini)
I guess this doesn't work from sandboxed child processes?
It was not my original scope no. I think it's an API meant for embedded devices that run as root anyway (system app like processes). I think that at one point we might want to open up this API more, but f.e. only to SD card. When working on the gallery app proper filesystem APIs really hurt, as we could not rotate a photo and then just overwrite the original file. Only way was to make a new file, and delete the old one, but then you couldn't change the creation date and thus the photo was no longer in the right position in the gallery photo stream. The bug to fix this on the deviceStorage API side has been open for nearly 3 years: https://bugzilla.mozilla.org/show_bug.cgi?id=752724. If we want that I think it should be a follow up, but it could affect design of the API.
Flags: needinfo?(fabrice)
Hi Jan, can you submit the full patch? Or use reviewboard. Otherwise the comments I do are not visible here in bugzilla. Thanks.
Flags: needinfo?(janjongboom)
Attachment #8583964 - Flags: feedback?(amarchesini)
Attached patch 0001-Add-stat-lstat.patch (obsolete) — — Splinter Review
Hey Andrea, I've created patches from Jan's commit and attached them.
Flags: needinfo?(amarchesini)
Comment on attachment 8584525 [details] [diff] [review] 0001-Implement-fopen-fclose.patch Review of attachment 8584525 [details] [diff] [review]: ----------------------------------------------------------------- I give you f- just for this OSFile. Tell me what you think about it. ::: dom/os/OsManager.cpp @@ +33,4 @@ > return OsManagerBinding::Wrap(aCx, this, aGivenProto); > } > > +// ToNewCString ?? ? @@ +35,5 @@ > > +// ToNewCString ?? > + > +void > +OsManager::Fopen(const nsAString& path, const nsAString& mode, DOMString& result) aPath, aMode, aResult @@ +43,5 @@ > + > + FILE* file = fopen(real_path, NS_LossyConvertUTF16toASCII(mode).get()); > + free(real_path); > + if (file == NULL) { > + result.SetNull(); Would be nice to have some error result here. @@ +50,5 @@ > + > + char ptrStringBuffer[sizeof(size_t)]; > + sprintf(ptrStringBuffer, "%p", file); > + > + this->valid_file_pointers.push_back((size_t)file); no this-> @@ +51,5 @@ > + char ptrStringBuffer[sizeof(size_t)]; > + sprintf(ptrStringBuffer, "%p", file); > + > + this->valid_file_pointers.push_back((size_t)file); > + printf("fopen string is %s\n", ptrStringBuffer); in the final patch we should not have this printf. @@ +53,5 @@ > + > + this->valid_file_pointers.push_back((size_t)file); > + printf("fopen string is %s\n", ptrStringBuffer); > + > + result.SetOwnedString(NS_ConvertASCIItoUTF16(ptrStringBuffer)); I don't like to have the pointer stored into a string. What about if you have an object? I'm thinking about something like: interface OSFile {} interface OSManager { OSFiler? fopen(USVString aPath, USVString aMode); void fclose(OSFile aFile); } OSFile is an interface without any attribute/method but you can use it to store the pointer. @@ +60,5 @@ > +int > +OsManager::Fclose(const nsAString& ptr) > +{ > + const char* ptrstring = NS_LossyConvertUTF16toASCII(ptr).get(); > + extra spaces @@ +62,5 @@ > +{ > + const char* ptrstring = NS_LossyConvertUTF16toASCII(ptr).get(); > + > + printf("fclose comes in %s\n", ptrstring); > + size_t realptr = (size_t)strtoull(ptrstring, const_cast<char**>(&ptrstring), 16); If you do this OSFile you don't need to do all this valid_file_pointers. @@ +69,5 @@ > + std::list<size_t> ptrs = this->valid_file_pointers; > + > + // detect if ptr is valid > + if (std::find(ptrs.begin(), ptrs.end(), realptr) == ptrs.end()) { > + printf("Could not find the pointer\n"); We should have a better error handling here. ::: dom/os/OsManager.h @@ +32,4 @@ > * WebIDL Interface > */ > virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > + extra spaces @@ +33,5 @@ > */ > virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > + > + std::list<size_t> valid_file_pointers; > + extra spaces @@ +35,5 @@ > + > + std::list<size_t> valid_file_pointers; > + > + // file operations > + void Fopen(const nsAString& path, const nsAString& mode, DOMString& result); aPath, aMode, aResult @@ +36,5 @@ > + std::list<size_t> valid_file_pointers; > + > + // file operations > + void Fopen(const nsAString& path, const nsAString& mode, DOMString& result); > + int Fclose(const nsAString& ptr); 1. aPtr 2. no extra spaces between 'int' and 'Fclose' ::: dom/webidl/OsManager.webidl @@ +1,3 @@ > [Exposed=(Worker,System)] > interface OsManager { > + // hmm, is long enough? JS number is not big enough I think... what are you talking about? :) @@ +1,4 @@ > [Exposed=(Worker,System)] > interface OsManager { > + // hmm, is long enough? JS number is not big enough I think... > + extra space.
Attachment #8584525 - Flags: feedback-
Comment on attachment 8584528 [details] [diff] [review] 0001-Add-an-API-that-is-accessible-from-a-worker.patch Review of attachment 8584528 [details] [diff] [review]: ----------------------------------------------------------------- You need more stuff to decide where this new API has to be exposed. ::: dom/bindings/Bindings.conf @@ +840,5 @@ > }, > > +'OsManager': { > + 'nativeType': 'mozilla::dom::os::OsManager', > + 'headerFile': 'OsManager.h' mozilla/dom/os/OsManager.h ::: dom/os/OsManager.cpp @@ +33,5 @@ > + > +long > +OsManager::Hello() > +{ > + return 41; 42 ::: dom/os/OsManager.h @@ +6,5 @@ > +#ifndef mozilla_dom_os_OsManager_h > +#define mozilla_dom_os_OsManager_h > + > +#include "mozilla/DOMEventTargetHelper.h" > +#include "../workers/WorkerFeature.h" #inlcude "WorkerFeature.h" and so on. and add LOCAL_INCLUDES /dom/workers in moz.build @@ +18,5 @@ > +namespace mozilla { > +namespace dom { > +namespace os { > + > +class OsManager : public DOMEventTargetHelper final ? ::: dom/os/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# 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/. > + > +EXPORTS.mozilla.dom += [ mozilla.dom.os ::: dom/webidl/OsManager.webidl @@ +1,1 @@ > +[Exposed=(Worker,System)] add an header ::: dom/webidl/WorkerNavigator.webidl @@ +11,4 @@ > WorkerNavigator implements NavigatorLanguage; > WorkerNavigator implements NavigatorOnLine; > WorkerNavigator implements NavigatorDataStore; > +partial interface WorkerNavigator { I guess we want to protect this interface with: 1. permissions? 2. preferences? 3. something else: only main-process? ::: dom/workers/Navigator.cpp @@ +10,4 @@ > #include "mozilla/dom/Promise.h" > #include "mozilla/dom/PromiseWorkerProxy.h" > #include "mozilla/dom/WorkerNavigatorBinding.h" > +#include "mozilla/dom/OsManager.h" alphabetic order in the include headers (here and everywhere else) @@ +401,5 @@ > +bool mOsManagerInitialized = false; > + > +os::OsManager* > +WorkerNavigator::GetOs(ErrorResult& aRv) > +{ Why do you need mOsManagerIntialized? Can you do: if (!mOsManager) { mOsManager = new .. mOsManager->Init(); } return mOsManager; ::: dom/workers/Navigator.h @@ +32,4 @@ > > NavigatorProperties mProperties; > bool mOnline; > + os::OsManager* mOsManager; nsRefPtr<os::OsManager>
Attachment #8584528 - Flags: feedback+
Comment on attachment 8584527 [details] [diff] [review] 0001-Add-stat-lstat.patch Review of attachment 8584527 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +845,5 @@ > }, > > +'OsManagerStat': { > + 'nativeType': 'mozilla::dom::os::Stat', > + 'headerFile': 'Stat.h' mozilla/dom/os/Stat.h ::: dom/os/OsManager.cpp @@ +23,5 @@ > > OsManager::OsManager(workers::WorkerGlobalScope* aScope) > : DOMEventTargetHelper(static_cast<DOMEventTargetHelper*>(aScope)) > +{ > + this->mScope = aScope; , mScope(aScope) @@ +95,5 @@ > + return nullptr; > + } > + > + // cant make it into nsCOMPtr for some reason, need to find out why > + return new class Stat(this->mScope, sb); do: nsRefPtr<Stat> stat = new Stat(mScope, sb); return stat.forget(); and change the prototype of this method: already_AddRefs<Stat> OsManager::LStat(const nsAString& aPath, ErrorResult& aRv); ::: dom/os/OsManager.h @@ +43,5 @@ > void Fopen(const nsAString& path, const nsAString& mode, DOMString& result); > int Fclose(const nsAString& ptr); > > + // stat operations > + class Stat* Stat(const nsAString& path, ErrorResult& aRv); already_addRefs<Stat> ::: dom/os/Stat.h @@ +12,5 @@ > +#include <sys/stat.h> > +#include <unistd.h> > +#include "mozilla/dom/OsManagerBinding.h" > +#include "mozilla/dom/Date.h" > +#include "../workers/WorkerScope.h" WorkerScope.h @@ +20,5 @@ > +namespace mozilla { > +namespace dom { > +namespace os { > + > +class Stat : public nsWrapperCache final ? @@ +23,5 @@ > + > +class Stat : public nsWrapperCache > +{ > +public: > + explicit Stat(workers::WorkerGlobalScope* aScope, struct stat aStat) no explicit if more than 1 arg @@ +24,5 @@ > +class Stat : public nsWrapperCache > +{ > +public: > + explicit Stat(workers::WorkerGlobalScope* aScope, struct stat aStat) > + : mScope(aScope) Use OSManager as parent object. @@ +26,5 @@ > +public: > + explicit Stat(workers::WorkerGlobalScope* aScope, struct stat aStat) > + : mScope(aScope) > + { > + this->mStat = aStat; no this-> and actually you can do: : mScope(aScope) , mStat(aStat) @@ +38,5 @@ > + > + // WebIDL interface > + int64_t Dev() const > + { > + return this->mStat.st_dev; remove this-> everywhere @@ +140,5 @@ > + > +private: > + ~Stat() { } > + struct stat mStat; > + nsRefPtr<workers::WorkerGlobalScope> mScope; nsRefPtr<OsManager> mParent; ::: dom/os/test/test_stat.html @@ +20,5 @@ > + return new Promise((res, rej) => { > + var workerCode = ` > + try { > + var stat = navigator.os.` + fn + `("../../../../dom/os/test/test_stat.html"); > + extra spaces here and above
Attachment #8584527 - Flags: feedback+
Comment on attachment 8584526 [details] [diff] [review] 0001-Make-it-not-crash-on-destructor.patch Review of attachment 8584526 [details] [diff] [review]: ----------------------------------------------------------------- This seems an intermediate patch. Can you merge it with the previous ones? Thanks. ::: dom/os/OsManager.cpp @@ +25,5 @@ > : DOMEventTargetHelper(static_cast<DOMEventTargetHelper*>(aScope)) > {} > > +already_AddRefed<OsManager> > +OsManager::Constructor(GlobalObject& aGlobal, ErrorResult& aRv) remove the constructor. This is not needed. ::: dom/os/OsManager.h @@ +41,1 @@ > extra space ::: dom/workers/Navigator.cpp @@ +409,5 @@ > > mOsManagerInitialized = true; > } > > + // cant have local already_AddRefed, don't know how this would work see my previous review. ::: dom/workers/Navigator.h @@ +32,4 @@ > > NavigatorProperties mProperties; > bool mOnline; > + os::OsManager* mOsManager = 0; I already commented this in the previous patch. Do: nsRefPtr<OsManager> mOsManager; no OsManagerIntialized is needed.
This is the first feedback about your patches. Would be nice if you can create a 'stable' webidl interface so that I can understand the full picture. I'm happy to review new patches.
Flags: needinfo?(amarchesini)
Attached patch os-worker.patch (obsolete) — — Splinter Review
Andre: I know you're excited about this, but best not to interfere with on-going review/feedback cycles, unless the author of the patch has not been active on the bug. This shouldn't be reviewed on a patch-by-patch basis. baku: This is a highly work-in-progress thing, and it was supposed to be one coherent thing, not 5 patches. It's not anywhere close to landing, so I'm looking for a high-level review of how this work. Specifically: * Is the way I initialize OsManager from WorkerNavigator correct? * Do I need to free() OsManager on destructor of the WorkerNavigator? * Are Stat instances that I return in OsManager refcounted, and do I need to manage any memory? * We're executing blocking code in OsManager, which runs on a worker, and thus on a seperate thread, but I still see Runnable's implemented in other modules. Are we fine with blocking code here? I also want to add interrupts at some point (although I probably do that in a different thread). The idea is that the worker would be blocked on such code as well. Thanks already for your feedback, especially the stuff regarding just adding a File type instead of throwing pointers to userland; and thanks for the pointers regarding Mozilla code style. FYI: the patch as attached is the full diff against mc as I have it now, it can do lstat/stat, which can be run through mochitest.
Attachment #8583964 - Attachment is obsolete: true
Attachment #8584525 - Attachment is obsolete: true
Attachment #8584526 - Attachment is obsolete: true
Attachment #8584527 - Attachment is obsolete: true
Attachment #8584528 - Attachment is obsolete: true
Flags: needinfo?(janjongboom)
Attachment #8584569 - Flags: feedback?(amarchesini)
Hey Jan, sry for this. I will keep my fingers on the table in future. :)
> * Is the way I initialize OsManager from WorkerNavigator correct? OsManager is refcnted. What I think you should do is: 1. in workers/Navigator.h: nsRefPtr<os::OsManager> mOsManager; 2. in GetOs(): if (!mOsManager) { mOsManager = new ... } return mOsManager; 3. TRAVERSE/UNLINK mOsManager 4. then I think we should expose this API in WorkerScope and not in navigator. > * Do I need to free() OsManager on destructor of the WorkerNavigator? No if it's refcnted. > * Are Stat instances that I return in OsManager refcounted, and do I need to > manage any memory? No. Just set the corrent parent obj (mOsManager). > * We're executing blocking code in OsManager, which runs on a worker, and > thus on a seperate thread, but I still see Runnable's implemented in other > modules. Are we fine with blocking code here? I also want to add interrupts > at some point (although I probably do that in a different thread). The idea > is that the worker would be blocked on such code as well. That's fine. You can block the worker.
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #5) > It was not my original scope no. I think it's an API meant for embedded > devices that run as root anyway (system app like processes). Well, we will have to agree that we disagree. And we even plan to run the system app OOP. > > If we want that I think it should be a follow up, but it could affect design > of the API. Which is why this should not be a follow-up, but part of the initial design. For sure this adds complexity!
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #20) > Well, we will have to agree that we disagree. And we even plan to run the > system app OOP. > > Which is why this should not be a follow-up, but part of the initial design. > For sure this adds complexity! Alright, good point. I'll take it into account :-)
This wouldn't be that hard for sandboxed child processes, would it? Sandboxes can't open any FDs but can operate on FDs passed to them. So only the open/close calls need to communicate with the parent process.
(In reply to Thomas Daede from comment #22) > This wouldn't be that hard for sandboxed child processes, would it? > Sandboxes can't open any FDs but can operate on FDs passed to them. So only > the open/close calls need to communicate with the parent process. Correct. This is already what we do in the openremotefile protocol to open app:// zips.
I give it some thought and have three questions about this: 1. Listing the paths the app needs access to in manifest file would be equal to giving the process access to these paths, right? I mean, you can do open/close so all bets will be off anyway. Wouldn't it be easier to use ACL to give the process user ID access to these paths? Can set the ACL rules when the app is created and remove when destructed / dies. Wouldn't clear out the ACL groups when someone yanks the battery out but do we care? 2. With system app running OOP, does the main process that would handle the open/close calls still run as root? Because we need root for some things, like accessing /dev/mem when dealing with GPIO on Raspberry Pi. 3. Do we have any thread cond_wait like methods when communicating with main process? Because open/close should still be sync. You know a module that uses this pattern?
Flags: needinfo?(fabrice)
Comment on attachment 8584569 [details] [diff] [review] os-worker.patch I think all the comments I did on the previous patches still apply here. Can you fix them and submit a new patch for a feedback/review?
Attachment #8584569 - Flags: feedback?(amarchesini)
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #24) > I give it some thought and have three questions about this: > > 1. Listing the paths the app needs access to in manifest file would be equal > to giving the process access to these paths, right? I mean, you can do > open/close so all bets will be off anyway. Wouldn't it be easier to use ACL > to give the process user ID access to these paths? Can set the ACL rules > when the app is created and remove when destructed / dies. Wouldn't clear > out the ACL groups when someone yanks the battery out but do we care? We could use ACL probably, yes. I would rather grant permissions at first use of the api than when creating the process but that's an implementation detail. > 2. With system app running OOP, does the main process that would handle the > open/close calls still run as root? Because we need root for some things, > like accessing /dev/mem when dealing with GPIO on Raspberry Pi. Gecko (the chrome side) will still run as root. So anything IPCed to the parent will have root access like now. There's also an ongoing effort to not run b2g as root and moving the "need root" actions to another process but that will make things worse for this api. > 3. Do we have any thread cond_wait like methods when communicating with main > process? Because open/close should still be sync. You know a module that > uses this pattern? There is synchronous IPC from child to parent. It's discouraged to use that on the main thread but in a worker that should not be a problem. Look for the "sync" keyword in ipdl files.
Flags: needinfo?(fabrice)
> 2. With system app running OOP, does the main process that would handle the > open/close calls still run as root? Because we need root for some things, > like accessing /dev/mem when dealing with GPIO on Raspberry Pi. Are you talking about /dev/mem for additional features not available over /sys/class/gpio? Or faster access? Generally for RPi usage, you probably wouldn't want /dev/mem directly, but some device that is limited to only the I/O regions. This can be done by a very simple custom kernel module (in fact I'm surprised the RPi doesn't include one already).
> (In reply to Fabrice Desré [:fabrice] from comment #26) Thanks fabrice, helpful. I'll implement next week. (In reply to Thomas Daede from comment #27) > > 2. With system app running OOP, does the main process that would handle the > > open/close calls still run as root? Because we need root for some things, > > like accessing /dev/mem when dealing with GPIO on Raspberry Pi. > > Are you talking about /dev/mem for additional features not available over > /sys/class/gpio? Or faster access? > > Generally for RPi usage, you probably wouldn't want /dev/mem directly, but > some device that is limited to only the I/O regions. This can be done by a > very simple custom kernel module (in fact I'm surprised the RPi doesn't > include one already). Faster access. I got the idea from the most popular library for Rpi: WiringPi, which uses /dev/mem to do all actions. e.g. PWM is not fast enough over fd. See https://github.com/janjongboom/wiringPi/blob/master/wiringPi/wiringPi.c#L1850. Cross compiling this library to emscripten is my first goal of the new API. I first did it for node.js to see what APIs we'd miss in Gecko: https://github.com/kripken/emscripten/pull/3275
I'm trying to get my head around the IPC stuff. I created a an IPDL file that has sync Fopen declared on the parent, generated the code, implemented the function. But now I need to set up a channel between the main process and the worker. Do you know how to do that? The tutorials on MDN don't handle this at all, so it feels like it should be something magically... This is what I have now: https://github.com/jan-os/gecko-dev/commit/92272760580ee1b81a1b5e56417e079fdba17ffb
Flags: needinfo?(fabrice)
Also I don't know where this openremotefile protocol would be implemented. It's not IPDL at least.
You want to use PBackground because this API will run in workers and we don't want to go to the main-thread just to make child and parent processes talk. Take a look how BroadcastChannel API does it. The steps are: 1. create an IPDL file and make it be managed by PBackground.ipdl 2. PBackgroundChild* actor = BackgroundChild::GetForCurrentThread() will give you an PBackground actor in workers where you can send a Constructor message to have the Child actor in the worker thread and the Parent actor in the parent process PBackground thread. 3. When you want to do some operation, just send a message from the child actor to the parent, using your protocol. The parent actor will be in the PBackground thread on the main-process. Here do the operation and then send the result back (async) to the child process. 4. the child actor will receive a callback in the thread where it belongs to (the worker thread in this case). Let me know if you need help for this. I guess we are in the same timezone, so ping me on IRC if you need.
Flags: needinfo?(fabrice)
:baku, I got it working, but I have a weird thing. I implemented fopen/fread, and I dispatch the fopen call to main thread. It then returns the pointer address. When I use that pointer to call fread in worker, I get the actual data back. But it's a different thread right? Isn't it weird I have access to the main processes address space? See my note here: https://github.com/jan-os/gecko-dev/commit/fe8e9bc152268816f6b2ebb050cdee51c634514d I assume this'd break when running in b2g with different content processes. Would it?
Flags: needinfo?(amarchesini)
Jan, you need to send back the fopen'ed fd to the child through IPC. See what is done in https://mxr.mozilla.org/mozilla-central/find?text=&string=remoteopenfile
Yeah I know, I was just wondering why/how this memory sharing thing works...
Flags: needinfo?(fabrice)
> the actual data back. But it's a different thread right? Isn't it weird I > have access to the main processes address space? Let me comment here instead on github: All the F* operations must done in the parent process: fopen, fread, fwrite, fclose, whatever you want to expose. These functions must be part of the IPDL protocol. But you do want to expose FILE* directly. I would do this: 1. the parent must keep a list (hashtable more likely) of the opened FILE* and an unique identifier (an incremental ID is more than enough). 2. SendFopen (or the RecvFopen in case you want to do it async) returns this unique ID. 3. any following operations (SendFclose, SendFread, etc) will work with this ID. 4. for instance, the parent will receive a RecvWrite(uint32_t ID, nsACString& buffer), and you will do: FILE* file; if (!mOpenedFilesTable.get(id, &file)) { return false; // kill the child in case the ID is wrong } MOZ_ASSERT(file); fwrite(file, buffer.get(), buffer.size()); ... 5. in case you receive a parent::ActorDestroy() call, fclose all the opened FILE. 6. in DTOR of tha perent, MOZ_ASSERT(mOpenedFileTable.IsEmpty()); This is just how I would do it :) Feel free to propose a different approach.
Flags: needinfo?(amarchesini)
Attached patch os-worker-patch2.diff (obsolete) — — Splinter Review
Hi baku, this is an updated patch that comes with working IPC and has open/read/write/close/stat/lstat/fstat implemented. There's tests (also for remote mozbrowser) in the dom/os/test folder, and I added error handling around the thing. Please let me know your thoughts (feel free to skip nits). FYI, this patch can be applied on 6febe17d90262a52f230bbcbd2c0a23a40777361 (5 days ago), haven't tried against mc.
Attachment #8584569 - Attachment is obsolete: true
Attachment #8591710 - Flags: feedback?(amarchesini)
Flags: needinfo?(fabrice)
(In reply to Andrea Marchesini (:baku) from comment #35) > > the actual data back. But it's a different thread right? Isn't it weird I > > have access to the main processes address space? > > Let me comment here instead on github: > > All the F* operations must done in the parent process: fopen, fread, fwrite, > fclose, whatever you want to expose. > These functions must be part of the IPDL protocol. But you do want to expose > FILE* directly. So, this is what I wanted to avoid (and the current patch, if I'm reading it correctly, avoids it too). fopen and fclose happen in the parent process, but then the content process gets the fd and is allowed to read and write. This is vastly more efficient (avoiding two context switches per call) and allows the use of shared memory in the future. Also, if it is decided to implement a more specific MozGPIO / MozSPI / etc protocol in the future, it would be really nice if the permissions matched the per-fd permissions here - so that those APIs could be implemented by talking to the fd in the content process. This is how talking to other hardware, like graphics accelerators, is done on Linux in an efficient manner.
Uber-nit-let's-bikeshed: I don't think that dom/os/ and the Os prefix in general to be a good name. And no, I don't have a nice proposal either ;)
> Faster access. I got the idea from the most popular library for Rpi: > WiringPi, which uses /dev/mem to do all actions. e.g. PWM is not fast enough > over fd. See > https://github.com/janjongboom/wiringPi/blob/master/wiringPi/wiringPi. > c#L1850. Cross compiling this library to emscripten is my first goal of the > new API. > > I first did it for node.js to see what APIs we'd miss in Gecko: > https://github.com/kripken/emscripten/pull/3275 Okay, cool. You also get access to lots of registers that aren't available under the /sys/class/gpio API. However, allowing access to the full /dev/mem is a bit extreme. This is a problem that can be easily solved with a simple kernel module, which provides a /dev/mem-like interface but limited to the range of MMIO for each peripheral. The alternative is to filter /dev/mem accesses by sending fread/fwrite over IPC, but that comes at a huge performance cost that I'd like to avoid. I could write an example kernel module for the RPi, if you'd like. In fact, I would expect there to be some written already.
(In reply to Thomas Daede from comment #37) > So, this is what I wanted to avoid (and the current patch, if I'm reading it > correctly, avoids it too). Correct. This is my feeling too, and as such implemented in the API for open/read/write. (In reply to Thomas Daede from comment #39) > I could write an example kernel module for the RPi, if you'd like. In fact, > I would expect there to be some written already. Permission-wise this wouldn't matter right? You'd still need to be root? (In reply to Fabrice Desré [:fabrice] from comment #38) > Uber-nit-let's-bikeshed: I don't think that dom/os/ and the Os prefix in > general to be a good name. And no, I don't have a nice proposal either ;) NativeIO or something? I have no clue either.
Comment on attachment 8591710 [details] [diff] [review] os-worker-patch2.diff Review of attachment 8591710 [details] [diff] [review]: ----------------------------------------------------------------- the approach seems correct. Add more tests. ::: dom/os/OsManager.cpp @@ +88,5 @@ > + > +void > +OsManager::Read(JSContext* aCx, File& aFile, int aBytes, JS::MutableHandle<JSObject*> aRet, ErrorResult& aRv) > +{ > + unsigned char* buffer = (unsigned char*)malloc(aBytes + 1); buffer can be null. check it. @@ +89,5 @@ > +void > +OsManager::Read(JSContext* aCx, File& aFile, int aBytes, JS::MutableHandle<JSObject*> aRet, ErrorResult& aRv) > +{ > + unsigned char* buffer = (unsigned char*)malloc(aBytes + 1); > + size_t bytes_read = read(aFile.GetFd(), buffer, aBytes); What about: nsAutoCString buffer; if (!buffer.SetLength(aBytes, fallible)) { do something; } size_t bytes_read = read(aFile.getFd(), buffer.BeginWriting(), aBytes); if (bytes_read == 1) { do something; } buffer.Truncate(bytes_read); ... @@ +95,5 @@ > + HandleErrno(errno, aRv); > + return; > + } > + > + JSObject* outView = nullptr; I guess you have to check bytes_read == 0 @@ +125,5 @@ > +OsManager::Close(File& aFile, ErrorResult& aRv) > +{ > + if (close(aFile.GetFd()) == -1) { > + HandleErrno(errno, aRv); > + return -1; If you already have ErrorResult, why do you need this -1 ? Here an in the other methods. @@ +133,5 @@ > + > +already_AddRefed<os::Stat> > +OsManager::Lstat(const nsAString& aPath, ErrorResult& aRv) > +{ > + StatWrapper* sw = new StatWrapper(); Do you really need to allocate it? ::: dom/os/OsManager.h @@ +53,5 @@ > + already_AddRefed<os::Stat> Stat(const nsAString& aPath, ErrorResult& aRv); > + already_AddRefed<os::Stat> Lstat(const nsAString& aPath, ErrorResult& aRv); > + already_AddRefed<os::Stat> Fstat(const File& aFile, ErrorResult& aRv); > + > + int RDONLY() const { return O_RDONLY; } why all of these are methods? @@ +68,5 @@ > + > + int IWRITE() const { return S_IWRITE; } > + int IREAD() const { return S_IREAD; } > + > + void GetTEMP_DIR(nsString& aRetVal) { nsresult GetTempDir(nsAString& aRetVal) { and return the rv in case it fails. @@ +80,5 @@ > + } > + > +protected: > + virtual ~OsManager() { > + // ? free(mActor) ? you have to call Send__delete__ ::: dom/os/ipc/OsFileChannelParent.cpp @@ +27,5 @@ > +} > + > +bool > +OsFileChannelParent::RecvOpen(const nsString& aPath, const int& aAccess, const int& aPermission, FileDescriptorResponse* aFd) > +{ 80chars ::: dom/os/test/remote.html @@ +7,5 @@ > + var file = navigator.os.open("../../../../dom/os/test/payload.txt", > + navigator.os.RDONLY, navigator.os.IREAD); > + var data = navigator.os.read(file, 100); > + var cr = navigator.os.close(file); > + extra spaces ::: dom/webidl/OsManager.webidl @@ +15,5 @@ > + OsManagerStat stat(DOMString path); > + [Throws] > + OsManagerStat fstat(OsManagerFile file); > + > + readonly attribute long RDONLY; make these const so that you don't need the methods and you can do: OsManager.RDONLY. ::: ipc/ipdl/ipdl/lower.py @@ +1720,1 @@ > why these changes?
Attachment #8591710 - Flags: feedback?(amarchesini) → feedback+
(In reply to Andrea Marchesini (:baku) from comment #41) > buffer can be null. check it. Ok > What about: > > nsAutoCString buffer; > if (!buffer.SetLength(aBytes, fallible)) { > do something; > } > > size_t bytes_read = read(aFile.getFd(), buffer.BeginWriting(), aBytes); > if (bytes_read == 1) { > do something; > } > > buffer.Truncate(bytes_read); Hmm.. But then you need to convert it again to normal char* right thereafter when calling Uint8Array::Create because that expects a char*. You think it's better that way? Feels like overkill to me. > > @@ +95,5 @@ > I guess you have to check bytes_read == 0 bytes_read 0 is valid I think. "If no process has the pipe open for writing, read() shall return 0 to indicate end-of-file." If that's the case then returning an empty uint8array seems like a good choice. > > @@ +125,5 @@ > > +OsManager::Close(File& aFile, ErrorResult& aRv) > > +{ > > + if (close(aFile.GetFd()) == -1) { > > + HandleErrno(errno, aRv); > > + return -1; > > If you already have ErrorResult, why do you need this -1 ? Here an in the > other methods. Correct. Will change to void everywhere. > > @@ +133,5 @@ > > + > > +already_AddRefed<os::Stat> > > +OsManager::Lstat(const nsAString& aPath, ErrorResult& aRv) > > +{ > > + StatWrapper* sw = new StatWrapper(); > > Do you really need to allocate it? > > ::: dom/os/OsManager.h > @@ +53,5 @@ > > + already_AddRefed<os::Stat> Stat(const nsAString& aPath, ErrorResult& aRv); > > + already_AddRefed<os::Stat> Lstat(const nsAString& aPath, ErrorResult& aRv); > > + already_AddRefed<os::Stat> Fstat(const File& aFile, ErrorResult& aRv); > > + > > + int RDONLY() const { return O_RDONLY; } > > why all of these are methods? Because this is what the WebIDL declaration expects. Perhaps will be different when defining as const there. > nsresult GetTempDir(nsAString& aRetVal) { > > and return the rv in case it fails. Can't really because it's an attribute right. Attributes shouldn't throw. > @@ +80,5 @@ > > + } > > + > > +protected: > > + virtual ~OsManager() { > > + // ? free(mActor) ? > > you have to call Send__delete__ Hmm, but __delete__ is declared under child in the protocol, so there is no send__delete__ call on mActor, and OsManager.h doesn't have it itself of course. Any idea how to fix that? > > ::: ipc/ipdl/ipdl/lower.py > @@ +1720,1 @@ > > > > why these changes? Bug 1135344 - Fix building on new xcode
Flags: needinfo?(amarchesini)
> > + int RDONLY() const { return O_RDONLY; } > why all of these are methods? This cant be declared as const in webidl because the value of O_RDONLY depends on host system, so need to implement it as methods.
Another question, I need a reference to the current mozIApplication, figured I could get that from nsIAppsService, which I can feed nsIPrincipal->GetAppId. and I can get the principal from workers::GetCurrentThreadWorkerPrivate()->GetPrincipal(). But how do I fake that I'm an app in my mochitest... It always says that it does not have an App ID (which makes sense).
And last thing, I wrote the following code to see which path you need rights to (manifest should contain a list of whitelisted paths where you can use navigator.os). F.e. if you open() /Users/janjongboom/test.txt then you need permission to /Users/janjongboom or /Users/ or /. But if /Users/janjongboom doesn't exist, you need /Users. So need to normalize. This is my take on such function, please let me know what you think. It feels kinda dirty code.
Attachment #8594744 - Flags: feedback?(amarchesini)
Attachment #8594744 - Attachment is patch: true
Comment on attachment 8591710 [details] [diff] [review] os-worker-patch2.diff Review of attachment 8591710 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/OsManager.webidl @@ +15,5 @@ > + OsManagerStat stat(DOMString path); > + [Throws] > + OsManagerStat fstat(OsManagerFile file); > + > + readonly attribute long RDONLY; If you can't define these as 'const' because of the OS dependency, you can tag the getters as [Constant] to allow the JS engine to optimize code that uses them: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Constant
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #44) > Another question, I need a reference to the current mozIApplication, figured > I could get that from nsIAppsService, which I can feed > nsIPrincipal->GetAppId. and I can get the principal from > workers::GetCurrentThreadWorkerPrivate()->GetPrincipal(). But how do I fake > that I'm an app in my mochitest... It always says that it does not have an > App ID (which makes sense). I suspect you don't want to fake things, that you actually want to be an installed app. I made this work for mochitests for mozDownloadManager in https://dxr.mozilla.org/mozilla-central/source/dom/downloads/tests/test_downloads_adopt_download.html using https://dxr.mozilla.org/mozilla-central/source/dom/downloads/tests/shim_app_as_test.js and its friends. However the important note is that I did that when we only had mochitest-plain for b2g and no chrome mochitests. I understand that use of SpecialPowers is regarded as a bad idea in general, so it might be preferred to convert this mechanism to be a chrome mochitest (or new category of mochitest?). Hopefully someone with more wisdom shows up to yell at me here and you can benefit from that :)
Also, the AppsService is implemented in JS so it's not available in workers afaik.
Fabrice, any idea on how we can read the manifest to see which paths we have access too in that case? If there's no access to AppsService in the worker...
Flags: needinfo?(fabrice)
well, the path is known by the parent when you open the fd, right? So maybe it's ok to do the check in the parent instead of doing it in the worker from the child side. You would still have to go to the main thread though.
Flags: needinfo?(fabrice)
(In reply to Andrew Sutherland [:asuth] from comment #47) > (In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #44) > > Another question, I need a reference to the current mozIApplication, figured > > I could get that from nsIAppsService, which I can feed > > nsIPrincipal->GetAppId. and I can get the principal from > > workers::GetCurrentThreadWorkerPrivate()->GetPrincipal(). But how do I fake > > that I'm an app in my mochitest... It always says that it does not have an > > App ID (which makes sense). > > I suspect you don't want to fake things, that you actually want to be an > installed app. > > I made this work for mochitests for mozDownloadManager in > https://dxr.mozilla.org/mozilla-central/source/dom/downloads/tests/ > test_downloads_adopt_download.html using > https://dxr.mozilla.org/mozilla-central/source/dom/downloads/tests/ > shim_app_as_test.js and its friends. However the important note is that I > did that when we only had mochitest-plain for b2g and no chrome mochitests. > I understand that use of SpecialPowers is regarded as a bad idea in general, > so it might be preferred to convert this mechanism to be a chrome mochitest > (or new category of mochitest?). Hopefully someone with more wisdom shows > up to yell at me here and you can benefit from that :) Thanks, this worked :-)
I got the app stuff working in tests, and I can acquire the appId from the IPrincipal in the worker. So far, so good. Now I need to get a reference to the appsService to read something there. I figure I can only do this from: 1. The worker itself, and then pass it over IPC to the background process, but according to fabrice in https://bugzilla.mozilla.org/show_bug.cgi?id=1141021#c48 that wouldn't work 2. The IPC Parent Why: because the security check is done in the IPC parent, so I need a list of data from the manifest. Afaik a new IPC parent is constructed for every worker (as we do |mActor = backgroundChild->SendPOsFileChannelConstructor();| in OsManager.cpp) so if I would pass the list of paths down from the Worker constructor that wouldn't be shared. Anyway, I tried in scenarios 1. and 2. the following code: #include "mozIApplication.h" #include "nsIAppsService.h" #include "nsIServiceManager.h" nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID); if (!appsService) { printf("No appsService...\n"); return false; } However this code crashes the whole browser for me. It doesn't reach the printf, just a cold-hard crash in both cases. Is that a sign that I can't get the appsService in the background thread either or am I doing something stupid?
Flags: needinfo?(fabrice)
You can only access this service on the main thread, because it's implemented in JS. No surprise that you crash in workers and background threads.
Flags: needinfo?(fabrice)
I guess I can go from worker to main thread via something like PostMessageToParentInternal on WorkerPrivate, but where does this end up in the main thread?
Flags: needinfo?(fabrice)
You can use NS_DispatchToMainThread to schedule a runnable on the main thread.
Flags: needinfo?(fabrice)
Attached patch hanging_thread.diff (obsolete) — — Splinter Review
Soap goes on. I have the following code, which should run something on main thread and pause the current thread if I check other files in mc / read the docs. The dispatching to the main thread goes fine, it ends up there, and does the printf. However, it then hangs forever. Doesn't return back. What the *#&*@ is going on here?
Flags: needinfo?(fabrice)
(it runs fine w/o SYNC by the way, but I want to run this sync in the constructor of OsManager)
Synchronous dispatch spins a nested event loop on the calling thread. (https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.cpp#586) Maybe some other event is getting in there and wedging your thread? It should be pretty obvious what's happening in gdb; does gdb not work on the target you're using?
Attached patch permissions.diff (obsolete) — — Splinter Review
Here's the work to read the paths from the manifest. Does this make sense? (It should still be sync though)
Attachment #8599929 - Flags: feedback?(fabrice)
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #59) > Created attachment 8599929 [details] [diff] [review] > permissions.diff > > Here's the work to read the paths from the manifest. Does this make sense? > > (It should still be sync though) You changed the expectation about the format of the permission field in manifests. Could you do that instead: "permissions": { "posix-files": { "paths": [...] } } Also, it may be more efficient to not add osPaths to mozIApplication but to add a method call on nsIAppsService along the lines of getPosixPathsByLocalId() instead.
Flags: needinfo?(fabrice)
Attachment #8599929 - Flags: feedback?(fabrice)
(In reply to Andrew Sutherland [:asuth] from comment #58) > Synchronous dispatch spins a nested event loop on the calling thread. > (https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread. > cpp#586) Maybe some other event is getting in there and wedging your thread? > > It should be pretty obvious what's happening in gdb; does gdb not work on > the target you're using? Hi Andrew, what do you mean with 'getting in there and wedging your thread'? I just want a simple thing Worker->MainThread->Worker while the thing blocks, which is done in a gazillion places like https://dxr.mozilla.org/mozilla-central/source/dom/media/android/AndroidMediaPluginHost.cpp#64 and I can't wrap my head around it anymore.
Flags: needinfo?(bugmail)
Context (I may be restating stuff you already know, just skip that): The steady state of all nsThreads is an event loop where they spin a while loop, using NS_ProcessNextEvent, pulling runnables off the thread's event queue and running them. The usual idiom for doing something synchronous is to spin a nested event loop so that the thread can still do other things. When your source worker thread issues the 'sync' dispatch, a nested event loop is spun at https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.cpp#586 on your source thread, which I guess is a worker. Having briefly looked at the sync dispatch code and your example, it seems like it should work. So then the best explanation is that the problem is that the nested event loop is running some other runnable that is not returning, or a series of long-running runnables that are just taking a long time to return. The worst type of scenario would be one where the problematic runnable spins a nested event loop beneath your nested event loop, and its completion has a logical dependency on your synchronous-y code doing something once control flow has returned to it. But there isn't really a need to guess here, because if your OK/NOK printf's aren't running, it means that your code is still going to be on the stack and the smoking gun will be visible with gdb. Please see https://developer.mozilla.org/en-US/Firefox_OS/Debugging/Debugging_B2G_using_gdb for details on how to attach gdb to your target. If it's not clear which thread is yours "thread apply all bt" will get you backtraces for all threads and then you can switch to your thread with "thread N". In the event it's JS code doing something funky and so the stack is basically useless, "call DumpJSStack()" (https://developer.mozilla.org/en-US/docs/Debugging_Mozilla_with_gdb) should print the stack to stdout for the b2g process. (So you might need to have run b2g directly from "adb shell" to see that if it's not ending up in logcat, I forget how our wrapper works.) (Note: I apologize if my previous comment about gdb came across as snarky; it seems snarky to me now, but was not meant as such. I know you're working with different targets than we usually work with and it wouldn't surprise me if there exist platforms where b2g works but gdb doesn't work.)
Flags: needinfo?(bugmail)
Attached patch Patch v3 (obsolete) — — Splinter Review
All the moving parts are here now. Please give me your feedback. Open question: how to do proper initialization of the OsManager as the Pref stuff in WebIDL is not available if the scope is a worker. Should we add a Func="" ? Need to check both if the permission is there or if there is the pref set, so need to go to main thread both of these times. Also: I use MOZ_ASSERT now but I tihnk that's only in debug builds that it will halt, so how do we do this properly?
Attachment #8591710 - Attachment is obsolete: true
Attachment #8594744 - Attachment is obsolete: true
Attachment #8599832 - Attachment is obsolete: true
Attachment #8599929 - Attachment is obsolete: true
Attachment #8594744 - Flags: feedback?(amarchesini)
Flags: needinfo?(amarchesini)
Attachment #8600952 - Flags: review?(amarchesini)
Attachment #8600952 - Flags: feedback?(fabrice)
> Also, it may be more efficient to not add osPaths to mozIApplication but to > add a method call on nsIAppsService along the lines of > getPosixPathsByLocalId() instead. But nsIAppsService uses nsIApplication to get the data in those get*ByLocalId calls right, so need to add it anyway.
This patch can be applied to 36c84bbd25a87bde0d6b864c8ad7942af8a3fae2
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #64) > > Also, it may be more efficient to not add osPaths to mozIApplication but to > > add a method call on nsIAppsService along the lines of > > getPosixPathsByLocalId() instead. > > But nsIAppsService uses nsIApplication to get the data in those > get*ByLocalId calls right, so need to add it anyway. No, AppsServices delegates most of the work to Webapps.jsm or AppsServiceChild.jsm and they don't need a nsIApplication object.
(In reply to Fabrice Desré [:fabrice] from comment #66) > (In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #64) > > > Also, it may be more efficient to not add osPaths to mozIApplication but to > > > add a method call on nsIAppsService along the lines of > > > getPosixPathsByLocalId() instead. > > > > But nsIAppsService uses nsIApplication to get the data in those > > get*ByLocalId calls right, so need to add it anyway. > > No, AppsServices delegates most of the work to Webapps.jsm or > AppsServiceChild.jsm and they don't need a nsIApplication object. OK. I'll adjust it in the next revision of this patch.
Any ETA on this?
Any ETA on this?
Flags: needinfo?(amarchesini)
Comment on attachment 8600952 [details] [diff] [review] Patch v3 Review of attachment 8600952 [details] [diff] [review]: ----------------------------------------------------------------- Looks good but I would like to review it again when these points are fixed: 1. no manual allocation of strings. 2. security check not in RecvInit Thanks! (and sorry for the delay) ::: dom/apps/AppsUtils.jsm @@ +123,5 @@ > + aApp.manifest.permissions['posix-files'] && > + aApp.manifest.permissions['posix-files'].paths) || []; > + os.forEach(function(path) { > + let wrapper = Cc["@mozilla.org/supports-string;1"] > + .createInstance(Ci.nsISupportsString); strange indentation. @@ +126,5 @@ > + let wrapper = Cc["@mozilla.org/supports-string;1"] > + .createInstance(Ci.nsISupportsString); > + wrapper.data = path; > + osPaths.appendElement(wrapper, false); > + }); I want Fabrice to review this changes. ::: dom/interfaces/apps/mozIApplication.idl @@ +62,5 @@ > readonly attribute DOMString kind; > + > + /* If the app has access to the os module, lists all the paths that the app > + is allowed to access */ > + readonly attribute nsIArray osPaths; // DOMString[] You must change the UUID of this interface. ::: dom/os/OsManager.cpp @@ +36,5 @@ > +NS_INTERFACE_MAP_BEGIN(OsManager) > +NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper) > + > +OsManager::OsManager(workers::WorkerGlobalScope* aScope) > + : DOMEventTargetHelper(static_cast<DOMEventTargetHelper*>(aScope)), You should not need this static_cast @@ +38,5 @@ > + > +OsManager::OsManager(workers::WorkerGlobalScope* aScope) > + : DOMEventTargetHelper(static_cast<DOMEventTargetHelper*>(aScope)), > + mScope(aScope) > +{ Can oyu assert that this created on the worker Thread? @@ +45,5 @@ > + mActor = backgroundChild->SendPOsFileChannelConstructor(); > + > + MOZ_ASSERT(mActor); > + > + auto principal = workers::GetCurrentThreadWorkerPrivate()->GetPrincipal(); Instead doing this, can you make OsManager to receive a WorkerPrivate in the CTOR? Doing this you can get the scope doing GlobalScope(). Plus: GetPrincipal _must_ be called from the main thread. I guess you want to do some MainThread runnable. Just because thsi code runs in a worker, you can use a sync Runnable and block the runnable until this operation is fully completed. @@ +47,5 @@ > + MOZ_ASSERT(mActor); > + > + auto principal = workers::GetCurrentThreadWorkerPrivate()->GetPrincipal(); > + uint32_t appId; > + nsresult rv = principal->GetAppId(&appId); Can you move all this in the Init() method in order to propagate the error in the WorkerNavigator? @@ +66,5 @@ > +} > + > +void > +OsManager::HandleErrno(int aErr, ErrorResult& aRv) > +{ Assert we are in the workerThread. @@ +84,5 @@ > +already_AddRefed<File> > +OsManager::Open(const nsAString& aPath, int aAccess, int aPermission, > + ErrorResult& aRv) > +{ > + aRv.MightThrowJSException(); 1. you don't need this. 2. assert the thread in any method. @@ +106,5 @@ > +void > +OsManager::Read(JSContext* aCx, File& aFile, int aBytes, > + JS::MutableHandle<JSObject*> aRet, ErrorResult& aRv) > +{ > + unsigned char* buffer = (unsigned char*)malloc(aBytes + 1); Instead doing this, can you do this: nsAutoArrayPtr<char> buffer(new (fallible) char[aBytes + 1]); if (!buffer) { aRv.Throw(NS_ERROR_OUT_OF_MEMORY); return; } size_t bytes_read = read(aFile.GetFd(), buffer, aBytes); ... @@ +119,5 @@ > + return; > + } > + > + JSObject* outView = nullptr; > + outView = Uint8Array::Create(aCx, bytes_read, buffer); JSObject* outView = Uint8Array::Create(aCx, bytes_read, reinterpret_cast<uint8_t*>(buffer.get()) @@ +121,5 @@ > + > + JSObject* outView = nullptr; > + outView = Uint8Array::Create(aCx, bytes_read, buffer); > + > + free(buffer); remove this. @@ +155,5 @@ > + > +already_AddRefed<os::Stat> > +OsManager::Lstat(const nsAString& aPath, ErrorResult& aRv) > +{ > + StatWrapper* sw = new StatWrapper(); Just to avoid leaks, do: nsAutoPtr<StatWrapper> sw = new StatWrapper(); @@ +158,5 @@ > +{ > + StatWrapper* sw = new StatWrapper(); > + bool ret = mActor->SendLstat((nsString&)aPath, sw); > + if (!ret) { > + aRv.Throw(NS_ERROR_FAILURE); here for instance you are leaking sw. @@ +169,5 @@ > + } > + > + nsRefPtr<os::Stat> stat = new os::Stat(this, sw->GetWrappedObject()); > + > + free(sw); Then remove this. @@ +177,5 @@ > + > +already_AddRefed<os::Stat> > +OsManager::Stat(const nsAString& aPath, ErrorResult& aRv) > +{ > + StatWrapper* sw = new StatWrapper(); same logic here. @@ +200,5 @@ > +already_AddRefed<os::Stat> > +OsManager::Fstat(const File& aFile, ErrorResult& aRv) > +{ > + struct stat sb; > + if (fstat(aFile.GetFd(), &sb) == -1) { Where a comment saying that we don't need to use IPDL if we have a filedescriptor because... @@ +214,5 @@ > +void > +OsManager::Chmod(const nsAString& aPath, int aMode, ErrorResult& aRv) > +{ > + int rv; > + bool ret = mActor->SendChmod((nsString&)aPath, aMode, &rv); nsString(aPath) @@ +216,5 @@ > +{ > + int rv; > + bool ret = mActor->SendChmod((nsString&)aPath, aMode, &rv); > + if (!ret) { > + aRv.Throw(NS_ERROR_FAILURE); return; @@ +218,5 @@ > + bool ret = mActor->SendChmod((nsString&)aPath, aMode, &rv); > + if (!ret) { > + aRv.Throw(NS_ERROR_FAILURE); > + } > + else if (rv != 0) { and no 'else' here. I would do: MOZ_ASSERT(rv <= 0) can it be some > value? @@ +240,5 @@ > + bool ret = mActor->SendUnlink((nsString&)aPath, &rv); > + if (!ret) { > + aRv.Throw(NS_ERROR_FAILURE); > + } > + else if (rv != 0) { ditto. @@ +250,5 @@ > +OsManager::Utimes(const nsAString& aPath, const Date& aActime, > + const Date& aModtime, ErrorResult& aRv) > +{ > + int rv; > + bool ret = mActor->SendUtimes((nsString&)aPath, aActime.TimeStamp(), nsString(aPath) @@ +255,5 @@ > + aModtime.TimeStamp(), &rv); > + if (!ret) { > + aRv.Throw(NS_ERROR_FAILURE); > + } > + else if (rv != 0) { ditto. @@ +265,5 @@ > +OsManager::Lutimes(const nsAString& aPath, const Date& aActime, > + const Date& aModtime, ErrorResult& aRv) > +{ > + int rv; > + bool ret = mActor->SendLutimes((nsString&)aPath, aActime.TimeStamp(), nsString(aPath) @@ +269,5 @@ > + bool ret = mActor->SendLutimes((nsString&)aPath, aActime.TimeStamp(), > + aModtime.TimeStamp(), &rv); > + if (!ret) { > + aRv.Throw(NS_ERROR_FAILURE); > + } ditto. @@ +298,5 @@ > +void > +OsManager::Truncate(const nsAString& aPath, int aLength, ErrorResult& aRv) > +{ > + int rv; > + bool ret = mActor->SendTruncate((nsString&)aPath, aLength, &rv); nsString(..) @@ +301,5 @@ > + int rv; > + bool ret = mActor->SendTruncate((nsString&)aPath, aLength, &rv); > + if (!ret) { > + aRv.Throw(NS_ERROR_FAILURE); > + } ditto @@ +320,5 @@ > +void > +OsManager::Mkdir(const nsAString& aPath, int aMode, ErrorResult& aRv) > +{ > + int rv; > + bool ret = mActor->SendMkdir((nsString&)aPath, aMode, &rv); ditto. @@ +322,5 @@ > +{ > + int rv; > + bool ret = mActor->SendMkdir((nsString&)aPath, aMode, &rv); > + if (!ret) { > + aRv.Throw(NS_ERROR_FAILURE); ditto. @@ +333,5 @@ > +void > +OsManager::Rmdir(const nsAString& aPath, ErrorResult& aRv) > +{ > + int rv; > + bool ret = mActor->SendRmdir((nsString&)aPath, &rv); I think I don't have to write it everywhere :) ::: dom/os/OsManager.h @@ +8,5 @@ > + > +#include <fcntl.h> > +#include <list> > +#include "File.h" > +#include "mozilla/dom/os/POsFileChannelChild.h" Forward declaration of these 2 @@ +39,5 @@ > + > + explicit OsManager(workers::WorkerGlobalScope* aScope); > + > + void Init(); > + void Shutdown(); Why these 2 are public? @@ +43,5 @@ > + void Shutdown(); > + > + virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > + > + POsFileChannelChild* mActor; Also this should be private. @@ +130,5 @@ > + int IRWXU() const { return S_IRWXU; } > + int IRWXG() const { return S_IRWXG; } > + int IRWXO() const { return S_IRWXO; } > + > + void GetTEMP_DIR(nsString& aRetVal) { 1. { in a new line. 2. it should be private. 3. it should return an error code. @@ +142,5 @@ > + } > + > +protected: > + virtual ~OsManager() { > + // ? free(mActor) ? not at all. you should manage this in the logic of this class sending a __delete__ ::: dom/os/Stat.h @@ +91,5 @@ > + { > + return mStat.st_blocks; > + } > + > + // @todo: should we free() this stuff? no :) @@ +94,5 @@ > + > + // @todo: should we free() this stuff? > + mozilla::dom::Date Atime() const > + { > + return mozilla::dom::Date((double)mStat.st_atime * 1000); remove mozilla::dom:: everywhere ::: dom/os/ipc/OsFileChannelChild.h @@ +24,5 @@ > +public: > + NS_INLINE_DECL_REFCOUNTING(OsFileChannelChild) > + > + void SetParent(OsManager* aOsManager) > + { MOZ_ASSERT(aOsManager) @@ +34,5 @@ > + return mActorDestroyed; > + } > + > +private: > + explicit OsFileChannelChild(); no explicit here ::: dom/os/ipc/OsFileChannelParent.cpp @@ +37,5 @@ > +{ > + AssertIsOnBackgroundThread(); > +} > + > +class GetAllowedPaths : public nsRunnable { { in a new line. @@ +44,5 @@ > + : mAppId(aAppId), mResult(aResult) {} > + > + NS_IMETHOD Run() { > + MOZ_ASSERT(NS_IsMainThread()); > + extra soaces @@ +45,5 @@ > + > + NS_IMETHOD Run() { > + MOZ_ASSERT(NS_IsMainThread()); > + > + printf("Unknown appId, security=%d\n", Preferences::GetBool("dom.os.security.disabled")); remove this. @@ +53,5 @@ > + } > + > + nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID); > + if (!appsService) { > + printf("No appsService...\n"); extra prints here. @@ +112,5 @@ > + * before returning! > + * If the return value is true, you need to free() yourself. > + */ > +bool > +OsFileChannelParent::VerifyRights(char* aPath) this should receive a const nsACString& aPath @@ +123,5 @@ > + char* path = aPath; > + > + // do a real_path call on path > + char* real_path; > + while ((real_path = realpath(path, NULL)) == NULL) { can you do: char real_path[PATH_MAX]; while(realpath(path, real_path)) ... @@ +128,5 @@ > + // if it's NULL that means that we have an issue > + // when ENOENT we check parent directory (and up and up, etc.) > + // when something else, break and return false > + if (errno != ENOENT) { > + printf("VerifyRights for '%s' failed with %d (%s)\n", NS_WARNING instead printf to print warning messages. here and everywhere else. @@ +130,5 @@ > + // when something else, break and return false > + if (errno != ENOENT) { > + printf("VerifyRights for '%s' failed with %d (%s)\n", > + aPath, errno, strerror(errno)); > + free(aPath); no manual freee. @@ +155,5 @@ > + } > + > + // not null? then time to check... real_path is the path we need to check > + > + nsString rp = NS_ConvertASCIItoUTF16(real_path); ASCII? what about if the filesystem supports unicode? @@ +173,5 @@ > +} > + > +bool > +OsFileChannelParent::RecvInit(const int& aAppId) > +{ AssertIsOnBackgroundThread() @@ +175,5 @@ > +bool > +OsFileChannelParent::RecvInit(const int& aAppId) > +{ > + nsCOMPtr<nsIRunnable> event = new GetAllowedPaths(aAppId, &mAllowedPaths); > + nsresult rv = NS_DispatchToMainThread(event, NS_DISPATCH_SYNC); I think you should get rid of this init and send the appId in the ctr of this OSFileChannelParent. Check how we do the security check in BackgroundChannel: http://mxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundParentImpl.cpp#345 you should do something similar and in case kill the child process. @@ +396,5 @@ > +} > + > +bool > +OsFileChannelParent::RecvTruncate(const nsString& aPath, > + const int& aLength, indentation @@ +499,5 @@ > + ReaddirResponse* aRetVal) > +{ > + nsTArray<nsString> files; > + > + auto path = ToNewCString(aPath); do here and everywhere you using ToNewCString: NS_ConvertUTF16toUTF8 path(aPath); and remove free(path). @@ +572,5 @@ > + int buffer_size = 255; > + char* buffer = NULL; > + > + while (1) { > + buffer = (char*)realloc(buffer, buffer_size); Don't do allocation manually. Do something similar to what I suggested in the other file. @@ +600,5 @@ > + } > +} > + > +void > +OsFileChannelParent::ActorDestroy(ActorDestroyReason aWhy) remove this. you are not actually using this method: you don't need to implement it. ::: dom/os/ipc/StatSerializer.h @@ +8,5 @@ > + > +namespace mozilla { > +namespace dom { > +namespace os { > +class StatWrapper final { { in a new line ::: dom/os/test/remote.html @@ +1,1 @@ > +<html> Add headers in the test files. @@ +7,5 @@ > + var file = navigator.os.open("../../../../dom/os/test/payload.txt", > + navigator.os.RDONLY, navigator.os.IREAD); > + var data = navigator.os.read(file, 100); > + navigator.os.close(file); > + extra spaces here and below. ::: dom/webidl/OsManager.webidl @@ +1,1 @@ > +[Exposed=(Worker,System), AvailableIn=CertifiedApps] Some header? The license + documentation URL. ::: dom/webidl/WorkerNavigator.webidl @@ +11,4 @@ > WorkerNavigator implements NavigatorLanguage; > WorkerNavigator implements NavigatorOnLine; > WorkerNavigator implements NavigatorDataStore; > +partial interface WorkerNavigator { Add a comment about this partial interface. This seems to be a mozilla specific API. It needs comments and documentation. ::: dom/workers/Navigator.cpp @@ +408,5 @@ > + mOsManager = new os::OsManager(workerPrivate->GlobalScope()); > + } > + > + nsRefPtr<os::OsManager> osManager = mOsManager; > + return osManager.forget(); definitely you must use nsRefPtr<> in WorkerNavigator otherwise you are going to have an invalid pointer. Think about this: nsRefPtr<os::OsManager> manager = workerNavigator->GetOs(rv); manager = nullptr; This will reduce the refcnt of manager and this will delete it. This means that WorkerNavigator->mOsManager will be an invalid pointer. ::: dom/workers/Navigator.h @@ +19,1 @@ > forwarding class. @@ +31,4 @@ > > NavigatorProperties mProperties; > bool mOnline; > + os::OsManager* mOsManager = nullptr; nsRefPtr<os::OsManager> mOsManager; @@ +45,5 @@ > { > + if (mOsManager) { > + free(mOsManager); > + } > + extra spaces. Plus, if you use nsRefPtr<os::OsManager>, you don't need to do the manual free.
Attachment #8600952 - Flags: review?(amarchesini) → review-
In the bikeshed department: what about using dom/iot instead of dom/os? And proably a bunch of other renaming like changing |interface OsManager| to |interface IotFileManager| etc.
This API isn't specific to IoT - which is a basically meaningless term anyway. OS seems to pretty well describe what it's doing. (But maybe doesn't make sense when the OS *is* the Web programming surface.)
Flags: needinfo?(amarchesini)
Sorry, didn't get to this in the past week and a half due to JSConf eating up all my time, but we can go the node.js direction and call it filesystem. I'm not too crazy about os, but haven't heard a better suggestion.
I'm afraid filesystem will be confused with some other filesystem webapi. Bah, naming is hard...
How about something like `systemIO`/`deviceIO`?
systemIO would make sense to me. Fabrice? /going to work on this again today.
Andrea, thanks a lot for your feedback. The changes I made based on your patch are attached (let me know if you need the full patch file), and I have a number of questions: (In reply to Andrea Marchesini (:baku) from comment #70) > ::: dom/os/OsManager.cpp > @@ +36,5 @@ > > +NS_INTERFACE_MAP_BEGIN(OsManager) > > +NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper) > > + > > +OsManager::OsManager(workers::WorkerGlobalScope* aScope) > > + : DOMEventTargetHelper(static_cast<DOMEventTargetHelper*>(aScope)), > > You should not need this static_cast I need to cast it anyway (changed it to normal cast), but why'd you choose a static_cast over a normal cast (or the other way around). > @@ +45,5 @@ > > + mActor = backgroundChild->SendPOsFileChannelConstructor(); > > + > > + MOZ_ASSERT(mActor); > > + > > + auto principal = workers::GetCurrentThreadWorkerPrivate()->GetPrincipal(); > > Plus: GetPrincipal _must_ be called from the main thread. I guess you want > to do some MainThread runnable. > Just because thsi code runs in a worker, you can use a sync Runnable and > block the runnable until this operation is fully completed. Changed the ctor, but are you sure about the GetPrincipal? This works fine, both from a normal web page and from an app loaded via mozbrowser iframe. > > @@ +200,5 @@ > > +already_AddRefed<os::Stat> > > +OsManager::Fstat(const File& aFile, ErrorResult& aRv) > > +{ > > + struct stat sb; > > + if (fstat(aFile.GetFd(), &sb) == -1) { > > Where a comment saying that we don't need to use IPDL if we have a > filedescriptor because... I put a comment on how this works on top of the file. Should we put extra comments inline (there are 7 places we use the fd at this point. probably gonna grow in time) for every occurrence as well? > I would do: MOZ_ASSERT(rv <= 0) can it be some > value? rv here is the value that comes out of errno, which as far as I know does not give guarantees on the numbers it returns, as the values are based on whatever is listed in errno.h. Don't think we should make the assumption. It's default value is 0 and if it's different then it's set from the other side of the IPC channel. > ::: dom/os/OsManager.h > @@ +8,5 @@ > > + > > +#include <fcntl.h> > > +#include <list> > > +#include "File.h" > > +#include "mozilla/dom/os/POsFileChannelChild.h" > > Forward declaration of these 2 What do you mean? > @@ +39,5 @@ > > + > > + explicit OsManager(workers::WorkerGlobalScope* aScope); > > + > > + void Init(); > > + void Shutdown(); > > Why these 2 are public? So I moved the code around getting the app ID and then sending it over the IPC channel to the Init method now, but the Init method has to be called, which I do in the WorkerNavigator whenever the object is created. So I have to mark it public. Or you think Init/Shutdown should be called automatically from some glue? Because that's not the case for me in this module. > > @@ +43,5 @@ > > + void Shutdown(); > > + > > + virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > > + > > + POsFileChannelChild* mActor; > > Also this should be private. You mean only mActor right? Because WrapObject is called from WorkerNavigatorBinding. > > @@ +130,5 @@ > > + int IRWXU() const { return S_IRWXU; } > > + int IRWXG() const { return S_IRWXG; } > > + int IRWXO() const { return S_IRWXO; } > > + > > + void GetTEMP_DIR(nsString& aRetVal) { > > 1. { in a new line. > 2. it should be private. > 3. it should return an error code. It cannot be private, because it's defined in the WebIDL file as 'readonly attribute DOMString TEMP_DIR;', so it's mapped in the glue. I made it throw aRv.Throw(rv) whenever rv is NS_FAILED. Should be enough right? > > @@ +130,5 @@ > > + // when something else, break and return false > > + if (errno != ENOENT) { > > + printf("VerifyRights for '%s' failed with %d (%s)\n", > > + aPath, errno, strerror(errno)); > > + free(aPath); > > no manual freee. So I changed that code and there's no free's anymore and I think it feels nice, but I have one question. I need the path in a char* because dirname wants to manipulate it. So I did the following: char* path = strdup(PromiseFlatCString(aPath).get()); I'm pretty sure I need to free this myself. Right? dirname() can manipulate it, so no way we can auto-track which bytes it uses. Please correct me if I'm wrong. Same goes for calls to f.e. realpath. It creates a string for me, how can I make Gecko do the memory management for me there? > @@ +155,5 @@ > > + } > > + > > + // not null? then time to check... real_path is the path we need to check > > + > > + nsString rp = NS_ConvertASCIItoUTF16(real_path); > > ASCII? what about if the filesystem supports unicode? I changed it to NS_ConvertUTF8toUTF16, is that enough? > @@ +175,5 @@ > > +bool > > +OsFileChannelParent::RecvInit(const int& aAppId) > > +{ > > + nsCOMPtr<nsIRunnable> event = new GetAllowedPaths(aAppId, &mAllowedPaths); > > + nsresult rv = NS_DispatchToMainThread(event, NS_DISPATCH_SYNC); > > I think you should get rid of this init and send the appId in the ctr of > this OSFileChannelParent. Check how we do the security check in > BackgroundChannel: > > http://mxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundParentImpl. > cpp#345 > > you should do something similar and in case kill the child process. I did this now, but it introduces timing errors. I think because the ctor is not marked as sync it does not properly block the thread until it returns (what it does with init). If you have any tips on it it might be interesting. > > @@ +600,5 @@ > > + } > > +} > > + > > +void > > +OsFileChannelParent::ActorDestroy(ActorDestroyReason aWhy) > > remove this. you are not actually using this method: you don't need to > implement it. No, if I omit it it doesn't compile anymore. > > ::: dom/os/test/remote.html > @@ +1,1 @@ > > +<html> > > Add headers in the test files. What kind of headers? We don't use headers in the test files in dom/inputmethod either. > ::: dom/webidl/WorkerNavigator.webidl > @@ +11,4 @@ > > WorkerNavigator implements NavigatorLanguage; > > WorkerNavigator implements NavigatorOnLine; > > WorkerNavigator implements NavigatorDataStore; > > +partial interface WorkerNavigator { > > Add a comment about this partial interface. This seems to be a mozilla > specific API. It needs comments and documentation. This pattern is really common (173 occurrences in WebIDL files). I added a comment that it's Mozilla specific, that seems to be enough for a lot of other IDL files. Let me know if you think we need more. > ::: dom/workers/Navigator.h > @@ +19,1 @@ > > > > forwarding class. > ?
Attached patch Patch v3 2nd part (obsolete) — — Splinter Review
Attachment #8622551 - Flags: feedback?(amarchesini)
Comment on attachment 8622551 [details] [diff] [review] Patch v3 2nd part Review of attachment 8622551 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Can you send me the whole patch all together? I promise I'll be quicker to give a feedback! ::: dom/apps/AppsUtils.jsm @@ +146,5 @@ > aApp.manifest.permissions['posix-files'] && > aApp.manifest.permissions['posix-files'].paths) || []; > os.forEach(function(path) { > + let wrapper = Cc["@mozilla.org/supports-string;1"].createInstance( > + Ci.nsISupportsString); what about: let wrapper = Cc["@mozilla.org/supports-string;1"]. createInstance(Ci.nsISupportsString); ::: dom/os/OsManager.cpp @@ +65,3 @@ > > +void > +OsManager::Init() Init(ErrorResult& aRv) @@ +70,2 @@ > uint32_t appId; > nsresult rv = principal->GetAppId(&appId); aRv = principal->GetAppId(&appId); if (NS_WARN_IF(aRv.Failed())) { return; } @@ +113,1 @@ > if (!ret) { NS_WARN_IF(... @@ +128,1 @@ > OsManager::Read(JSContext* aCx, File& aFile, int aBytes, int32_t ? @@ +129,5 @@ > JS::MutableHandle<JSObject*> aRet, ErrorResult& aRv) > { > + mWorkerPrivate->AssertIsOnWorkerThread(); > + > + nsAutoArrayPtr<char> buffer(new (fallible) char[aBytes + 1]); check for overflow. @@ +134,1 @@ > if (!buffer) { same here @@ +188,1 @@ > if (!ret) { NS_WARN_IF @@ +210,1 @@ > if (!ret) { same here and everywhere else. ::: dom/os/OsManager.h @@ +134,3 @@ > nsCOMPtr<nsIFile> tmpDir; > nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, > getter_AddRefs(tmpDir)); aRv = ... if (NS_WARN_IF(aRv.Failed()) { return; } ::: dom/os/ipc/OsFileChannelParent.cpp @@ +38,4 @@ > AssertIsOnBackgroundThread(); > } > > +class GetAllowedPaths : public nsRunnable final @@ +139,5 @@ > } > > // not null? then time to check... real_path is the path we need to check > > + nsString rp = NS_ConvertUTF8toUTF16(real_path); NS_ConvertUTF8toUTF16 rp(real_path); @@ +173,4 @@ > { > AssertIsOnBackgroundThread(); > > + auto path = NS_ConvertUTF16toUTF8(aPath); same here. @@ +198,4 @@ > > struct stat sb; > > + auto path = NS_ConvertUTF16toUTF8(aPath); and here. @@ +229,4 @@ > > struct stat sb; > > + auto path = NS_ConvertUTF16toUTF8(aPath); yeah, everywhere :) ::: dom/workers/Navigator.cpp @@ +405,5 @@ > WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate(); > MOZ_ASSERT(workerPrivate); > > + mOsManager = new os::OsManager(workerPrivate); > + mOsManager->Init(); mOsManager->Init(aRv);
Attachment #8622551 - Flags: feedback?(amarchesini)
Out of curiosity. Why would you prefer this? > NS_ConvertUTF8toUTF16 rp(real_path); over > auto path = NS_ConvertUTF16toUTF8(aPath); ?
Flags: needinfo?(amarchesini)
Because NS_ConvertUTF8toUTF16 is already a nsAString.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #79) > @@ +128,1 @@ > > OsManager::Read(JSContext* aCx, File& aFile, int aBytes, > > int32_t ? I changed all the functions in OsManager.h now to take and return int32_t's. Is that correct? Or just this function? > > @@ +129,5 @@ > > JS::MutableHandle<JSObject*> aRet, ErrorResult& aRv) > > { > > + mWorkerPrivate->AssertIsOnWorkerThread(); > > + > > + nsAutoArrayPtr<char> buffer(new (fallible) char[aBytes + 1]); > > check for overflow. I made it so that in the two cases where we do this we max. allocate 1MB and otherwise fail. Together with the check thereafter this should be sufficient I presume? Other comments are addressed
Attached patch bug1141021-v4.patch (obsolete) — — Splinter Review
Attachment #8600952 - Attachment is obsolete: true
Attachment #8622551 - Attachment is obsolete: true
Attachment #8600952 - Flags: feedback?(fabrice)
Attachment #8628275 - Flags: review?(amarchesini)
Stuff to do: - Only build on B2G - Put it behind a build flag? Or is the pref and the certified check good enough?
Comment on attachment 8628275 [details] [diff] [review] bug1141021-v4.patch Review of attachment 8628275 [details] [diff] [review]: ----------------------------------------------------------------- Can you submit an interdiff with this changes? Thanks! ::: dom/interfaces/apps/mozIApplication.idl @@ +8,4 @@ > #include "domstubs.idl" > > interface nsIPrincipal; > +interface nsIArray; move it before nsIPrincipal. ::: dom/os/File.h @@ +8,5 @@ > + > +#include "mozilla/dom/OsManagerBinding.h" > +#include "nsCycleCollectionParticipant.h" > +#include "nsWrapperCache.h" > +#include "OsManager.h" alphabetic order. @@ +22,5 @@ > +public: > + File(OsManager* aParent, int aFd) > + : mParent(aParent) > + , mFd(aFd) > + {} MOZ_ASSERT(aParent); ::: dom/os/OsManager.cpp @@ +56,5 @@ > + MOZ_ASSERT(aWorkerPrivate); > + aWorkerPrivate->AssertIsOnWorkerThread(); > + > + ipc::PBackgroundChild* backgroundChild = > + ipc::BackgroundChild::GetForCurrentThread(); MOZ_ASSERT(backgroundChild); @@ +99,5 @@ > + > + JSString* strErr = JS_NewStringCopyZ(cx, strerror(aErr)); > + JS::Rooted<JS::Value> valErr(cx, JS::StringValue(strErr)); > + aRv.ThrowJSException(cx, valErr); > + return; remove this "return;" @@ +142,5 @@ > + HandleErrno(errno, aRv); > + return; > + } > + > + JSObject* outView = Uint8Array::Create(aCx, bytes_read, JS::Rooted @@ +184,5 @@ > +{ > + mWorkerPrivate->AssertIsOnWorkerThread(); > + > + nsAutoPtr<StatWrapper> sw(new StatWrapper()); > + bool ret = mActor->SendLstat((nsString)aPath, sw); nsString(aPath) @@ +206,5 @@ > +{ > + mWorkerPrivate->AssertIsOnWorkerThread(); > + > + nsAutoPtr<StatWrapper> sw(new StatWrapper()); > + bool ret = mActor->SendStat((nsString)aPath, sw); nsString(aPath) @@ +244,5 @@ > +{ > + mWorkerPrivate->AssertIsOnWorkerThread(); > + > + int32_t rv; > + bool ret = mActor->SendChmod((nsString)aPath, aMode, &rv); ditto @@ +271,5 @@ > +{ > + mWorkerPrivate->AssertIsOnWorkerThread(); > + > + int32_t rv; > + bool ret = mActor->SendUnlink((nsString)aPath, &rv); ditto, here and anywhere else. ::: dom/os/OsManager.h @@ +29,5 @@ > +namespace os { > + > +using mozilla::dom::Date; > + > +typedef void (*PermissionsCallback)(OsManager* aOsManager, const nsTArray<nsString>& aResult); 80chars. @@ +41,5 @@ > + > + void Init(ErrorResult& aRv); > + void Shutdown(); > + > + virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; 80chars @@ +128,5 @@ > + int32_t IRWXU() const { return S_IRWXU; } > + int32_t IRWXG() const { return S_IRWXG; } > + int32_t IRWXO() const { return S_IRWXO; } > + > + void GetTEMP_DIR(nsString& aRetVal, ErrorResult& aRv) can it be GetTempDir? or GetTemporaryDirectory? ::: dom/os/Stat.h @@ +93,5 @@ > + } > + > + Date Atime() const > + { > + return Date((double)mStat.st_atime * 1000); static_cast<double>(.. here and in the rest of the patch. ::: dom/os/ipc/OsFileChannelParent.cpp @@ +40,5 @@ > + > +class GetAllowedPaths final : public nsRunnable > +{ > +public: > + GetAllowedPaths(const int aAppId, nsTArray<nsString>* aResult) nsTArray<nsString>& aResult @@ +43,5 @@ > +public: > + GetAllowedPaths(const int aAppId, nsTArray<nsString>* aResult) > + : mAppId(aAppId), mResult(aResult) {} > + > + NS_IMETHOD Run() { 1. ovverride 2. { in a new line @@ +72,5 @@ > + nsCOMPtr<nsIArray> osPaths; > + app->GetOsPaths(getter_AddRefs(osPaths)); > + > + uint32_t length; > + osPaths->GetLength(&length); I guess this can fail, in theory. @@ +76,5 @@ > + osPaths->GetLength(&length); > + > + for (uint32_t j = 0; j < length; ++j) { > + nsCOMPtr<nsISupportsString> iss = do_QueryElementAt(osPaths, j); > + if (!iss) {} @@ +80,5 @@ > + if (!iss) > + return NS_ERROR_FAILURE; > + > + nsAutoString path; > + iss->GetData(path); can this fail? @@ +82,5 @@ > + > + nsAutoString path; > + iss->GetData(path); > + > + if (path.Equals(NS_LITERAL_STRING("TEMPDIR"))) { EqualsLiteral @@ +83,5 @@ > + nsAutoString path; > + iss->GetData(path); > + > + if (path.Equals(NS_LITERAL_STRING("TEMPDIR"))) { > + nsString tmpDirPath; nsAutoString @@ +85,5 @@ > + > + if (path.Equals(NS_LITERAL_STRING("TEMPDIR"))) { > + nsString tmpDirPath; > + nsCOMPtr<nsIFile> tmpDir; > + nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, you already have a nsresult @@ +86,5 @@ > + if (path.Equals(NS_LITERAL_STRING("TEMPDIR"))) { > + nsString tmpDirPath; > + nsCOMPtr<nsIFile> tmpDir; > + nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, > + getter_AddRefs(tmpDir)); indentation @@ +87,5 @@ > + nsString tmpDirPath; > + nsCOMPtr<nsIFile> tmpDir; > + nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, > + getter_AddRefs(tmpDir)); > + if (NS_FAILED(rv)) { NS_WARN_IF @@ +90,5 @@ > + getter_AddRefs(tmpDir)); > + if (NS_FAILED(rv)) { > + continue; > + } > + tmpDir->GetPath(tmpDirPath); can this fail? @@ +94,5 @@ > + tmpDir->GetPath(tmpDirPath); > + mResult->AppendElement(tmpDirPath); > + } > + else { > + mResult->AppendElement(path); probably it's better t odo: if (!path.EqualsLiteral(...)) { mResult->AppendElement(path); continue; } ... @@ +103,5 @@ > + } > + > +private: > + const int mAppId; > + nsTArray<nsString>* mResult; nsTArray<nsString>& mResult; @@ +110,5 @@ > +bool > +OsFileChannelParent::VerifyRights(const nsACString& aPath) > +{ > + // use path while calling dirname > + char* path = strdup(PromiseFlatCString(aPath).get()); mmm no manual allocation. It seems that doing: char* path = aPath.BeginReading(); should work for you. does it? Or if you really need to take a copy do: char path[PATH_MAX]; MOZ_ASSERT(aPath.Length() < PATH_MAX); strcpy(path, aPath.BeginReading()); @@ +120,5 @@ > + // when ENOENT we check parent directory (and up and up, etc.) > + // when something else, break and return false > + if (errno != ENOENT) { > + NS_WARNING("VerifyRights failed"); > + return false; here you don't free 'path'. @@ +122,5 @@ > + if (errno != ENOENT) { > + NS_WARNING("VerifyRights failed"); > + return false; > + } > + extra space. @@ +123,5 @@ > + NS_WARNING("VerifyRights failed"); > + return false; > + } > + > + if (strlen(path) > 1 * 1024 * 1024) { 1 * ?!? :) @@ +124,5 @@ > + return false; > + } > + > + if (strlen(path) > 1 * 1024 * 1024) { > + return false; here too, no free path. @@ +130,5 @@ > + > + // dirname changes the pointer you feed into it so we need to copy it first > + nsAutoArrayPtr<char> old_path(new (fallible) char[strlen(path) + 1]); > + if (!old_path) { > + return false; //@todo: NS_ERROR_OUT_OF_MEMORY is todo managed by a follow-up? Add: NS_WARNING("A message... @@ +146,5 @@ > + > + NS_ConvertUTF8toUTF16 rp(real_path); > + uint32_t len = mAllowedPaths.Length(); > + for (uint32_t j = 0; j < len; j++) { > + const char* s = NS_ConvertUTF16toUTF8(mAllowedPaths[j]).get(); do it in 2 steps: NS_ConvertUTF16toUTF8 allowedPath(mAllowedPaths[j]); int res = rp.Find(allowedPath.BeginReading(), false, 0, -1); @@ +163,5 @@ > + AssertIsOnBackgroundThread(); > + > + nsCOMPtr<nsIRunnable> event = new GetAllowedPaths(aAppId, &mAllowedPaths); > + nsresult rv = NS_DispatchToMainThread(event, NS_DISPATCH_SYNC); > + mInitialized = true; well... I guess you want to do: if (NS_WARN_IF(NS_FAILED(rv))) { return false; } mInitialized = true; return true; @@ +186,5 @@ > + int fd; > + if (aPermission == 0) { // @todo make method overload? > + fd = open(path.get(), aAccess); > + } > + else { same line: } else { @@ +270,5 @@ > + > + if (unlink(path.get()) == -1) { > + *aRetVal = errno; > + } > + else { } else { here and everywhere @@ +537,5 @@ > + } > + > + int buffer_size = 255; > + > + while (buffer_size < 1 * 1024 * 1024) { // 1MB remove '1 *' ::: dom/os/test/remote.html @@ +7,5 @@ > + var file = navigator.os.open("../../../../dom/os/test/payload.txt", > + navigator.os.RDONLY, navigator.os.IREAD); > + var data = navigator.os.read(file, 100); > + navigator.os.close(file); > + extra spaces @@ +17,5 @@ > + catch (ex) { > + postMessage("open err: " + ex); > + } > + `; > + extra spaces ::: dom/os/test/shim_app_as_test.js @@ +44,5 @@ > + var gManifestURL; > + var gApp; > + var gOptions; > + > + // Load the chrome script. why this 2 extra spaces everywhere in this file?
Attachment #8628275 - Flags: review?(amarchesini) → review-
(In reply to Andrea Marchesini (:baku) from comment #85) > Comment on attachment 8628275 [details] [diff] [review] > bug1141021-v4.patch > > +#include "mozilla/dom/OsManagerBinding.h" > > +#include "nsCycleCollectionParticipant.h" > > +#include "nsWrapperCache.h" > > +#include "OsManager.h" > > alphabetic order. This is in alphabetic order already. Or do we value uppercase higher than lowercase? > @@ +128,5 @@ > > + int32_t IRWXU() const { return S_IRWXU; } > > + int32_t IRWXG() const { return S_IRWXG; } > > + int32_t IRWXO() const { return S_IRWXO; } > > + > > + void GetTEMP_DIR(nsString& aRetVal, ErrorResult& aRv) > > can it be GetTempDir? or GetTemporaryDirectory? No, the file the WebIDL generates expects it to be called |GetMember|, and the member name in WebIDL is TEMP_DIR. I don't think I can override that anywhere. > @@ +82,5 @@ > > + > > + nsAutoString path; > > + iss->GetData(path); > > + > > + if (path.Equals(NS_LITERAL_STRING("TEMPDIR"))) { > > EqualsLiteral According to https://developer.mozilla.org/en-US/docs/nsAutoString#EqualsLiteral%28const_char, EqualsLiteral expects a const char*, but https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings states that the return value of NS_LITERAL_STRING is a nsString. So I'd guess that Equals would be correct here... > @@ +110,5 @@ > > +bool > > +OsFileChannelParent::VerifyRights(const nsACString& aPath) > > +{ > > + // use path while calling dirname > > + char* path = strdup(PromiseFlatCString(aPath).get()); > > mmm no manual allocation. > > It seems that doing: > > char* path = aPath.BeginReading(); > > should work for you. does it? Or if you really need to take a copy do: > > char path[PATH_MAX]; > MOZ_ASSERT(aPath.Length() < PATH_MAX); > strcpy(path, aPath.BeginReading()); I think there's no way around manual allocation here, because later on I'm also using dirname on it (also why I need a copy), which can resize the string at will. So no way of tracking without manually free'ing I think. > > @@ +123,5 @@ > > + NS_WARNING("VerifyRights failed"); > > + return false; > > + } > > + > > + if (strlen(path) > 1 * 1024 * 1024) { > > 1 * ?!? :) At least to me it's instantly clear that it's 1 MB this way. :-) > @@ +130,5 @@ > > + > > + // dirname changes the pointer you feed into it so we need to copy it first > > + nsAutoArrayPtr<char> old_path(new (fallible) char[strlen(path) + 1]); > > + if (!old_path) { > > + return false; //@todo: NS_ERROR_OUT_OF_MEMORY > > is todo managed by a follow-up? > Add: NS_WARNING("A message... I wanted to change the signature of this function to return nsresult at some point, but I've left it bool. Added a warning and removed the todo. > ::: dom/os/test/shim_app_as_test.js > @@ +44,5 @@ > > + var gManifestURL; > > + var gApp; > > + var gOptions; > > + > > + // Load the chrome script. > > why this 2 extra spaces everywhere in this file? I copied this over from another place, didn't change anything. Removed trailing spaces.
Attached patch bug1141021-v5.patch (obsolete) — — Splinter Review
Hi, this is the diff on top of v4.
Attachment #8639871 - Flags: review?(amarchesini)
ping
Flags: needinfo?(amarchesini)
Comment on attachment 8639871 [details] [diff] [review] bug1141021-v5.patch Review of attachment 8639871 [details] [diff] [review]: ----------------------------------------------------------------- looks good. Thanks for the ping, I completely forgot, sorry. If you can implement this RAII and submit the full patch again, I'll give my last stamp for this week! ::: dom/os/ipc/OsFileChannelParent.cpp @@ +132,4 @@ > // when something else, break and return false > if (errno != ENOENT) { > NS_WARNING("VerifyRights failed"); > + free(path); can you implement a RAII class for this in order to avoid the manual free?
Attachment #8639871 - Flags: review?(amarchesini)
Flags: needinfo?(amarchesini)
any news on this?
Flags: needinfo?(janjongboom)
Resurrecting this bug. This changes VerifyRights to use RAII class.
Flags: needinfo?(janjongboom)
Attachment #8675624 - Flags: review?(amarchesini)
Full patch, rebased against mc, including the add-raii part.
Attachment #8628275 - Attachment is obsolete: true
Attachment #8639871 - Attachment is obsolete: true
This makes the patch run on b2g, but I need to read the posix-files stuff in AppsUtils, and I can't manage to get this to work. Do you have any input?
Attachment #8675807 - Flags: feedback?(fabrice)
Comment on attachment 8675624 [details] [diff] [review] bug1141021-add-raii.diff Review of attachment 8675624 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/os/ipc/OsFileChannelParent.cpp @@ +121,5 @@ > + > +/** > + * A class that automatically free's path when it goes out of scope. > + */ > +class MOZ_RAII AutoFreePath { class MOZ_RAII AutoFreePath final {
Attachment #8675624 - Flags: review?(amarchesini) → review+
Comment on attachment 8675807 [details] [diff] [review] bug1141021-b2g.patch Review of attachment 8675807 [details] [diff] [review]: ----------------------------------------------------------------- I don't have time right now to look at that. Try with one of gsvelto/tzimmerman/dhylands I still don't like the dom/os directory though...
Attachment #8675807 - Flags: feedback?(fabrice)
Comment on attachment 8675807 [details] [diff] [review] bug1141021-b2g.patch Hi, do you have time to look into this?
Attachment #8675807 - Flags: feedback?(gsvelto)
I'll have a look ASAP, possibly right after the 2.5 RA (this Friday). Sorry for not being able to get back at you earlier but the last few days before the branching are hectic.
OK, I'm finally able to work on this because a) I have the time and b) this is being prioritized (yay!). Jan, can you describe what problem you were encountering? I've read your changes but I'm not sure I understand what the problem is...
Flags: needinfo?(janjongboom)
Hi Gabriele, the issue is that I add a new section in manifest file that lists the available paths for the application. This works fine in the tests, but when I install the application in the phone this list is not copied to the installed manifest which is read in AppsUtils. But from there I need the list of paths to pass it into OsManager. I can't get that to work, would need someone who has touched manifests before to help me. But regardless, I'm no longer interested in championing this bug. If you can find someone who is, I'm happy to transfer some knowledge and general ideas, but I won't be writing any more code.
Flags: needinfo?(janjongboom) → needinfo?(gsvelto)
OK, thanks Jan. I'll see if I can move this forward myself.
Comment on attachment 8675807 [details] [diff] [review] bug1141021-b2g.patch OK, clearing flags while I figure out how to proceed here.
Flags: needinfo?(gsvelto)
Attachment #8675807 - Flags: feedback?(gsvelto)
Priority: -- → P5
Component: DOM → DOM: Core & HTML
Severity: normal → S3

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: janjongboom → nobody

That sounds like some concurrent/equivalent project for file system ? Is this still a thing or superseded by newer spec evolution?

Flags: needinfo?(rjesup)

No, this is something different than the FileSystem API/OPFS/etc. See comment 99 (and some of the early comments). However, given that the original patch writer has abandoned it (after a lot of work), I don't see a way/reason to go forward with this.

Flags: needinfo?(rjesup)

Yeah, this is unrelated to any recent spec work and has no path forward.

Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: