Allow FileList objects to contain Directory objects

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: jwatt, Assigned: baku)

Tracking

({dev-doc-needed})

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(8 attachments, 16 obsolete attachments)

189.56 KB, patch
Details | Diff | Splinter Review
12.75 KB, patch
Details | Diff | Splinter Review
13.82 KB, patch
Details | Diff | Splinter Review
44.02 KB, patch
smaug
: review-
Details | Diff | Splinter Review
24.35 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.38 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.53 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.38 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Allow FileList objects to contain Directory objects.
Posted patch WIP (obsolete) — Splinter Review
Still moving stuff around between patches in my queue, but this is roughly there and compiles okay.
Attachment #8620382 - Flags: feedback?(amarchesini)
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())
Attachment #8620382 - Flags: feedback?(amarchesini)
Blocks: 1188880
Attachment #8640502 - Flags: review?(amarchesini)
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.
Attachment #8640502 - Flags: review?(amarchesini) → review+
R+ was four months ago - anything blocking this from moving forward? Jonathan are you still available to work on it, or need new owner?
Posted patch fileOrDir.patch (obsolete) — Splinter Review
Attachment #8705790 - Flags: review?(bugs)
Attachment #8705790 - Flags: review?(bugs)
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).
Assignee: jwatt → amarchesini
Attachment #8620382 - Attachment is obsolete: true
Attachment #8640502 - Attachment is obsolete: true
Attachment #8705790 - Attachment is obsolete: true
Attachment #8706876 - Flags: review?(bugs)
Attachment #8706876 - Attachment is obsolete: true
Attachment #8706876 - Flags: review?(bugs)
Attachment #8706881 - Flags: review?(bugs)
Follow ups: directories in workers and FileList in workers again.
Attachment #8706882 - Flags: review?(bugs)
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.
Attachment #8706881 - Flags: review?(bugs) → review-
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.)
Attachment #8706882 - Flags: review?(bugs) → review+
> >-#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.
>+  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.
Posted patch inderdiff patch 1 (obsolete) — Splinter Review
I want to add a test for DataTransfer because it's unclear how we receive directories.
Attachment #8707390 - Flags: review?(bugs)
(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.
ni for the comment 15. I still don't understand the path handling.
Flags: needinfo?(amarchesini)
Comment on attachment 8707390 [details] [diff] [review]
inderdiff patch 1

Waiting for answer to the ni? so removing from review queue until that.
Attachment #8707390 - Flags: review?(bugs)
> 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.
Flags: needinfo?(amarchesini)
But if /home/smaug/ is passed, the parent is /home/smaug
Do we somewhere assert that sane paths are passed around?
Attachment #8706881 - Attachment is obsolete: true
Attachment #8712246 - Flags: review?(bugs)
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 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.
Attachment #8712246 - Flags: review?(bugs) → review-
This patch is big, but I think it makes everything much cleaner.
Attachment #8707390 - Attachment is obsolete: true
Attachment #8712246 - Attachment is obsolete: true
Attachment #8714516 - Flags: review?(bugs)
That is massive. Could you give a short description what all the patch does.
(and hopefully it doesn't contain any unrelated fixes ;) )
Mainly rebased. I changed StructuredCloneHelper in order to propagate mIsRoot
Attachment #8706882 - Attachment is obsolete: true
Attachment #8714737 - Flags: review?(bugs)
Posted patch patch 4 - cleanup (obsolete) — Splinter Review
Attachment #8714738 - Flags: review?(bugs)
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 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
Attachment #8714736 - Flags: review?(bugs) → review+
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.
Attachment #8714738 - Flags: review?(bugs) → review-
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.
Attachment #8714737 - Flags: review?(bugs) → review-
I'll get to the largest patch tomorrow.
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 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 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.
Attachment #8714516 - Flags: review?(bugs) → review-
> 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.
Posted patch patch 1 - interdiff (obsolete) — Splinter Review
Attachment #8723995 - Flags: review?(bugs)
Attachment #8714516 - Attachment is obsolete: true
Attachment #8723996 - Flags: review?(bugs)
(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.
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 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.
Attachment #8723996 - Flags: review?(bugs) → review-
Attachment #8723995 - Flags: review?(bugs)
(and bear with me here, the patch is massive and the old code isn't exactly clear either.)
> 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'.
Attachment #8724692 - Flags: review?(bugs)
Attachment #8723996 - Attachment is obsolete: true
Attachment #8727480 - Flags: review?(bugs)
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.
Attachment #8724692 - Flags: review?(bugs) → review+
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.
Attachment #8727480 - Flags: review?(bugs) → review+
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.
Attachment #8727480 - Attachment is obsolete: true
Attachment #8723995 - Attachment is obsolete: true
Attachment #8724692 - Attachment is obsolete: true
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.
Attachment #8714737 - Attachment is obsolete: true
Attachment #8730729 - Flags: review?(bugs)
Attachment #8714738 - Attachment is obsolete: true
Attachment #8730734 - Flags: review?(bugs)
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-
Attachment #8730729 - Flags: review?(bugs) → review-
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.
Attachment #8730734 - Flags: review?(bugs) → review+
> 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.
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(bugs)
Can I consider the last r- as a r+ then?
Blocks: 1257179
Blocks: 1257180
(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) {
... we need to do that only conditionally if there are Directories in the list.
(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 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+
Attachment #8731090 - Flags: review?(bugs) → review+
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.
Attachment #8732455 - Flags: review?(bugs)
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.
Attachment #8732500 - Flags: review?(bugs)
Duplicate of this bug: 1176942
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.
Attachment #8732500 - Flags: review?(bugs) → review+
Attachment #8732455 - Flags: review?(bugs) → review+
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.
This is still disabled by pref, but it's important to have documentation about it when we'll enabled Directory/FileSystem API.
Keywords: dev-doc-needed
Depends on: 1258482
No longer blocks: 1257179
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.