1173320 - Allow FileList objects to contain Directory objects

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

Allow FileList objects to contain Directory objects.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: WIP

Still moving stuff around between patches in my queue, but this is roughly there and compiles okay.

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8620382 [details] [diff] [review]
WIP

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

::: dom/base/File.h
@@ +904,5 @@
> + *
> + * Rather than store the wrapped object using an nsRefPtr/nsCOMPtr, this class
> + * stores it as a raw void* so that it can use the least-significant bits of
> + * the pointer as a flag.  This means that manual refcounting is needed though.
> + *

I understand all of this, but what about if we implement a base class for Blob and Directory? Maybe refcnted?
I think having manual refcounting should be avoided.

Plus, why do you assume that the least-significant bit of a File* is != 1 ? Does it work in any architecture?

I guess we can easily have a |class BlobOrDirectory| base class for Blob and Directory. With a IsDirectory() and/or IsBlob() (or IsBlobOrFile())

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 1 - create a common base class for Blob and Directory

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8640502 [details] [diff] [review]
part 1 - create a common base class for Blob and Directory

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

Sorry for the delay.
What about the patch of bug 1176942? It overlaps many of these files.

::: dom/base/BlobOrDir.h
@@ +14,5 @@
> +
> +class Blob;
> +class Directory;
> +
> +// CID for the CSSStyleSheet class

Doesn't seem a CSSStyleSheet :)

@@ +51,5 @@
> +} // dom namespace
> +} // file namespace
> +
> +#endif // mozilla_dom_BlobOrDir_h
> +

no extra line here.

Comment 5

[Dietrich Ayala (:dietrich)]

R+ was four months ago - anything blocking this from moving forward? Jonathan are you still available to work on it, or need new owner?

Comment 6

[Andrea Marchesini [:baku]]

Attachment: fileOrDir.patch

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Directory vs Blob vs File

This first patch removes the dependence between Blob and Directory.
At the beginning I proposed to unify Directory and File under BlobImpl because that would simplify the IPC communication in the nsIFilePicker and so on.
I changed my mind (sorry jwatt, my fault).
This approach is better, imo: for IPC we share just the full path of the directory. Then we recreate the directory object starting from that in the new context (thread, process).

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Directory vs Blob vs File

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch 2 - FileList with Directories

Follow ups: directories in workers and FileList in workers again.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8706881 [details] [diff] [review]
patch 1 - Directory vs Blob vs File

>       if (file) {
> #ifdef DEBUG
>         if (XRE_GetProcessType() == GeckoProcessType_Default) {
>           bool isDir;
>           file->IsDirectory(&isDir);
>           MOZ_ASSERT(!isDir, "How did we get here?");
>         }
> #endif
>-        domFile = File::CreateFromFile(GetParentObject(), file);
>+
>+        nsAutoString path;
>+        rv = file->GetPath(path);
>+        if (NS_WARN_IF(NS_FAILED(rv))) {
>+          continue;
>+        }
>+
>+        nsCOMPtr<nsINode> parentNode = do_QueryInterface(mParent);
>+        nsPIDOMWindow* window = parentNode->OwnerDoc()->GetInnerWindow();
>+
>+        RefPtr<Directory> directory = Directory::Create(window, path);
I don't understand this code. First we assert that we are not dealing with directories here, but
then we create a Directory object.

>-        domFile = File::Create(GetParentObject(), blobImpl);
>+        RefPtr<File> domFile =File::Create(GetParentObject(), blobImpl);
missing a space after =

>-    RefPtr<OSFileSystem> fs;
>-    for (uint32_t i = 0; i < mFiles->Length(); ++i) {
>-      if (mFiles->Item(i)->Impl()->IsDirectory()) {
>-#if defined(ANDROID) || defined(MOZ_B2G)
>-        MOZ_ASSERT(false,
>-                   "Directory picking should have been redirected to normal "
>-                   "file picking for platforms that don't have a directory "
>-                   "picker");
>-#endif
Make sure we don't drop this assert in the the final code.

>+Directory::Create(nsPIDOMWindow* aWindow,
>+                  const nsAString& aPath)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  int32_t leafSeparatorIndex = nsString(aPath).RFind(FILE_PATH_SEPARATOR);
So this is platform specific code. Do we somehow ensure this ::Create isn't called with cross platform paths?
Perhaps rename to ::CreateWithPlatformPath or some such


>+  nsDependentSubstring dirname = Substring(aPath, 0, leafSeparatorIndex);
oh, this method takes path to ending / or some filename. Not /home/smaug for example
I wonder how to assert that we're dealing with sane paths here.
At least assert that the last character of aPath is FILE_PATH_SEPARATOR?

>     while (NS_SUCCEEDED(iter->HasMoreElements(&hasMore)) && hasMore) {
>       iter->GetNext(getter_AddRefs(tmp));
>       nsCOMPtr<nsIDOMBlob> domBlob = do_QueryInterface(tmp);
>       MOZ_ASSERT(domBlob,
>                  "Null file object from FilePicker's file enumerator?");
>-      if (domBlob) {
>-        newFiles.AppendElement(static_cast<File*>(domBlob.get()));
>+      if (!domBlob) {
>+        continue;
I don't understand this code. We assert that domBlob is non-null, but then we still
null check it.

>+    nsCOMPtr<nsIGlobalObject> global = OwnerDoc()->GetScopeObject();
>+    RefPtr<File> domFile = File::CreateFromFile(global, file);
I think I'd feel better if we null checked the return value of GetScopeObject()


>@@ -5582,23 +5642,28 @@ HTMLInputElement::SubmitNamesValues(nsFo
>   }
> 
>   //
>   // Submit file if its input type=file and this encoding method accepts files
>   //
>   if (mType == NS_FORM_INPUT_FILE) {
>     // Submit files
> 
>-    const nsTArray<RefPtr<File>>& files = GetFilesInternal();
>-
>+    const nsTArray<OwningFileOrDirectory>& files =
>+      GetFilesOrDirectoriesInternal();
>+
>+    bool hasFiles = false;
>     for (uint32_t i = 0; i < files.Length(); ++i) {
>-      aFormSubmission->AddNameFilePair(name, files[i]);
>-    }
>-
>-    if (files.IsEmpty()) {
>+      if (files[i].IsFile()) {
>+        hasFiles = true;
>+        aFormSubmission->AddNameFilePair(name, files[i].GetAsFile());
>+      }
>+    }
Can you explain what this all is supposed to work with directories?

>       case VALUE_MODE_FILENAME:
>         {
>-          const nsTArray<RefPtr<BlobImpl>>& blobImpls = inputState->GetBlobImpls();
>-
>-          nsCOMPtr<nsIGlobalObject> global = OwnerDoc()->GetScopeObject();
>-          MOZ_ASSERT(global);
>-
>-          nsTArray<RefPtr<File>> files;
>-          for (uint32_t i = 0, len = blobImpls.Length(); i < len; ++i) {
>-            RefPtr<File> file = File::Create(global, blobImpls[i]);
>-            MOZ_ASSERT(file);
>-
>-            files.AppendElement(file);
>-          }
>-
>-          SetFiles(files, true);
>+          nsPIDOMWindow* window = OwnerDoc()->GetInnerWindow();
The old code used scopeobject, this on innerwindow. Which one is right? Is this code suppose to run in documents which aren't
in any browsing contexts? I guess no, so inner window should be fine. But I would still null check GetInnerWindow()'s return value.


>+FilePickerParent::SendFilesOrDirectories(const nsTArray<BlobImplOrString>& aData)
> {
>+  if (mMode == nsIFilePicker::modeGetFolder) {
>+    MOZ_ASSERT(aData.Length() <= 1);
Hmm, not possible to select multiple directories?

>+struct InputDirectory
> {
>-  InputFiles;
>+  nsString directoryPath;
>+};
Why you need InputDirectory?
Though, I guess it is a bit clearer to hide the string inside a struct. so fine.


I think I'd like to see some clarifications, especially to the first comment I gave.
And since this is a massive patch, an interdiff would be great with the next version of the patch.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8706882 [details] [diff] [review]
patch 2 - FileList with Directories

>+OSFileSystem::Init(nsISupports* aParent)
> {
>   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>-  MOZ_ASSERT(!mWindow, "No duple Init() calls");
>-  MOZ_ASSERT(aWindow);
>-  mWindow = aWindow;
>+  MOZ_ASSERT(!mParent, "No duple Init() calls");
>+  MOZ_ASSERT(aParent);
>+  mParent = aParent;
> }
Please add ifdef DEBUG code here which checks that aParent is always nsIGlobalObject.
Though, I wonder why to not just store nsIGlobalObject and not nsISupports


> HTMLInputElement::GetFiles()
> {
>   if (mType != NS_FORM_INPUT_FILE) {
>     return nullptr;
>   }
> 
>-  if (HasAttr(kNameSpaceID_None, nsGkAtoms::directory)) {
>-    return nullptr;
>-  }
Whaat, why do we have this code. We don't expose support for directories to web atm, yet
we prevent input.files to work if input element has directory attribute.
That code is in FF43. Should we possibly remove HasAttr check on branches?
Separate bug for that please.
(The code should on branches check whether we have the pref for directories enabled or something like that.)

Comment 12

[Andrea Marchesini [:baku]]

> >-#if defined(ANDROID) || defined(MOZ_B2G)
> >-        MOZ_ASSERT(false,
> >-                   "Directory picking should have been redirected to normal "
> >-                   "file picking for platforms that don't have a directory "
> >-                   "picker");
> >-#endif
> Make sure we don't drop this assert in the the final code.

I move this in the next patch.

> >       MOZ_ASSERT(domBlob,
> >                  "Null file object from FilePicker's file enumerator?");
> >-      if (domBlob) {
> >-        newFiles.AppendElement(static_cast<File*>(domBlob.get()));
> >+      if (!domBlob) {
> >+        continue;
> I don't understand this code. We assert that domBlob is non-null, but then
> we still
> null check it.

for non debug builds. I kept the same logic of before.

> >-    if (files.IsEmpty()) {
> >+      if (files[i].IsFile()) {
> >+        hasFiles = true;
> >+        aFormSubmission->AddNameFilePair(name, files[i].GetAsFile());
> >+      }
> >+    }
> Can you explain what this all is supposed to work with directories?

We don't support directories in the form submission in the current patch.

> >+FilePickerParent::SendFilesOrDirectories(const nsTArray<BlobImplOrString>& aData)
> > {
> >+  if (mMode == nsIFilePicker::modeGetFolder) {
> >+    MOZ_ASSERT(aData.Length() <= 1);
> Hmm, not possible to select multiple directories?

No at the moment.

> >+struct InputDirectory
> > {
> >-  InputFiles;
> >+  nsString directoryPath;
> >+};
> Why you need InputDirectory?
> Though, I guess it is a bit clearer to hide the string inside a struct. so
> fine.

So that I can check if the union contains type ::TInputDirectory.

> I think I'd like to see some clarifications, especially to the first comment
> I gave.

Right. That was wrong.

Comment 13

[Andrea Marchesini [:baku]]

>+  nsDependentSubstring dirname = Substring(aPath, 0, leafSeparatorIndex);
> oh, this method takes path to ending / or some filename. Not /home/smaug for
> example
> I wonder how to assert that we're dealing with sane paths here.
> At least assert that the last character of aPath is FILE_PATH_SEPARATOR?

No, it takes /home/smaug. The dirname is the first part of the directory. 'smaug' in our case.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: inderdiff patch 1

I want to add a test for DataTransfer because it's unclear how we receive directories.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrea Marchesini (:baku) from comment #13)
> >+  nsDependentSubstring dirname = Substring(aPath, 0, leafSeparatorIndex);
> > oh, this method takes path to ending / or some filename. Not /home/smaug for
> > example
> > I wonder how to assert that we're dealing with sane paths here.
> > At least assert that the last character of aPath is FILE_PATH_SEPARATOR?
> 
> No, it takes /home/smaug. The dirname is the first part of the directory.
> 'smaug' in our case.
How so. The substring starts from aPath[0] and ends to aPath[lastLeafSeparator -1].
So dirname is /home.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

ni for the comment 15. I still don't understand the path handling.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8707390 [details] [diff] [review]
inderdiff patch 1

Waiting for answer to the ni? so removing from review queue until that.

Comment 18

[Andrea Marchesini [:baku]]

> So dirname is /home.

Correct. but the dirname is used by the OSFileSystem that works on the parent directory of this Directory obj path.
The directory path is the basename.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

But if /home/smaug/ is passed, the parent is /home/smaug
Do we somewhere assert that sane paths are passed around?

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Directory vs Blob vs File

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

It was _very_ hard to see if Directory::CreateFromPlatformPath is working for root directory in unix. We end up removing the trailing "/" and then both dirname and basename are "", so both OSFileSystem and Directory point to some bogus directory.

Looks like Directory::GetPath does have a special case for root handling, but I don't see anything in OSFileSystem.
Though, I do see some odd root dir special casing in GetFileOrDirectoryTask::Work() and in
GetDirectoryListingTask::Work().  So I think it is fine.

But, Directory objects are now created differently. The old code seems to pass fs and /<path> to the ctor.
The new setup may create it also using fs and FILE_PATH_SEPARATOR<path>. That doesn't look right.

(this is hard to review stuff.)

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8712246 [details] [diff] [review]
patch 1 - Directory vs Blob vs File

I can't prove my self that Directory objects are now created the right way.
Please explain or fix.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Directory vs Blob vs File

This patch is big, but I think it makes everything much cleaner.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

That is massive. Could you give a short description what all the patch does.
(and hopefully it doesn't contain any unrelated fixes ;) )

Comment 25

[Andrea Marchesini [:baku]]

Attachment: patch 2 - makes Windows happy - deal with windows paths

Comment 26

[Andrea Marchesini [:baku]]

Attachment: patch 3 - FileList with Directories

Mainly rebased. I changed StructuredCloneHelper in order to propagate mIsRoot

Comment 27

[Andrea Marchesini [:baku]]

Attachment: patch 4 - cleanup

Comment 28

[Andrea Marchesini [:baku]]

Sorry smaug, I forgot to tell you something about these patches.

patch 1: Here we do 2 things: we split BlobImpl and Directories. Directory objs need just a path in order to work. Plus they are not thread-safe. Everywhere we used to have BlobImpl for Directory, now we have a BlobImplOrDirectory struct.
The second task of this patch is to use nsIFile instead strings in dom/filesystem/*. The reason I did this is because it's hard to follow what we pass between IPC tasks (relative or full path?) and it's hard to be bullet proof when paths are manipulated as strings. See FileSystemUtils and all the normalization, localPath to dom Path and so on.

patch 2: This second patch cleans some small details to make windows happy. Mainly: no manual string manipulation for paths in Directory::IsValidRelativePath. With patch 1 and patch 2 we are green on try.

patch 3: You already reviewed this patch but because patch 1 and patch 2, would be nice if you can take a look again. This implement FileList with Directory support.

patch 4: This patch cleans up dom/filesystem/* removing all the FileSystemUtils path parsing methods. If you prefer this can go in a follow up.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8714736 [details] [diff] [review]
patch 2 - makes Windows happy - deal with windows paths

>+bool
>+IsValidRelativePath(const nsString& aPath, nsTArray<nsString>& aParts)
So it is not clear whether this deals paths with system or DOM format

>+{
>+  // We don't allow empty relative path to access the root.
>+  if (aPath.IsEmpty()) {
>+    return false;
>+  }
>+
>+  // Leading and trailing "/" are not allowed.
this comment hints dom

>+  if (aPath.First() == FileSystemUtils::kSeparatorChar ||
>+      aPath.Last() == FileSystemUtils::kSeparatorChar) {
>+    return false;
>+  }
but then, FileSystemUtils::kSeparatorChar sounds like OS dependent thing. 
However, apparently it is always '/'. So, could you think of some more precise name for this function.
IsValidRelativeDOMPath?


>+  // Split path and check each path component.
>+  nsCharSeparatedTokenizer tokenizer(aPath, FileSystemUtils::kSeparatorChar);
>+  while (tokenizer.hasMoreTokens()) {
>+    nsDependentSubstring pathComponent = tokenizer.nextToken();
>+    // The path containing empty components, such as "foo//bar", is invalid.
>+    // We don't allow paths, such as "../foo", "foo/./bar" and "foo/../bar",
>+    // to walk up the directory.
Could you clarify in the comment that this is per spec.
Although I don't know what there says '.' is illegal. And ".." is also just mentioned as an issue.
But why is using nsCharSeparatedTokenizer right here? We don't want any special whitespace handling for directories, unless I'm missing something from the spec.
I see that you just move "..", "." and nsCharSeparatedTokenizer usage. So, please either fix in this bug or file a followup bug which is somehow marked to block 
enabling all this Directory API. (If we don't have a bug to enable the API, please file that too)

Do we have tests for case like "foo//////foo////bar/foo"?
If not, please add some.


>+#ifdef DEBUG
>+      nsCOMPtr<nsIFile> rootPath;
>+      rv = NS_NewLocalFile(mFileSystem->GetLocalRootPath(), false,
>+                           getter_AddRefs(rootPath));
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        mPromise->MaybeReject(rv);
>+        mPromise = nullptr;
>+        return;
>+      }
>+
>+      MOZ_ASSERT(FileSystemUtils::IsDescendantPath(rootPath, directoryPath));
>+#endif
I so don't understand why we reject anything only in DEBUG builds.
But again, you're moving code. Please file a followup bug to clean this up or fix in this bug.



> OSFileSystem::GetRootName(nsAString& aRetval) const
> {
>-  return aRetval.AssignLiteral("/");
>+  aRetval.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR);
It is a bit annoying that we have both FILESYSTEM_DOM_PATH_SEPARATOR and kSeparatorChar, a
and those are even defined in the same .h. Could you please make kSeparatorChar also a #define and name it similarly
to the other one, perhaps they could be FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL and FILESYSTEM_DOM_PATH_SEPARATOR_CHAR

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8714738 [details] [diff] [review]
patch 4 - cleanup

>+DeviceStorageFile::IsSafePath(const nsAString& aPath,
>+                              nsTArray<nsString>* aParts) const
> {
>   nsAString::const_iterator start, end;
>   aPath.BeginReading(start);
>   aPath.EndReading(end);
> 
>   // if the path is a '~' or starts with '~/', return false.
>   NS_NAMED_LITERAL_STRING(tilde, "~");
>   NS_NAMED_LITERAL_STRING(tildeSlash, "~/");
>   if (aPath.Equals(tilde) ||
>       StringBeginsWith(aPath, tildeSlash)) {
>     NS_WARNING("Path name starts with tilde!");
>     return false;
>-   }
>-  // split on /.  if any token is "", ., or .., return false.
>-  NS_ConvertUTF16toUTF8 cname(aPath);
>-  char* buffer = cname.BeginWriting();
>-  const char* token;
>-
>-  while ((token = nsCRT::strtok(buffer, "/", &buffer))) {
>-    if (PL_strcmp(token, "") == 0 ||
>-        PL_strcmp(token, ".") == 0 ||
>-        PL_strcmp(token, "..") == 0 ) {
>+  }
>+
>+  NS_NAMED_LITERAL_STRING(kCurrentDir, ".");
>+  NS_NAMED_LITERAL_STRING(kParentDir, "..");
>+
>+  // Split path and check each path component.
>+  nsCharSeparatedTokenizer tokenizer(aPath, FileSystemUtils::kSeparatorChar);
>+  while (tokenizer.hasMoreTokens()) {
>+    nsDependentSubstring pathComponent = tokenizer.nextToken();
>+    // The path containing empty components, such as "foo//bar", is invalid.
>+    // We don't allow paths, such as "../foo", "foo/./bar" and "foo/../bar",
>+    // to walk up the directory.
>+    if (pathComponent.IsEmpty() ||
>+        pathComponent.Equals(kCurrentDir) ||
>+        pathComponent.Equals(kParentDir)) {
>       return false;
>     }
>+
>+    if (aParts) {
>+      aParts->AppendElement(pathComponent);
>+    }
So you're adding new nsCharSeparatedTokenizer usage here, and it isn't right for directory names since it ends up eating whitespace.
One option is to use nsCharSeparatedTokenizerTemplate, but initialize the template with a dummy IsWhitespace method which always returns false
(by default NS_IsAsciiWhitespace seems to be used.). IDB seems to do something like that.


>+DeviceStorageFile::NormalizeFilePath()
>+{
>+#if defined(XP_WIN)
>+  char16_t* cur = mPath.BeginWriting();
>+  char16_t* end = mPath.EndWriting();
>+  for (; cur < end; ++cur) {
>+    if (char16_t('\\') == *cur)
>+      *cur = FileSystemUtils::kSeparatorChar;
>+  }
>+#endif
> }

So "IsSafePath" and such seem to deal with DOMPaths, but NormalizeFilePath OS specific paths, or actually converting mPath from being
in OSPath to be in DOMPath. Could we please not have store two different things in mPath.
NormalizeFilePath is added in some other patch in this bug, but it should be made static so that it can't be used for converting member variables, but
it should just take input and convert it and store the result in output.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8714737 [details] [diff] [review]
patch 3 - FileList with Directories

>-  File* Item(uint32_t aIndex)
>+  const OwningFileOrDirectory& Item(uint32_t aIndex) const
>   {
>-    return mFiles.SafeElementAt(aIndex);
>+    MOZ_ASSERT(aIndex < Length());
>+    return mFilesOrDirectories[aIndex];
>   }
Could we please not have one Item which isn't boundary safe, when other Item() methods here do check that index is within the bounds.
So, rename this variant to UnsafeItem or some such
> 
> // The format of the FileList serialization is:
> // - pair of ints: SCTAG_DOM_FILELIST, Length of the FileList
>-// - pair of ints: 0, The offset of the BlobImpl array
Why we need this...

>+// - for each element of the FileList:
>+//   - if it's a blob:
>+//    - pair of ints: SCTAG_DOM_BLOB, index of the BlobImpl in the array
>+//       mBlobImplArray.
...when we store this.

You don't seem to use the first one for anything. The old code has uint32_t index = offset + i;
but I don't see anything similar in the new code

>-function create_fileList() {
>-  var url = SimpleTest.getTestFileURL("script_postmessages_fileList.js");
>-  var script = SpecialPowers.loadChromeScript(url);
>-
>-  function onOpened(message) {
>-    var fileList = document.getElementById('fileList');
>-    SpecialPowers.wrap(fileList).mozSetFileArray([message.file]);
>-
>-    // Just a simple test
>-    var domFile = fileList.files[0];
>-    is(domFile.name, "prefs.js", "fileName should be prefs.js");
>-
>-    clonableObjects.push(fileList.files);
>-    script.destroy();
>-    next();
>-  }
>-
>-  script.addMessageListener("file.opened", onOpened);
>-  script.sendAsyncMessage("file.open");
>-}
I don't understand why we want/need to remove these tests.
We certainly need to get some new tests then.


Would like to see a new patch, hopefully also interdiff.
Fix the structured cloning handling and please explain that why tests are removed but no new ones added to replace them.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

I'll get to the largest patch tomorrow.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Finally getting back to this

(In reply to Andrea Marchesini (:baku) from comment #28)
> Sorry smaug, I forgot to tell you something about these patches.
> 
> patch 1: Here we do 2 things: we split BlobImpl and Directories. Directory
> objs need just a path in order to work. Plus they are not thread-safe.
> Everywhere we used to have BlobImpl for Directory, now we have a
> BlobImplOrDirectory struct.
Hmm, a bit odd to have struct for threadsafe and not-threadsafe things
But looking.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8714516 [details] [diff] [review]
patch 1 - Directory vs Blob vs File

>-  // array of files, containing only the files present in the dataTransfer
>-  RefPtr<FileList> mFiles;
>+  // array of files and directories, containing only the files present in the
>+  // dataTransfer
>+  RefPtr<FileList> mFileList;
This kind of variable renames should really go to a separate patch and not be included in such a massive patch.
(but reviewing this anyhow)

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8714516 [details] [diff] [review]
patch 1 - Directory vs Blob vs File

>-    for (uint32_t i = 0; i < mFiles->Length(); ++i) {
>-      if (mFiles->Item(i)->Impl()->IsDirectory()) {
>-#if defined(ANDROID) || defined(MOZ_B2G)
>-        MOZ_ASSERT(false,
>-                   "Directory picking should have been redirected to normal "
>-                   "file picking for platforms that don't have a directory "
>-                   "picker");
>-#endif
>-        nsAutoString path;
>-        mFiles->Item(i)->GetMozFullPathInternal(path, aRv);
>-        if (aRv.Failed()) {
>-          return nullptr;
>-        }
>-        int32_t leafSeparatorIndex = path.RFind(FILE_PATH_SEPARATOR);
>-        nsDependentSubstring dirname = Substring(path, 0, leafSeparatorIndex);
>-        nsDependentSubstring basename = Substring(path, leafSeparatorIndex);
>-
>-        RefPtr<OSFileSystem> fs = new OSFileSystem(dirname);
>-        fs->Init(parentNode->OwnerDoc()->GetInnerWindow());
>-
>-        RefPtr<Directory> directory = new Directory(fs, basename);
>-        directory->SetContentFilters(NS_LITERAL_STRING("filter-out-sensitive"));
>-        filesAndDirsSeq[i].SetAsDirectory() = directory;
>-      } else {
>-        filesAndDirsSeq[i].SetAsFile() = mFiles->Item(i);
>-      }
>+    for (uint32_t i = 0; i < mFileList->Length(); ++i) {
>+      filesAndDirsSeq[i].SetAsFile() = mFileList->Item(i);
>     }
I so not understand this. We always pretend mFileList contains only Files.
Doesn't this just crash.


> CreateDirectoryTask::CreateDirectoryTask(
>   FileSystemBase* aFileSystem,
>   const FileSystemCreateDirectoryParams& aParam,
>-  FileSystemRequestParent* aParent)
>+  FileSystemRequestParent* aParent,
>+  ErrorResult& aRv)
>   : FileSystemTaskBase(aFileSystem, aParam, aParent)
> {
>   MOZ_ASSERT(XRE_IsParentProcess(),
>              "Only call from parent process!");
>   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>   MOZ_ASSERT(aFileSystem);
>-  mTargetRealPath = aParam.realPath();
>+
>+  NS_ConvertUTF16toUTF8 path(aParam.realPath());
>+  aRv = NS_NewNativeLocalFile(path, true, getter_AddRefs(mTargetPath));
>+  NS_WARN_IF(aRv.Failed());
> }
Hmm, this cries for an Init method which may then return null task and some error code.


>-  RefPtr<Directory> dir = new Directory(mFileSystem, mTargetRealPath);
>+  RefPtr<Directory> dir = Directory::Create(mFileSystem->GetWindow(),
>+                                            mTargetPath,
>+                                            false, // is root directory
ok, that comment hints that we need an enum parameter, not a boolean. eRootDirectory and eNotRootDirectory or some such



> class CreateFileTask final
>   : public FileSystemTaskBase
> {
> public:
>   CreateFileTask(FileSystemBase* aFileSystem,
>-                 const nsAString& aPath,
>+                 nsIFile* aFile,
>                  Blob* aBlobData,
>                  InfallibleTArray<uint8_t>& aArrayData,
>                  bool replace,
>                  ErrorResult& aRv);
>   CreateFileTask(FileSystemBase* aFileSystem,
>                  const FileSystemCreateFileParams& aParam,
>-                 FileSystemRequestParent* aParent);
>+                 FileSystemRequestParent* aParent,
>+                 ErrorResult& aRv);
yeah, so, I don't like constructors returning errorresult. This really
wants a static Create method or ctor + Init().

>+++ b/dom/filesystem/DeviceStorageFileSystem.cpp
>@@ -41,27 +41,27 @@ DeviceStorageFileSystem::DeviceStorageFi
>     !mozilla::Preferences::GetBool("device.storage.prompt.testing", false);
> 
>   // Get the permission name required to access the file system.
>   nsresult rv =
>     DeviceStorageTypeChecker::GetPermissionForType(mStorageType, mPermission);
>   NS_WARN_IF(NS_FAILED(rv));
> 
>   // Get the local path of the file system root.
>-  // Since the child process is not allowed to access the file system, we only
>-  // do this from the parent process.
>-  if (!XRE_IsParentProcess()) {
>-    return;
>-  }
>   nsCOMPtr<nsIFile> rootFile;
>   DeviceStorageFile::GetRootDirectoryForType(aStorageType,
>                                              aStorageName,
>                                              getter_AddRefs(rootFile));
> 
>   NS_WARN_IF(!rootFile || NS_FAILED(rootFile->GetPath(mLocalRootPath)));
>+
>+  if (!XRE_IsParentProcess()) {
>+    return;
>+  }
Why we can do this change?



>+Directory::Create(nsPIDOMWindowInner* aWindow, nsIFile* aFile,
>+                  bool aIsRoot, FileSystemBase* aFileSystem)
> {
>-  MOZ_ASSERT(aFileSystem, "aFileSystem should not be null.");
>-  // Remove the trailing "/".
>-  mPath.Trim(FILESYSTEM_DOM_PATH_SEPARATOR, false, true);
>+  MOZ_ASSERT(NS_IsMainThread());
>+  MOZ_ASSERT(aWindow);
>+  MOZ_ASSERT(aFile);
>+
>+#ifdef DEBUG
>+  bool isDir;
>+  nsresult rv = aFile->IsDirectory(&isDir);
>+  MOZ_ASSERT(NS_SUCCEEDED(rv) && isDir);
>+#endif
ok, thanks, this MOZ_ASSERT is like the key thing of the whole patch.

> Directory::RemoveInternal(const StringOrFileOrDirectory& aPath, bool aRecursive,
>                           ErrorResult& aRv)
> {
>   nsresult error = NS_OK;
>-  nsAutoString realPath;
>+  nsCOMPtr<nsIFile> realPath;
>   RefPtr<BlobImpl> blob;
> 
>   // Check and get the target path.
> 
>   if (aPath.IsFile()) {
>     blob = aPath.GetAsFile().Impl();
>   } else if (aPath.IsString()) {
>-    if (!DOMPathToRealPath(aPath.GetAsString(), realPath)) {
>-      error = NS_ERROR_DOM_FILESYSTEM_INVALID_PATH_ERR;
>-    }
>-  } else if (!mFileSystem->IsSafeDirectory(&aPath.GetAsDirectory())) {
>+    error = DOMPathToRealPath(aPath.GetAsString(), getter_AddRefs(realPath));
>+  } else if (!GetFileSystem()->IsSafeDirectory(&aPath.GetAsDirectory())) {
So GetFileSystem() as a name hints that it may return null. Why is this call here safe?

>+Directory::GetPath(nsAString& aRetval, ErrorResult& aRv)
> {
>-  if (mPath.IsEmpty()) {
>-    // The Directory ctor removes any trailing '/'; this is the root directory.
>+  if (mIsRoot) {
>     aRetval.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR);
>   } else {
>-    aRetval = mPath;
>+    // TODO: this should be a bit different...
>+    GetName(aRetval, aRv);
will this be fixed in some other patch in this bug?


> FileSystemBase::GetLocalFile(const nsAString& aRealPath) const
> {
>   MOZ_ASSERT(XRE_IsParentProcess(),
>              "Should be on parent process!");
>+
>+  // Let's convert the input path to /path
>   nsAutoString localPath;
>-  FileSystemUtils::NormalizedPathToLocalPath(aRealPath, localPath);
>-  localPath = mLocalRootPath + localPath;
>+  if (!aRealPath.IsEmpty() &&
>+      !StringBeginsWith(aRealPath,
>+                        NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR))) {
>+    localPath.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR);
>+  }
>+  localPath.Append(aRealPath);
I'm lost here. the param is called aRealPath so I assume it is using OS dependent separators, but
the code here deals only DOM_PATH_SEPARATOR

>   // We cannot store File or Directory objects bacause this object is created
>   // on a different thread and File and Directory are not thread-safe.
>-  nsTArray<RefPtr<BlobImpl>> mTargetBlobImpls;
>+  struct BlobImplOrDirectoryPath
>+  {
>+    RefPtr<BlobImpl> mBlobImpl;
>+    nsString mDirectoryPath;
>+
>+    enum {
>+      eBlobImpl,
>+      eDirectoryPath
>+    } mType;
>+  };
ah, this is all indeed thread safe. good.

>+GetFileOrDirectoryTask::GetRequestParams(const nsString& aFileSystem,
>+                                         ErrorResult& aRv) const
what does aFileSystem as a param name mean? Is that a path? filename? does it use
OS dependent path separators if it is a path? Or is it something totally different?
 In other words, from the name of the method and params I have no clue what the
method actually does.

>+
>+    struct BlobImplOrDirectoryPath
>+    {
>+      enum
>+      {
>+        eBlobImpl,
>+        eDirectoryPath
>+      } mType;
>+
>+      RefPtr<BlobImpl> mBlobImpl;
>+      nsString mDirectoryPath;
>+    };
So why do we have the same struct several times. Could you have it just onces

>@@ -5788,17 +5899,17 @@ HTMLInputElement::AddStates(EventStates 
>                                        NS_EVENT_STATE_FOCUSRING));
>     if (!focusStates.IsEmpty()) {
>       HTMLInputElement* ownerNumberControl = GetOwnerNumberControl();
>       if (ownerNumberControl) {
>         ownerNumberControl->AddStates(focusStates);
>       }
>     }
>   }
>-  nsGenericHTMLFormElementWithState::AddStates(aStates);                          
>+  nsGenericHTMLFormElementWithState::AddStates(aStates);
totally unrelated change.



>+  struct BlobImplOrString
>+  {
>+    RefPtr<BlobImpl> mBlobImpl;
>+    nsString mDirectoryPath;
>+
>+    enum {
>+      eBlobImpl,
>+      eDirectoryPath
>+    } mType;
>+  };
oh, again here

> 
>-  class FileSizeAndDateRunnable : public nsRunnable
>+  class IORunnable : public nsRunnable
Please document what IORunnable is for. The new name doesn't hint at all what it is doing.


>-  RefPtr<SimpleEnumerator> enumerator = new SimpleEnumerator(mFilesOrDirectories);
>+  RefPtr<SimpleEnumerator> enumerator =
>+    new SimpleEnumerator(mFilesOrDirectories);
Please don't do unrelated stuff in this kind of massive patches




I'm probably missing still many cases, but please update the patch and upload both interdiff and the actual patch.

Comment 36

[Andrea Marchesini [:baku]]

> I so not understand this. We always pretend mFileList contains only Files.
> Doesn't this just crash.

This is the first patch of a set. At this point mFileList contains only Files.
This code is correct because we don't store any directory there yet.

> >+
> >+  if (!XRE_IsParentProcess()) {
> >+    return;
> >+  }
> Why we can do this change?

I also checked this with dougt: we must have the localRoot correctly set in the child process too.

> 
> >+    // TODO: this should be a bit different...
> >+    GetName(aRetval, aRv);
> will this be fixed in some other patch in this bug?

No. it will be a follow up. I kept the current behavior. I'll write the bug ID.

> >+  localPath.Append(aRealPath);
> I'm lost here. the param is called aRealPath so I assume it is using OS
> dependent separators, but
> the code here deals only DOM_PATH_SEPARATOR

We are dealing with DOM paths. No OS paths here. In all this patch, all the times there is a string, it's a DOM path, otherwise it's a nsIFile.

> >+GetFileOrDirectoryTask::GetRequestParams(const nsString& aFileSystem,
> >+                                         ErrorResult& aRv) const
> what does aFileSystem as a param name mean? Is that a path? filename? does
> it use
> OS dependent path separators if it is a path? Or is it something totally
> different?
>  In other words, from the name of the method and params I have no clue what
> the
> method actually does.


FileSystem object must implement a 'ToString()' method. This is used to recreate the FileSystem on the other thread.
I renamed that to aFileSystemString.

Comment 37

[Andrea Marchesini [:baku]]

Attachment: patch 1 - interdiff

Comment 38

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Directory vs Blob vs File

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrea Marchesini (:baku) from comment #36)
> > >+  localPath.Append(aRealPath);
> > I'm lost here. the param is called aRealPath so I assume it is using OS
> > dependent separators, but
> > the code here deals only DOM_PATH_SEPARATOR
> 
> We are dealing with DOM paths. No OS paths here. In all this patch, all the
> times there is a string, it's a DOM path, otherwise it's a nsIFile.
Usually you call string realpath when it is assign to a value taken from an nsIFile, but in this method it is something else.


> FileSystem object must implement a 'ToString()' method. This is used to
> recreate the FileSystem on the other thread.
> I renamed that to aFileSystemString.
Still no idea what the string contains.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Yeah, after looking at the source code, I have zero idea what ToString() actually returns.
Is that a path or what? Apparently normally path but devicestorage does something else.
I think we shouldn't then store mString at all, but different subclasses should have their own member variables (like OSFileSystem seems to have already, but it still uses also this mysteriously named mString). And ToString() needs to be renamed to something which actually hints what it is returning.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8723996 [details] [diff] [review]
patch 1 - Directory vs Blob vs File

So, I would like to see that ToString() handling to be fixed in this bug, given that this touches all the relevant code anyhow.
ToString should be renamed to something which tells what the returned string is about.
That should be a followup patch since it is rather separate thing from this patch.


+GetDirectoryListingTask::GetDirectoryListingTask(FileSystemBase* aFileSystem,
+                                                 nsIFile* aTargetPath,
+                                                 Directory::DirectoryType aType,
+                                                 const nsAString& aFilters)

Why do we actually need the type. In previous patch it was a boolean, but effectively the same.
Why isn't aTargetPath enough? see also comment below...

+++ b/dom/html/HTMLInputElement.cpp
...
+    void
+    GetFilesOrDirectories(nsPIDOMWindowInner* aWindow,
+                          nsTArray<OwningFileOrDirectory>& aResult) const
     {
-      return mBlobImpls;
-    }
-
-    void SetBlobImpls(const nsTArray<RefPtr<File>>& aFile)
+      for (uint32_t i = 0; i < mBlobImplsOrDirectoryPaths.Length(); ++i) {
+        if (mBlobImplsOrDirectoryPaths[i].mType == Directory::BlobImplOrDirectoryPath::eBlobImpl) {
+          RefPtr<File> file =
+            File::Create(aWindow,
+                         mBlobImplsOrDirectoryPaths[i].mBlobImpl);
+          MOZ_ASSERT(file);
+
+          OwningFileOrDirectory* element = aResult.AppendElement();
+          element->SetAsFile() = file;
+        } else {
+          MOZ_ASSERT(mBlobImplsOrDirectoryPaths[i].mType == Directory::BlobImplOrDirectoryPath::eDirectoryPath);
+
+          nsCOMPtr<nsIFile> file;
+          NS_ConvertUTF16toUTF8 path(mBlobImplsOrDirectoryPaths[i].mDirectoryPath);
+          nsresult rv = NS_NewNativeLocalFile(path, true, getter_AddRefs(file));
+          if (NS_WARN_IF(NS_FAILED(rv))) {
+            continue;
+          }
+
+          RefPtr<Directory> directory = Directory::Create(aWindow, file,
+                                                          Directory::eRootDirectory);
... I don't actually understand why we pass eRootDirectory here. I guess I didn't understand before either what the boolean was about, but comment
said it was about root.
Here we create a Directory object pointing to directory (==file), so what is the 3rd param to ::Create about?
I'd like to see another patch to fix root handling somehow. Perhaps it is just renaming the enum values or maybe it is something more. Hard to say, since I don't know what
the param is about.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

(and bear with me here, the patch is massive and the old code isn't exactly clear either.)

Comment 43

[Andrea Marchesini [:baku]]

> Why do we actually need the type. In previous patch it was a boolean, but
> effectively the same.
> Why isn't aTargetPath enough? see also comment below...

In case we are dealing with the root directory of the filesystem, we create it if it doesn't exist.

> ... I don't actually understand why we pass eRootDirectory here. I guess I
> didn't understand before either what the boolean was about, but comment
> said it was about root.

By spec, the root directory is not the root directory of the filesystem, but the chosen directory from the file picker.
This means that if I select /home/baku/foo, the name of that directory will be '/'. But then, the sub directory will have they real names. For instance: /home/baku/foo/bar will be called 'bar'.

Comment 44

[Andrea Marchesini [:baku]]

Attachment: patch 1b - toString/fromString

Comment 45

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Directory vs Blob vs File

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8724692 [details] [diff] [review]
patch 1b - toString/fromString

I think I would call it SerializePath or SerializeDOMPath, since we aren't serializing the whole filesystem here.

And then DeserializePath or DeserializeDOMPath.

I think I'd prefer those with DOM, since otherwise someone might think of normal file path.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8727480 [details] [diff] [review]
patch 1 - Directory vs Blob vs File


>-CreateDirectoryTask::GetRequestParams(const nsString& aFileSystem) const
>+CreateDirectoryTask::GetRequestParams(const nsString& aFileSystemString,
>+                                      ErrorResult& aRv) const
I don't understand the param name change. It is equally unclear before and after.
I think the param is "DOM path" here, so the name should hint about it. aSerializedDOMPath? or something, not quite sure what is the clearest name.

>-CreateFileTask::GetRequestParams(const nsString& aFileSystem) const
>+CreateFileTask::GetRequestParams(const nsString& aFileSystemString,
>+                                 ErrorResult& aRv) const
Ditto

>+Directory::GetPath(nsAString& aRetval, ErrorResult& aRv)
> {
>-  if (mPath.IsEmpty()) {
>-    // The Directory ctor removes any trailing '/'; this is the root directory.
>+  if (mType == eDOMRootDirectory) {
>     aRetval.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR);
>   } else {
>-    aRetval = mPath;
>+    // TODO: this should be a bit different...
>+    GetName(aRetval, aRv);
Followup patch? Or maybe some patch in this bug actually fixes this already.

>+Directory::GetFullPath(nsAString& aPath)
I think I'd call this GetFullRealPath to be clear what this is about

>-public:
>   static already_AddRefed<Promise>
>   GetRoot(FileSystemBase* aFileSystem, ErrorResult& aRv);
> 
>-  Directory(FileSystemBase* aFileSystem, const nsAString& aPath);
>+  enum DirectoryType {
>+    eDOMRootDirectory,
>+    eNotRootDirectory
>+  };
Please add still some comment what DirectoryType is about.

>-GetDirectoryListingTask::GetRequestParams(const nsString& aFileSystem) const
>+GetDirectoryListingTask::GetRequestParams(const nsString& aFileSystemString,
Again here. No idea what the param means.

>+GetDirectoryListingTask::CreateNormalizedRelativePath(const nsAString& aPath,
>+                                                      nsAString& aRelativePath) const
the method name isn't too clear, but looks like there is already LocalPathToNormalizedPath so fine.

>+struct FileSystemDirectoryListingResponseBlob
>+{
>+  PBlob blob;
>+};
>+
>+struct FileSystemDirectoryListingResponseDirectory
>+{
>+  nsString directoryPath;
>+};
This really needs a comment whether the value is a real path or DOM path. And I think the name of the property should indicate what it contains


>+nsresult
>+LocalFileToISupports(nsPIDOMWindowInner* aWindow,
>+                     bool aIsDirectory,
>+                     nsIFile* aFile,
>+                     nsISupports** aResult)
This really need some other name. When I first read this, I thought this would just do
nsCOMPtr<nsISupports> supports = do_QueryInterface(aFile);
supports.swap(aResult);
Perhaps call it FileToDirectoryOrBlob() ?




This is all very complicated. Do we have plan how to get this code enabled? This needs tons of testing before that. Mostly manual testing I think.
And the API isn't being used in the wild yet? Or has MS shipped this? If not, this really needs lots of testing before we can enabled in beta/release, since
if there isn't content using the API, nightly/aurora don't really help much.

Comment 48

[Andrea Marchesini [:baku]]

I would say that it's over-compilcated because we have DeviceStorage API. This is a non standardized API that eventually we will remove (maybe).

Currently all of this is disabled by pref. If you don't mind, I would like to have these tons of tests in follow ups, and of course, before enabling it by pref.

Comment 49

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Directory vs Blob vs File

Comment 50

[Andrea Marchesini [:baku]]

Attachment: patch 2 - toString/fromString

Comment 51

[Andrea Marchesini [:baku]]

Attachment: patch 3 - makes Windows happy - deal with windows paths

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

I just wonder how to have good tests for this. But I guess we can use some testing filepicker.
But in general this is among those features which are quite hard to test. Similar to drag-and-drop.

Comment 53

[Andrea Marchesini [:baku]]

Attachment: patch 4 - FileList with Directories

Comment 54

[Andrea Marchesini [:baku]]

Attachment: patch 5 - cleanup

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8730729 [details] [diff] [review]
patch 4 - FileList with Directories

>+      if (tag == SCTAG_DOM_BLOB) {
>+        MOZ_ASSERT(indexOrLengthOfString < aHolder->BlobImpls().Length());
> 
>-      RefPtr<File> file = File::Create(aHolder->ParentDuringRead(), blobImpl);
>-      if (!fileList->Append(file)) {
>+        RefPtr<BlobImpl> blobImpl =
>+          aHolder->BlobImpls()[indexOrLengthOfString];
>+        MOZ_ASSERT(blobImpl->IsFile());
>+
>+        ErrorResult rv;
>+        blobImpl = EnsureBlobForBackgroundManager(blobImpl, nullptr, rv);
>+        if (NS_WARN_IF(rv.Failed())) {
>+          rv.SuppressException();
>+          return nullptr;
>+        }
>+
>+        RefPtr<File> file =
>+          File::Create(aHolder->ParentDuringRead(), blobImpl);
>+        MOZ_ASSERT(file);
>+
>+        fileList->Append(file);
>+        continue;
>+      }
>+
>+      nsAutoString path;
>+      path.SetLength(indexOrLengthOfString);
>+      size_t charSize = sizeof(nsString::char_type);
>+      if (!JS_ReadBytes(aReader, (void*) path.BeginWriting(),
>+                        indexOrLengthOfString * charSize)) {
>         return nullptr;
>       }
>+
>+      nsCOMPtr<nsIFile> file;
>+      nsresult rv = NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(path), true,
>+                                          getter_AddRefs(file));
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        return nullptr;
>+      }
>+
>+      RefPtr<Directory> directory =
>+        Directory::Create(aHolder->ParentDuringRead(), file,
>+                          tag ? Directory::eDOMRootDirectory
>+                              : Directory::eNotDOMRootDirectory);
Hmm, tag has some odd triple meaning. Either SCTAG_DOM_BLOB or DOMRootDirectory or eNotDOMRootDirectory.
Could you rename tag to something else? tagOrDirectoryType perhaps?

> // The format of the FileList serialization is:
> // - pair of ints: SCTAG_DOM_FILELIST, Length of the FileList
>-// - pair of ints: 0, The offset of the BlobImpl array
>+// - for each element of the FileList:
>+//   - if it's a blob:
>+//    - pair of ints: SCTAG_DOM_BLOB, index of the BlobImpl in the array
>+//       mBlobImplArray.
>+//   - else:
>+//     - pair of ints: 0/1 is root, string length
>+//     - value string
Add static assertion somewhere that SCTAG_DOM_BLOB is not 0 or 1.



>@@ -957,16 +999,17 @@ StructuredCloneHolder::CustomReadHandler
> {
>   MOZ_ASSERT(mSupportsCloning);
> 
>   if (aTag == SCTAG_DOM_BLOB) {
>     return ReadBlob(aCx, aIndex, this);
>   }
> 
>   if (aTag == SCTAG_DOM_FILELIST) {
>+    MOZ_ASSERT(mSupportedContext == SameProcessSameThread);
...


>   // See if this is a FileList object.
>-  {
>+  if (mSupportedContext == SameProcessSameThread) {
Wait what? We do currently let one to pass FileList to workers, even though the interface isn't exposed there (please file a bug to expose the interface in workers).
FileList should be exposed in Workers.
But anyway, this change breaks existing behavior. So please fix in a follow up patch (but in this bug anyhow)
Because of this issue r-

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8730734 [details] [diff] [review]
patch 5 - cleanup

>+  bool IsSafePath() const;
>+  bool IsSafePath(const nsAString& aPath,
>+                  nsTArray<nsString>* aParts = nullptr) const;
I don't understand the latter IsSafePath. It checks if path is safe but does also something totally different.
Should be renamed to something which describes what the method actually does.

Comment 57

[Andrea Marchesini [:baku]]

> FileList should be exposed in Workers.

Ok... what about if, just because directories are not currently exposed, we support the cloning of FileList objects to workers only if they don't contain directories.
Then in a follow up we make Directory 'thread-safe' and we mark it as a dependency to enable the API.

I'm saying this because make FileSystem API thread-safe it's not 1 single patch, I guess.

Comment 58

[Andrea Marchesini [:baku]]

Attachment: patch 6 - FileList::ClonableToWorkers

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrea Marchesini (:baku) from comment #57)
> > FileList should be exposed in Workers.
> 
> Ok... what about if, just because directories are not currently exposed, we
> support the cloning of FileList objects to workers only if they don't
> contain directories.
> Then in a follow up we make Directory 'thread-safe' and we mark it as a
> dependency to enable the API.
Sounds ok
Just make sure we have the bug to enable the API filed so that the follow-up bug to fix Directory implementation can
block it.

Comment 60

[Andrea Marchesini [:baku]]

Can I consider the last r- as a r+ then?

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrea Marchesini (:baku) from comment #60)
> Can I consider the last r- as a r+ then?

No. We can't land a patch with 
>-  {
>+  if (mSupportedContext == SameProcessSameThread) {

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

... we need to do that only conditionally if there are Directories in the list.

Comment 63

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #62)
> ... we need to do that only conditionally if there are Directories in the
> list.

Oh yes. there is patch 6 for that.

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8731090 [details] [diff] [review]
patch 6 - FileList::ClonableToWorkers

Hmm, is CloneableToWorkers the right name. Don't we actually support cloning across process boundaries too, since blobs can be sent that way anyhow.
So with the method name fix, r+

Comment 65

[Andrea Marchesini [:baku]]

Attachment: patch 7 - Test for FileList and Directories

directory.path is still broken. But it was already broken and I'm going to fix it in a follow up. It should contain the "absolute" path starting from the DOM root directory.

Comment 66

[Andrea Marchesini [:baku]]

Attachment: patch 8 - make e10s tests happy again

The reason why we need this extra patch is because in our tests we use TmpD/device-storage-testing as devicestorage directory, but in the child and parent processes have different TmpD (in the child we add a UUID). This makes IsSafePath() to fail because, of course, the path received from the child is not a descendant path from that the parent is using.

With this patch I overwrite the path in our tests to be TmpD/device-storage-testing always.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8732500 [details] [diff] [review]
patch 8 - make e10s tests happy again

So why do we have kPrefTesting set then? Could the existing code work if that isn't set but kPrefOverrideRootDir is?
Hmm, I guess we use kPrefTesting (== "device.storage.testing") also for other stuff.

Comment 69

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac58a56021ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf65b38f759e
https://hg.mozilla.org/integration/mozilla-inbound/rev/401c1e7df8ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/e94ced2f8e3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/56bbea4fe199
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7e79d9a750
https://hg.mozilla.org/integration/mozilla-inbound/rev/db971ba7c26d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c224d964bc

Comment 70

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd8284e36c7c

https://treeherder.mozilla.org/logviewer.html#?job_id=24175720&repo=mozilla-inbound

Comment 71

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/176128ba757f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a965c2f796
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb298010e12e
https://hg.mozilla.org/integration/mozilla-inbound/rev/3de390c6f47f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf8e2fcedad
https://hg.mozilla.org/integration/mozilla-inbound/rev/23b0e55ff1db
https://hg.mozilla.org/integration/mozilla-inbound/rev/094819fbb07f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e3a105b9160

Comment 72

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/de7baace275a for test_basic.html failing on Android, https://treeherder.mozilla.org/logviewer.html#?job_id=24185444&repo=mozilla-inbound

Comment 73

[Ryan VanderMeulen [:RyanVM]]

More failures: https://treeherder.mozilla.org/logviewer.html#?job_id=24189230&repo=mozilla-inbound

This has bounced twice now. Please verify that this is clean on Try before pushing to inbound again.

Comment 74

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2417713773ac&selectedJob=18321245

Right. Sorry for this.

Comment 75

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa6fadb22e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d56933b2c2e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/13d57375f694
https://hg.mozilla.org/integration/mozilla-inbound/rev/4542fb0e2c4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0eb0f900c53
https://hg.mozilla.org/integration/mozilla-inbound/rev/98de88ce586d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6befcf45fa6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0b6d057313

Comment 76

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eaa6fadb22e8
https://hg.mozilla.org/mozilla-central/rev/d56933b2c2e5
https://hg.mozilla.org/mozilla-central/rev/13d57375f694
https://hg.mozilla.org/mozilla-central/rev/4542fb0e2c4c
https://hg.mozilla.org/mozilla-central/rev/f0eb0f900c53
https://hg.mozilla.org/mozilla-central/rev/98de88ce586d
https://hg.mozilla.org/mozilla-central/rev/6befcf45fa6c
https://hg.mozilla.org/mozilla-central/rev/7f0b6d057313

Comment 77

[Andrea Marchesini [:baku]]

This is still disabled by pref, but it's important to have documentation about it when we'll enabled Directory/FileSystem API.