Closed Bug 910412 Opened 11 years ago Closed 10 years ago

Change DeviceStorage API to use FileSystem API spec

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: xyuan, Assigned: xyuan)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(6 files, 58 obsolete files)

11.72 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
72.77 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
11.57 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
11.02 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
29.76 KB, patch
Details | Diff | Splinter Review
23.91 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
As the new FileSystem API covers all user cases of the DeviceStorage API and their structure are similar, we will change the DeviceStorage API to use the FileSystem API spec.

The device storage directory may be accessed by the same interface as the sandboxed filesystem: 
partial interface Navigator {
  Promise<Directory> getFilesystem({storage: 'sdcard'});
};

Or use the original interface with Promise<Directory> as return value

partial interface Navigator {
  Promise<Directory> getDeviceStorage(DOMString type);
};
Depends on: 717103
I tend to like the FileSystem API, but understand that Jonas has some other plans.
Flags: needinfo?(jonas)
Doug: Not sure what you are referring to. I definitely think that we should change DeviceStorage to use the more polished FileSystem API than what it's currently using. We'll still need to support the old one in a transition period though.

What you might be referring to is that I think that we should also add a notification system so that apps don't have to rescan the DeviceStorage on each startup. But that's orthogonal to this bug I believe.
Flags: needinfo?(jonas)
Blocks: 910387
Depends on: 882076
Why are we even implementing the DeviceStorage API then?
Flags: needinfo?(jonas)
Ok, to be clear, I don't think we can get rid of the DeviceStorage interface. At least not in the short term. For now we'll have to have something like:

interface nsIDOMDeviceStorage : nsIDOMEventTarget
{
    // This one is new. Promise resolves to a Directory
    Promise getRoot();

    // These we still don't have replacements for
    attribute jsval onchange;

    nsIDOMDOMRequest freeSpace();
    nsIDOMDOMRequest usedSpace();
    nsIDOMDOMRequest available();

    readonly attribute DOMString storageName;

    readonly attribute bool default;


    // These we should phase out. Add warnings when they are used saying that they are going away.
    nsIDOMDOMRequest add(in nsIDOMBlob aBlob);
    nsIDOMDOMRequest addNamed(in nsIDOMBlob aBlob, in DOMString aName);
    nsIDOMDOMRequest get([Null(Stringify)] in DOMString aName);
    nsIDOMDOMRequest getEditable([Null(Stringify)] in DOMString aName);
    nsIDOMDOMRequest delete([Null(Stringify)] in DOMString aName);
};
Flags: needinfo?(jonas)
Assignee: nobody → xyuan
Status: NEW → ASSIGNED
I'm going to implement the Filesystem API for device storage. The following parts will be implemented as a start:

interface DeviceStorage {
  ...
  Promise<Directory> getRoot();
  ...
};

interface Directory {
  ...
  Promise<Directory> createDirectory(DOMString path);
  Promise<(File or Directory)> get(DOMString path);
  ...
};

@sicking, are you the right guy to help and review the patch, or can you recommend someone else? 
Thanks ;-)
Flags: needinfo?(jonas)
Add WebIDL and corresponding C++ files.
Attachment #825844 - Flags: feedback?(jonas)
Implements DeviceStorage.getRoot, Directory.get and Directory.createDirectory.
Attachment #825846 - Flags: feedback?(jonas)
Attachment #825847 - Flags: feedback?(jonas)
Blocks: 934367
Blocks: 934368
Blocks: 934370
I'm not really the person to review. Someone on the DOM or WebAPI teams probably could.
Blocks: 934371
Blocks: 934373
Comment on attachment 825844 [details] [diff] [review]
0001-WIP-Part1-v1-WebIDL-and-building.patch

Hi, Ehsan could help to review the patch?
Attachment #825844 - Flags: feedback?(jonas) → feedback?(ehsan)
Attachment #825846 - Flags: feedback?(jonas) → feedback?(ehsan)
Attachment #825847 - Flags: feedback?(jonas) → feedback?(ehsan)
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #9)
> I'm not really the person to review. Someone on the DOM or WebAPI teams
> probably could.

@sicking, thanks and I'll find someone else to help:-)
Flags: needinfo?(jonas)
Attachment #825844 - Flags: feedback?(ehsan) → feedback?(dhylands)
Attachment #825846 - Flags: feedback?(ehsan) → feedback?(dhylands)
Attachment #825847 - Flags: feedback?(ehsan) → feedback?(dhylands)
No longer depends on: 882076
Blocks: 935883
Attachment #825844 - Attachment is obsolete: true
Attachment #825844 - Flags: feedback?(dhylands)
Attachment #829124 - Flags: feedback?(dhylands)
Attachment #825846 - Attachment is obsolete: true
Attachment #825846 - Flags: feedback?(dhylands)
Attachment #829125 - Flags: feedback?(dhylands)
Attachment #825847 - Attachment is obsolete: true
Attachment #825847 - Flags: feedback?(dhylands)
Attachment #829126 - Flags: feedback?(dhylands)
Attachment #829125 - Attachment description: 0002-WIP-Part2-v1-Implement-getRoot-createDirectory-and-g.patch → 0002-WIP-Part2-v1-Implement-getRoot-createDirectory-and-g.patch v2
Attachment #829124 - Attachment description: 0001-WIP-Part1-v1-WebIDL-and-building v1.patch → 0001-WIP-Part1-v2-WebIDL-and-building.patch
Attachment #829125 - Attachment description: 0002-WIP-Part2-v1-Implement-getRoot-createDirectory-and-g.patch v2 → 0002-WIP-Part2-v2-Implement-getRoot-createDirectory-and-g.patch
Attachment #829126 - Attachment description: 0003-WIP-Part3-v1-mochitest v2.patch → 0003-WIP-Part3-v2-mochitest.patch
Comment on attachment 829124 [details] [diff] [review]
0001-WIP-Part1-v2-WebIDL-and-building.patch

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

So I didn't make it all the way through the files, but I think the issue about exposing the root directory is something that needs to be addressed, and this will probably have an impact on the rest of the files.

I'll post the feedback that I've written thus far on the 3 patches (not to be considered complete yet - but I figured the sooner you get this the better).

::: dom/directory/Directory.cpp
@@ +170,5 @@
> +  if (!FilesystemFile::IsValidRelativePath(fs, relativePath)) {
> +    return false;
> +  }
> +
> +  aRealPath = mFile->GetPath() + NS_LITERAL_STRING("/") + relativePath;

Shouldn't this do: mFile->Append(relativePath) (I believe that mFile under windows uses backslash to seperate file elements)

::: dom/webidl/DeviceStorage.webidl
@@ +48,5 @@
>    // for storing new files.
>    readonly attribute boolean default;
> +
> +  [NewObject]
> +  Promise getRoot();

So I have an issue with exposing the root directory to content.

Currently, device storage abstracts away the top level directory by doing:

/storageName/somedir/somefile.txt

So content only sees /storageName. Internally, this gets mapped to the actual file directory (for example /sdcard maps to /storage/emulated/legacy on the Nexus4).

I would really like to continue along this vein and have the top level directory remain abstract, and everything below that can come from the real filesystem.

From a security standpoint, this also makes it easier to ensure that people aren't accesing things that they shouldn't.

From a security standpoint, we definitely don't want to be passing fully qualified paths from the child to the parent (we used to do this in device storage, but now send storageName and relative path - see bug 860934)

::: dom/webidl/Directory.webidl
@@ +15,5 @@
> +  [NewObject]
> +  Promise createDirectory(DOMString path);
> +
> +  [NewObject]
> +  Promise get(DOMString path);

What does this do? Is this like devicestorage's get which retrieves the contents of a file?

@@ +20,5 @@
> +
> +  [NewObject]
> +  AbortableProgressPromise
> +    move((DOMString or File or Directory) path,
> +         optional (DOMString or Directory or DestinationDict) dest);

Is this to rename a directory? What does it mean when you only pass one argument?

@@ +55,5 @@
> +};
> +
> +enum CreateIfExistsMode { "replace", "fail" };
> +enum OpenIfExistsMode { "open", "fail" };
> +enum OpenIfNotExistsMode { "create", "fail" };

So, from the standpoint of using these, if I'm understanding things properly, you'd do:

dir.createFile(someFileName, 'fail');

Looking at this, I can't really tell what 'fail' means. I think that adding the IfExists to the string makes the calling code look really obvious.

dir.createFile(someFileName, 'failIfExists');
dir.createFile(someFileName, 'replaceIfExists');
Comment on attachment 829125 [details] [diff] [review]
0002-WIP-Part2-v2-Implement-getRoot-createDirectory-and-g.patch

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

I only made it partway through these files, but figured I'd post what I have so far.

::: dom/ipc/ContentChild.cpp
@@ +905,5 @@
> +
> +bool
> +ContentChild::DeallocPFilesystemRequestChild(PFilesystemRequestChild* aFilesystem)
> +{
> +    mozilla::dom::TaskBase* child = static_cast<mozilla::dom::TaskBase*>(iframe);

So where does iframe come from? I couldn't find any class variables named iframe.

../../../b2g-inbound/dom/ipc/ContentChild.cpp: In member function 'virtual bool mozilla::dom::ContentChild::DeallocPFilesystemRequestChild(mozilla::dom::PContentChild::PFilesystemRequestChild*)':
../../../b2g-inbound/dom/ipc/ContentChild.cpp:909:74: error: 'iframe' was not declared in this scope

@@ +906,5 @@
> +bool
> +ContentChild::DeallocPFilesystemRequestChild(PFilesystemRequestChild* aFilesystem)
> +{
> +    mozilla::dom::TaskBase* child = static_cast<mozilla::dom::TaskBase*>(iframe);
> +    NS_RELEASE(child);

I couldn't find the increment to go with this release.

::: dom/ipc/ContentParent.cpp
@@ +2193,5 @@
> +
> +bool
> +ContentParent::DeallocPFilesystemRequestParent(PFilesystemRequestParent* doomed)
> +{
> +  FilesystemRequestParent *parent = static_cast<FilesystemRequestParent*>(doomed);

nit: FilesystemRequestParent* parent
Comment on attachment 829126 [details] [diff] [review]
0003-WIP-Part3-v2-mochitest.patch

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

Presumably this will need to be fleshed out more and have more tests added.

I think this will need to redone once the getRoot issue is resolved.

::: dom/devicestorage/test/test_fs_basic.html
@@ +16,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=910412">Mozilla Bug 910412</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +  

nit: trailing space
(In reply to Dave Hylands [:dhylands] from comment #15)
> Comment on attachment 829124 [details] [diff] [review]
> 0001-WIP-Part1-v2-WebIDL-and-building.patch
> 
> Review of attachment 829124 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I didn't make it all the way through the files, but I think the issue
> about exposing the root directory is something that needs to be addressed,
> and this will probably have an impact on the rest of the files.
> 
> I'll post the feedback that I've written thus far on the 3 patches (not to
> be considered complete yet - but I figured the sooner you get this the
> better).
> 
> ::: dom/directory/Directory.cpp
> @@ +170,5 @@
> > +  if (!FilesystemFile::IsValidRelativePath(fs, relativePath)) {
> > +    return false;
> > +  }
> > +
> > +  aRealPath = mFile->GetPath() + NS_LITERAL_STRING("/") + relativePath;
> 
> Shouldn't this do: mFile->Append(relativePath) (I believe that mFile under
> windows uses backslash to seperate file elements)

This should append a relative path to the given path.

I'll always use "/" as separator to pass "path" around before creating local file.
I call this kind of internal path by the name "absolute DOM path". When creating
local file, |FilesystemUtils::CetLocalFile| will be called to convert the DOM path
to the right OS dependent path. Below is the implementation of that utility
function:

// static
already_AddRefed<nsIFile>
FilesystemUtils::CetLocalFile(const nsAString& aRealPath)
{
  nsString localPath;
  localPath = aRealPath;
#if defined(XP_WIN)
  PRUnichar* cur = localPath.BeginWriting();
  PRUnichar* end = localPath.EndWriting();
  for (; cur < end; ++cur) {
    if (PRUnichar('\\') == *cur)
      *cur = PRUnichar('/');
  }
#endif
  nsCOMPtr<nsIFile> file;
  nsresult rv = NS_NewLocalFile(localPath, false, getter_AddRefs(file));
  NS_ENSURE_SUCCESS(rv, nullptr);
  return file.forget();
}

The reason I introduce the absolute DOM path is that we will use virtual
path for the sandboxed filesystem, which is going to be implemented later. "The
sandboxed filesystem uses a database to store things like directory structures
and file metadata. Whereas the actual files stored in the filesystem
should create real OS files so that we can do quick reading and
writing." For the sandboxed filesystem, it will use an OS independent path
without limitation on file names or paths.

> 
> ::: dom/webidl/DeviceStorage.webidl
> @@ +48,5 @@
> >    // for storing new files.
> >    readonly attribute boolean default;
> > +
> > +  [NewObject]
> > +  Promise getRoot();
> 
> So I have an issue with exposing the root directory to content.
> 
> Currently, device storage abstracts away the top level directory by doing:
> 
> /storageName/somedir/somefile.txt
> 
> So content only sees /storageName. Internally, this gets mapped to the
> actual file directory (for example /sdcard maps to /storage/emulated/legacy
> on the Nexus4).
> 
> I would really like to continue along this vein and have the top level
> directory remain abstract, and everything below that can come from the real
> filesystem.
> 
> From a security standpoint, this also makes it easier to ensure that people
> aren't accesing things that they shouldn't.
> 
> From a security standpoint, we definitely don't want to be passing fully
> qualified paths from the child to the parent (we used to do this in device
> storage, but now send storageName and relative path - see bug 860934)
Filesystem API uses the similar mechanism to ensure the path security. Here is
a short explanation about the path:
 http://lists.w3.org/Archives/Public/public-script-coord/2013JulSep/0379.html

The path string passed to directory is relative and the content can only
access the descendent files of a given directory. There are two constraints to
enforce the security. 
1) The functions on Directory that accept DOMString arguments for filenames allow
names like "path/to/file.txt". Such paths are always interpreted as relative to the
directory itself, never relative to the root. And the paths aren't allowd to walk
up the directory tree. So paths like "../foo", "..", "/foo/bar" or "foo/../bar"
are not allowed.
2) Passing a File/Directory object to an operation of Directory where the
File/Directory object isn't contained in that directory or its descendents also
results in an error.

These two constraints prevent content from modifying the root directory or getting its parent.
> 
> ::: dom/webidl/Directory.webidl
> @@ +15,5 @@
> > +  [NewObject]
> > +  Promise createDirectory(DOMString path);
> > +
> > +  [NewObject]
> > +  Promise get(DOMString path);
> 
> What does this do? Is this like devicestorage's get which retrieves the
> contents of a file?

Actually, this should be 
 Promise<(File or Directory)> get(DOMString path);

It gets a descendent file or directory with the given relative path. For
example there is a file and a directory under the root:
  root +--- x.txt
       |
       ---- subdirectory

|root.get('x.txt')| returns a File object(http://dev.w3.org/2006/webapi/FileAPI/),
while |root.get('subdirectory') returns a Directory object.
     

> 
> @@ +20,5 @@
> > +
> > +  [NewObject]
> > +  AbortableProgressPromise
> > +    move((DOMString or File or Directory) path,
> > +         optional (DOMString or Directory or DestinationDict) dest);
> 
> Is this to rename a directory? What does it mean when you only pass one
> argument?
Yes, this is to rename a directory. The second argument is required as the spec(http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface) and should not be optional.
But the WebIDL compiler complains about the second argument and says 
"Dictionary argument or union argument containing a dictionary not
followed by a required argument must be optional". So I have to make
it optional. I should leave a comment about the second argument.
(In reply to Dave Hylands [:dhylands] from comment #15)
> @@ +55,5 @@
> > +};
> > +
> > +enum CreateIfExistsMode { "replace", "fail" };
> > +enum OpenIfExistsMode { "open", "fail" };
> > +enum OpenIfNotExistsMode { "create", "fail" };
> 
> So, from the standpoint of using these, if I'm understanding things
> properly, you'd do:
> 
> dir.createFile(someFileName, 'fail');
> 
> Looking at this, I can't really tell what 'fail' means. I think that adding
> the IfExists to the string makes the calling code look really obvious.
> 
> dir.createFile(someFileName, 'failIfExists');
> dir.createFile(someFileName, 'replaceIfExists');

Looks like a good suggestion. I don't have much experience about WebAPI
designing and the implementation will follow the API proposal doc
(http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface).

Jonas is responsible for API proposal. Let's cc him and let him drive
the API designing.
Flags: needinfo?(jonas)
(In reply to Dave Hylands [:dhylands] from comment #16)
> 
> ::: dom/ipc/ContentChild.cpp
> @@ +905,5 @@
> > +
> > +bool
> > +ContentChild::DeallocPFilesystemRequestChild(PFilesystemRequestChild* aFilesystem)
> > +{
> > +    mozilla::dom::TaskBase* child = static_cast<mozilla::dom::TaskBase*>(iframe);
> 
> So where does iframe come from? I couldn't find any class variables named
> iframe.

There is a mistake. |iframe| should be |aFilesystem|.

> 
> @@ +906,5 @@
> > +bool
> > +ContentChild::DeallocPFilesystemRequestChild(PFilesystemRequestChild* aFilesystem)
> > +{
> > +    mozilla::dom::TaskBase* child = static_cast<mozilla::dom::TaskBase*>(iframe);
> > +    NS_RELEASE(child);
> 
> I couldn't find the increment to go with this release.
It is increased in TaskBase::Start of TaskBase.cpp as following:

  void
  TaskBase::Start()
  {
    ...
    } else {
      // Run in child process.
  
      // Retain a reference so the task object isn't deleted without IPDL's
      // knowledge. The reference will be released by
      // mozilla::dom::ContentChild::DeallocPFilesystemRequestChild.
      AddRef();
      ContentChild::GetSingleton()->SendPFilesystemRequestConstructor(this,
        GetRequestParams());
    }
  }
(In reply to Dave Hylands [:dhylands] from comment #17)
> Comment on attachment 829126 [details] [diff] [review]
> 0003-WIP-Part3-v2-mochitest.patch
> 
> Review of attachment 829126 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Presumably this will need to be fleshed out more and have more tests added.
> 
> I think this will need to redone once the getRoot issue is resolved.

Yes, I'll add more tests.
So, to help people read the WebIDL:

createFile is defined as:

interface Directory {
  createFile(DOMString path, optional CreateFileOptions options);
  ...
}

I.e. the second argument is of type 'CreateFileOptions'.

CreateFileOptions is defined in CreateFileOptions.webidl as

dictionary CreateFileOptions {
  CreateIfExistsMode ifExists = "fail";
  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;
};

A 'dictionary' is an object with a number of optional properties, with our without default values. Properties in dictionaries are by definition *always* optional. In this case there are two properties, 'ifExists' and 'data'. Again, even properties without default values are optional.

That means that you call createFile using something like any of the following:

dir.createFile('filename.txt', { ifExists: ..., data: ... });
dir.createFile('filename.txt', { ifExists: ... });
dir.createFile('filename.txt', { data: ... });
dir.createFile('filename.txt', { });
dir.createFile('filename.txt');

You can actually even do

dir.createFile('filename.txt', { ifExists: ..., data: ..., flubber: ..., hello: ... });

in which case the 'flubber' and 'hello' properties will simply be ignored.

Oo, lets look at what values you can pass for the 'ifExists' and 'data' properties.

'ifExists' is defined as a CreateIfExistsMode. CreateIfExistsMode is defined as

enum CreateIfExistsMode { "replace", "fail" };

this means that 'ifExists' can be set to either the string "replace" or the string "fail". Since CreateFileOptions defines 'fail' as default value for ifExists, if no value is provided, the value 'fail' is used.

'data' is defined as (DOMString or Blob or ArrayBuffer or ArrayBufferView). So you can pass any object of those types.

So here are some full examples of calls to createFile

A) dir.createFile('filename.txt', { ifExists: "fail", data: myBlob });
B) dir.createFile('filename.txt', { ifExists: "replace", data: "filecontents" });
C) dir.createFile('filename.txt', { ifExists: "fail" });
D) dir.createFile('filename.txt', { ifExists: "replace" });
E) dir.createFile('filename.txt', { data: myBlob });
F) dir.createFile('filename.txt', { });
G) dir.createFile('filename.txt');


A) If the file already exists, produce an error. Otherwise create the file and write the contents of myBlob into the file.

B) Unconditionally create the file, overwrite the file contents if the file already exists. Write the string "filecontents" into the file.

C) If the file already exists, produce an error. Otherwise create the file and leave it empty.

D) Unconditionally create the file, overwrite the file contents if the file already exists. Leave the newly created file empty.

E) Same as A) (since 'fail' is used as default for ifExists)

F) Same as C) (since 'fail' is used as default for ifExists)

G) Same as C) (since 'fail' is used as default for ifExists)

Hope that explains things?

Note that implementation-wise the WebIDL generator should help with the handling of dictionaries.
Flags: needinfo?(jonas)
(In reply to Yuan Xulei [:yxl] from comment #18)
> (In reply to Dave Hylands [:dhylands] from comment #15)
...snip...
> > So I have an issue with exposing the root directory to content.
> > 
> > Currently, device storage abstracts away the top level directory by doing:
> > 
> > /storageName/somedir/somefile.txt
> > 
> > So content only sees /storageName. Internally, this gets mapped to the
> > actual file directory (for example /sdcard maps to /storage/emulated/legacy
> > on the Nexus4).
> > 
> > I would really like to continue along this vein and have the top level
> > directory remain abstract, and everything below that can come from the real
> > filesystem.
> > 
> > From a security standpoint, this also makes it easier to ensure that people
> > aren't accesing things that they shouldn't.
> > 
> > From a security standpoint, we definitely don't want to be passing fully
> > qualified paths from the child to the parent (we used to do this in device
> > storage, but now send storageName and relative path - see bug 860934)
> Filesystem API uses the similar mechanism to ensure the path security. Here
> is
> a short explanation about the path:
>  http://lists.w3.org/Archives/Public/public-script-coord/2013JulSep/0379.html
> 
> The path string passed to directory is relative and the content can only
> access the descendent files of a given directory. There are two constraints
> to
> enforce the security. 

So what are the paths relative to?

For DeviceStorage we can have multiple storage areas. How do you choose the storage area?

I can't really provide much more feedback on the webapi related stuff. I'm more than happy to review any of the implementation details underneath the webapi stuff.

So I'm going to remove the r? for me from the attachments for now.
Attachment #829124 - Flags: feedback?(dhylands)
Attachment #829125 - Flags: feedback?(dhylands)
Attachment #829126 - Flags: feedback?(dhylands)
(In reply to Dave Hylands [:dhylands] from comment #23)
> (In reply to Yuan Xulei [:yxl] from comment #18)
> > (In reply to Dave Hylands [:dhylands] from comment #15)
> ...snip...
> > > So I have an issue with exposing the root directory to content.
> > > 
> > > Currently, device storage abstracts away the top level directory by doing:
> > > 
> > > /storageName/somedir/somefile.txt
> > > 
> > > So content only sees /storageName. Internally, this gets mapped to the
> > > actual file directory (for example /sdcard maps to /storage/emulated/legacy
> > > on the Nexus4).
> > > 
> > > I would really like to continue along this vein and have the top level
> > > directory remain abstract, and everything below that can come from the real
> > > filesystem.
> > > 
> > > From a security standpoint, this also makes it easier to ensure that people
> > > aren't accesing things that they shouldn't.
> > > 
> > > From a security standpoint, we definitely don't want to be passing fully
> > > qualified paths from the child to the parent (we used to do this in device
> > > storage, but now send storageName and relative path - see bug 860934)
> > Filesystem API uses the similar mechanism to ensure the path security. Here
> > is
> > a short explanation about the path:
> >  http://lists.w3.org/Archives/Public/public-script-coord/2013JulSep/0379.html
> > 
> > The path string passed to directory is relative and the content can only
> > access the descendent files of a given directory. There are two constraints
> > to
> > enforce the security. 
> 
> So what are the paths relative to?
The patchs are relative to the Directory that accept it as aguments.

> 
> For DeviceStorage we can have multiple storage areas. How do you choose the
> storage area?
>

We keep the entry of the original DeviceStorage and choose the storage area with
the original API:
  var storage = navigator.getDeviceStorage('storagearea');

The root directory is not the top level directory for all storage areas, but
has the same level with the corresponding storage area. We have multiple root
directories. Each storage area is mapped to a Directory object and the content
gets the mapped Directory by calling |storage.getRoot|. 

So for the device storage
path:
  /storageName/somedir/somefile.txt
The root is "/storageName", but not "/". To access the "somefile.txt", we should do:
  var storage = navigator.getDeviceStorage('storagearea');
  storage.getRoot(function(root) {
    root.get('somedir/somefile.txt');
  });

Will this resolve the root directory issue?

> I can't really provide much more feedback on the webapi related stuff. I'm
> more than happy to review any of the implementation details underneath the
> webapi stuff.
> 
> So I'm going to remove the r? for me from the attachments for now.

I'm sorry. Please focus on the implementation detail. I didn't mean to discuss the
WebAPI stuff here, but just wanted to provide the API details. I should have given
enough explanation about API details before asking for review. I'll write comments
in the WebIDL file and explain each function in as much detail as possible. Would
you like to continue to review?
Flags: needinfo?(dhylands)
(In reply to Yuan Xulei [:yxl] from comment #24)
> (In reply to Dave Hylands [:dhylands] from comment #23)
> > (In reply to Yuan Xulei [:yxl] from comment #18)
> > > (In reply to Dave Hylands [:dhylands] from comment #15)
> > ...snip...

> > For DeviceStorage we can have multiple storage areas. How do you choose the
> > storage area?
> >
> 
> We keep the entry of the original DeviceStorage and choose the storage area
> with
> the original API:
>   var storage = navigator.getDeviceStorage('storagearea');
> 
> The root directory is not the top level directory for all storage areas, but
> has the same level with the corresponding storage area. We have multiple root
> directories. Each storage area is mapped to a Directory object and the
> content
> gets the mapped Directory by calling |storage.getRoot|. 
> 
> So for the device storage
> path:
>   /storageName/somedir/somefile.txt
> The root is "/storageName", but not "/". To access the "somefile.txt", we
> should do:
>   var storage = navigator.getDeviceStorage('storagearea');
>   storage.getRoot(function(root) {
>     root.get('somedir/somefile.txt');
>   });
> 
> Will this resolve the root directory issue?

So as long as querying root (i.e. asking for the path of root returns /storageName and not the underlying real directory name), then that addresses any security concerns I have.

> > I can't really provide much more feedback on the webapi related stuff. I'm
> > more than happy to review any of the implementation details underneath the
> > webapi stuff.
> > 
> > So I'm going to remove the r? for me from the attachments for now.
> 
> I'm sorry. Please focus on the implementation detail. I didn't mean to
> discuss the
> WebAPI stuff here, but just wanted to provide the API details. I should have
> given
> enough explanation about API details before asking for review. I'll write
> comments
> in the WebIDL file and explain each function in as much detail as possible.
> Would
> you like to continue to review?

Sure, I just figured I'd let you know that you'll want to line up somebody else for looking at the webapi aspects of this.

Thanks very much for the explanation above, it helps to provide context.
Flags: needinfo?(dhylands)
(In reply to Dave Hylands [:dhylands] from comment #25)
> So as long as querying root (i.e. asking for the path of root returns
> /storageName and not the underlying real directory name), then that
> addresses any security concerns I have.
I'll make a fix about the root directory name.
> 
> > > I can't really provide much more feedback on the webapi related stuff. I'm
> > > more than happy to review any of the implementation details underneath the
> > > webapi stuff.
> > > 
> > > So I'm going to remove the r? for me from the attachments for now.
> > 
> > I'm sorry. Please focus on the implementation detail. I didn't mean to
> > discuss the
> > WebAPI stuff here, but just wanted to provide the API details. I should have
> > given
> > enough explanation about API details before asking for review. I'll write
> > comments
> > in the WebIDL file and explain each function in as much detail as possible.
> > Would
> > you like to continue to review?
> 
> Sure, I just figured I'd let you know that you'll want to line up somebody
> else for looking at the webapi aspects of this.
> 
Thanks for you help and reminding. I may ask Jonas or someone else to review
the webapi stuff. ;-)
Add full comments to dom/webidl/Directory.webidl about the WebAPI details.

@Jonas Could you review the comments of Directory.webid to make sure I understand the API spec correctly.
Attachment #829124 - Attachment is obsolete: true
Attachment #831324 - Flags: feedback?(jonas)
Attachment #831324 - Flags: feedback?(dhylands)
The patch addresses comment 6.
Attachment #829125 - Attachment is obsolete: true
Attachment #831327 - Flags: feedback?(dhylands)
Attached patch Diff-Part2-v2-v3 (obsolete) — Splinter Review
This is a rough diff for part2 between v2 and v3. It shows the main changes of part2 v3, though not all changes. 

Hope it helps.
Add a mochitest file to test |createDirectory| function.
Attachment #829126 - Attachment is obsolete: true
Attachment #831330 - Flags: feedback?(dhylands)
(In reply to Yuan Xulei [:yxl] from comment #28)
> Created attachment 831327 [details] [diff] [review]
> 0002-WIP-Part2-v3-Implement-getRoot-createDirectory-and-g.patch
> 
> The patch addresses comment 6.

Also querying the root directory name will return the device storage name and not the underlying real directory name.
Comment on attachment 831324 [details] [diff] [review]
0001-WIP-Part1-v3-WebIDL-and-building.patch

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

Just looked over the webidl files. Looks good with the below nits.

You should probably have Baku review the Promise implementation. I'm a bit surprised that you don't need to inherit the existing Promise class?

::: dom/webidl/CreateFileOptions.webidl
@@ +5,5 @@
> + */
> +
> +dictionary CreateFileOptions {
> +  CreateIfExistsMode ifExists = "fail";
> +  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;

we should probably change this to

(DOMString or Blob or ArrayBuffer or ArrayBufferView)? data = null;

so that passing null is allowed.

@@ +6,5 @@
> +
> +dictionary CreateFileOptions {
> +  CreateIfExistsMode ifExists = "fail";
> +  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;
> +};

Move this into Directory.webidl with the other dictionaries.

::: dom/webidl/Directory.webidl
@@ +29,5 @@
> +   * If 'ifExists' is 'fail' and the path already exists, createFile must fail;
> +   * If 'ifExists' is 'replace', the path already exists, and is a file, create
> +   * a new file to replace the existing one;
> +   * If 'ifExists' is 'replace', the path already exists, but is a directory,
> +   * createFile must fail.

It's a good question what to do if the path exists but is a directory. I think failing is the right behavior, but we should get the spec clarified.

@@ +37,5 @@
> +   * File object. Otherwise, rejected with a DOM error.
> +   *
> +   * It should be
> +   *   Promise<File> createFile(DOMString path, CreateFileOptions options),
> +   * but gecko currently requires the last dictionary argument to be optional.

The second argument should be optional, sounds like the spec is wrong here.

@@ +70,5 @@
> +  Promise get(DOMString path);
> +
> +  /*
> +   * Moves (renames) a file or directory. The source and destination path
> +   * must be contained in current directory or its descendents.

Actually, the destination doesn't need to be a descendent. However a destination string can't contain '..' or start with '/'. So the following should all be forbidden:

move("filename", "../foo");
move("filename", "/foo");
move("filename", { name: "../foo", dir: myDir });
move("filename", { name: "/foo", dir: myDir });

@@ +77,5 @@
> +   * Directory object.
> +   * @param dest The destination. If a DOM string is passed, it is the relative
> +   * path to the destination file. If a Directory object is passed the source
> +   * will be moved to that directory. If a DestinationDict is passed, the parent
> +   * directory and the new name of the destination will be specified.

Change "will" to "must" here. Though maybe we should allow passing a dictionary with either a name or a dictionary specified too. Good question for the spec.

@@ +81,5 @@
> +   * directory and the new name of the destination will be specified.
> +   * @return If the source doesn't exist or the destination already exists, move
> +   * should fail and the returning AbortableProgressPromise is rejected with a
> +   * DOM error. During the operation, the current file being moved will be sent
> +   * as progress value of the AbortableProgressPromise.

Hmm.. It's a good question if moves should always fail if the destination exists. I could see allowing overwriting the destination if the destination is a file and not a directory.

Though always failing sounds like a safe start. Either way we should get this clarified in the spec.

@@ +95,5 @@
> +    move((DOMString or File or Directory) path,
> +         optional (DOMString or Directory or DestinationDict) dest);
> +
> +  /*
> +   * Deletes a file or an empty directory. The target should be a descendent of

Change "should" to "must"

@@ +102,5 @@
> +   * target. Otherwise, the File or Directory object of the target should be
> +   * passed.
> +   * @return If the target exists, the promise is resolved with boolean true or
> +   * false to indicate success. If the target is a non-empty directory or other
> +   * error occurrs, rejected with a DOM error.

The description here is a bit unclear. If the target is a non-empty directory, or if deleting the target fails, the promise is rejected with a DOM error. If the target did not exist, the promise is resolved with boolean false. If the target did exist and was successfully deleted, the promise is resolved with boolean true.

@@ +115,5 @@
> +   * @param path If a DOM string is passed, it is the relative path of the
> +   * target. Otherwise, the File or Directory object of the target should be
> +   * passed.
> +   * @return If the target exists, the promise is resolved with boolean true or
> +   * false to indicate success. Otherwise, rejected with a DOM error.

Same here. Resolved true means that it did exist and was deleted. Resolved false means that it did not exist. Rejected error means that an error occurred.

@@ +152,5 @@
> +   *
> +   * It should be
> +   *   Promise<FileHandleWritable> openWrite((DOMString or File) path,
> +   *                                         OpenWriteOptions options),
> +   * but gecko currently requires the last dictionary argument to be optional.

This should be optional in spec. So gecko is right and spec needs updating.

@@ +187,5 @@
> +callback VoidAnyCallback = void (optional any value);
> +interface AbortableProgressPromise : Promise {
> +  void abort();
> +  AbortableProgressPromise progress(VoidAnyCallback callback);
> +};

Move this into its own .webidl file since it's not specific to Directory.
Attachment #831324 - Flags: feedback?(jonas) → feedback+
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #32)
> Comment on attachment 831324 [details] [diff] [review]
> 0001-WIP-Part1-v3-WebIDL-and-building.patch
> 
> Review of attachment 831324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just looked over the webidl files. Looks good with the below nits.
> 
> You should probably have Baku review the Promise implementation. I'm a bit
> surprised that you don't need to inherit the existing Promise class?

OK, I will. I haven't implemented AbortableProgressPromise in this patch.
The corresponding class implementation are for passing the compiling, so don't inherit
Promise class yet.

> 
> @@ +6,5 @@
> > +
> > +dictionary CreateFileOptions {
> > +  CreateIfExistsMode ifExists = "fail";
> > +  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;
> > +};
> 
> Move this into Directory.webidl with the other dictionaries.

If moving this into Directory.webidl, I get a problem that two generated WebIDL binding
headers will include each others. So I have to leave CreateFileOptions in a sperate 
WebIDL file.
@Jonas, thanks for reviewing the WebIDL. I'll update the WebIDL later to address other parts of comment 32.
> > Move this into Directory.webidl with the other dictionaries.
> 
> If moving this into Directory.webidl, I get a problem that two generated
> WebIDL binding headers will include each others.

That sounds strange since that problem doesn't seem to affect any of the other dictionaries that are currently in Directory.webidl. What's different about CreateFileOptions?

Which two headers end up including each other?

Not a big deal, just was surprised.
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #35)
> > > Move this into Directory.webidl with the other dictionaries.
> > 
> > If moving this into Directory.webidl, I get a problem that two generated
> > WebIDL binding headers will include each others.
> 
> That sounds strange since that problem doesn't seem to affect any of the
> other dictionaries that are currently in Directory.webidl. What's different
> about CreateFileOptions?
> 
> Which two headers end up including each other?
If moving CreateFileOptions into Directory.webidl, the DirectoryBinding.h
(generated from Directory.webidl) and mozilla/dom/UnionTypes.h will include
 each others. 

Because 'CreateFileOptions' is declared in DirectoryBinding.h,
and requires including UnionTypes.h to declare the |data| property of union type
OwningStringOrBlobOrArrayBufferOrArrayBufferView, DirectoryBinding.h will
include UnionTypes.h.

The member function of Directory, such as 
 Promise<boolean> remove((DOMString or File or Directory) path),
has union type as argument and one memeber of the union is Directory, so declaring
these kinds of union type in UnionTypes.h needs to include UnionTypes.h.
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #32)
> ::: dom/webidl/CreateFileOptions.webidl
> @@ +5,5 @@
> > + */
> > +
> > +dictionary CreateFileOptions {
> > +  CreateIfExistsMode ifExists = "fail";
> > +  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;
> 
> we should probably change this to
> 
> (DOMString or Blob or ArrayBuffer or ArrayBufferView)? data = null;
> 
> so that passing null is allowed.
It seems the gecko doesn't allow to set a dictionary property as nullable.
If user don't use the data property, he/she can leave that propety unassigned.
The patch Addresses comment 32 of Jonas:
1. Fix the nits in the WebIDL comments.
2. Create the AbortableProgressPromise.webidl for AbortableProgressPromise and move its dummy implementation for dom/Directory to dom/Promise.

@Arun, please help to verify the spec description in dom/webidl/Directory.webidl. Thanks in advance.
Attachment #831324 - Attachment is obsolete: true
Attachment #831324 - Flags: feedback?(dhylands)
Attachment #8333645 - Flags: review?(dhylands)
Attachment #8333645 - Flags: feedback?(arun)
Comment on attachment 831327 [details] [diff] [review]
0002-WIP-Part2-v3-Implement-getRoot-createDirectory-and-g.patch

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

So I managed to get a chance to read some more of this. I really like the documentation in the TaskBase.h

I figured I'd post what comments I have so far.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3128,5 @@
> +  static const nsString kInvalidChars = NS_LITERAL_STRING(":\0");
> +#elif defined (XP_UNIX)
> +  static const nsString kInvalidChars = NS_LITERAL_STRING("\0");
> +#endif
> +  return kInvalidChars;

So I'm not sure that this is sufficient.

You've made the characters in a filename be OS dependent. But don't they also need to be filesystem dependent?

For example, under linux, the valid characters are different when storing into a file on the ext4 file system versus storing into a file on the VFAT file system. Real sdcards (on those phones that use them) are almost always VFAT. However, internal filesystems (like the Nexus 4) are typically done through a fuse-based filesystem. I'll need to check the list of valid characters available there.

You get the same problem on desktop, where you can have many different types of filesystems available.

@@ +3152,5 @@
> +  for (; cur < end; ++cur) {
> +    if (PRUnichar('\\') == *cur)
> +      *cur = PRUnichar('/');
> +  }
> +#endif

I see this duplicated in DeviceStorageFile::NormalizeFilePath

Perhaps this should be factored out into a standalone function?

::: dom/directory/CreateDirectoryTask.cpp
@@ +75,5 @@
> +void
> +CreateDirectoryTask::Work()
> +{
> +  // Resolve nsIFile from mTargetRealPath.
> +  nsCOMPtr<nsIFile> file = FilesystemUtils::CetLocalFile(mTargetRealPath);

typo: Was this supposed to be GetLocalFile?

It looks like we're passing a fully qualified filename from the child to the parent and that the parent is then just blindly using that path. This is fairly significant security problem. We have to assume that a child can be compromised.

We need to make sure that we only pass a relative path from the child to the parent and that the parent fills in the root portion of the pathname. We also need to ensure that relative path doesn't include any ..'s

::: dom/directory/FilesystemBase.h
@@ +15,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class FilesystemBase : public nsSupportsWeakReference

Could you explain (with a comment) why you're using a weak reference?

::: dom/directory/TaskBase.cpp
@@ +36,5 @@
> +
> +  if (IsParentProcess()) {
> +    // Run in parent process.
> +    if (!mWorkerThread) {
> +      nsresult rv = NS_NewThread(getter_AddRefs(mWorkerThread));

It seems to me that this will wind up launching alot of worker threads (or it may just be that since I don't really understand the lifetime of this particular object there may not be that many threads).

Does it make sense to use the LazyIdleThread (which will go away from time to time and need to respawned, but if it already exists, then it would just use that)
Spawning lots of threads is fairly expensive in terms of memory used for the stack, which is where my concern stems from.

@@ +46,5 @@
> +        return;
> +      }
> +      // Start worker thread
> +      mWorkerThread->Dispatch(this, NS_DISPATCH_NORMAL);
> +    }

nit: Add return and unindent the rest of this function

@@ +66,5 @@
> +  if (!NS_IsMainThread()) {
> +    // Run worker thread tasks
> +    Work();
> +    // Dispatch itself to main thread
> +    NS_DispatchToMainThread(this);

nit: return NS_OK here and you can unindent the rest of this function.

@@ +97,5 @@
> +{
> +  MOZ_ASSERT(IsParentProcess(), "Only call from parent process!");
> +  if (HasError()) {
> +    return FilesystemErrorResponse(mErrorName);
> +  } else {

Drop the else and unindent the next line
(In reply to Dave Hylands [:dhylands] from comment #39)
> Comment on attachment 831327 [details] [diff] [review]
> 0002-WIP-Part2-v3-Implement-getRoot-createDirectory-and-g.patch
> 
> Review of attachment 831327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I managed to get a chance to read some more of this. I really like the
> documentation in the TaskBase.h
> 
> I figured I'd post what comments I have so far.
> 
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +3128,5 @@
> > +  static const nsString kInvalidChars = NS_LITERAL_STRING(":\0");
> > +#elif defined (XP_UNIX)
> > +  static const nsString kInvalidChars = NS_LITERAL_STRING("\0");
> > +#endif
> > +  return kInvalidChars;
> 
> So I'm not sure that this is sufficient.
> 
> You've made the characters in a filename be OS dependent. But don't they
> also need to be filesystem dependent?

It is a problem and it needs to be filesystem dependent. I should reconsider
a way to do this check.

> 
> For example, under linux, the valid characters are different when storing
> into a file on the ext4 file system versus storing into a file on the VFAT
> file system. Real sdcards (on those phones that use them) are almost always
> VFAT. However, internal filesystems (like the Nexus 4) are typically done
> through a fuse-based filesystem. I'll need to check the list of valid
> characters available there.
> 
> You get the same problem on desktop, where you can have many different types
> of filesystems available.
> 
> @@ +3152,5 @@
> > +  for (; cur < end; ++cur) {
> > +    if (PRUnichar('\\') == *cur)
> > +      *cur = PRUnichar('/');
> > +  }
> > +#endif
> 
> I see this duplicated in DeviceStorageFile::NormalizeFilePath
> 
> Perhaps this should be factored out into a standalone function?

I'll add functions to deal with local path conversion.

> 
> ::: dom/directory/CreateDirectoryTask.cpp
> @@ +75,5 @@
> > +void
> > +CreateDirectoryTask::Work()
> > +{
> > +  // Resolve nsIFile from mTargetRealPath.
> > +  nsCOMPtr<nsIFile> file = FilesystemUtils::CetLocalFile(mTargetRealPath);
> 
> typo: Was this supposed to be GetLocalFile?

Yes, a typo.

> 
> It looks like we're passing a fully qualified filename from the child to the
> parent and that the parent is then just blindly using that path. This is
> fairly significant security problem. We have to assume that a child can be
> compromised.
> 
> We need to make sure that we only pass a relative path from the child to the
> parent and that the parent fills in the root portion of the pathname. We
> also need to ensure that relative path doesn't include any ..'s
> 

OK, I'll make change to enforce the security.

> ::: dom/directory/FilesystemBase.h
> @@ +15,5 @@
> > +
> > +namespace mozilla {
> > +namespace dom {
> > +
> > +class FilesystemBase : public nsSupportsWeakReference
> 
> Could you explain (with a comment) why you're using a weak reference?

The comment will be added to the updated path. The reason I use a weak reference
is that when the child window is closed, the child doesn't need to cancel all
background tasks and wait until the they finish. If a background task find
FilesystemBase is null, it should exit silently.
> 
> ::: dom/directory/TaskBase.cpp
> @@ +36,5 @@
> > +
> > +  if (IsParentProcess()) {
> > +    // Run in parent process.
> > +    if (!mWorkerThread) {
> > +      nsresult rv = NS_NewThread(getter_AddRefs(mWorkerThread));
> 
> It seems to me that this will wind up launching alot of worker threads (or
> it may just be that since I don't really understand the lifetime of this
> particular object there may not be that many threads).

A task will launch only one worker thread. Everytime we calling |Directory.createDirectory 
(or |Directory.get|) creates a new task and hence lauches its worker thread.

> 
> Does it make sense to use the LazyIdleThread (which will go away from time
> to time and need to respawned, but if it already exists, then it would just
> use that)
> Spawning lots of threads is fairly expensive in terms of memory used for the
> stack, which is where my concern stems from.

I should use LazyIdleThread.
This patch addresses all parts of Comment 39 except that passing relative path  instead of full path from the child to the parent. I'm going to make another patch to address the passing relative path problem.

The patch fixes the nits and makes following changes:

1. Introduces two functions to help convert the path separators between local file path and dom file path.

2. Uses thread pool to manager threads without creating and destroying threads frequently.
Attachment #831327 - Attachment is obsolete: true
Attachment #831328 - Attachment is obsolete: true
Attachment #831327 - Flags: feedback?(dhylands)
Attachment #8336609 - Flags: feedback?(dhylands)
Attachment #8336609 - Attachment description: 0001-WIP-Part2-v4-Implement-getRoot-createDirectory-and-get.patch → 0002-WIP-Part2-v4-Implement-getRoot-createDirectory-and-get.patch
Attached patch Diff-Part2-v3-v4.patch (obsolete) — Splinter Review
The diff of part2 between v4 and v3 to help review.
It would be interesting to hear how you want to integrate with FileService once you get to implementing FileHandle parts of the FileSystem API.

I'm asking because I quickly went through the patches and it seems you are creating a new thread pool.
I would also suggest to use a better name for new dom/ subdirectory. Maybe dom/filesystem ?
(In reply to Jan Varga [:janv] from comment #43)
> It would be interesting to hear how you want to integrate with FileService
> once you get to implementing FileHandle parts of the FileSystem API.
I haven't considered about the implementing that parts?
> 
> I'm asking because I quickly went through the patches and it seems you are
> creating a new thread pool.
Yes, I create a new thread pool. Maybe we can share the same thread pool with FileService?
Any suggestion?
> I would also suggest to use a better name for new dom/ subdirectory. Maybe
> dom/filesystem ?
It seems dom/filesystem is better than dom/directory. If I changed the directory to dom/filesystem, should I create the namespace dom::mozilla:filesystem?
(In reply to Yuan Xulei [:yxl] from comment #44)
> Yes, I create a new thread pool. Maybe we can share the same thread pool
> with FileService?
> Any suggestion?

Well, this will require much more thinking once we get to the FileHandle stuff, I just wanted to be sure you were aware of it.

> It seems dom/filesystem is better than dom/directory. If I changed the
> directory to dom/filesystem, should I create the namespace
> dom::mozilla:filesystem?

Yeah, I think dom/filesystem is better, I would also use mozilla::dom::filesystem as a C++ namespace for it.
Comment on attachment 8336609 [details] [diff] [review]
0002-WIP-Part2-v4-Implement-getRoot-createDirectory-and-get.patch

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

So I went over everything again, and this looks good to me.

I think it will be important to test everything on the Nexus 4 (i.e. JB) to make sure that everything works as expected.

::: dom/directory/CreateDirectoryTask.cpp
@@ +92,5 @@
> +    SetError(FilesystemUtils::DOM_ERROR_PATH_EXISTS);
> +    return;
> +  }
> +
> +  rv = file->Create(nsIFile::DIRECTORY_TYPE, 0700);

Using 0700 could leave the file unaccessible. Files created on sdcard should have at least group privileges, since sdcard_r and sdcard_rw are groups.

This will also fail (under JB) if executed in the child The child processes have no privileges to open/create files on the sdcard - so all file opens/directory/file creation/deletion has to take place in the parent.
Bug 910498 adds support for the one place in DeviceStorage where we used to open a file in the child.
Attachment #8336609 - Flags: feedback?(dhylands) → feedback+
Comment on attachment 8333645 [details] [diff] [review]
0001-WIP-Part1-v4-WebIDL-and-building.patch

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

I don't think that I have enough experience with this code to give it r+ for review. It looks fine to me, so I'm comfortable giving it feedback+.
Attachment #8333645 - Flags: review?(dhylands) → review+
Comment on attachment 831330 [details] [diff] [review]
0003-WIP-Part3-v3-mochitest.patch

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

Looks reasonable to me.
Comment on attachment 8333645 [details] [diff] [review]
0001-WIP-Part1-v4-WebIDL-and-building.patch

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

Whoops. Meant to do feedback+
Attachment #8333645 - Flags: review+ → feedback+
Attachment #831330 - Flags: feedback?(dhylands) → feedback+
Fix the implementation of AbortableProgressPromise to inherit Promise.

@Jan, could you help to review the patch?
Attachment #8333645 - Attachment is obsolete: true
Attachment #8333645 - Flags: feedback?(arun)
Attachment #8354982 - Flags: review?(Jan.Varga)
Attachment #8354982 - Flags: feedback?(dhylands)
The patch fixes:

1. Create directory with 0777 mode bits instead of 0700.
2. Use virtual DOM path instead of the full local path to pass file path between child and parent.
3. Share the same thread pool with stream transport service.
4. Move the files to dom/filesystem directory. But I still use the namespace mozilla::dom, as it is the default namespace for IPC and dom binding related code and changing it to mozilla::dom::filesystem causes troubles.
Attachment #8336609 - Attachment is obsolete: true
Attachment #8336610 - Attachment is obsolete: true
Attachment #8354985 - Flags: review?(dhylands)
Attachment #8354985 - Flags: review?(Jan.Varga)
Rebased mochitest. Nothing has been changed with the previous patch.
Attachment #831330 - Attachment is obsolete: true
Comment on attachment 8354982 [details] [diff] [review]
Part1 v1 WebIDL and DOM binding for filesystem API

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

I can't really provide any useful feedabck on the AbortableProgressPromise stuff, so I'm just going to clear the feedback flag.
Attachment #8354982 - Flags: feedback?(dhylands)
Comment on attachment 8354982 [details] [diff] [review]
Part1 v1 WebIDL and DOM binding for filesystem API

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

@baku, I want to implement the AbortableProgressPromise which inherits Promise. Could you help to review the related WebIDL and dom binding? Thank you.
Attachment #8354982 - Flags: review?(amarchesini)
Comment on attachment 8354982 [details] [diff] [review]
Part1 v1 WebIDL and DOM binding for filesystem API

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

The WebIDL part looks good but I want to have a comment from annevk about AbortProgressPromise.
There was a discussion about such kind of Promise so probably he can give good advices.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3220,5 @@
>  }
>  
> +already_AddRefed<Promise>
> +nsDOMDeviceStorage::GetRoot()
> +{

why this method?

::: dom/filesystem/Directory.cpp
@@ +10,5 @@
> +#include "nsStringGlue.h"
> +
> +#include "mozilla/dom/AbortableProgressPromise.h"
> +#include "mozilla/dom/DirectoryBinding.h"
> +#include "mozilla/dom/Promise.h"

remove this.

@@ +35,5 @@
> +
> +nsPIDOMWindow*
> +Directory::GetParentObject() const
> +{
> +  return nullptr;

can we have a parent for this object?

@@ +46,5 @@
> +}
> +
> +void
> +Directory::GetName(nsString& retval) const
> +{

aRetval.Truncate();
// TODO

@@ +51,5 @@
> +}
> +
> +already_AddRefed<Promise>
> +Directory::CreateFile(const nsAString& path, const CreateFileOptions& options)
> +{

// TODO

@@ +57,5 @@
> +}
> +
> +already_AddRefed<Promise>
> +Directory::CreateDirectory(const nsAString& aPath)
> +{

// TODO

::: dom/filesystem/Directory.h
@@ +38,5 @@
> +  ~Directory();
> +
> +  // ========= Begin WebIDL bindings. ===========
> +
> +  nsPIDOMWindow* GetParentObject() const;

returnType
method();

here and everywhere in the .h files

@@ +43,5 @@
> +
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  void GetName(nsString& retval) const;

aRetval (and the same pattern everywhere)

@@ +45,5 @@
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  void GetName(nsString& retval) const;
> +
> +  already_AddRefed<Promise> CreateFile(const nsAString& path, const CreateFileOptions& options);

80chars?

@@ +51,5 @@
> +  already_AddRefed<Promise> CreateDirectory(const nsAString& path);
> +
> +  already_AddRefed<Promise> Get(const nsAString& aPath);
> +
> +  already_AddRefed<AbortableProgressPromise> Move(const StringOrFileOrDirectory& path, const StringOrDirectoryOrDestinationDict& dest);

this is definitely not 80chars :)

@@ +67,5 @@
> +
> +  // Mark this as resultNotAddRefed to return raw pointers
> +  already_AddRefed<EventStream> EnumerateDeep(const Optional<nsAString >& path);
> +
> +  // =========== End WebIDL bindings.============

remove this... nothing is after this

::: dom/filesystem/EventStream.cpp
@@ +30,5 @@
> +
> +EventStream*
> +EventStream::GetParentObject() const
> +{
> +  return nullptr;

something here.

@@ +40,5 @@
> +  return EventStreamBinding::Wrap(aCx, aScope, this);
> +}
> +
> +already_AddRefed<Promise>
> +EventStream::ForEach(ForEachCallback& callback)

aCallback + TODO

::: dom/filesystem/EventStream.h
@@ +20,5 @@
> +
> +class Promise;
> +class ForEachCallback;
> +
> +class EventStream MOZ_FINAL : public nsISupports /* Change nativeOwnership in the binding configuration if you don't want this */,

remove these comments

@@ +32,5 @@
> +  EventStream();
> +
> +  ~EventStream();
> +
> +  // TODO: return something sensible here, and change the return type

this comment should go away.

@@ +35,5 @@
> +
> +  // TODO: return something sensible here, and change the return type
> +  EventStream* GetParentObject() const;
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;

In any .h file:

returnType
Method();

@@ +38,5 @@
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  // Mark this as resultNotAddRefed to return raw pointers
> +  already_AddRefed<Promise> ForEach(ForEachCallback& callback);

already_AddRefed<Promise>
ForEach(ForEachCallback& aCallback);

::: dom/promise/AbortableProgressPromise.cpp
@@ +9,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +#define NS_IMPL_CYCLE_COLLECTION_INHERITED_0(_class, _base)                    \

move this in nsCycleCollectionParticipant.h

@@ +46,5 @@
> +}
> +
> +// Mark this as resultNotAddRefed to return raw pointers
> +already_AddRefed<AbortableProgressPromise>
> +AbortableProgressPromise::Progress(VoidAnyCallback& callback)

aCallback

::: dom/webidl/Directory.webidl
@@ +87,5 @@
> +   * but gecko currently requires the last union argument with Dictionary
> +   * memeber to be optional.
> +   */
> +  [NewObject]
> +  AbortableProgressPromise

You should talk with annevk about this new type of promise.
Attachment #8354982 - Flags: review?(amarchesini) → feedback?(annevk)
See https://twitter.com/jaffathecake/status/413240852333359105 for some discussion on promises that can be terminated.
Attachment #8354982 - Flags: feedback?(annevk)
(In reply to Andrea Marchesini (:baku) from comment #55)
> Comment on attachment 8354982 [details] [diff] [review]
> Part1 v1 WebIDL and DOM binding for filesystem API
> 
> Review of attachment 8354982 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The WebIDL part looks good but I want to have a comment from annevk about
> AbortProgressPromise.
> There was a discussion about such kind of Promise so probably he can give
> good advices.
> 
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +3220,5 @@
> >  }
> >  
> > +already_AddRefed<Promise>
> > +nsDOMDeviceStorage::GetRoot()
> > +{
> 
> why this method?

We want to expose the filesystem API thought DeviceStorage interface. See comment 4 for details (https://bugzilla.mozilla.org/show_bug.cgi?id=910412#c4).

> 
> ::: dom/filesystem/Directory.cpp
> @@ +10,5 @@
> > +#include "nsStringGlue.h"
> > +
> > +#include "mozilla/dom/AbortableProgressPromise.h"
> > +#include "mozilla/dom/DirectoryBinding.h"
> > +#include "mozilla/dom/Promise.h"
> 
> remove this.
> 
> @@ +35,5 @@
> > +
> > +nsPIDOMWindow*
> > +Directory::GetParentObject() const
> > +{
> > +  return nullptr;
> 
> can we have a parent for this object?

Yes, but not in this patch. The next patch () will return a window as parent.

> @@ +67,5 @@
> > +
> > +  // Mark this as resultNotAddRefed to return raw pointers
> > +  already_AddRefed<EventStream> EnumerateDeep(const Optional<nsAString >& path);
> > +
> > +  // =========== End WebIDL bindings.============
> 
> remove this... nothing is after this
The next patch will add something after this :-)
> 
> ::: dom/filesystem/EventStream.cpp
> @@ +30,5 @@
> > +
> > +EventStream*
> > +EventStream::GetParentObject() const
> > +{
> > +  return nullptr;
> 
> something here.
> 
I'm going to implement EventStream in another bug (bug 935883), not in this bug.
So I have nothing to return. I'll add a TOTO comment here to remind me to return
something as parent.
 
> ::: dom/webidl/Directory.webidl
> @@ +87,5 @@
> > +   * but gecko currently requires the last union argument with Dictionary
> > +   * memeber to be optional.
> > +   */
> > +  [NewObject]
> > +  AbortableProgressPromise
> 
> You should talk with annevk about this new type of promise.
Thanks. I'm going to discuss and implement AbortableProgressPromise in bug 935883.
In this bug, I'll add a dummy interface with basic interface members to pass the compiling.
(In reply to Anne (:annevk) from comment #56)
> See https://twitter.com/jaffathecake/status/413240852333359105 for some
> discussion on promises that can be terminated.

Thanks for link.
@baku, thank you.

The patch addresses all of comment 55 except a few that I explained in comment 57.
Attachment #8354982 - Attachment is obsolete: true
Attachment #8354982 - Flags: review?(Jan.Varga)
Attachment #8356549 - Flags: review?(amarchesini)
Attachment #8356549 - Flags: review?(Jan.Varga)
Comment on attachment 8354985 [details] [diff] [review]
Part2 v1 Implement getRoot createDirectory and get for Directory.

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

Looks good.

Overall comment. All file operations can only be done from the parent (getting file size, checking existence of a file, etc), so I'd like to see asserts to verify that we're in the parent for all of these. The file operations may work in the child on pre-JB phones, but they won't work on JB or newer (i.e. Nexus 4 ot 5).

So for now, I'm just clearing the review flag.

I also had some concerns about only seeming to use the default storage area? (or maybe I'm just missing something)

::: dom/base/domerr.msg
@@ +69,5 @@
>  
>  DOM_MSG_DEF(NS_ERROR_DOM_INDEXEDDB_RECOVERABLE_ERR, "The operation failed because the database was prevented from taking an action. The operation might be able to succeed if the application performs some recovery steps and retries the entire transaction. For example, there was not enough remaining storage space, or the storage quota was reached and the user declined to give more space to the database.")
>  
> +/* Filesystem DOM errors. */
> +DOM4_MSG_DEF(InvalidAccessError, "Invalid file system path.", NS_ERROR_DOM_FILESYSTEM_INVALID_PATH_ERR)  

nit: trailing space

::: dom/devicestorage/DeviceStorage.h
@@ +326,5 @@
>        DEVICE_STORAGE_TYPE_SHARED,
>        DEVICE_STORAGE_TYPE_EXTERNAL
>    };
> +
> +  nsCOMPtr<DeviceStorageFilesystem> mFilesystem;

nit: My understanding was that you should use nsCOMPtr for nsIXxx classes, and use nsRefPtr for concrete classes (DeviceStorageFileSystem seems to be a concrete class)

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3205,5 @@
>  {
> +  if (!mFilesystem) {
> +    mFilesystem = new DeviceStorageFilesystem(mStorageType, GetOwner());
> +  }
> +  return mozilla::dom::Directory::GetRoot(mFilesystem);

Each nsDOMDeviceStorage object is for a particular storageName.

But this creates a DeviceStorageFileSystem only for the default storage area, which doesn't seem right to me.

Each named storage area will have a different root.

::: dom/filesystem/DeviceStorageFilesystem.cpp
@@ +26,5 @@
> +  if (!DeviceStorageTypeChecker::IsVolumeBased(mStorageType)) {
> +    // The storage name will be the empty string
> +    mStorageName.Truncate();
> +  } else {
> +    nsDOMDeviceStorage::GetDefaultStorageName(mStorageType, mStorageName);

So why is this using the default storage name?

The default storage name is controlled by the user (through the settings app) and can change over time.

If there are multiple storage areas, will this allow them to all be used? Or just the default one?

::: dom/filesystem/FilesystemFile.cpp
@@ +88,5 @@
> +    nsCOMPtr<nsIFile> file = aFilesystem->GetLocalFile(mPath);
> +    if (NS_WARN_IF(!file)) {
> +      return JSVAL_NULL;
> +    }
> +    nsCOMPtr<nsIDOMFile> domFile = new nsDOMFileFile(file);

File operations are only guaranteed to work in the parent (they happen to work on some phones in the child, but in general they only work in the parent). Can we assert that we're in the parent?

If you're only planning on doing filename manipulations in the child then you're ok, but you won't be able to do things like domFile->GetSize() or domFile->GetLastModifiedDate() from the child on JB or newer.

::: dom/filesystem/FilesystemFile.h
@@ +34,5 @@
> +  void GetName(nsAString& aName) const;
> +
> +  JS::Value ToJsValue(JSContext* cx, FilesystemBase* aFilesystem) const;
> +private:
> +  static const PRUnichar kSeparatorChar = PRUnichar('/');

nit: Use char16_t rather than PRUnichar

::: dom/filesystem/FilesystemRequestParent.cpp
@@ +33,5 @@
> +    case FilesystemParams::TFilesystemCreateDirectoryParams: {
> +      FilesystemCreateDirectoryParams& p = mParams;
> +      mFilesystem = FilesystemBase::FromString(p.filesystem());
> +      MOZ_ASSERT(mFilesystem, "Filesystem should not be null.");
> +      task = new CreateDirectoryTask(mFilesystem, p, this);

It seems kind of weird to me to create a new object just to start an operation.

Wouldn't it make more sense to just have a helper function?

This looks weird because you create the task object and don't do anything with it except to destruct it.

Even pulling the Start out of the CreateDirectoryTask constructor would make this "look" better. Then at least you'd be doing:

task = new CreateDirectoryTask(...)
task->Start();

You're probably getting compiler warning about not using the task variable.

@@ +41,5 @@
> +    case FilesystemParams::TFilesystemGetFileOrDirectoryParams: {
> +      FilesystemGetFileOrDirectoryParams& p = mParams;
> +      mFilesystem = FilesystemBase::FromString(p.filesystem());
> +      MOZ_ASSERT(mFilesystem, "Filesystem should not be null.");
> +      task = new GetFileOrDirectoryTask(mFilesystem, p, this);

Similar comment here

::: dom/filesystem/FilesystemUtils.cpp
@@ +25,5 @@
> +  if (NS_WARN_IF(!globalObject)) {
> +    return JSVAL_NULL;
> +  }
> +
> +  JS::Rooted<JSObject*> global(cx, globalObject->GetGlobalJSObject());

Should you assert cx to be non-NULL?

@@ +27,5 @@
> +  }
> +
> +  JS::Rooted<JSObject*> global(cx, globalObject->GetGlobalJSObject());
> +
> +  return OBJECT_TO_JSVAL(aObject->WrapObject(cx, global));

Should you assert aObject to be non-NULL?

@@ +66,5 @@
> +  nsString result;
> +  result = aLocal;
> +#if defined(XP_WIN)
> +  PRUnichar* cur = result.BeginWriting();
> +  PRUnichar* end = result.EndWriting();

nit: PRUnichar needs to be changed to char16_t

See: https://groups.google.com/forum/#!topic/mozilla.dev.platform/HMIS6Wb45sY

::: dom/filesystem/GetFileOrDirectoryTask.cpp
@@ +90,5 @@
> +    return;
> +  }
> +
> +  bool ret;
> +  nsresult rv = file->Exists(&ret);

This only works in the parent, so lets assert that we're in the parent.
Attachment #8354985 - Flags: review?(dhylands)
(In reply to Dave Hylands [:dhylands] from comment #60)
> Comment on attachment 8354985 [details] [diff] [review]
> Part2 v1 Implement getRoot createDirectory and get for Directory.
> 
> Review of attachment 8354985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> Overall comment. All file operations can only be done from the parent
> (getting file size, checking existence of a file, etc), so I'd like to see
> asserts to verify that we're in the parent for all of these. The file
> operations may work in the child on pre-JB phones, but they won't work on JB
> or newer (i.e. Nexus 4 ot 5).
> 
> So for now, I'm just clearing the review flag.
> 
> I also had some concerns about only seeming to use the default storage area?
> (or maybe I'm just missing something)
Dave, thank you:-) I'm quite happy to get your reply.

That's true. Is it a problem that the patch only uses the default storage? I will correct 
it soon as well as other parts.
(In reply to Yuan Xulei [:yxl] from comment #61)
> (In reply to Dave Hylands [:dhylands] from comment #60)
> > Comment on attachment 8354985 [details] [diff] [review]
> > Part2 v1 Implement getRoot createDirectory and get for Directory.
> > 
> > Review of attachment 8354985 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good.
> > 
> > Overall comment. All file operations can only be done from the parent
> > (getting file size, checking existence of a file, etc), so I'd like to see
> > asserts to verify that we're in the parent for all of these. The file
> > operations may work in the child on pre-JB phones, but they won't work on JB
> > or newer (i.e. Nexus 4 ot 5).
> > 
> > So for now, I'm just clearing the review flag.
> > 
> > I also had some concerns about only seeming to use the default storage area?
> > (or maybe I'm just missing something)
> Dave, thank you:-) I'm quite happy to get your reply.
> 
> That's true. Is it a problem that the patch only uses the default storage? I
> will correct 
> it soon as well as other parts.

Since DeviceStorage allows access to all of the storage areas, not just the default one, I assumed that the FileSystem API should allow access to all of the storage areas as well.

Especially, since the location of the default storage area is changable by the user, so when you start your app using the FileSystem API, the default storage area might be in one place, and then the user changes it in the settings app, and goes back to the app using the FileSystem API, things will probably be very confused.

The Gallery, Music, and Video apps scan all of the storage areas not just the default one. The default storage area is primarily used as the location to put new files, since the user doesn't have any other way of specifying that.

But the user is probably interested in seeing all of their files.
Almost the same with the previous version and just fix a few nits about the code format.
Attachment #8356549 - Attachment is obsolete: true
Attachment #8356549 - Flags: review?(amarchesini)
Attachment #8356549 - Flags: review?(Jan.Varga)
Attachment #8358253 - Flags: review?(amarchesini)
Attachment #8358253 - Flags: review?(Jan.Varga)
I'll take a look once I finish other reviews.
Fix all items of comment 60:

1. Format code, remove trailing spaces and add asserts.

2. Use nsRefPtr for concrete classes

3. Support multiple storage areas

4. All file operations are performed in the parent. DOMFile will be created in the parent instead of the child and returned to child as blob by IPC.

5. Use char16_t rather than PRUnichar

6. Add static helper functions on TaskBase for creating TaskBase subclass instances.

7. Merge FilesystemFile into Directory and remove FilesystemFile class.

8. Create root the directory if not exits. See GetFileOrDirectoryTask::Work.
Attachment #8354985 - Attachment is obsolete: true
Attachment #8354985 - Flags: review?(Jan.Varga)
Attachment #8358260 - Flags: review?(dhylands)
Add test_file_get.html to test Directory#get.
Attachment #8354986 - Attachment is obsolete: true
Attachment #8358261 - Flags: review?(dhylands)
(In reply to Jan Varga [:janv] from comment #64)
> I'll take a look once I finish other reviews.

Thank you :)
Attachment #8358260 - Flags: feedback?(Jan.Varga)
Comment on attachment 8358260 [details] [diff] [review]
Part2 v2 Implement getRoot createDirectory and get for Directory.

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

::: dom/filesystem/FilesystemBase.cpp
@@ +17,5 @@
> +already_AddRefed<FilesystemBase>
> +FilesystemBase::FromString(const nsAString& aString)
> +{
> +  if (StringBeginsWith(aString, NS_LITERAL_STRING("devicestorage-"))) {
> +    // The string representation of devicestroage file system is of the format:

nit: typo (devicestroage should be devicestorage)
Attachment #8358260 - Flags: review?(dhylands) → review+
Comment on attachment 8358261 [details] [diff] [review]
Part3 v2 Basic tests for filesystem API

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

::: dom/devicestorage/test/test_fs_createDirectory.html
@@ +74,5 @@
> +      d.createDirectory('..').then(function(what) {
> +        ok(false, "Should not overwrite an existing directory.");
> +        devicestorage_cleanup();
> +      }, function(e) {
> +        ok(true, "Accessubg parent directory with '..' is not allowed.");

nit: typo
Attachment #8358261 - Flags: review?(dhylands) → review+
Ok, I have some time now, where does the spec live these days, http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface ?
(In reply to Jan Varga [:janv] from comment #70)
> Ok, I have some time now, where does the spec live these days,
> http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface ?
Just in the comments of the Directory.webidl file of first patch (Part1 v2 WebIDL and DOM binding for filesystem API).
It is a draft version based on my understanding and reviewed by Jonas in this bug (see comment 22 and comment 32).

AFAIK, we don't have a more detailed one than http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface.
Comment on attachment 8358253 [details] [diff] [review]
Part1 v2 WebIDL and DOM binding for filesystem API

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

::: dom/webidl/Directory.webidl
@@ +89,5 @@
> +   */
> +  [NewObject]
> +  AbortableProgressPromise
> +    move((DOMString or File or Directory) path,
> +         optional (DOMString or Directory or DestinationDict) dest);

Question: If the source and dest are in different storage areas, then this needs to be implemented as a copy/delete. If the source and destination are in the same storage area then this can be a rename or move. So I just thought I'd ask if this had been considered?
(In reply to Dave Hylands [:dhylands] from comment #72)
> Comment on attachment 8358253 [details] [diff] [review]
> Part1 v2 WebIDL and DOM binding for filesystem API
> 
> Review of attachment 8358253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Directory.webidl
> @@ +89,5 @@
> > +   */
> > +  [NewObject]
> > +  AbortableProgressPromise
> > +    move((DOMString or File or Directory) path,
> > +         optional (DOMString or Directory or DestinationDict) dest);
> 
> Question: If the source and dest are in different storage areas, then this
> needs to be implemented as a copy/delete. If the source and destination are
> in the same storage area then this can be a rename or move. So I just
> thought I'd ask if this had been considered?

we need to document this, since the former isn't atomic
(In reply to Dave Hylands [:dhylands] from comment #72)
> Comment on attachment 8358253 [details] [diff] [review]
> Part1 v2 WebIDL and DOM binding for filesystem API
> 
> Review of attachment 8358253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Directory.webidl
> @@ +89,5 @@
> > +   */
> > +  [NewObject]
> > +  AbortableProgressPromise
> > +    move((DOMString or File or Directory) path,
> > +         optional (DOMString or Directory or DestinationDict) dest);
> 
> Question: If the source and dest are in different storage areas, then this
> needs to be implemented as a copy/delete. If the source and destination are
> in the same storage area then this can be a rename or move. So I just
> thought I'd ask if this had been considered?

Yes, I had a rough thought about that in bug935883. See https://bugzilla.mozilla.org/show_bug.cgi?id=935883#c1
@baku, I want to land this bug with the following parts implemented first:

interface DeviceStorage {
  Promise<Directory> getRoot();
};

interface Directory {
  Promise<Directory> createDirectory(DOMString path);
  Promise<(File or Directory)> get(DOMString path);
};

Other parts including the AbortableProgressPromise are going to be discussed and implemented in follow up bugs. I marked them as TODOs in this bug's patch and made dummy implementation for them to pass compiling.

dhylands has reviewed the last two patches. Could you help to review and verify the WebIDL and dom binding implementation of of the first patch (Part1 v2 WebIDL and DOM binding for filesystem API)?
Flags: needinfo?(amarchesini)
That sounds like an ok start to me. Though please don't land functions with dummy implementations. Just remove the functions from the WebIDL instead. That way applications can more easily test which functions are available and which ones aren't.
OK, I'll remove the dummy implementations and update the patches.
Flags: needinfo?(amarchesini)
Remove unimplemented functions from WebIDL.
Attachment #8358253 - Attachment is obsolete: true
Attachment #8358253 - Flags: review?(amarchesini)
Attachment #8358253 - Flags: review?(Jan.Varga)
Attachment #8373944 - Flags: review?(amarchesini)
Fix nits
Attachment #8358260 - Attachment is obsolete: true
Attachment #8358260 - Flags: feedback?(Jan.Varga)
Attachment #8373946 - Flags: review+
Fix nits.
Attachment #8358261 - Attachment is obsolete: true
Attachment #8373948 - Flags: review+
Comment on attachment 8373944 [details] [diff] [review]
Part1 v3 WebIDL and DOM binding for filesystem API

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

It's unfinished. I cannot give you a r+. Can I see the rest of the code when written? Thanks!

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3475,5 @@
>  }
>  
> +already_AddRefed<Promise>
> +nsDOMDeviceStorage::GetRoot()
> +{

TODO

::: dom/filesystem/Directory.cpp
@@ +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/. */
> +
> +#include "Directory.h"

mozilla/dom/Directory.h

@@ +7,5 @@
> +#include "Directory.h"
> +
> +#include "nsStringGlue.h"
> +
> +#include "mozilla/dom/DirectoryBinding.h"

move it to line 8

@@ +52,5 @@
> +
> +already_AddRefed<Promise>
> +Directory::CreateDirectory(const nsAString& aPath)
> +{
> +  // TODO

This will fail. You cannot return nullptr without an error. But I think it's enough for now. New patches will come, I guess.

::: dom/filesystem/Directory.h
@@ +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/. */
> +
> +#ifndef mozilla_dom_Directory_h__

remove __
Attachment #8373944 - Flags: review?(amarchesini) → feedback+
Comment on attachment 8373944 [details] [diff] [review]
Part1 v3 WebIDL and DOM binding for filesystem API

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

Ok I see. there are other patches :)
Attachment #8373944 - Flags: feedback+ → review+
(In reply to Andrea Marchesini (:baku) from comment #81)
...
> 
> ::: dom/filesystem/Directory.h
> @@ +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/. */
> > +
> > +#ifndef mozilla_dom_Directory_h__
> 
> remove __

I found some macros of the header files under the dom directory have the "__"(2 underscores) suffix, or even "___"(3 underscores), but others don't.
When should I add the "__" suffix? I checked the code style guide on MDN (https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style) and didn't find any explanation.
Look in this section: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#C.2FC.2B.2B_practices

The 11th bullet item details include guards, and following that guideine neither trailing nor leading underscores should be used.
Fix issues in Comment 81.
Attachment #8373944 - Attachment is obsolete: true
Attachment #8374676 - Flags: review+
Fix include guards.
Attachment #8373946 - Attachment is obsolete: true
Attachment #8374677 - Flags: review+
Attached patch Part4 v1 Fix DOM API test. (obsolete) — Splinter Review
Filesystem API introduces a new WebIDL interface - Directory. Add the new interface to test_interfaces.html.
Attachment #8374681 - Flags: review?(bzbarsky)
Can you please link me to the spec in question (and add that link to Directory.webidl as well)?  Is it stable enough that we want to be exposing this to web content?
Also, if this is _not_ stable and only needed for DeviceStorage stuff, we should either make it NoInterfaceObject or at least hide it when DeviceStorage is not available.  See bug 971732.
@bz, this is not stable and we only have an editor's draft spec:
http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface
Attachment #8374681 - Attachment is obsolete: true
Attachment #8374681 - Flags: review?(bzbarsky)
Comment on attachment 8374677 [details] [diff] [review]
Part2 v4 Implement getRoot createDirectory and get for Directory.

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

::: dom/filesystem/GetFileOrDirectoryTask.cpp
@@ +190,5 @@
> +        mTargetFile, &NS_GET_IID(nsIDOMFile));
> +    }
> +    NS_WARN_IF(JSVAL_IS_NULL(value));
> +    JS::Rooted<JS::Value> val(cx, value);
> +    mPromise->MaybeResolve(cx, val);

I get a problem with |mPromise->MaybeResolve(cx, val)|. |mPromise->MaybeResolve| will try to get the js |then| property from |val| parameter with the following code:
 JS::Rooted<JS::Value> then(cx);
 JS_GetProperty(cx, val, "then", &then)

The js compartments of cx and val are not the same. It makes debug build crash. How can I resolve this issue?
@dhylands, I got the above issue when rebasing the patch. Any idea to solve it?
Flags: needinfo?(dhylands)
> How can I resolve this issue?

You need to enter the correct compartment.  In general, any time you call a function with a cx and a value the compartments need to match.

Is "val" guaranteed to be an object in this case?

Looking at the code more carefully, WrapperCacheObjectToJsval is completely wrong.  You should be using WrapNewBindingObject instead; that won't clobber existing values.  And you should be entering the compartment of the correct global first; possibly as soon as you put that AutoSafeJSContext on the stack.

InterfaceToJsval is also reinventing some wheels... and should again expect the caller to enter the right compartment.

What you may want a helper for is "get the global JSObject from this nsPIDOMWindow".
On general, I strongly suggest getting a review for this patch from someone familiar with jsapi and bindings...
@bz, thanks. Can you help to review this patch? or recommend someone else for me?
(In reply to Boris Zbarsky [:bz] from comment #93)
> > How can I resolve this issue?
> 
> You need to enter the correct compartment.  In general, any time you call a
> function with a cx and a value the compartments need to match.
> 
> Is "val" guaranteed to be an object in this case?
Yes, "val" is an object.
I can try, but likely not until Monday.  Olli, would you be able to take a look before then, and if so, are you happy doing it?
Flags: needinfo?(bugs)
Attachment #8374676 - Flags: review?(bugs)
Flags: needinfo?(bugs)
(In reply to Yuan Xulei [:yxl] from comment #92)
> @dhylands, I got the above issue when rebasing the patch. Any idea to solve
> it?

Sorry - this a bit out of my depth.
Flags: needinfo?(dhylands)
Update patch based on comment 93.
@bz and @smaug, if either of you can take a look, please help. Thank you ;-)
Attachment #8374677 - Attachment is obsolete: true
Attachment #8377026 - Flags: review?(bzbarsky)
Attachment #8377026 - Flags: review?(bugs)
The above patch fixes GetFileOrDirectoryTask::HandlerCallback of dom/filesystem/GetFileOrDirectoryTask.cpp and CreateDirectoryTask::HandlerCallback of dom/filesystem/CreateDirectoryTask.cpp. It also adds a helper function FilesystemUtils::GetGetGlobalJSObject to dom/filesystem/FilesystemUtils.h for "getting the global JSObject from this nsPIDOMWindow" and removes unused functions of FilesystemUtils. Other parts remain unchanged.
Comment on attachment 8377026 [details] [diff] [review]
Part2 v5 Implement getRoot createDirectory and get for Directory.

All the non-main-thread handling requires plenty of MOZ_ASSERTs.
Like, those nsIFile methods which may do sync IO must not be run in the main thread of parent process.

In case you have mPromise member variable in classes inheriting TaskBase (which is thread safe, and used in several threads), please
document why mPromise is ever used only in one thread. Please add assertions.
Especially in case of parent process only I don't quite understand what guarantees mPromise to be addrefed or released only in the
thread it was created. (Promise objects are cycle collectable, so one must not addref or release them in the wrong thread.)

>+DOM4_MSG_DEF(AbortError, "File already exits.", NS_ERROR_DOM_FILESYSTEM_PATH_EXISTS_ERR)
exists

>+  nsCOMPtr<nsIFile> file = filesystem->GetLocalFile(mTargetRealPath);
>+  if (!file) {
>+    SetError(NS_ERROR_DOM_FILESYSTEM_INVALID_PATH_ERR);
>+    return;
>+  }
>+
>+   bool ret;
>+   nsresult rv = file->Exists(&ret);
some odd indentation here.


>+  // The local path of the root. Only available in the parent process.
>+  // In the child process, we don't use it and its value should be empty.
>+  nsString mLocalRootPath;
>+  nsPIDOMWindow* mWindow;
This is super scary. Nothing guarantees mWindow never points to a dead object.

>+Directory::IsValidRelativePath(const nsString& aPath)
>+{
>+  if (aPath.IsEmpty()) {
>+    return true;
>+  }
>+
>+  // Absolute path is not allowed.
>+  if (aPath.First() == FilesystemUtils::kSeparatorChar) {
>+    return false;
>+  }
>+
>+  static const nsString kCurrentDir = NS_LITERAL_STRING(".");
>+  static const nsString kParentDir = NS_LITERAL_STRING("..");
NS_NAMED_LITERAL_STRING

>+/*
>+ * Helper class for holding and accessing a weak reference of FilesystemBase.
>+ */
>+class FilesystemWeakRef
>+{
>+public:
>+  FilesystemWeakRef(FilesystemBase* aFilesystem);
>+  ~FilesystemWeakRef();
>+
>+  already_AddRefed<FilesystemBase>
>+  Get();
>+private:
>+  nsWeakPtr mFilesystem;
>+};
This is odd. What is wrong with normal do_QueryReferent?

>+class FilesystemRequestParent
>+  : public PFilesystemRequestParent
>+{
>+public:
>+  FilesystemRequestParent(const FilesystemParams& aParams);
>+  ~FilesystemRequestParent();
>+
>+  NS_IMETHOD_(nsrefcnt)
>+  AddRef();
>+
>+  NS_IMETHOD_(nsrefcnt)
>+  Release();
>+
What is wrong with normal thread safe refcounting macros?


>+class TaskBase
TaskBase is way too generic name dealing filesystem stuff, yet live in mozilla::dom.
(HTML spec is all about Tasks, but TaskBase has nothing to do with those tasks.)
So perhaps just FSTaskBase
Attachment #8377026 - Flags: review?(bugs) → review-
Attachment #8374676 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #101)
> Comment on attachment 8377026 [details] [diff] [review]
> Part2 v5 Implement getRoot createDirectory and get for Directory.
...
> >+class FilesystemRequestParent
> >+  : public PFilesystemRequestParent
> >+{
> >+public:
> >+  FilesystemRequestParent(const FilesystemParams& aParams);
> >+  ~FilesystemRequestParent();
> >+
> >+  NS_IMETHOD_(nsrefcnt)
> >+  AddRef();
> >+
> >+  NS_IMETHOD_(nsrefcnt)
> >+  Release();
> >+
> What is wrong with normal thread safe refcounting macros?
Do you mean the macro of NS_DECL_THREADSAFE_ISUPPORTS? FilesystemRequestParent 
doesn't inherit nsISupports, and we only need to add the refcounting functions 
"AddRef" and "Release". But NS_DECL_THREADSAFE_ISUPPORTS will add an extra
function "QueryInterface" besides the refcouting ones. That's why I didn't use 
NS_DECL_THREADSAFE_ISUPPORTS. Is there a macro for this case?
The patch addresses all the items of Comment 101 except the "thread safe refcounting macros" issue.

TaskBase is renamed to FilesystemTaskBase to keep consistent with other class names, such as FilesystemBase, FilesystemUtils.
Attachment #8377026 - Attachment is obsolete: true
Attachment #8377026 - Flags: review?(bzbarsky)
Attachment #8377397 - Flags: review?(bugs)
maybe NS_INLINE_DECL_THREADSAFE_REFCOUNTING(class) ?
Attached patch interdiff of comment 101 (obsolete) — Splinter Review
This is the interdiff to help review the above patch.
(In reply to Jan Varga [:janv] from comment #104)
> maybe NS_INLINE_DECL_THREADSAFE_REFCOUNTING(class) ?

Thank you. It works :)
Fix "thread safe refcounting macros" issue.

The interdiff:

diff --git a/dom/filesystem/FilesystemRequestParent.cpp b/dom/filesystem/FilesystemRequestParent.cpp
--- a/dom/filesystem/FilesystemRequestParent.cpp
+++ b/dom/filesystem/FilesystemRequestParent.cpp
@@ -7,19 +7,16 @@
 
 #include "mozilla/dom/FilesystemBase.h"
 #include "mozilla/dom/FilesystemTaskBase.h"
 #include "mozilla/dom/FilesystemUtils.h"
 
 namespace mozilla {
 namespace dom {
 
-NS_IMPL_ADDREF(FilesystemRequestParent)
-NS_IMPL_RELEASE(FilesystemRequestParent)
-
 FilesystemRequestParent::FilesystemRequestParent(
   const FilesystemParams& aParams)
   : mParams(aParams)
 {
 }
 
 FilesystemRequestParent::~FilesystemRequestParent()
 {
diff --git a/dom/filesystem/FilesystemRequestParent.h b/dom/filesystem/FilesystemRequestParent.h
--- a/dom/filesystem/FilesystemRequestParent.h
+++ b/dom/filesystem/FilesystemRequestParent.h
@@ -14,38 +14,30 @@
 namespace mozilla {
 namespace dom {
 
 class FilesystemBase;
 
 class FilesystemRequestParent
   : public PFilesystemRequestParent
 {
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(FilesystemRequestParent)
 public:
   FilesystemRequestParent(const FilesystemParams& aParams);
   ~FilesystemRequestParent();
 
-  NS_IMETHOD_(nsrefcnt)
-  AddRef();
-
-  NS_IMETHOD_(nsrefcnt)
-  Release();
-
   bool
   IsRunning()
   {
     return state() == PFilesystemRequest::__Start;
   }
 
   void
   Dispatch();
 private:
-  ThreadSafeAutoRefCnt mRefCnt;
-  NS_DECL_OWNINGTHREAD
-
   FilesystemParams mParams;
   nsCOMPtr<FilesystemBase> mFilesystem;
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_FilesystemRequestParent_h
Attachment #8377397 - Attachment is obsolete: true
Attachment #8377397 - Flags: review?(bugs)
Attachment #8377400 - Flags: review?(bugs)
Update test. Fix the test_fs_get.html failure on emulator.
Attachment #8373948 - Attachment is obsolete: true
Attachment #8377407 - Flags: review+
Comment on attachment 8377400 [details] [diff] [review]
Part2 v7 Implement getRoot createDirectory and get for Directory.

>+  nsDOMDeviceStorage* mDeviceStorage;
This another super scary member variable.
I don't see anything making sure mDeviceStorage can't point to a deleted object.
Especially, nothing sets it to null.
So, who is supposed to own what and how long?
I'd like to get answer to that and some fixes to the patch before reviewing the rest.
Attachment #8377400 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #109)
> Comment on attachment 8377400 [details] [diff] [review]
> Part2 v7 Implement getRoot createDirectory and get for Directory.
> 
> >+  nsDOMDeviceStorage* mDeviceStorage;
> This another super scary member variable.
> I don't see anything making sure mDeviceStorage can't point to a deleted
> object.
> Especially, nothing sets it to null.
> So, who is supposed to own what and how long?
> I'd like to get answer to that and some fixes to the patch before reviewing
> the rest.

nsDOMDeviceStorage owns DeviceStorageFilesystem. It creates DeviceStorageFilesystem with nsDOMDeviceStorage#GetRoot and releases DeviceStorageFilesystem when it destructs.
(In reply to Olli Pettay [:smaug] from comment #109)
> Comment on attachment 8377400 [details] [diff] [review]
> Part2 v7 Implement getRoot createDirectory and get for Directory.
> 
> >+  nsDOMDeviceStorage* mDeviceStorage;
> This another super scary member variable.
> I don't see anything making sure mDeviceStorage can't point to a deleted
> object.
> Especially, nothing sets it to null.
> So, who is supposed to own what and how long?
> I'd like to get answer to that and some fixes to the patch before reviewing
> the rest.
This patch adds the |SetDeviceStorage| function to DeviceStorageFilesystem class to set mDeviceStorage to null when mDeviceStorage is deleted.

Also I refactored TaskBase class to FilesystemTaskBase and removed its static StartXXXTask functions to make it thinner.
Attachment #8377398 - Attachment is obsolete: true
Attachment #8377400 - Attachment is obsolete: true
Attachment #8378852 - Flags: review?(bugs)
Sorry, this is the updated patch.
Attachment #8378852 - Attachment is obsolete: true
Attachment #8378852 - Flags: review?(bugs)
Attachment #8378854 - Flags: review?(bugs)
The interdiff of part2 between the above patch and the one before the previous.
Add permission request and check logic, which is similar to the original DeviceStorageRequest.
Attachment #8379023 - Flags: review?(dhylands)
Most of the code is copied from test_app_permissions.html of devicestorage.
Attachment #8379026 - Flags: review?(dhylands)
Comment on attachment 8379023 [details] [diff] [review]
Part4 v1 permission request and check

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

You patches aren't including much context. If you're using git to create the patches, please use -U8. If you're using mercurial, please see https://developer.mozilla.org/en/docs/Installing_Mercurial (Basic Configuration section).

Otherwise, looks good.

::: dom/filesystem/DeviceStorageFilesystem.cpp
@@ +32,5 @@
>    mString.AppendLiteral("-");
>    mString.Append(mStorageName);
>  
> +  mIsTesting =
> +    mozilla::Preferences::GetBool("device.storage.prompt.testing", false);

nit: Preferences only work on the main thread, so we should assert that we're on the main thread (you might be doing this already, I just can't tell)

::: dom/filesystem/FilesystemPermissionRequest.cpp
@@ +84,5 @@
> +  return CreatePermissionArray(mPermissionType, mPermissionAccess, aTypes);
> +}
> +
> +NS_IMETHODIMP
> +FilesystemPermissionRequest::GetPrincipal(nsIPrincipal * *aRequestingPrincipal)

nit: *'s to the left (also a few places below)

::: dom/filesystem/FilesystemPermissionRequest.h
@@ +42,5 @@
> +  NS_DECL_NSICONTENTPERMISSIONREQUEST
> +  NS_DECL_NSIRUNNABLE
> +private:
> +  FilesystemPermissionRequest(FilesystemTaskBase* aTask);
> +  ~FilesystemPermissionRequest();

nit: should be declared virtual (it inherits the virtualness from the base, but it also acts as a form of documentaiton)
Attachment #8379023 - Flags: review?(dhylands) → review+
(trying to review this tomorrow.)
Comment on attachment 8379026 [details] [diff] [review]
Part5 v1 Test for checking permission

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

r=me with a few more test cases added

::: dom/devicestorage/test/test_fs_app_permissions.html
@@ +109,5 @@
> +    type: 'music',
> +    shouldPass: false,
> +    fileExtension: '.ogg',
> +    test: TestGet
> +  },

So I can think of a few more test cases to add:

.ogg and .mp4 can both be either video or music, so they should get picked up by either one.

We should make sure that wrong file types are ignored (so creating a .png and then reading using music shouldn't pick up the png file).
Attachment #8379026 - Flags: review?(dhylands) → review+
(In reply to Dave Hylands [:dhylands] from comment #116)
> Comment on attachment 8379023 [details] [diff] [review]
> Part4 v1 permission request and check
> 
> Review of attachment 8379023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You patches aren't including much context. If you're using git to create the
> patches, please use -U8. If you're using mercurial, please see
> https://developer.mozilla.org/en/docs/Installing_Mercurial (Basic
> Configuration section).
Thanks. I'll provide hg patch next time:)

> 
> Otherwise, looks good.
> 
> ::: dom/filesystem/DeviceStorageFilesystem.cpp
> @@ +32,5 @@
> >    mString.AppendLiteral("-");
> >    mString.Append(mStorageName);
> >  
> > +  mIsTesting =
> > +    mozilla::Preferences::GetBool("device.storage.prompt.testing", false);
> 
> nit: Preferences only work on the main thread, so we should assert that
> we're on the main thread (you might be doing this already, I just can't tell)
I'll fix it.
> 
> ::: dom/filesystem/FilesystemPermissionRequest.cpp
> @@ +84,5 @@
> > +  return CreatePermissionArray(mPermissionType, mPermissionAccess, aTypes);
> > +}
> > +
> > +NS_IMETHODIMP
> > +FilesystemPermissionRequest::GetPrincipal(nsIPrincipal * *aRequestingPrincipal)
> 
> nit: *'s to the left (also a few places below)
Should be fixed:)
> 
> ::: dom/filesystem/FilesystemPermissionRequest.h
> @@ +42,5 @@
> > +  NS_DECL_NSICONTENTPERMISSIONREQUEST
> > +  NS_DECL_NSIRUNNABLE
> > +private:
> > +  FilesystemPermissionRequest(FilesystemTaskBase* aTask);
> > +  ~FilesystemPermissionRequest();
> 
> nit: should be declared virtual (it inherits the virtualness from the base,
> but it also acts as a form of documentaiton)
FilesystemPermissionRequest is a final class and we don't delete it with a base pointer. Should we still declare virtual base for it?
(In reply to Dave Hylands [:dhylands] from comment #118)
> Comment on attachment 8379026 [details] [diff] [review]
> Part5 v1 Test for checking permission
> 
> Review of attachment 8379026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with a few more test cases added
> 
> ::: dom/devicestorage/test/test_fs_app_permissions.html
> @@ +109,5 @@
> > +    type: 'music',
> > +    shouldPass: false,
> > +    fileExtension: '.ogg',
> > +    test: TestGet
> > +  },
> 
> So I can think of a few more test cases to add:
> 
> .ogg and .mp4 can both be either video or music, so they should get picked
> up by either one.
> 
> We should make sure that wrong file types are ignored (so creating a .png
> and then reading using music shouldn't pick up the png file).
This Filesystem API doesn't do such kind of type check. We don't prevent reading 
a .png file under the music device storage. And it seems the original DeviceStorage 
API doesn't prevent adding or reading a .png file under the music device storage.

If we need to enforce this, I'll implement it.
(In reply to Yuan Xulei [:yxl] from comment #120)
> (In reply to Dave Hylands [:dhylands] from comment #118)
> > Comment on attachment 8379026 [details] [diff] [review]
> > Part5 v1 Test for checking permission
> > 
> > Review of attachment 8379026 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with a few more test cases added
> > 
> > ::: dom/devicestorage/test/test_fs_app_permissions.html
> > @@ +109,5 @@
> > > +    type: 'music',
> > > +    shouldPass: false,
> > > +    fileExtension: '.ogg',
> > > +    test: TestGet
> > > +  },
> > 
> > So I can think of a few more test cases to add:
> > 
> > .ogg and .mp4 can both be either video or music, so they should get picked
> > up by either one.
> > 
> > We should make sure that wrong file types are ignored (so creating a .png
> > and then reading using music shouldn't pick up the png file).
> This Filesystem API doesn't do such kind of type check. We don't prevent
> reading 
> a .png file under the music device storage. And it seems the original
> DeviceStorage 
> API doesn't prevent adding or reading a .png file under the music device
> storage.
> 
> If we need to enforce this, I'll implement it.
I made a mistake. The original DeviceStorage API does checks the file type when adding or reading files. So I'll update my patch to add similar function.
(In reply to Yuan Xulei [:yxl] from comment #120)
> (In reply to Dave Hylands [:dhylands] from comment #118)
> > Comment on attachment 8379026 [details] [diff] [review]
...snip...
> a .png file under the music device storage. And it seems the original
> DeviceStorage 
> API doesn't prevent adding or reading a .png file under the music device
> storage.
> 
> If we need to enforce this, I'll implement it.

On the phone, sdcard, music, video, and pictures are all stored in the same directory tree (the fact that there happens to be a Photos folder is just a user convenience. For music, cover art might be stored in the same directory as the music files.

So any file you store in music can be enumerated by sdcard or vice-versa.

Hence the issue with .ogg and .mp4 files which could contain music or video.
Attachment #8381349 - Attachment description: Interdiff for Comment 15 → Interdiff for Comment 116
Fix items of comment 116 and make sure the file type is correct.
Attachment #8379023 - Attachment is obsolete: true
Attachment #8381352 - Flags: review?(dhylands)
Comment on attachment 8381352 [details] [diff] [review]
Part4 v2 permission request and check

The interdiff with the previous version:
https://bugzilla.mozilla.org/attachment.cgi?id=8381349&action=diff
Add more test cases as comment 118 mentioned.
The interdiff with previous version:
https://bugzilla.mozilla.org/attachment.cgi?id=8381352&action=diff
Attachment #8379026 - Attachment is obsolete: true
Attachment #8381356 - Flags: review?(dhylands)
Comment on attachment 8381352 [details] [diff] [review]
Part4 v2 permission request and check

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

Just a minor potential issue with the descendant logic.

::: dom/filesystem/FilesystemUtils.cpp
@@ +70,5 @@
> +  prefix = aPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR);
> +
> +  // Check the sub-directory path to see if it has the parent path as prefix.
> +  if (aDescendantPath.Length() < prefix.Length() ||
> +      !StringBeginsWith(aDescendantPath, prefix)) {

If aDescendantPath is greater in length than prefix, it might not be a descendant if the prefix matches.

If prefix ends with a directory separator, then this logic is fine.

Put if prefix was /sdcard/someDir
and aDescendant was /sdcard/someDirElsewhere/

then descendant passes your test but isn't a descendant.
Attachment #8381352 - Flags: review?(dhylands) → review+
(In reply to Yuan Xulei [:yxl] from comment #120)
> (In reply to Dave Hylands [:dhylands] from comment #118)
> > Comment on attachment 8379026 [details] [diff] [review]
> > Part5 v1 Test for checking permission
> > 
> > Review of attachment 8379026 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with a few more test cases added
> > 
> > ::: dom/devicestorage/test/test_fs_app_permissions.html
> > @@ +109,5 @@
> > > +    type: 'music',
> > > +    shouldPass: false,
> > > +    fileExtension: '.ogg',
> > > +    test: TestGet
> > > +  },
> > 
> > So I can think of a few more test cases to add:
> > 
> > .ogg and .mp4 can both be either video or music, so they should get picked
> > up by either one.
> > 
> > We should make sure that wrong file types are ignored (so creating a .png
> > and then reading using music shouldn't pick up the png file).
> This Filesystem API doesn't do such kind of type check. We don't prevent
> reading 
> a .png file under the music device storage. And it seems the original
> DeviceStorage 
> API doesn't prevent adding or reading a .png file under the music device
> storage.
> 
> If we need to enforce this, I'll implement it.

device storage does that checking here:
http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#2622
(In reply to Yuan Xulei [:yxl] from comment #119)
> (In reply to Dave Hylands [:dhylands] from comment #116)
> > ::: dom/filesystem/FilesystemPermissionRequest.h
> > @@ +42,5 @@
> > > +  NS_DECL_NSICONTENTPERMISSIONREQUEST
> > > +  NS_DECL_NSIRUNNABLE
> > > +private:
> > > +  FilesystemPermissionRequest(FilesystemTaskBase* aTask);
> > > +  ~FilesystemPermissionRequest();
> > 
> > nit: should be declared virtual (it inherits the virtualness from the base,
> > but it also acts as a form of documentaiton)
> FilesystemPermissionRequest is a final class and we don't delete it with a
> base pointer. Should we still declare virtual base for it?

I think that the "rule" or "guideline" is that: if a class contains virtual methods then it should have a virtual destructor. Whether you destruct through a base pointer or not shouldn't matter, especially since somebody could change the code down the road and then get weird behaviour.

If a base class has a virtual destructor then the derived destructor will be virtual regardless of whether its declared virtual or not.

So since it is, in fact, virtual, we should mention this as a form of documentation. Otherwise people looking at this may think that you forgot to declare it virtual.
Comment on attachment 8381356 [details] [diff] [review]
Part5 v2 Test for checking permission

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

r=me with issue addressed.

::: dom/devicestorage/test/test_fs_app_permissions.html
@@ +153,5 @@
> +    type: 'videos',
> +    shouldPass: true,
> +    fileExtension: '.ogg',
> +
> +    permissions: ["device-storage:music"],

Shouldn't the permission match the type? Otherwise, this should fail, not pass.

@@ +212,5 @@
> +    shouldPass: true,
> +    fileExtension: '.ogg',
> +
> +    app: "https://example.com/manifest_cert.webapp",
> +    permissions: ["device-storage:music"],

Similary here.
Attachment #8381356 - Flags: review?(dhylands) → review+
Comment on attachment 8378854 [details] [diff] [review]
Part2 v8 Implement getRoot createDirectory and get for Directory.


>+  rv = file->Create(nsIFile::DIRECTORY_TYPE, 0777);
Do we really want 0777 here? I guess so, and DeviceStorage etc do that too.


>+CreateDirectoryTask::HandlerCallback()
>+{
>+  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>+  nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
>+  if (!filesystem) {
>+    return;
>+  }
>+
>+  AutoSafeJSContext cx;
>+  JSObject* global =
>+    FilesystemUtils::GetGetGlobalJSObject(filesystem->GetWindow());
>+  if (!global) {
>+    return;
>+  }
>+  JSAutoCompartment ac(cx, global);
>+  JS::Rooted<JSObject*> scopeObj(cx, global);
>+
>+  JS::Rooted<JS::Value> val(cx);
>+  if (!HasError()) {
>+    nsRefPtr<Directory> dir = new Directory(filesystem, mTargetRealPath);
>+    if (!dom::WrapNewBindingObject(cx, scopeObj, dir, &val)) {
>+      return;
>+    }
>+    mPromise->MaybeResolve(cx, val);
>+  } else {
>+    nsRefPtr<DOMError> domError = new DOMError(filesystem->GetWindow(),
>+      mErrorValue);
>+    if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
>+      return;
>+    }
>+    mPromise->MaybeReject(cx, val);
>+  }
>+}
This all will become a lot simpler once Bug 974120 lands.
Do you think you could base your patch on top of that.


>+GetFileOrDirectoryTask::HandlerCallback()
>+{
>+  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>+  nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
>+  if (!filesystem) {
>+    return;
>+  }
>+
>+  AutoSafeJSContext cx;
>+  JSObject* global =
>+    FilesystemUtils::GetGetGlobalJSObject(filesystem->GetWindow());
>+  if (!global) {
>+    return;
>+  }
>+  JSAutoCompartment ac(cx, global);
>+  JS::Rooted<JSObject*> scopeObj(cx, global);
>+
>+  JS::Rooted<JS::Value> val(cx);
>+  if (!HasError()) {
>+    if (mIsDirectory) {
>+      nsRefPtr<Directory> dir = new Directory(filesystem, mTargetRealPath);
>+      if (!dom::WrapNewBindingObject(cx, scopeObj, dir, &val)) {
>+        return;
>+      }
>+    } else {
>+      if (NS_FAILED(nsContentUtils::WrapNative(cx, scopeObj, mTargetFile,
>+          &NS_GET_IID(nsIDOMFile), &val))) {
>+        return;
>+      }
>+    }
>+    mPromise->MaybeResolve(cx, val);
>+  } else {
>+    nsRefPtr<DOMError> domError = new DOMError(filesystem->GetWindow(),
>+      mErrorValue);
>+    if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
>+      return;
>+    }
>+    mPromise->MaybeReject(cx, val);
>+  }
Also this would be simpler on top of bug 974120.

So, I'd like to see a new patch on top of bug 974120, since that way there should be less
error prone JS API and binding API usage.
Attachment #8378854 - Flags: review?(bugs)
(In reply to Dave Hylands [:dhylands] from comment #128)
> Comment on attachment 8381352 [details] [diff] [review]
> Part4 v2 permission request and check
> 
> Review of attachment 8381352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a minor potential issue with the descendant logic.
> 
> ::: dom/filesystem/FilesystemUtils.cpp
> @@ +70,5 @@
> > +  prefix = aPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR);
> > +
> > +  // Check the sub-directory path to see if it has the parent path as prefix.
> > +  if (aDescendantPath.Length() < prefix.Length() ||
> > +      !StringBeginsWith(aDescendantPath, prefix)) {
> 
> If aDescendantPath is greater in length than prefix, it might not be a
> descendant if the prefix matches.
> 
> If prefix ends with a directory separator, then this logic is fine.
> 
> Put if prefix was /sdcard/someDir
> and aDescendant was /sdcard/someDirElsewhere/
> 
> then descendant passes your test but isn't a descendant.
That's what the patch has done. The prefix will always ends with a directory separator:
prefix = aPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR);
(In reply to Dave Hylands [:dhylands] from comment #130)
> (In reply to Yuan Xulei [:yxl] from comment #119)
> > (In reply to Dave Hylands [:dhylands] from comment #116)
> > > ::: dom/filesystem/FilesystemPermissionRequest.h
> > > @@ +42,5 @@
> > > > +  NS_DECL_NSICONTENTPERMISSIONREQUEST
> > > > +  NS_DECL_NSIRUNNABLE
> > > > +private:
> > > > +  FilesystemPermissionRequest(FilesystemTaskBase* aTask);
> > > > +  ~FilesystemPermissionRequest();
> > > 
> > > nit: should be declared virtual (it inherits the virtualness from the base,
> > > but it also acts as a form of documentaiton)
> > FilesystemPermissionRequest is a final class and we don't delete it with a
> > base pointer. Should we still declare virtual base for it?
> 
> I think that the "rule" or "guideline" is that: if a class contains virtual
> methods then it should have a virtual destructor. Whether you destruct
> through a base pointer or not shouldn't matter, especially since somebody
> could change the code down the road and then get weird behaviour.
> 
> If a base class has a virtual destructor then the derived destructor will be
> virtual regardless of whether its declared virtual or not.
> 
> So since it is, in fact, virtual, we should mention this as a form of
> documentation. Otherwise people looking at this may think that you forgot to
> declare it virtual.

Thanks for clarification. I'll fix them all in my patch:)
(In reply to Olli Pettay [:smaug] from comment #132)
> Comment on attachment 8378854 [details] [diff] [review]
> Part2 v8 Implement getRoot createDirectory and get for Directory.
> 
> 
> >+  rv = file->Create(nsIFile::DIRECTORY_TYPE, 0777);
> Do we really want 0777 here? I guess so, and DeviceStorage etc do that too.
It just follows the DeviceStorage and see comment 46 (https://bugzilla.mozilla.org/show_bug.cgi?id=910412#c46) for details. It might be restricted to 0770, but I 
haven't test it.
> 
> 
> >+CreateDirectoryTask::HandlerCallback()
> >+{
> >+  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> >+  nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
> >+  if (!filesystem) {
> >+    return;
> >+  }
> >+
> >+  AutoSafeJSContext cx;
> >+  JSObject* global =
> >+    FilesystemUtils::GetGetGlobalJSObject(filesystem->GetWindow());
> >+  if (!global) {
> >+    return;
> >+  }
> >+  JSAutoCompartment ac(cx, global);
> >+  JS::Rooted<JSObject*> scopeObj(cx, global);
> >+
> >+  JS::Rooted<JS::Value> val(cx);
> >+  if (!HasError()) {
> >+    nsRefPtr<Directory> dir = new Directory(filesystem, mTargetRealPath);
> >+    if (!dom::WrapNewBindingObject(cx, scopeObj, dir, &val)) {
> >+      return;
> >+    }
> >+    mPromise->MaybeResolve(cx, val);
> >+  } else {
> >+    nsRefPtr<DOMError> domError = new DOMError(filesystem->GetWindow(),
> >+      mErrorValue);
> >+    if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
> >+      return;
> >+    }
> >+    mPromise->MaybeReject(cx, val);
> >+  }
> >+}
> This all will become a lot simpler once Bug 974120 lands.
> Do you think you could base your patch on top of that.
> 
> 
> >+GetFileOrDirectoryTask::HandlerCallback()
> >+{
> >+  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> >+  nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
> >+  if (!filesystem) {
> >+    return;
> >+  }
> >+
> >+  AutoSafeJSContext cx;
> >+  JSObject* global =
> >+    FilesystemUtils::GetGetGlobalJSObject(filesystem->GetWindow());
> >+  if (!global) {
> >+    return;
> >+  }
> >+  JSAutoCompartment ac(cx, global);
> >+  JS::Rooted<JSObject*> scopeObj(cx, global);
> >+
> >+  JS::Rooted<JS::Value> val(cx);
> >+  if (!HasError()) {
> >+    if (mIsDirectory) {
> >+      nsRefPtr<Directory> dir = new Directory(filesystem, mTargetRealPath);
> >+      if (!dom::WrapNewBindingObject(cx, scopeObj, dir, &val)) {
> >+        return;
> >+      }
> >+    } else {
> >+      if (NS_FAILED(nsContentUtils::WrapNative(cx, scopeObj, mTargetFile,
> >+          &NS_GET_IID(nsIDOMFile), &val))) {
> >+        return;
> >+      }
> >+    }
> >+    mPromise->MaybeResolve(cx, val);
> >+  } else {
> >+    nsRefPtr<DOMError> domError = new DOMError(filesystem->GetWindow(),
> >+      mErrorValue);
> >+    if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
> >+      return;
> >+    }
> >+    mPromise->MaybeReject(cx, val);
> >+  }
> Also this would be simpler on top of bug 974120.
> 
> So, I'd like to see a new patch on top of bug 974120, since that way there
> should be less
> error prone JS API and binding API usage.
OK, I'll provide the patch soon:)
Attached patch interdiff for Comment 132 (obsolete) — Splinter Review
Attachment #8381349 - Attachment is obsolete: true
Attachment #8381351 - Attachment is obsolete: true
rebased.
Attachment #8374676 - Attachment is obsolete: true
Attachment #8382005 - Flags: review+
Attachment #8382005 - Attachment description: 0001-Bug-910412-WebIDL-and-DOM-binding-for-filesystem-API.patch → Part1 v4 WebIDL and DOM binding for filesystem API
Fix the items of comment 132. The interdiff:
https://bugzilla.mozilla.org/attachment.cgi?id=8381995&action=diff
Attachment #8378854 - Attachment is obsolete: true
Attachment #8378858 - Attachment is obsolete: true
Attachment #8382011 - Flags: review?(bugs)
rebased.
Attachment #8377407 - Attachment is obsolete: true
Attachment #8382013 - Flags: review+
Rebase and address Comment 130.
Attachment #8381352 - Attachment is obsolete: true
Attachment #8382014 - Flags: review+
Address Comment 131.
Attachment #8381356 - Attachment is obsolete: true
Attachment #8382017 - Flags: review+
Comment on attachment 8382011 [details] [diff] [review]
Part2 v9 Implement getRoot createDirectory and get for Directory

Shouldn't everything named *Filesystem* be actually *FileSystem*

>+Directory::DOMPathToRealPath(const nsAString& aPath, nsAString& aRealPath) const
>+{
>+  aRealPath.Truncate();
>+
>+  nsString relativePath;
>+  relativePath = aPath;
>+
>+  // Trim white spaces.
>+  static const char kWhitespace[] = "\b\t\r\n ";
>+  relativePath.Trim(kWhitespace);
>+
>+  // Remove the leading "./".
>+  if (StringBeginsWith(relativePath, NS_LITERAL_STRING("./"))) {
What if relativePath is something like "./././foobar"?
In other words, I don't quite understand what this leading "./"
removal is for.
IsValidRelativePath seems to then later reject paths with ./ but why
why ./ is ok but two isn't?


>+  // Remove trailing "/".
>+  relativePath.Trim(FILESYSTEM_DOM_PATH_SEPARATOR, false, true);
And do we need to care about if relativePath ends with several "/" ?
>+FilesystemTaskBase::FilesystemTaskBase(FilesystemBase* aFilesystem,
>+                   const FilesystemParams& aParam,
>+                                       FilesystemRequestParent* aParent)
Align the latter two params under the first one


>+FilesystemTaskBase::Start()
>+{
>+  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>+
>+  if (HasError()) {
>+    HandlerCallback();
>+    return;
>+  }
>+
>+  if (FilesystemUtils::IsParentProcess()) {
>+    // Run in parent process.
>+    // Start worker thread.
>+    nsCOMPtr<nsIEventTarget> target
>+      = do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
>+    NS_ASSERTION(target, "Must have stream transport service.");
>+    target->Dispatch(this, NS_DISPATCH_NORMAL);
>+    return;
>+  }
>+
>+  // Run in child process.
>+  nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
>+  if (!filesystem) {
>+    return;
>+  }
>+
>+  // Retain a reference so the task object isn't deleted without IPDL's
>+  // knowledge. The reference will be released by
>+  // mozilla::dom::ContentChild::DeallocPFilesystemRequestChild.
>+  AddRef();
NS_ADDREF_THIS();

>+  /*
>+   * To create a parent process task delivered from the child process through
>+   * IPC.
>+   */
>+  FilesystemTaskBase(FilesystemBase* aFilesystem,
>+           const FilesystemParams& aParam,
>+           FilesystemRequestParent* aParent);
Align params



No need to ask another review for coding style nits or renames, just fix before landing,
but I'd like to get some explanation why path string handling is what it is.
So conditional r+, waiting for explanation and possible another patch on top of this
changing the path handling.
Attachment #8382011 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #142)
> Comment on attachment 8382011 [details] [diff] [review]
> Part2 v9 Implement getRoot createDirectory and get for Directory
--snip--
> 
> No need to ask another review for coding style nits or renames, just fix
> before landing,
> but I'd like to get some explanation why path string handling is what it is.
> So conditional r+, waiting for explanation and possible another patch on top
> of this
> changing the path handling.
The relative path should not contain any segment of ".." or ".", and not start 
or end with "/". This patch tried to correct the path.

I'd like to change patch to make strict checks on the path and don't allow the 
following invalid paths:
"../foo", "..", "foo/../bar", "/foo/bar", "./", "foo/./bar", "bar/", ""bar/."
Fix path string handling issue based on the above two comments (comment 142 and 143). I'll rename all *Filesystem*s to *FileSystem*s after this gets reviewed.

The interdiff:

diff --git a/dom/filesystem/Directory.cpp b/dom/filesystem/Directory.cpp
--- a/dom/filesystem/Directory.cpp
+++ b/dom/filesystem/Directory.cpp
@@ -129,51 +129,38 @@ Directory::DOMPathToRealPath(const nsASt
 
   nsString relativePath;
   relativePath = aPath;
 
   // Trim white spaces.
   static const char kWhitespace[] = "\b\t\r\n ";
   relativePath.Trim(kWhitespace);
 
-  // Remove the leading "./".
-  if (StringBeginsWith(relativePath, NS_LITERAL_STRING("./"))) {
-    relativePath = Substring(relativePath, 2);
-  } else {
-    relativePath = relativePath;
-  }
-
-  // Remove trailing "/".
-  relativePath.Trim(FILESYSTEM_DOM_PATH_SEPARATOR, false, true);
-
-  // We don't allow empty relative path to access the root.
-  if (relativePath.IsEmpty()) {
-    return false;
-  }
-
   if (!IsValidRelativePath(relativePath)) {
     return false;
   }
 
   aRealPath = mPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR) +
     relativePath;
 
   return true;
 }
 
 // static
 bool
 Directory::IsValidRelativePath(const nsString& aPath)
 {
+  // We don't allow empty relative path to access the root.
   if (aPath.IsEmpty()) {
-    return true;
+    return false;
   }
 
-  // Absolute path is not allowed.
-  if (aPath.First() == FilesystemUtils::kSeparatorChar) {
+  // Leading and trailing "/" are not allowed.
+  if (aPath.First() == FilesystemUtils::kSeparatorChar ||
+      aPath.Last() == FilesystemUtils::kSeparatorChar) {
     return false;
   }
 
   NS_NAMED_LITERAL_STRING(kCurrentDir, ".");
   NS_NAMED_LITERAL_STRING(kParentDir, "..");
 
   // Split path and check each path component.
   nsCharSeparatedTokenizer tokenizer(aPath, FilesystemUtils::kSeparatorChar);
diff --git a/dom/filesystem/FilesystemTaskBase.cpp b/dom/filesystem/FilesystemTaskBase.cpp
--- a/dom/filesystem/FilesystemTaskBase.cpp
+++ b/dom/filesystem/FilesystemTaskBase.cpp
@@ -22,18 +22,18 @@ FilesystemTaskBase::FilesystemTaskBase(F
   : mErrorValue(NS_OK)
 {
   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
   MOZ_ASSERT(aFilesystem, "aFilesystem should not be null.");
   mFilesystem = do_GetWeakReference(aFilesystem);
 }
 
 FilesystemTaskBase::FilesystemTaskBase(FilesystemBase* aFilesystem,
-                   const FilesystemParams& aParam,
-                   FilesystemRequestParent* aParent)
+                                       const FilesystemParams& aParam,
+                                       FilesystemRequestParent* aParent)
   : mErrorValue(NS_OK)
   , mRequestParent(aParent)
 {
   MOZ_ASSERT(FilesystemUtils::IsParentProcess(),
              "Only call from parent process!");
   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
   MOZ_ASSERT(aFilesystem, "aFilesystem should not be null.");
   mFilesystem = do_GetWeakReference(aFilesystem);
@@ -68,17 +68,17 @@ FilesystemTaskBase::Start()
   nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
   if (!filesystem) {
     return;
   }
 
   // Retain a reference so the task object isn't deleted without IPDL's
   // knowledge. The reference will be released by
   // mozilla::dom::ContentChild::DeallocPFilesystemRequestChild.
-  AddRef();
+  NS_ADDREF_THIS();
   ContentChild::GetSingleton()->SendPFilesystemRequestConstructor(this,
     GetRequestParams(filesystem->ToString()));
 }
 
 NS_IMETHODIMP
 FilesystemTaskBase::Run()
 {
   if (!NS_IsMainThread()) {
diff --git a/dom/filesystem/FilesystemTaskBase.h b/dom/filesystem/FilesystemTaskBase.h
--- a/dom/filesystem/FilesystemTaskBase.h
+++ b/dom/filesystem/FilesystemTaskBase.h
@@ -134,18 +134,18 @@ protected:
    */
   FilesystemTaskBase(FilesystemBase* aFilesystem);
 
   /*
    * To create a parent process task delivered from the child process through
    * IPC.
    */
   FilesystemTaskBase(FilesystemBase* aFilesystem,
-           const FilesystemParams& aParam,
-           FilesystemRequestParent* aParent);
+                     const FilesystemParams& aParam,
+                     FilesystemRequestParent* aParent);
 
   virtual
   ~FilesystemTaskBase();
 
   /*
    * The function to perform task operation. It will be run on the worker
    * thread of the parent process.
    * Overrides this function to define the task operation for individual task.
Attachment #8381995 - Attachment is obsolete: true
Attachment #8382011 - Attachment is obsolete: true
Attachment #8384414 - Flags: review?(bugs)
Comment on attachment 8384414 [details] [diff] [review]
Part2 v10 Implement getRoot createDirectory and get for Directory

Ok, makes more sense, since ./ handling is now at least more consistent.

We may want to think whether ./ should be allowed, but not in this bug.
(and whoever designed the API should make the decision.)
Attachment #8384414 - Flags: review?(bugs) → review+
We should never allow filenames that contain a path segment that is "." or "..". So all of the following should be treated as illegal directory or file names "./foo", "./", "foo/./bar", "../foo", "../", "foo/../bar".

All of those should result in an asynchronous error. I.e. the returned promise from the operation should be rejected.
OK, thanks.

yxl, could you add some more tests for invalid path names.
Oh, also "." and ".." are forbidden directory or file names.
@smaug and @sicking, really thanks for your help :-)
I'll add more tests for invalid path names before landing this bug.
rebase.
Attachment #8382005 - Attachment is obsolete: true
Attachment #8385144 - Flags: review+
Attachment #8385144 - Attachment description: 0001-Bug-910412-WebIDL-and-DOM-binding-for-filesystem-API.patch → Part1 v5 WebIDL and DOM binding for filesystem API
rebase.
Attachment #8384414 - Attachment is obsolete: true
Attachment #8385145 - Flags: review+
I'll add more tests for invalid path names
Attachment #8382013 - Attachment is obsolete: true
Attachment #8385146 - Flags: review+
rebase.
Attachment #8385147 - Flags: review+
rebase
Attachment #8382014 - Attachment is obsolete: true
Attachment #8385148 - Flags: review+
Attachment #8382017 - Attachment is obsolete: true
Rename all Filesystem to FileSystem.
Attachment #8385144 - Attachment is obsolete: true
Attachment #8385837 - Flags: review+
Rename all Filesystem to FileSystem.
Attachment #8385145 - Attachment is obsolete: true
Attachment #8385839 - Flags: review+
Rename all Filesystem to FileSystem.
Attachment #8385146 - Attachment is obsolete: true
Attachment #8385841 - Flags: review+
Rename all Filesystem to FileSystem.
Attachment #8385147 - Attachment is obsolete: true
Attachment #8385843 - Flags: review+
Rename all Filesystem to FileSystem.
Attachment #8385148 - Attachment is obsolete: true
Attachment #8385844 - Flags: review+
Comment on attachment 8385837 [details] [diff] [review]
Part1 v6 WebIDL and DOM binding for filesystem API

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

::: dom/webidl/Directory.webidl
@@ +9,5 @@
> +/*
> + * All functions on Directory that accept DOMString arguments for file or
> + * directory names only allow relative path to current directory itself. The
> + * path should be a descendent path like "path/to/file.txt" and not contain a
> + * segment of ".." or ".". So the paths aren't allowd to walk up the directory

Nit: s/allowd/allowed
Why did you put this code in gklayout and not xul?  <https://hg.mozilla.org/integration/b2g-inbound/rev/b1c0310cdac1#l5.19>  That's not going to work on Windows if you call any of the libxul internal symbols.

(That should be FINAL_LIBRARY = 'xul').
Hmm, it seems like the code in dom/devicestorage is also linked into gklayout.  That is a very bizarre choice.
(In reply to Jan Varga [:janv] from comment #163)
> Comment on attachment 8385837 [details] [diff] [review]
> Part1 v6 WebIDL and DOM binding for filesystem API
> 
> Review of attachment 8385837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Directory.webidl
> @@ +9,5 @@
> > +/*
> > + * All functions on Directory that accept DOMString arguments for file or
> > + * directory names only allow relative path to current directory itself. The
> > + * path should be a descendent path like "path/to/file.txt" and not contain a
> > + * segment of ".." or ".". So the paths aren't allowd to walk up the directory
> 
> Nit: s/allowd/allowed

Thanks, I'll fix it in bug 934368.
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #165)
> Hmm, it seems like the code in dom/devicestorage is also linked into
> gklayout.  That is a very bizarre choice.

It was linked into xul, but bug 935881 changed it to gklayout as well as most dom modules. So I followed them.
Depends on: 980449
Depends on: 980372
Another new crash with bug 981145 landed on inbound:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35943511&tree=Mozilla-Inbound

It seems we've been stuck in a week-long game of whack-a-mole fixing crashes from this bug. I've backed this out for now until it can be stabilized and re-landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/428e5462fe29
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla30 → ---
FileSystemBase is required to be thread-safe, but its base class nsSupportsWeakReference depends on the non-thread-safe class nsWeakReference. This non-thread-safe class causes the above crash.

I'll fix the thread-safe issue by removing nsSupportsWeakReference dependence FileSystemBase.
Fix nits of Comment 163.
Attachment #8385837 - Attachment is obsolete: true
Attachment #8390362 - Flags: review+
Comment on attachment 8390364 [details] [diff] [review]
Part3 v7 Basic tests for filesystem API

rebase
Attachment #8390364 - Flags: review+
Attachment #8385841 - Attachment is obsolete: true
Attachment #8385843 - Attachment is obsolete: true
Attachment #8390365 - Flags: review?(dhylands)
rebase
Attachment #8385844 - Attachment is obsolete: true
Attachment #8390366 - Flags: review+
This is the interdiff for all patches with the previous version. I made the following changes:
1. As weak pointer isn't thread-safe, I had to make FileSystemBase a strong pointer. 

2. Incorporate improvement from bug 934368.

The try server result:
https://tbpl.mozilla.org/?tree=Try&rev=89cd63b7a522
Flags: needinfo?(dhylands)
I retriggered a bunch more M2's on Debug build of various platforms. Lets wait for those results to come in.
Flags: needinfo?(dhylands)
Comment on attachment 8390365 [details] [diff] [review]
Part4 v5 permission request and check

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

::: dom/filesystem/FileSystemPermissionRequest.cpp
@@ +37,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  mTask->GetPermissionAccessType(mPermissionAccess);
> +
> +  FileSystemBase* filesystem = mTask->GetFileSystem();

You were using an nsRefPtr here before. Why did you change to a bare pointer?

I'm seeing other places that are still using nsRefPtr (like in the FileSystemPermissionRequest::Run below.

I'd much rather see the use of nsRefPtr
Comment on attachment 8390363 [details] [diff] [review]
Part2 v13 Implement getRoot createDirectory and get for Directory

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

Looks good to me.

::: dom/filesystem/FileSystemTaskBase.cpp
@@ +49,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> +
> +  if (HasError()) {
> +    NS_DispatchToMainThread(this);

nit: Since you know you're already on the main thread, you can use NS_DispatchToCurrentThread(this) (which I was told was slightly more efficient).
Attachment #8390363 - Flags: review?(dhylands) → review+
(In reply to Dave Hylands [:dhylands] from comment #179)
> Comment on attachment 8390365 [details] [diff] [review]
> Part4 v5 permission request and check
> 
> Review of attachment 8390365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/filesystem/FileSystemPermissionRequest.cpp
> @@ +37,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  mTask->GetPermissionAccessType(mPermissionAccess);
> > +
> > +  FileSystemBase* filesystem = mTask->GetFileSystem();
> 
> You were using an nsRefPtr here before. Why did you change to a bare pointer?
mTask holds an owning reference to FileSystemBase, and we holds an owning reference to mTask.
So we holds an indirect owning reference to FileSystemBase. That's why I changed to a bare pointer.
Do I stil need to use nsRefPtr here?
> 
> I'm seeing other places that are still using nsRefPtr (like in the
> FileSystemPermissionRequest::Run below.
> 
> I'd much rather see the use of nsRefPtr
It isn't so much about whether its needed now, its more about good coding practice.

Its more of a maintenance thing.

Somebody comes along and decides that they need to get the filesystem, and sees the example there and just copies it and winds up with weird problems because they perhaps should have used an nsRefPtr.

Or makes a change in the code in the future that then violates one of the assumptions.

So if you're returning a bare pointer, then why do you use nsRefPtr in FileSystemPermissionRequest::Run

I guess I'd have expected to see a bare pointer everywhere (if its ok) or using nsRefPtr everywhere, and not a mixture of the 2.
Dave, really thanks. I learned a lot from you.

Update the patch to use nsRefPtr instead of raw FileSystemBase pointer.

I checked the source code of NS_DispatchToMainThread and NS_DispatchToCurrentThread. It seems NS_DispatchToMainThread will
be a little more efficient.

Both gets the main thread instance of type nsIThread and then dispatches the runnable (http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.cpp#161).

NS_DispatchToMainThread gets the main thread instance by one function call from nsThreadManager (http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThreadManager.cpp#266), while NS_DispatchToCurrentThread needs two function 
calls (http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThreadManager.cpp#276).
Attachment #8390365 - Attachment is obsolete: true
Attachment #8390365 - Flags: review?(dhylands)
Attachment #8391007 - Flags: review+
Flags: needinfo?(dhylands)
I agree with your analysis. NS_DispatchToMainThread is more efficient than NS_DispatchToCurrentThread (I didn't actually look it - so you learn something new every day :)

The try run looks good.
Flags: needinfo?(dhylands)
Comment on attachment 8390370 [details] [diff] [review]
fs_base_interdiff.patch

>-  nsRefPtr<FileSystemBase> fs = do_QueryReferent(mFileSystem);

>-  nsRefPtr<FileSystemBase> fs = do_QueryReferent(mFileSystem);

>-  nsRefPtr<FileSystemBase> fs = do_QueryReferent(mFileSystem);

>-    nsCOMPtr<DeviceStorageFileSystem> f =
>+    nsRefPtr<DeviceStorageFileSystem> f =
I couldn't find the patch that introduced these lines but getting rid of this bogus code is a good move; the nsCOMPtr/nsRefPtr is a bit of a correctness improvement although bug 514280 is hoping to make that error fail to compile while it's pure luck if do_QueryReferent is returning anything sane here.
You need to log in before you can comment on or make changes to this bug.