The basic idea is to offer privileged access to a special storage area on device. But instead of a general file system API, offer a simpler interface to enumerate available content. Gecko would store some content like pictures in standard formats so that external tools could recognize them as such. Jonas has some ideas for the actual API. This overlaps with intents, but seems like a simpler evolutionary step.

This sounds strange to me. Either we should have some real File system API (not necessarily the current draft from W3C but maybe something better) or we don't have such API at all and rely on IndexedDB. (The IndexedDB implementation could in the background handle files so that external tools can recognize them.)

(In reply to Olli Pettay [:smaug] from comment #1) > This sounds strange to me. Either we should have some real File system API > (not necessarily the > current draft from W3C but maybe something better) See the discussion at http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/383183a56f11b3a3/bad7933120422ca7?lnk=gst&q=gallery+api#bad7933120422ca7 , particularly the last few comments. Briefly, the arguments for this are - evolutionary path to implementation built on more general mechanisms (IndexedDB/intents) - much friendlier to authors than file system API Also, with a general file-system API we can't rely on authors to respect standard media naming+layout conventions, which would be needed to interface with external tools. > or we don't have such API at all and > rely on IndexedDB. (The IndexedDB implementation could in the background > handle files > so that external tools can recognize them.) Can you elaborate on how this would work?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2) > - much friendlier to authors than file system API Yes, please! > Can you elaborate on how this would work? Maybe something like having pre-defined databases where files can be stored, but the implementation stores the files actually in some special directory for media files. But API would be just IndexedDB

OK. I don't know the IDB API well enough to comment on that wrt authors. But it seems that like that gives us equivalent functionality with a smaller set of new APIs, so I'm tentatively in favor.

I had a similar idea in bug 704128 comment 4. I think we should create an API that lets external applications read files stored in the IndexedDB (maybe with http://webintents.org/pick ?) So the web applications (for example a b2g application) will simply write in an IndexedDB and the external applications (for example a synchronization software) could read these files. The problem is if an external application wants to write something (and this should be feasible, for example for the synchronization software), because this could cause security concerns. I think we should create a more general web api to allow this, not specifically tied to some types of files.

AFAICT all of the W3C file APIs and IndexedDB are sandboxed to a single origin. Olli: Are you suggesting we could have either special IndexedDB databases which could be shared between different origins or a version of the File API: Directories and System spec without the concept of a folder structure and with file stores which can be shared between origins? Either works for me, but as I understand it there's currently no persistence mechanism dedicated to files in gecko, despite having FileReader and FileWriter so maybe we are missing a piece. Obviously shared stores of either kind should only be accessible by apps with a special user-granted permission.

IndexedDB can store files in Gecko.

I'm just saying we shouldn't create specialized APIs _if_ common/generic API could work just fine. It is not clear to me how to handle origin, but that would be a problem in any case, whatever API is used.

(In reply to Olli Pettay [:smaug] from comment #8) > I'm just saying we shouldn't create specialized APIs _if_ common/generic > API could work > just fine. > > It is not clear to me how to handle origin, but that would be a problem in > any case, whatever > API is used. With IndexedDB you can store files, but external applications can't access to them. Instead the FileSystem API stores files in an actual directory of the system, so an external application can simply read or modify the files or even create new ones. This "lack" of a way to share data between IndexedDB and other applications is what we should solve. We could either create a new API completely different from IndexedDB, or, and I think it's better, we could create an API that extends IndexedDB functionalities.

This is a privileged API. Multiple origins can share the same DBs. There's a problem with cross-process race conditions, but we have the same problem with file system access. The proposal around IDB was to have some magic hooks for blobs stored in media DBs that put them in the right place, AIUI. So storing a named blob in imagedb would write the file to /sdcard/DCIM on my phone, and maybe ~/pictures on my desktop. I just realized the problem with this proposal is that it's less clear how it would interact with external applications *storing* data to the special media store. For example, on my phone, my camera app can store to a media DB, and some magic hooks write the files to /sdcard/DCIM/[blah]. No problem. *But*: what happens if I plug in my phone, and my host computer stores files to there? Our IDB backend doesn't know about external writes (short of inotify magic). Similarly on desktop, for ~/pictures. So I think I'm back to being in favor of a dedicated API.

Unless someone can come up with a proposal for how to automatically rebuild the indexes in the DBs.

I think I'm right in saying that the use cases which triggered this discussion are: 1. Sharing media files between apps. 2. Moving media files on and off the device. Jonas has suggested a possible API which has specific types for picture, audio, movie and music files so that there's a pre-defined location for these types of media, which can be accessed from multiple apps given certain permissions. We have also discussed that an implementation of this API could store files on the file system of the device using the DCIM standard, so if that file system is mounted as USB MSC, existing software on a USB host could recognise those photos and transfer them. However... As has already been mentioned, there's a possibility that Web Intents can fulfill use case 1, although there are issues about sharing of files in cases where the action is not directly initiated by the user (e.g. for synchronisation). This is something we can explore through prototyping intents. For use case 2, perhaps a better approach is a JavaScript implementation of the Media Transfer Protocol (http://en.wikipedia.org/wiki/Media_Transfer_Protocol), which can be written against the WebUSB & WebBluetooth APIs as required. This is assuming that the WebUSB API allows the user agent to act as both a USB host and a USB client, which I have asked about in the relevant bug. If we were to go ahead with a new API for local file storage, I think I would advocate something more generic than a pre-defined list of media types, but simpler than a directory structure. Perhaps something like Amazon's S3 service with the concept of "buckets". Then allow apps to create a bucket and store files indexed by a name (which can be any string). I have to admit this doesn't offer much that IndexedDB doesn't do, except maybe that it could be more optimised. If we still feel there is a need for a storage medium which can be shared between apps we could argue to either extend this new API or the IndexedDB API to allow a list of origins which can access a bucket, database or object store - rather than only allowing one origin.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10) > I just realized the problem with this proposal is that it's less clear how > it would interact with external applications *storing* data to the special > media store. For example, on my phone, my camera app can store to a media > DB, and some magic hooks write the files to /sdcard/DCIM/[blah]. No > problem. *But*: what happens if I plug in my phone, and my host computer > stores files to there? Our IDB backend doesn't know about external writes > (short of inotify magic). Similarly on desktop, for ~/pictures. I still agree that this is a problem if we want to store files in a real file system on a memory card that can be mounted by a host using USB MSC. Do you think that a Gallery app and a Media Player app could each act as an MTP server (with the help of a library I'm sure Andreas would call MTP.js!), so that an external host can write files to the app's IndexedDB database via a USB or Bluetooth interface?

For the immediate future, I think mediastorage-on-filesystem is our shortest path to victory. We don't have webusb support yet, probably won't in the demo-phone timeline. When we have richer support for intents and usb, we should reinvestigate.

I don't disagree, but do you think we need mediastorage-on-filesystem for the demo milestone? It now doesn't seem like we'll be using the camera at all in the demo and the gallery and media player could just use pre-loaded media in IndexedDB?

Not clear yet. I do think we should walk with mediastorage before running with something else. MediaStorage has the advantage of integrating well with current shipping versions of browsers, so it's an api we probably want to support in the long run.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11) > Unless someone can come up with a proposal for how to automatically rebuild > the indexes in the DBs. inotify(7) provided by Linux kernel can help us to monitor directories and files. We can write a daemon to monitor DCIM directory and update IndexedDB. We can also use libkqueue to provide a cross-platform API.

Isn't the goal of b2g to avoid relying on non-web based component, and create new APIs instead ? We could create a file system notification API for the web content, but I fail to see a lot of use cases.

Aww. That was a bad idea to post this message in the bug *and* in dev-webapi [1]: the discussion is happening in two places and that's clearly a bad idea. Could you guys reply to the dev-webapi thread instead of in this bug? [1] https://www.mozilla.org/about/forums/#dev-webapi

Nothing requires mediastorage to be implemented with a local file system. It's just an API. No one is discussing inotify for the web.

assigned to me for sec action to schedule a meeting

Please use these instructions to schedule https://wiki.mozilla.org/Security/Reviews/Review_Request_Form

couple things I know about: 1) full permission granted. 2) leaky - not completely hooked up to the cc 3) only two directory locations supported (temp and mac picture directory) 4) file writer not supported (*Editable) 5) overwrite not implemented. Maybe we shouldn't or don't need to. We can discuss. 6) need to figure out what to do about the naming convention when user doesn't give us a file name. 7) Gregor is investigating b2g implementation. DCF / DCIM should be very simple.

Doug, is this implementation e10s ready? We'll need that for b2g.

8) E10s readiness needs to be investigated.

fixes the threadsafe isupports issues similar to what was done for idb

When ready for security review please answer the questions in bug 744542 comment 0

no leaks preference guards usage (must set devicestorage.enabled to true)

sets enable preference before testing.

1) getDeviceStorage() returns an array of device storage objects. This is require because there may be multiple 'picture directories'. For example, one picture directory may be at /sdcard/DCIM and other would be at /data/pictures. 2) allow device storage objects to assert what kind they are "external", "default", "shared". This allos app developers to determine if a directory is mounted on an sdcard, for example,

I haven't been following the latest developments here, sorry, but two questions - if I'm developing a music application, which "storage area" should I open to find music files? - if I connect my phone to my host computer through USB, how will I download images I've taken with the camera?

> if I'm developing a music application, which "storage area" should I open to find music files? You'd probably do something like: var storage = navigator.getDeviceStorage("music"); var cursor = storage[0].enumerate(prefix); cursor.onsuccess = enumerator; cursor.onerror = handleError; function enumerator(e) { var filename = e.target.result.name; if (! e.target.done) { e.target.continue(); } } You'll note that storage will be an array of DeviceStorage objects. This is to support the case where there may be multiple locations for a give time -- for example pictures. Pictures may be stored on the SDCard (DCIM), or in on 'internal' storage. > if I connect my phone to my host computer through USB, how will I download images I've taken with the camera? This is somewhat underneath the API. What David and I discussed was to add a flag to the DeviceStorage object to express that the storage is on the SD card. The implementation will put the pictures under the DCIM hierachy. var storage = navigator.getDeviceStorage("pictures"); for loop if (storage[i].type == "external") do something

The easiest way to try this out is to look here: http://hg.mozilla.org/users/dougt_mozilla.com/device_storage_pq/

This allows us to cleanup files in case there is anything left on disk after a mochitest run.

(In reply to Doug Turner (:dougt) from comment #43) > > if I'm developing a music application, which "storage area" should I open to find music files? > > You'd probably do something like: > > var storage = navigator.getDeviceStorage("music"); Why "music"? > > if I connect my phone to my host computer through USB, how will I download images I've taken with the camera? > > This is somewhat underneath the API. Not at all! That was a large part of the motivation for this API! > What David and I discussed was to add > a flag to the DeviceStorage object to express that the storage is on the SD > card. What do SD cards have to do with this? > The implementation will put the pictures under the DCIM hierachy. > > var storage = navigator.getDeviceStorage("pictures"); > Why "pictures"? > for loop > if (storage[i].type == "external") > do something I don't quite understand ... what entity is executing this code?

> Why "music"? "music" in my example was just a key. There will be a set number of key locations (like "music" and "pictures". Anything else passed to getDeviceStorage will return undefined.

Comment on attachment 620128 [details] [diff] [review] rollup v.1 bent told me this afternoon that jonas is going to review.

Comment on attachment 620128 [details] [diff] [review] rollup v.1 Review of attachment 620128 [details] [diff] [review]: ----------------------------------------------------------------- Still reviewing. I'm currently at WriteFileEvent, but I wanted to submit what I have so far. ::: dom/base/Navigator.cpp @@ +895,5 @@ > + } > + > + nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow)); > + > + if (!win || !win->GetOuterWindow() || !win->GetDocShell()) { Just change the GetOuterWindow check to an NS_ASSERTION. mWindow should never be able to be an outer window. ::: dom/devicestorage/nsDeviceStorage.cpp @@ +46,5 @@ > + > + child->AppendRelativePath(aName); > + > + parent->Normalize(); > + child->Normalize(); You're not actually using parent and child anywhere. Creating them seems like dead code. @@ +53,5 @@ > + > + nsAString::const_iterator start, end; > + aName.BeginReading(start); > + aName.EndReading(end); > + NS_NAMED_LITERAL_STRING(dotdot, "../"); I'm worried about what this would do to someone trying to open simply the path ".." I'm also worried that some systems would honor "~" in the path. I also think emacs let's you type paths like "foo/bar//usr/bin" which is treated as equivalent as "/usr/bin", but that might be an emacs feature. I also believe that windows treats "\" as equivalent to "/". So maybe do something like: 1. check for "\" and "~". If the path contains either of those return false 2. split on "/", if any of the resulting tokens are either "", "." or "..", return false @@ +80,5 @@ > + dirService->Get(NS_OS_TEMP_DIR, NS_GET_IID(nsILocalFile), getter_AddRefs(f)); > + } > + // Profile directory > + else if (aType.Equals(NS_LITERAL_STRING("profile")) && aIndex == 0) { > + dirService->Get(NS_APP_USER_PROFILE_50_DIR, NS_GET_IID(nsILocalFile), getter_AddRefs(f)); This one and temp seems scary to grant access to. The profile directory seems likely that it can contain things like passwords, browser cookies and other very sensitive information. The temp directory probably won't contain as sensitive information, however the value to the website seems low since data there can get randomly nuked, so the risk/value ratio is bad. In both cases it'll be very hard to create UI which explains to the user what is granted access to. So I think we should disable these two at least for now. @@ +97,5 @@ > +#if defined (MOZ_WIDGET_COCOA) > + if (aIndex == 0) { > + dirService->Get(NS_OSX_PICTURE_DOCUMENTS_DIR, NS_GET_IID(nsILocalFile), getter_AddRefs(f)); > + } > +#elif defined (XP_UNIX) I take it we don't have a constant for windows? :( @@ +104,5 @@ > + } > +#endif > + } > + > + NS_IF_ADDREF(*aFile = f.get()); f.forget(aFile); will save you an addref/release. @@ +124,5 @@ > + } > + > + nsCOMPtr<nsIDOMBlob> blob = new nsDOMFileFile(aFile); > + > + if (aWindow == nsnull) { Can't we just assert that. Seems like an implementation bug if this ever happens. @@ +206,5 @@ > + mError = NS_ConvertASCIItoUTF16(aMessage); > + mError.Append(NS_LITERAL_STRING(" Type = ")); > + mError.Append(aType); > + mError.Append(NS_LITERAL_STRING(" Directory = ")); > + mError.Append(aDirectory); This ends up as the .name on the DOMError, so we should use something that's more standardized. Elsewhere we use "UnknownError" for IO errors so I think we should use that here too. It would be awesome if we could subclass DOMError and have a secondary property which contains an error message which contains the type/directory though. (that's the purpose of DOMError, to be able to subclass and add further properties as needed). Or if you prefer we can keep this temporarily while debugging for now if you want, but if so please file a followup. @@ +218,5 @@ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); > + NS_ASSERTION(rs, "Must have request service"); > + rs->FireError(mRequest, mError); Just call mRequest->fireError directly instead. The request service is there just for scriptable access to firing results. @@ +271,5 @@ > + NS_IMETHOD Run() { > + NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!"); > + > + nsCOMPtr<nsILocalFile> file; > + nsDOMDeviceStorage::GetFileForLocation(mType, mIndex, getter_AddRefs(file)); I don't think you're allowed to call the directory service off the main thread :( (It's addref/release is marked threadsafe, but the implementation isn't). @@ +282,5 @@ > + } > + > + bool check = false; > + if (mName.Length() > 0) { > + file->AppendRelativePath(mName); Has this been checked for ".." etc? @@ +312,5 @@ > + NS_DispatchToMainThread(event); > + return NS_OK; > + } > + > + nsCOMPtr<nsIDirectoryEnumerator> files = do_QueryInterface(e); This won't do a deep enumeration, will it? The intent was for the enumerate() functions to do a deep enumeration (of files only) by default. @@ +323,5 @@ > + nsCOMPtr<nsILocalFile> lf = do_QueryInterface(f); > + cursor->mFiles.AppendElement(lf); > + } > + > + nsCOMPtr<ContinueCursorEvent> event = new ContinueCursorEvent(mRequest); Hmm.. is there anything preventing the page from calling continue() on the cursor before we dispatch this event? It seems that doing so would cause the code to get confused. We should probably make this runnable call some internal function instead @@ +380,5 @@ > + > +NS_IMETHODIMP > +nsDOMDeviceStorageCursor::GetUri(nsIURI * *aRequestingURI) > +{ > + NS_ENSURE_ARG_POINTER(aRequestingURI); Remove these null-checks. If null is ever passed in here we have a really bad bug somewhere and *should* crash. Same for other functions below. @@ +432,5 @@ > +NS_IMETHODIMP > +nsDOMDeviceStorageCursor::Continue() > +{ > + if (!mAllowed) { > + nsCOMPtr<nsIContentPermissionPrompt> prompt = do_GetService(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID); Won't this mean that we'll reprompt every time the page calls .continue() on the cursor? I think after we've thrown an error the cursor should be invalidated and calling .continue() should throw an exception. What we do in the IndexedDB spec (and our impl of course) is that we only allow .continue() to be called if there currently is a result available. If you call while a result is currently being loaded (including the first result), after an error, or after the cursor is done iterating, .continue() simply throws a InvalidStateError exception. I think we should do the same here, that would also take care of the problem described above. @@ +439,5 @@ > + } > + return NS_OK; > + } > + > + if (!isSafePath(mType, mIndex, mName)) { Shouldn't we check this the first thing we do rather than put up a permission prompt? @@ +502,5 @@ > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); > + NS_ASSERTION(rs, "Must have request service"); > + if (mFile) { > + jsval result = nsIFileToJsval(mDOMRequest->GetOwner(), mFile, mEditable); > + rs->FireSuccess(mDOMRequest, result); Just call FireSuccess on the request directly @@ +542,5 @@ > + { > + NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!"); > + > + //TODO - this might be faster if we check to see if > + //the blob is a nsIDOMFile, and if so, then just do For what it's worth, both nsIDOMFile and nsIDOMBlob can be backed by OS-files @@ +546,5 @@ > + //the blob is a nsIDOMFile, and if so, then just do > + //a copy() > + > + nsCOMPtr<nsILocalFile> file; > + nsDOMDeviceStorage::GetFileForLocation(mType, mIndex, getter_AddRefs(file)); Same here, can't use directory service off the main thread.

Comment on attachment 620128 [details] [diff] [review] rollup v.1 Review of attachment 620128 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Navigator.cpp @@ +895,5 @@ > + } > + > + nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow)); > + > + if (!win || !win->GetOuterWindow() || !win->GetDocShell()) { jonas, there are 3 other places in this file that do exactly the same check. If we are to change these to assertions, lets do all of them in a different followup. Should I file? ::: dom/devicestorage/nsDeviceStorage.cpp @@ +46,5 @@ > + > + child->AppendRelativePath(aName); > + > + parent->Normalize(); > + child->Normalize(); Yup. I'll fix up. @@ +80,5 @@ > + dirService->Get(NS_OS_TEMP_DIR, NS_GET_IID(nsILocalFile), getter_AddRefs(f)); > + } > + // Profile directory > + else if (aType.Equals(NS_LITERAL_STRING("profile")) && aIndex == 0) { > + dirService->Get(NS_APP_USER_PROFILE_50_DIR, NS_GET_IID(nsILocalFile), getter_AddRefs(f)); Agreed. However, for testing i am going to leave profile. It will only be available if a special preference is set. The reason for this is that the machines that run mochitest automatically clean and regenerate the profile directory. If we leave any files on disk, they are removed for us. @@ +97,5 @@ > +#if defined (MOZ_WIDGET_COCOA) > + if (aIndex == 0) { > + dirService->Get(NS_OSX_PICTURE_DOCUMENTS_DIR, NS_GET_IID(nsILocalFile), getter_AddRefs(f)); > + } > +#elif defined (XP_UNIX) Nope. I can (or one of the windows platform developers) can do this in a followup. @@ +312,5 @@ > + NS_DispatchToMainThread(event); > + return NS_OK; > + } > + > + nsCOMPtr<nsIDirectoryEnumerator> files = do_QueryInterface(e); a deep enumeration is a bad idea. (think of the case where the user have 50k or more pictures). What you want is to see directories and files. If you see a directory, you can enumerate down on that. Lets discuss this. @@ +323,5 @@ > + nsCOMPtr<nsILocalFile> lf = do_QueryInterface(f); > + cursor->mFiles.AppendElement(lf); > + } > + > + nsCOMPtr<ContinueCursorEvent> event = new ContinueCursorEvent(mRequest); There is a test on line 474 which prevents continue() from doing anything useful until the init event completes. If timing is just right, a dev could cause a problem. I think we can remove the continue event from here, actually. @@ +432,5 @@ > +NS_IMETHODIMP > +nsDOMDeviceStorageCursor::Continue() > +{ > + if (!mAllowed) { > + nsCOMPtr<nsIContentPermissionPrompt> prompt = do_GetService(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID); I tend to think throwing exceptions is a very bad thing. Do you find that you need lots of try/catches when using indexeddb? As far as the reprompting -- that is up to the person implementing the prompt (same for geolocation and others) For every new cursor option we create, we post a message to the UI/Firefox and ask "Is it okay to do this?". We pass the requesting window and the URL. @@ +439,5 @@ > + } > + return NS_OK; > + } > + > + if (!isSafePath(mType, mIndex, mName)) { yes.

this doesn't address the xpcom directory service non-threadsafety. I'll investigate how to address that next.

On using the directory service on a non-main thread: 1) if it isn't threadsafe, isupports shouldn't be threadsafe. blames to seth from 12 years ago... 2) this new code is only doing gets on properties that are set in nsDirectoryService directly (not through providers. I claim that is it pretty damn safe to use the directory service in the manner that I am using it. It could be racy, but you'd have to be doing really wrong stuff -- like installing a directory provider that frequently changes the location of the Pictures directory. If you don't like this, there are three options - all are shit: 1) on the main thread, cache all directories that we care about. Right now that is at max 3. 2) on demand, proxy sync back to the main thread, and get the nsIFile. 3) fix up the directory service to really be thread safe by introducing locking (3) is the right approach, but that is going to be some work for little payoff. Thoughts?

Let's chat regarding the recursive traversal. To be clear, I agree we need non-deep *as well*, however I actually think that for things like user data, applications will often want to traverse recursively. For example if you have all your photos for your mexico vacation in a folder named "Mexico 2012" and you want to display all pictures in that folder, you want to do a recursive traverse. I'll see if I can get out on IRC. The firewall here is getting in the way. As for IndexedDB, we only throw if there are clear bugs in the code. Basically we throw if you do either: var cursor = storage.enumerate(...); cursor.onsuccess = ...; cursor.continue(); // The continue call should be inside the success handler or if you do var cursor = storage.enumerate(...); cursor.onsuccess = function(event) { doStuffWith(cursor.result); cursor.continue(); cursor.continue(); } In neither case a try/catch "is needed". You simply have to remove the erroneous code. As far as I can tell, if someone does the first example above with the current storage cursor you first get a asynchronous call saying that the cursor was empty. Then once the InitCursorEvent finishes you get another success call providing the first file but with .done still returning true.

Here's what Bent said regarding directory service: Apparently it has a hash which is used as a cache for directories that someone has asked about. This means that if you are asking for a directory on a non-main-thread at the same time as someone is asking for a directory which hasn't previously been cached, then the main thread will be modifying the hash table while your thread is reading it. That's not safe and can result in crashes. We can't really make the directory service threadsafe since it apparently calls out in to JS sometimes if it doesn't have a cached answer. We can't call out into JS off the main thread. I'd say we should go with solution 1 of caching the folders we care about.

wtf.

doh! w.f.m,

Comment on attachment 622872 [details] [diff] [review] rollup v.2 Temporarily removing the obsolete flag on this so I can keep reviewing. Stupid splinter review tool

Comment on attachment 622872 [details] [diff] [review] rollup v.2 Review of attachment 622872 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/devicestorage/nsDeviceStorage.cpp @@ +415,5 @@ > +nsDOMDeviceStorageCursor::Allow() > +{ > + mAllowed = true; > + // TODO - maybe post this back to the event loop. > + Continue(); Rather than delegating through continue(), why not simply create and dispatch the InitCursorEvent from here and remove that code from continue()? @@ +554,5 @@ > + return NS_OK; > + } > + > + if (mName.Length() <= 0) { > + // TODO - no name given, so give up for now. File a followup? @@ +581,5 @@ > + nsCOMPtr<nsIInputStream> stream; > + mBlob->GetInternalStream(getter_AddRefs(stream)); > + > + if (!stream) { > + nsCOMPtr<PostErrorEvent> event = new PostErrorEvent(mDOMRequest, Hmm.. it might be worth refactoring this so that Run calls a method which has all this code. Have that method simply return an error code, and then let Run() be responsible for firing the error/success event as needed. Up to you. @@ +582,5 @@ > + mBlob->GetInternalStream(getter_AddRefs(stream)); > + > + if (!stream) { > + nsCOMPtr<PostErrorEvent> event = new PostErrorEvent(mDOMRequest, > + POST_ERROR_EVENT_UNKNOWN_FILE_LOCATION, I would say this is POST_ERROR_EVENT_UNKNOWN. Basically GetInternalStream should never return null. @@ +781,5 @@ > + Allow(); > + } > + else > + { > + nsCOMPtr<nsIContentPermissionPrompt> prompt = do_GetService(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID); Hmm.. I just realized that I don't think we can simply hide this behind a user prompt (sorry, i thought the patch did something else). Basically that's too little protection for the user getting all of his/her pictures deleted by any random website. We're still debating what the security model should be, but until we figure it out, can we require a manual pref plus a prompt? Or will we not actually put up a prompt until someone implements the front-end part of this? @@ +832,5 @@ > + > + switch(mRequestType) { > + case DEVICE_STORAGE_REQUEST_WRITE: > + { > + r = new WriteFileEvent(mBlob, mType, mIndex, mName, mEditable, mRequest); WriteFileEvent and DeleteFileEvent doesn't need to take an mEditable flag, does it? @@ +877,5 @@ > + > +NS_IMPL_CYCLE_COLLECTING_ADDREF(DeviceStorageRequest) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(DeviceStorageRequest) > +NS_IMPL_CYCLE_COLLECTION_CLASS(DeviceStorageRequest) > +NS_IMPL_CYCLE_COLLECTION_UNLINK_0(DeviceStorageRequest) Why do you not need to unlink here? @@ +915,5 @@ > + return NS_ERROR_NOT_AVAILABLE; > + } > + > + if (!aContentDom) { > + return NS_ERROR_FAILURE; How can this ever happen? @@ +919,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + mOwner = do_GetWeakReference(aContentDom); > + if (!mOwner) { This never fail since we have infallible malloc. I guess that's technically not part of the contract here though so feel free to leave this in if you prefer. @@ +925,5 @@ > + } > + > + // Grab the uri of the document > + nsCOMPtr<nsIDOMDocument> domdoc; > + aContentDom->GetDocument(getter_AddRefs(domdoc)); If you make this function take an nsPIDOMWindow instead (which is what you have at the call-site) you can call do_QueryInterface(aWindow->GetExtantDocument()) instead. so silly that we don't expose an nsIDocument getter though :( @@ +944,5 @@ > +nsDOMDeviceStorage::CreateDeviceStoragesFor(nsPIDOMWindow* aWin, > + const nsAString &aType, > + nsIVariant** _retval) > +{ > + nsTArray<nsRefPtr<nsDOMDeviceStorage> > stores; You should make this nsRefPtr<nsIDOMDeviceStorage> instead. Otherwise you end up doing an implicit reinterpret-cast from nsDOMDeviceStorage to nsIDOMDeviceStorage when you call SetAsArray, which will fail if we ever insert another interface as the first interface. @@ +961,5 @@ > + if (!result) { > + return; > + } > + > + result->SetAsArray(nsIDataType::VTYPE_INTERFACE, Should we set this to <null> if stores.IsEmpty() == true?

Comment on attachment 622872 [details] [diff] [review] rollup v.2 Review of attachment 622872 [details] [diff] [review]: ----------------------------------------------------------------- OK, i think that's basically it, but I should do a second pass through now that I understand the flow better. One comment though, rather than carrying around mType+mIndex everywhere, why not carry around an nsIFile representing the root. That way we can easily expand the API to supporting USB keys etc in the future, and it'll be fewer arguments to pass in a bunch of places ::: dom/devicestorage/nsDeviceStorage.cpp @@ +623,5 @@ > + > + if (bufSize != wrote) { > + // Something didn't work out. Lets not leave this > + // file around. > + file->Remove(false); Send an error from here too. @@ +627,5 @@ > + file->Remove(false); > + file = nsnull; > + } > + > + nsCOMPtr<PostResultEvent> event = new PostResultEvent(mDOMRequest, mEditable, file); For writes the result event should contain the filename. That way when adding an un-named file we can provide the resulting filename. This is useful for pictures since the application won't have to worry about generating a filename and ensure that it doesn't collide with existing files, or collide with other applications doing the same thing. We can certainly use a followup for generating filenames, but we should change to return the filename rather than file for now. @@ +976,5 @@ > + case DEVICE_STORAGE_TYPE_EXTERNAL: > + aType.AssignLiteral("external"); > + break; > + case DEVICE_STORAGE_TYPE_SHARED: > + aType.AssignLiteral("shared"); Bikeshed: Aren't all these storage areas shared? Maybe "device" is better than "shared"? Or "builtin". @@ +1056,5 @@ > + if (!win) { > + return NS_ERROR_UNEXPECTED; > + } > + > + JSString *jsstr = JSVAL_TO_STRING(aName); You need to first check that aName is a string, otherwise JSVAL_TO_STRING will assert and return something random and you'll crash on the next line. Though really we should use JS_ValueToString to convert arguments of any non-array types to a string. Please file a followup for accepting arrays as well. Same for .delete

> Should we set this to <null> if stores.IsEmpty() == true? If someone does: storage = navigator.getDeviceStorage("kilimanjaro"); What do you want storage to be. I think I want storage == null. If that is true, then the code that exists is what we want. > nsIContentPermissionPrompt and security/safety I fully expect some of this to change when the security model is more defined. However, what we have now is good enough. All of this code is protected with a pref ("device.storage.enabled"). If you want this working in a product, you need to set that preference to true and implement the prompt (or in B2G - grant some applications this permission). > Why do you not need to unlink here? I am sorry. I do not understand. What does that mean? > We can certainly use a followup for generating filenames, but we should change to return the filename rather than file for now. I do not understand this bit -- we can easily add support for : DOMRequest addNamed(Blob blob, DOMString name); // the name But I think the .result should always be a File - not a string. Right? > Aren't all these storage areas shared? Shared between users. Think Windows where there is a Shared users directory where you can put pictures there that your entire family can see. If you think of a better name, let me know. > Please file a followup for accepting arrays as well bug 754316.

> Rather than delegating through continue(), why not simply create and dispatch the InitCursorEvent from here and remove that code from continue()? For testing, we pre-grant allow or cancel. To keep the logic simple, everything flows through Continue() since Allow() might not be called directly.

clean up error strings follow up - bug 754350.

> > Rather than delegating through continue(), why not simply create and dispatch the InitCursorEvent from here and remove that code from continue()? I take that back. I think it might be better with the way you suggest - especially when I fix the double continue problem you mentioned.

we may need to adjust deep directory enumeration and enumeration semantics

now includes recursive directory enumeration

Comment on attachment 624510 [details] [diff] [review] v.4 Review of attachment 624510 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/devicestorage/nsDeviceStorage.cpp @@ +56,5 @@ > +// TODO - eventually, we will want to factor this method > +// out into different system specific subclasses (or > +// something) > +PRInt32 > +nsDOMDeviceStorage::GetFileForLocation(const nsAString& aType, const PRInt32 aIndex, nsIFile** aFile) Nit: Maybe remove the last aFile argument and have this function always set mFile. That way there's less risk that it'll be used as a utility function from the wrong thread. @@ +242,5 @@ > + > + NS_IMETHOD Run() { > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + nsDOMDeviceStorageCursor* cursor = static_cast<nsDOMDeviceStorageCursor*>(mRequest.get()); > + cursor->Continue(); Change this to call the new "fire success event" callback. @@ +282,5 @@ > + nsDOMDeviceStorageCursor* cursor = static_cast<nsDOMDeviceStorageCursor*>(mRequest.get()); > + > + collectFiles(cursor, mFile); > + > + cursor->mInitEventComplete = true; This should no longer be needed I think. @@ +290,5 @@ > + > + return NS_OK; > + } > + > + void collectFiles(nsDOMDeviceStorageCursor* aCursor, nsIFile* aFile) Wow! I didn't realize you were going to do recursive enumeration now. That's awesome! However it does mean that we have to modify nsDOMFileFile to support passing in a file-name. Currently nsDOMFileFile always returns the leaf-name of the nsIFile from the .name getter. What we want here I think is the path within the device-storage. I think all you need to do is to add a new ctor which allows specifying a filename and make the ctor set mName appropriately. @@ +349,5 @@ > + : DOMRequest(aWindow) > + , mInitEventComplete(false) > + , mUnsafeFile(false) > + , mFile(aFile) > + , mName(aName) Nit: would mPath be a better name? @@ +350,5 @@ > + , mInitEventComplete(false) > + , mUnsafeFile(false) > + , mFile(aFile) > + , mName(aName) > + , mArgc(aArgc) do you really need mArgc? wouldn't simply setting mName/mPath to the empty string have the same result? @@ +463,5 @@ > + > + nsCOMPtr<nsILocalFile> file = mFiles[0]; > + mFiles.RemoveElementAt(0); > + jsval val = nsIFileToJsval(GetOwner(), file, mEditable); > + FireSuccess(val); FireSuccess runs synchronously, so break this out into a runnable which is scheduled from Continue and from the init-runnable. The new runnable likely needs to handle the mFiles.Length()==0 case too. You'll also need to set some state like mOkToCallContinue which is initialized to false and set to true in the runnable and false at the end of continue(), and which makes continue() throw InvalidStateError if true when continue() is called. Right now It seems like the page can call continue() twice synchronously without us throwing. That should also remove the need for the mUnsafeFile and mInitEventComplete members. @@ +487,5 @@ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + > + if (mFile) { > + jsval result = nsIFileToJsval(mDOMRequest->GetOwner(), mFile, mEditable); > + mDOMRequest->FireSuccess(result); Change this to be the file-name rather than the file as discussed. @@ +527,5 @@ > + > + bool check = false; > + mFile->Exists(&check); > + if (check) { > + nsCOMPtr<PostErrorEvent> event = new PostErrorEvent(mDOMRequest, (I still think this would be simpler to do by breaking out the body of the Run function into a separate function, and let Run handle firing the appropriate event. Not sure if you didn't agree with that change or if you forgot it. You could also move remove-the-file-cleanup-code out too) @@ +535,5 @@ > + return NS_OK; > + } > + > + // This also creates all ancestors > + mFile->Create(nsIFile::NORMAL_FILE_TYPE, 00600); Don't you need to check for error here? @@ +543,5 @@ > + > + if (!stream) { > + nsCOMPtr<PostErrorEvent> event = new PostErrorEvent(mDOMRequest, > + POST_ERROR_EVENT_UNKNOWN, > + mFile); You should delete the file here too. @@ +554,5 @@ > + > + nsCOMPtr<nsIOutputStream> outputStream; > + NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), mFile); > + > + if (!outputStream) { And here. @@ +652,5 @@ > + NS_IMETHOD Run() > + { > + NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!"); > + > + mFile->Remove(true); We should probably also (recursively) check if this results in mFile's parent directory being empty, and if so delete the parent directory. @@ +832,5 @@ > + > + // Grab the uri of the document > + nsCOMPtr<nsIDOMDocument> domdoc; > + aWindow->GetDocument(getter_AddRefs(domdoc)); > + nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc); It'd be simpler to just do: nsCOMPtr<nsIDocument> doc = do_QI(aWindow->GetExtantDocument()); @@ +965,5 @@ > + return NS_ERROR_UNEXPECTED; > + } > + > + JSString *jsstr = JSVAL_TO_STRING(aName); > + const PRUnichar *data = JS_GetStringCharsZ(aCx, jsstr); Looks like you didn't fix this. See previous comments about this being unsafe and that we should use JS_ValueToString. This will crash if someone passes in a number for example. @@ +967,5 @@ > + > + JSString *jsstr = JSVAL_TO_STRING(aName); > + const PRUnichar *data = JS_GetStringCharsZ(aCx, jsstr); > + nsString name; > + name.Assign(data); You should use nsDependentJSString to convert from JSString->nsString @@ +1006,5 @@ > +{ > + JSString *jsstr = JSVAL_TO_STRING(aName); > + const PRUnichar *data = JS_GetStringCharsZ(aCx, jsstr); > + nsString name; > + name.Assign(data); Same as above

> If you make this function take an nsPIDOMWindow instead (which is what you have at the call-site) you can call do_QueryInterface(aWindow->GetExtantDocument()) instead. This does not work for gaia applications - mURI is null. Not sure why. Also, the form I am using is also used in other places.

> We should probably also (recursively) check if this results in mFile's parent directory being empty, and if so delete the parent directory. I'll file a follow up.

Comment on attachment 625269 [details] [diff] [review] v.5 Review of attachment 625269 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below fixed! ::: dom/devicestorage/nsDeviceStorage.cpp @@ +223,5 @@ > + > +protected: > + nsTArray<nsRefPtr<DeviceStorageFile> > mFiles; > + > + bool mInitEventComplete; This is not used any more. @@ +329,5 @@ > + cursor->mFiles.RemoveElementAt(0); > + val = nsIFileToJsval(cursor->GetOwner(), file, cursor->mEditable); > + } > + > + cursor->mOkToCallContinue = true; You could move this into the else-branch to make calling .continue() on a "done" cursor throw (that's what we do in IndexedDB). I think I have a weak preference for that, but I'm ok either way. @@ +366,5 @@ > + return NS_OK; > + } > + > + nsString fullpath; > + mFile->mFile->GetPath(fullpath); Crap, I think I explained this poorly. My idea is that we'd behave something like this: Say that the user has the following files in the ~/Pictures folder on OSX: ~/Pictures/vacation/pic1.jpg ~/Pictures/vacation/mom/pic2.jpg ~/Pictures/pic3.jpg Calling .enumerate("") should yield 3 Files with the names "vacation/pic1.jpg", "vacation/mom/pic2.jpg", "pic3.jpg". Calling .enumerate("vacation") should yield 2 Files with the names "vacation/pic1.jpg", "vacation/mom/pic2.jpg". Calling .enumerate("vacation/mom") should yield 1 File with the name "vacation/mom/pic2.jpg". That way files have consistent which doesn't change depending on what you enumerate. It also makes it easy to over-write or delete a file once enumerated by simply passing the filename back into the API. @@ +450,5 @@ > + , mEditable(aEditable) > +{ > + if (mozilla::Preferences::GetBool("device.storage.prompt.testing", false)) { > + Allow(); > + } Don't you need to fire off an nsIContentPermissionPrompt here if the pref is false? Otherwise it doesn't seem like we'll start iterating. @@ +499,5 @@ > +NS_IMETHODIMP > +nsDOMDeviceStorageCursor::Allow() > +{ > + mAllowed = true; > + if (mArgc > 0 && !isSafePath(mFile->mPath)) { Nit: I still don't think you nee mArgc. isSafePath returns true for empty strings. @@ +500,5 @@ > +nsDOMDeviceStorageCursor::Allow() > +{ > + mAllowed = true; > + if (mArgc > 0 && !isSafePath(mFile->mPath)) { > + mUnsafeFile = true; You shouldn't need mUnsafeFile given that mOkToCallContinue is never set to true when firing an error event. @@ +534,5 @@ > + if (!mAllowed) { > + nsCOMPtr<nsIContentPermissionPrompt> prompt = do_GetService(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID); > + if (prompt) { > + prompt->Prompt(this); > + } Can mAllowed ever be false here? Givent that you set mAllowed = true in the Allow() function which is the only way to get here. Actually, I wonder if that means that you can remove the mAllowed member. @@ +634,5 @@ > + > + nsCOMPtr<nsIFile> f = mFile->mFile; > + > + bool check = false; > + f->Exists(&check); Do you need this check given that nsIFile::Create fails if the file already exists? Relying on ::Create might be safer anyway since there's a risk that you're racing with other processes otherwise. Sorry for not catching that earlier. @@ +719,5 @@ > + NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!"); > + > + nsRefPtr<nsRunnable> r; > + > + if (!mEditable) { Not that it really matters for now, but I think we should do the Exists check for getEditable too.

> Don't you need to fire off an nsIContentPermissionPrompt here if the pref is false? Otherwise it doesn't seem like we'll start iterating. No. In the constructor, we grab the preference. If we are testing, we call Allow. Otherwise, we use the nsIContentPermissionPrompt. > Nit: I still don't think you nee mArgc. isSafePath returns true for empty strings. Removed. > You shouldn't need mUnsafeFile given that mOkToCallContinue is never set to true when firing an error event. Done. > Do you need this check given that nsIFile::Create fails if the file already exists? Relying on ::Create might be safer anyway since there's a risk that you're racing with other processes otherwise. Sorry for not catching that earlier. Done. > Not that it really matters for now, but I think we should do the Exists check for getEditable too. We can fix this up when we land support for editable. Go JanV! https://hg.mozilla.org/integration/mozilla-inbound/rev/027ed1748c81

Sorry, had to back out for M3 orange: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=027ed1748c81 https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e62a1e9809

(In reply to Doug Turner (:dougt) from comment #76) > > Don't you need to fire off an nsIContentPermissionPrompt here if the pref is false? Otherwise it doesn't seem like we'll start iterating. > > No. In the constructor, we grab the preference. If we are testing, we call > Allow. Otherwise, we use the nsIContentPermissionPrompt. Where? Note that the code that I commented on is in the constructor. I can only see that we fire the prompt from the ::Continue() function, but we can't get there without first iterating since mOkToCallContinue is false (and the page shouldn't need to call .continue() to get the first result). Am I missing something?

my bad. we need to call out to nsIContentPermissionPrompt if we aren't testing.

And that means we can remove the nsIContentPermissionPrompt call in ::Continue, right? Since mAllow will always be true.

yup. and mAllow can go, i think.

Review Meeting Set: https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html?view=month&action=view&invId=110473-110472&pstat=AC&exInvId=110473-185529&useInstance=1&instStartTime=1339012800000&instDuration=3600000

The list of the whole documentation for that API is available here: https://developer.mozilla.org/en-US/docs/tag/Device%20Storage Review more than welcome :)

What's necessary to enable this webapi on desktop and android?

I believe that it's enabled for all platforms. I see devicestorage tests being run in M2 on ubuntu (on TBPL). I'm not sure whether its included in android and if it is, I'm guessing it will need tweaks to setup the root directories for the various storage areas properly.

This is for privileged and certified applications only AFAIK. It might run on Desktop and Android if you can run a privileged applications in those runtimes.