Closed Bug 934368 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: xyuan, Assigned: xyuan)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 7 obsolete files)

6.99 KB, patch
Details | Diff | Splinter Review
1.76 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
34.40 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
To be implemented:

interface Directory {
  ...
  Promise<boolean> remove((DOMString or File or Directory) path);
  Promise<boolean> removeDeep((DOMString or File or Directory) path);
  ...
}
Attached patch remove v1.patch (obsolete) — Splinter Review
Implement Directory#remove and #removeDeep. This patch is based on the patch of bug 910412.
Attachment #8380562 - Flags: feedback?(dhylands)
Assignee: nobody → xyuan
Status: NEW → ASSIGNED
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?)
Attachment #8380562 - Flags: feedback?(dhylands) → feedback+
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.
(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?
We need to resolve boolean value from C++. So I made this patch.
Attachment #8382258 - Flags: review?(bugs)
Attached patch interdiff for comment 2. (obsolete) — Splinter Review
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.
Attachment #8380562 - Attachment is obsolete: true
Attachment #8382267 - Flags: review?(dhylands)
Attachment #8382258 - Flags: review?(bugs) → review+
Refactor and add permission check tests for "remove" and "removeDeep" functions.
Attachment #8382267 - Attachment is obsolete: true
Attachment #8382267 - Flags: review?(dhylands)
Attachment #8385968 - Flags: review?(dhylands)
Blocks: 910387
Hey Yuan!  Anything I can help with here?
Could help to make the patch review quicker? :-)
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan) → needinfo?(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.
Flags: needinfo?(dhylands)
Sounds good, thanks very much!
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 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)
Attachment #8385968 - Flags: review?(dhylands) → review+
(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.
(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.
update commit message.
Attachment #8382258 - Attachment is obsolete: true
Attachment #8388994 - Flags: review+
Attachment #8382259 - Attachment is obsolete: true
(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
I also see an orange M8 on B2G ICS Emulator Debug.
https://tbpl.mozilla.org/php/getParsedLog.php?id=35928071&tree=Try
Attachment #8388997 - Flags: review?(dhylands)
So I cleared the review flag for the time being.

Lets get all these intermittent assertions and stuff cleaned up.
rebase based on the patch of bug 910412.

And push to try:

https://tbpl.mozilla.org/?tree=Try&rev=59697268b8ee
Attachment #8388997 - Attachment is obsolete: true
Comment on attachment 8391047 [details] [diff] [review]
Part2 v5 Implement remove and removeDeep.patch

All try server tests for this bug passed.
Attachment #8391047 - Flags: review?(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
(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
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 on attachment 8391047 [details] [diff] [review]
Part2 v5 Implement remove and removeDeep.patch

The version should be v5.
Sorry for my mistake.
Attachment #8391047 - Attachment description: Part2 v3 Implement remove and removeDeep.patch → Part2 v5 Implement remove and removeDeep.patch
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
Attachment #8391047 - Flags: review?(dhylands) → review+
https://hg.mozilla.org/mozilla-central/rev/8c1ce4098df7
https://hg.mozilla.org/mozilla-central/rev/de5e89c89e37
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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
(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.
(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 :)
You need to log in before you can comment on or make changes to this bug.