934368 - [Filesystem API] Implement remove and removeDeep methods for device storage.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Yuan Xulei [:yxl] (Leave from May 18)]

To be implemented:

interface Directory {
  ...
  Promise<boolean> remove((DOMString or File or Directory) path);
  Promise<boolean> removeDeep((DOMString or File or Directory) path);
  ...
}

Comment 1

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: remove v1.patch

Implement Directory#remove and #removeDeep. This patch is based on the patch of bug 910412.

Comment 2

[Dave Hylands [:dhylands]]

Comment on attachment 8380562 [details] [diff] [review]
remove v1.patch

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

I'm not sure about the use of the word deep in describing the removal? Is that used in other places. I'm used to the term recursive. The option on rm is -r for remove recursive.

I understand the use of deep when talking about a graph, but we're talking about a filesystem here, so I would have expected filesystem terminology.

r=me with issues addressed.

::: dom/devicestorage/test/test_fs_remove.html
@@ +168,5 @@
> +    case 0:
> +      gRoot = r;
> +      // Get sub1
> +      gPath = "sub1";
> +      gRoot.get("sub1").then(getSuccess, cbError);

It seems a bit fragile to me that this switch statement has to have indicies that match the order things appear in the gTest array.

Would it be possible to put each little snippet of code as an optional function in the array itself? This way the function stays with the test case.

::: dom/filesystem/DeviceStorageFilesystem.cpp
@@ +94,5 @@
>    return file.forget();
>  }
>  
> +bool
> +DeviceStorageFilesystem::GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const

nit: nsIDOMFile* aFile

@@ +103,5 @@
> +
> +  aRealPath.Truncate();
> +
> +  nsAutoString filePath;
> +  if (NS_FAILED(aFile->GetMozFullPathInternal(filePath))) {

Just wanted to clarify that if this needs to call any filesystem related functions (like realpath or stat or anything like that), then it has to happen in the parent.

If its just doing string manipulations then it can stay in the child.

::: dom/filesystem/DeviceStorageFilesystem.h
@@ +33,5 @@
>    virtual already_AddRefed<nsIFile>
>    GetLocalFile(const nsAString& aRealPath) const MOZ_OVERRIDE;
>  
> +  virtual bool
> +  GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const MOZ_OVERRIDE;

nit: nsIDOMFile* aFile

::: dom/filesystem/FilesystemBase.h
@@ +15,1 @@
>  class nsPIDOMWindow; // You need |#include "nsPIDOMWindow.h"| in CPP files.

I don't think that the comment is quite correct, so I'd be inclined to just remove it.

You only need the #include if you're in a cpp file that needs to dereference an nsIDOMFile pointer. That seems to be quite a common paradigm.

@@ +63,5 @@
> +   * If succeeded, returns true. Otherwise, returns false and set aRealPath to
> +   * empty string.
> +   */
> +  virtual bool
> +  GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const = 0;

nit: nsIDOMFile* aFile,

::: dom/filesystem/RemoveTask.cpp
@@ +113,5 @@
> +RemoveTask::Work()
> +{
> +  MOZ_ASSERT(FilesystemUtils::IsParentProcess(),
> +             "Only call from parent process!");
> +  MOZ_ASSERT(!NS_IsMainThread(), "Only call on child thread!");

we've been using child/parent to describe the relationship between the processes. So I would say non-main to not confuse processes/threads.

@@ +186,5 @@
> +    if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
> +      return;
> +    }
> +    mPromise->MaybeReject(cx, val);
> +  }

One side or the other of this if should return early, and then you can remove the else and unindent.

::: dom/ipc/PContent.ipdl
@@ +191,4 @@
>    nsString realPath;
>  };
>  
> +union FilesystemPathOrFileValue 

nit: trailing space

@@ +193,5 @@
>  
> +union FilesystemPathOrFileValue 
> +{
> +  nsString;
> +  PBlob;

missing names? (maybe they're optional?)

Comment 3

[Yuan Xulei [:yxl] (Leave from May 18)]

Thanks for your quick reply:)
(In reply to Dave Hylands [:dhylands] from comment #2)
> Comment on attachment 8380562 [details] [diff] [review]
> remove v1.patch
> 
> Review of attachment 8380562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure about the use of the word deep in describing the removal? Is
> that used in other places. I'm used to the term recursive. The option on rm
> is -r for remove recursive.
> 
> I understand the use of deep when talking about a graph, but we're talking
> about a filesystem here, so I would have expected filesystem terminology.
> 
> r=me with issues addressed.
> 
> ::: dom/devicestorage/test/test_fs_remove.html
> @@ +168,5 @@
> > +    case 0:
> > +      gRoot = r;
> > +      // Get sub1
> > +      gPath = "sub1";
> > +      gRoot.get("sub1").then(getSuccess, cbError);
> 
> It seems a bit fragile to me that this switch statement has to have indicies
> that match the order things appear in the gTest array.
> 
> Would it be possible to put each little snippet of code as an optional
> function in the array itself? This way the function stays with the test case.
> 
OK.
> ::: dom/filesystem/DeviceStorageFilesystem.cpp
> @@ +94,5 @@
> >    return file.forget();
> >  }
> >  
> > +bool
> > +DeviceStorageFilesystem::GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const
> 
> nit: nsIDOMFile* aFile
> 
> @@ +103,5 @@
> > +
> > +  aRealPath.Truncate();
> > +
> > +  nsAutoString filePath;
> > +  if (NS_FAILED(aFile->GetMozFullPathInternal(filePath))) {
> 
> Just wanted to clarify that if this needs to call any filesystem related
> functions (like realpath or stat or anything like that), then it has to
> happen in the parent.
> 
> If its just doing string manipulations then it can stay in the child.
This function will only be called in the parent and the member variable mNormalizedLocalRootPath it depends is only available on the parent, so |MOZ_ASSERT(parent)| is used to ensure this function is called on parent.
> 
> ::: dom/filesystem/DeviceStorageFilesystem.h
> @@ +33,5 @@
> >    virtual already_AddRefed<nsIFile>
> >    GetLocalFile(const nsAString& aRealPath) const MOZ_OVERRIDE;
> >  
> > +  virtual bool
> > +  GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const MOZ_OVERRIDE;
> 
> nit: nsIDOMFile* aFile
> 
> ::: dom/filesystem/FilesystemBase.h
> @@ +15,1 @@
> >  class nsPIDOMWindow; // You need |#include "nsPIDOMWindow.h"| in CPP files.
> 
> I don't think that the comment is quite correct, so I'd be inclined to just
> remove it.
> 
> You only need the #include if you're in a cpp file that needs to dereference
> an nsIDOMFile pointer. That seems to be quite a common paradigm.
I'll remove it.
> 
> @@ +63,5 @@
> > +   * If succeeded, returns true. Otherwise, returns false and set aRealPath to
> > +   * empty string.
> > +   */
> > +  virtual bool
> > +  GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const = 0;
> 
> nit: nsIDOMFile* aFile,
> 
> ::: dom/filesystem/RemoveTask.cpp
> @@ +113,5 @@
> > +RemoveTask::Work()
> > +{
> > +  MOZ_ASSERT(FilesystemUtils::IsParentProcess(),
> > +             "Only call from parent process!");
> > +  MOZ_ASSERT(!NS_IsMainThread(), "Only call on child thread!");
> 
> we've been using child/parent to describe the relationship between the
> processes. So I would say non-main to not confuse processes/threads.
> 
> @@ +186,5 @@
> > +    if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
> > +      return;
> > +    }
> > +    mPromise->MaybeReject(cx, val);
> > +  }
> 
> One side or the other of this if should return early, and then you can
> remove the else and unindent.
> 
> ::: dom/ipc/PContent.ipdl
> @@ +191,4 @@
> >    nsString realPath;
> >  };
> >  
> > +union FilesystemPathOrFileValue 
> 
> nit: trailing space
> 
> @@ +193,5 @@
> >  
> > +union FilesystemPathOrFileValue 
> > +{
> > +  nsString;
> > +  PBlob;
> 
> missing names? (maybe they're optional?)
No:-) The union member for .ipdl file has no name.

Comment 4

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #2)
> Comment on attachment 8380562 [details] [diff] [review]
> remove v1.patch
> 
> Review of attachment 8380562 [details] [diff] [review]:
-- snip --
> 
> ::: dom/filesystem/RemoveTask.cpp
> @@ +113,5 @@
> > +RemoveTask::Work()
> > +{
> > +  MOZ_ASSERT(FilesystemUtils::IsParentProcess(),
> > +             "Only call from parent process!");
> > +  MOZ_ASSERT(!NS_IsMainThread(), "Only call on child thread!");
> 
> we've been using child/parent to describe the relationship between the
> processes. So I would say non-main to not confuse processes/threads.
How about worker thread?

Comment 5

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v1 Extends Promise#MaybeResolve to receive boolean value.

We need to resolve boolean value from C++. So I made this patch.

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: interdiff for comment 2.

Comment 7

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 Implement remove and removeDeep.patch

Address comments and the interdiff:
https://bugzilla.mozilla.org/attachment.cgi?id=8382259&action=diff

1. Use the term recursive instead of deep.

2. Polish mochitest.

3. Fix nits and rebase.

Comment 8

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v3 Implement remove and removeDeep.patch

Refactor and add permission check tests for "remove" and "removeDeep" functions.

Comment 9

[(no longer active)]

Hey Yuan!  Anything I can help with here?

Comment 10

[Yuan Xulei [:yxl] (Leave from May 18)]

Could help to make the patch review quicker? :-)

Comment 11

[(no longer active)]

Dave, do you have an ETA for the review on this patch?

Also, is there anybody else who can review this code?  It seems like Dave has been reviewing most of it so far.

Comment 12

[Dave Hylands [:dhylands]]

I'm hoping to review this on Monday. I had a couple of higher priority bugs pop up, which is why these were delayed.

Comment 13

[(no longer active)]

Sounds good, thanks very much!

Comment 14

[Yuan Xulei [:yxl] (Leave from May 18)]

Ehsan and Dave, thank you. Dave gave me quite a lot help with file system related bugs and reviewed a bunch of code as well as bug 980372 and bug 980136.

Comment 15

[Dave Hylands [:dhylands]]

Comment on attachment 8385968 [details] [diff] [review]
Part2 v3 Implement remove and removeDeep.patch

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

Looks reasonable to me.

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

I believe that the ENSURE_SUCCESS type macros have been deprecated (because they hide return paths).

https://groups.google.com/forum/#!topic/mozilla.dev.platform/1clMLuuhtWQ

::: dom/filesystem/Directory.cpp
@@ +146,5 @@
> +  nsString realPath;
> +  nsCOMPtr<nsIDOMFile> file;
> +
> +  if (!fs) {
> +    goto parameters_check_done;

Shouldn't this set an error?

Otherwise, why would a null-filesystem be acceptable? Perhaps add a comment to explain whats happening here if a null filesystem is allowed.

::: dom/filesystem/FileSystemBase.cpp
@@ +65,5 @@
>  
> +bool
> +FileSystemBase::IsSafeDirectory(Directory* aDir) const
> +{
> +  return true;

does it make sense to flip this around and assume false, unless the derived class says true?

It seems safer from an implementation standpoint (if somebody forgets to override then it will fail rather than succeed)

Comment 16

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #15)
> Comment on attachment 8385968 [details] [diff] [review]
--snip--
> 
> ::: dom/filesystem/Directory.cpp
> @@ +146,5 @@
> > +  nsString realPath;
> > +  nsCOMPtr<nsIDOMFile> file;
> > +
> > +  if (!fs) {
> > +    goto parameters_check_done;
> 
> Shouldn't this set an error?
Yes, I'll fix it.
> 
> Otherwise, why would a null-filesystem be acceptable? Perhaps add a comment
> to explain whats happening here if a null filesystem is allowed.
filesystem is a weak reference. When the content window unloads, filesystem 
is destroyed and its weak reference will be null before the directory, so 
the code accepts a null pointer here.

Comment 17

[Dave Hylands [:dhylands]]

(In reply to Yuan Xulei [:yxl] from comment #16)
> (In reply to Dave Hylands [:dhylands] from comment #15)
> > Comment on attachment 8385968 [details] [diff] [review]
> --snip--
> > 
> > ::: dom/filesystem/Directory.cpp
> > @@ +146,5 @@
> > > +  nsString realPath;
> > > +  nsCOMPtr<nsIDOMFile> file;
> > > +
> > > +  if (!fs) {
> > > +    goto parameters_check_done;
> > 
> > Shouldn't this set an error?
> Yes, I'll fix it.
> > 
> > Otherwise, why would a null-filesystem be acceptable? Perhaps add a comment
> > to explain whats happening here if a null filesystem is allowed.
> filesystem is a weak reference. When the content window unloads, filesystem 
> is destroyed and its weak reference will be null before the directory, so 
> the code accepts a null pointer here.

Then I think a comment is definitely warranted (to explain what you just explained). Anytime "the intuitively obvious" is wrong, I think that a comment should be added.

Comment 18

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v1 Extends Promise#MaybeResolve to receive boolean value.

update commit message.

Comment 19

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: interdiff for comment 15

Comment 20

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v4 Implement remove and removeDeep.patch

Address all of comment 15 and the interdiff:
https://bugzilla.mozilla.org/attachment.cgi?id=8388995&action=diff

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=74e93e07f85f

Comment 21

[Dave Hylands [:dhylands]]

(In reply to Yuan Xulei [:yxl] from comment #20)
> Try server result:
> https://tbpl.mozilla.org/?tree=Try&rev=74e93e07f85f

I did some of the retriggering mentioned in bug 934367 and I'm seeing one orange 2's that seems to be related to device storage.
https://tbpl.mozilla.org/php/getParsedLog.php?id=35952644&tree=Try

Comment 22

[Dave Hylands [:dhylands]]

I also see an orange M8 on B2G ICS Emulator Debug.
https://tbpl.mozilla.org/php/getParsedLog.php?id=35928071&tree=Try

Comment 23

[Dave Hylands [:dhylands]]

So I cleared the review flag for the time being.

Lets get all these intermittent assertions and stuff cleaned up.

Comment 24

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v1 Extends Promise#MaybeResolve to receive boolean value.

rebased

Comment 25

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v5 Implement remove and removeDeep.patch

rebase based on the patch of bug 910412.

And push to try:

https://tbpl.mozilla.org/?tree=Try&rev=59697268b8ee

Comment 26

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8391047 [details] [diff] [review]
Part2 v5 Implement remove and removeDeep.patch

All try server tests for this bug passed.

Comment 27

[Dave Hylands [:dhylands]]

(In reply to Yuan Xulei [:yxl] from comment #26)
> Comment on attachment 8391047 [details] [diff] [review]
> Part2 v3 Implement remove and removeDeep.patch
> 
> All try server tests for this bug passed.

It doesn't look like it. All of the Windows machines failed with message like these:
https://tbpl.mozilla.org/php/getParsedLog.php?id=36126697&tree=Try

Comment 28

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #27)
> (In reply to Yuan Xulei [:yxl] from comment #26)
> > Comment on attachment 8391047 [details] [diff] [review]
> > Part2 v3 Implement remove and removeDeep.patch
> > 
> > All try server tests for this bug passed.
> 
> It doesn't look like it. All of the Windows machines failed with message
> like these:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=36126697&tree=Try

I combined the patch of bug 934367 with that of this bug and the failed tests belonged to bug 934367.

Let's have a clean try:

https://tbpl.mozilla.org/?tree=Try&rev=253a81c2b108

Comment 29

[Dave Hylands [:dhylands]]

I'm slightly confused about the patch.

The patch you're asking me to review says: Part2 v3 Implement remove and removeDeep.patch

and the patch it obsoletes is Part2 v4 Implement remove and removeDeep.patch

Certain things in the patch also don't make sense if the v3 one is newer.

Could you double check that you're asking me to review the correct patch?

Comment 30

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8391047 [details] [diff] [review]
Part2 v5 Implement remove and removeDeep.patch

The version should be v5.
Sorry for my mistake.

Comment 31

[Dave Hylands [:dhylands]]

Comment on attachment 8391047 [details] [diff] [review]
Part2 v5 Implement remove and removeDeep.patch

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

Looks good to me

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8c1ce4098df7
https://hg.mozilla.org/integration/b2g-inbound/rev/de5e89c89e37

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c1ce4098df7
https://hg.mozilla.org/mozilla-central/rev/de5e89c89e37

Comment 34

[Jan Varga [:janv]]

Comment on attachment 8391047 [details] [diff] [review]
Part2 v5 Implement remove and removeDeep.patch

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

::: dom/filesystem/Directory.cpp
@@ +140,5 @@
> +  // Check and get the target path.
> +
> +  if (aPath.IsFile()) {
> +    file = aPath.GetAsFile();
> +    goto parameters_check_done;

"goto" ?
The first thing they taught us in the school was "never use goto" :)

I went through mozilla/dom and it seems that only 
dom/bluetooth/bluez uses it.

::: dom/filesystem/Directory.h
@@ +70,4 @@
>    // =========== End WebIDL bindings.============
> +
> +  FileSystemBase*
> +  GetFileSystem() const;

Nit: an empty line missing here

Comment 35

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Varga [:janv] from comment #34)
> Comment on attachment 8391047 [details] [diff] [review]
> Part2 v5 Implement remove and removeDeep.patch
> 
> Review of attachment 8391047 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/filesystem/Directory.cpp
> @@ +140,5 @@
> > +  // Check and get the target path.
> > +
> > +  if (aPath.IsFile()) {
> > +    file = aPath.GetAsFile();
> > +    goto parameters_check_done;
> 
> "goto" ?
> The first thing they taught us in the school was "never use goto" :)
The abuse of 'goto' makes code hard to understand and maintain. I think that's why we should avoid 'goto'.
In this case, 'goto' makes the code much easier without nested 'if-else's. I can convert 'goto' to a while loop with trick, but that will make the code less readable. So I choose to use 'goto'. :-)
> 
> I went through mozilla/dom and it seems that only 
> dom/bluetooth/bluez uses it.
> 
> ::: dom/filesystem/Directory.h
> @@ +70,4 @@
> >    // =========== End WebIDL bindings.============
> > +
> > +  FileSystemBase*
> > +  GetFileSystem() const;
> 
> Nit: an empty line missing here
I'll fix it in bug 935883.

Comment 36

[Jan Varga [:janv]]

(In reply to Yuan Xulei [:yxl] from comment #35)
> (In reply to Jan Varga [:janv] from comment #34)
> > Comment on attachment 8391047 [details] [diff] [review]
> > Part2 v5 Implement remove and removeDeep.patch
> > 
> > Review of attachment 8391047 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/filesystem/Directory.cpp
> > @@ +140,5 @@
> > > +  // Check and get the target path.
> > > +
> > > +  if (aPath.IsFile()) {
> > > +    file = aPath.GetAsFile();
> > > +    goto parameters_check_done;
> > 
> > "goto" ?
> > The first thing they taught us in the school was "never use goto" :)
> The abuse of 'goto' makes code hard to understand and maintain. I think
> that's why we should avoid 'goto'.
> In this case, 'goto' makes the code much easier without nested 'if-else's. I
> can convert 'goto' to a while loop with trick, but that will make the code
> less readable. So I choose to use 'goto'. :-)

Well, at the very least, I would add a comment that justifies this exception :)