1258221 - Port Directory/FileSystem/DeviceStorage APIs to PBackground

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

In order to support Directory objects in workers we must use PBackground instead PContent.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Do we actually like these APIs?  Without b2g I'd like to get rid of DeviceStorage ...

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch 1 - File::CreateFromFile only for main-thread

The current use of Blob/File in dom/filesystem/* doesn't work on PBackground because nsIMIMEService is not thread-safe.
Instead, I propose to use full real path as we do for Directories and only on the main-thread we create the real file (a not in the I/O thread).

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch 2 - PBackground

This patch is not simple and it changes the logic of the FileSystem API quite a bit. I wrote a lot of comments to describe the new logic but here a quick summary:

1. we use PBackground instead PContent

2. many tasks were written when BlobImpl was not fully thread-safe. I simplified the code of them, reducing the number of class members and the use of BlobImpl.

3. A task must implement the IOWork() method (previously called 'Work'). If they need to go to the main-thread before it, they can implement MainThreadWork(). Only CreateFileTask does it currently.

4. Also a FileSystem object could need to do something on the main-thread (only DeviceStorageFileSystem actually). If it implements a MainThreadWork() this operation is done as part of the MainThreadWork() of the task that we are running.

5. Permission check: some task (and some filesystem -DeviceStorageFileSystem-) must require some permission check before running. This can happen on the main-thread. This will be moved to step 4 eventually when I'll port all this to workers. See FileSystemTaskBase.h for how this check is implemented.

6. FileSystem objects are not thread-safe but they can be created on the PBackground thread. They have AssertOnOwningThread() everywrite.

7. FileSystemPermissionRequest takes care of the PBackground initialization the correct scheduling of tasks.

8. See FileSystemBase diagram for the IPC logic. It has been updated following the new implementation.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 1 - File::CreateFromFile only for main-thread

HTMLInputElement cannot share the same struct FileOrDirectoryPath with Directory.
This patch is fully green on treeherder and it can land separately in case we want.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch 2 - PBackground

Small changes to make treeherder happy again

Comment 6

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> Do we actually like these APIs?  Without b2g I'd like to get rid of
> DeviceStorage ...

Well, the key thing here is that we need to get Directory API to work in workers. If that means hopefully some minor updates to other API implementation, we can live with that.
Removing some old APIs should be decided separately.
(However if ,say, maintaining DeviceStorage starts to be a bottleneck, we definitely should consider removing it if no one uses it.)

Comment 7

[Andrea Marchesini [:baku]]

> (However if ,say, maintaining DeviceStorage starts to be a bottleneck, we
> definitely should consider removing it if no one uses it.)

DeviceStorage API and FileSystem API have many things in common.
I think we can totally build the full FileSystem API on top of the current setup (with some changes here and there of course).

Comment 8

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

Comment on attachment 8732727 [details] [diff] [review]
patch 1 - File::CreateFromFile only for main-thread

>   RefPtr<Promise> mPromise;
>   nsCOMPtr<nsIFile> mDirPath;
>-
>-  // This cannot be a File because this object will be used on a different
>-  // thread and File is not thread-safe. Let's use the BlobImpl instead.
>-  RefPtr<BlobImpl> mTargetBlobImpl;
>   nsCOMPtr<nsIFile> mTargetPath;

Ok, I have no idea what mDirPath is for and what is mTargetPath.
(not that I know what mTargetBlobImpl is either).
and mDirPath isn't in mxr, so it is from some other bug? But it is not from GetPath() (which is supposed to land before this bug, so I don't know where it is from)



...and looking at the other parts too, this is clearly on top of some other patch, but I can't now recognize which one. 
Is https://bugzilla.mozilla.org/show_bug.cgi?id=1258694#c3 not the right order?

Comment 9

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

Comment on attachment 8732728 [details] [diff] [review]
patch 2 - PBackground

and this is on top of part 1, and I can't figure on top of what that one is...
so please ask review one by one, given that there are so many different patches.


(I need to remember to re-read comment 3 when reviewing this.)

Comment 10

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

Comment on attachment 8732727 [details] [diff] [review]
patch 1 - File::CreateFromFile only for main-thread

(1) So it is ok to create BlobImpl from nsIFile in child process, even from sandboxing point of view (now that we're sending string paths all the time and not blobimpls)?


>+    if (mTargetData[i].mType == Directory::FileOrDirectoryPath::eDirectoryPath) {
>       RefPtr<Directory> directory =
>-        Directory::Create(mFileSystem->GetParentObject(),
>-                          directoryPath,
>-                          Directory::eNotDOMRootDirectory,
>-                          mFileSystem);
>+        Directory::Create(mFileSystem->GetParentObject(), path,
>+                          Directory::eNotDOMRootDirectory, mFileSystem);
(2) Do we assert in Directory::Create that path is not the local root when type is Directory::eNotDOMRootDirectory.
If not, please add such assertion


> 
>   RefPtr<Promise> mPromise;
>   nsCOMPtr<nsIFile> mDirPath;
>-
>-  // This cannot be a File because this object will be used on a different
>-  // thread and File is not thread-safe. Let's use the BlobImpl instead.
>-  RefPtr<BlobImpl> mTargetBlobImpl;
>   nsCOMPtr<nsIFile> mTargetPath;
(3) Please document what mDirPath is and what mTargetPath is.
And we really need some assertions to ensure that mDirPath is the parent of mTargetPath when both those are set. Perhaps we could assert in dtor?

All those fixed or explained, r+. (1) is absolutely critical, and (2) and (3) important too.

Comment 11

[Andrea Marchesini [:baku]]

> (1) So it is ok to create BlobImpl from nsIFile in child process, even from
> sandboxing point of view (now that we're sending string paths all the time
> and not blobimpls)?

Yes. It's fine. But main-thread is better because we avoid an extra runnable.
Plus, it's OK only in main-thread and in workers. we cannot create BlobFileImpl in I/O threads or PBackground.
Probably it's better to enforce this with assertions. Follow up for me.

Comment 12

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

Comment on attachment 8732728 [details] [diff] [review]
patch 2 - PBackground

So if FileSystemBase can be used on background thread, what about
OSFileSystem which inherits it and has a strong pointer to cycle collectable object?

OSFileSystem dtor needs to have some check that it is deleted on the owner thread.
And OSFileSystem::Init should have an assertion that it is created either in worker thread or main thread (and never background thread)

Based on the IPC graph, it is somewhat mystery to me why we use FileSystemTaskBase both in main thread and in background thread.
FileSystemTaskBase is inherited by many classes which have cycle collectable member variables.
And also, IOWork method on a class which is used on mainthread/workers doesn't make too much sense.
Doesn't this mean that FileSystemTaskBase has now some double meaning. Would it be simpler to have some *TaskBase for mainthread/worker case and another one for pbackground/IO thread?
It is also a bit odd to have MainThreadWork() on class which is anyhow supposed to be use on main thread often.
(I'm not sure, so, just asking here)


> CreateFileTask::~CreateFileTask()
> {
>-  MOZ_ASSERT((!mPromise && !mBlobData) || NS_IsMainThread(),
>-             "mPromise and mBlobData should be released on main thread!");
>-
>-  if (mBlobStream) {
>-    mBlobStream->Close();
>-  }
>+  MOZ_ASSERT((!mPromise) || NS_IsMainThread(),
>+             "mPromise should be released on main thread!");
So what guarantees mPromise is null if we aren't in the main thread?
Is this because of this double usage of the class where we use this in pbackground thread occasionally and occasionally
in cycle collectable thread?


>@@ -72,23 +78,22 @@ private:
>   CreateFileTask(FileSystemBase* aFileSystem,
>                  const FileSystemCreateFileParams& aParam,
>                  FileSystemRequestParent* aParent);
> 
>   void
>   GetOutputBufferSize() const;
> 
>   static uint32_t sOutputBufferSize;
>+
>   RefPtr<Promise> mPromise;
>   nsCOMPtr<nsIFile> mTargetPath;
> 
>-  // Not thread-safe and should be released on main thread.
>-  RefPtr<Blob> mBlobData;
>+  RefPtr<BlobImpl> mBlobImpl;
>+  InfallibleTArray<uint8_t> mArrayData;
I know you just move mArrayData but it really needs some comment. What kind of data is this about?
It isn't the blob, but something else.
>   RefPtr<FileSystemBase> mFileSystem;
>+  nsCString mPermissionName;
I know you're just changing the permission code a bit, but mPermissionName is rather unclear. What does that string actually contain?
It is something and then "-" and then something else.

>+
>+    // If we are here, it's because the I/O work has been done and we have to
>+    // handle the result back via IPC.
>+    if (IsOnBackgroundThread()) {
>+      mTask->HandleResult();
>+      return NS_OK;
>+    }
>+
>+    // Run I/O thread tasks
>+    nsresult rv = mTask->IOWork();
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      mTask->SetError(rv);
>+    }
>+
>+    // Let's go back to PBackground thread to finish the work.
>+    rv = mBackgroundEventTarget->Dispatch(this, NS_DISPATCH_NORMAL);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      // This is bad. We are going to leak the Task.
>+      return rv;
>+    }
Could we please order these steps to follow the order they are supposed to be executed.
That if (IsOnBackgroundThread()) { should be after IOWork stuff.
(so this would need a thread check for iothread, or !IsOnBackgroundThread()) 

>+FileSystemTaskBase::IPCStart()
I don't understand the meaning of the name of this method.
The method dispatches something from IPC to main thread or IO thread, and the name should somehow indicate that.

>   /*
>+   * Start the 'real' task. This must be called from the PBackground thread only.
>+   */
>+  void
>+  IPCStart();
So I think the method name could be changed and comment improved a bit.


Could you fix those and answer to the questions and update the patch.
I think I'll need to go through this patch couple of times, just because of its size, even though the code itself isn't that complicated.
(though that multi-usage FileSystemTaskBase is tiny bit surprising.)

Comment 13

[Andrea Marchesini [:baku]]

> So if FileSystemBase can be used on background thread, what about
> OSFileSystem which inherits it and has a strong pointer to cycle collectable
> object?

In PBackground we don't windows, so nothing must be CCed.

> OSFileSystem dtor needs to have some check that it is deleted on the owner
> thread.

It's already in the DTOR of FileSystemBase...

> And OSFileSystem::Init should have an assertion that it is created either in
> worker thread or main thread (and never background thread)

We already have it:   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");

> Doesn't this mean that FileSystemTaskBase has now some double meaning. Would
> it be simpler to have some *TaskBase for mainthread/worker case and another
> one for pbackground/IO thread?

Splitting all the tasks... yes. Maybe this would be useful to make the code cleaner.

> It is also a bit odd to have MainThreadWork() on class which is anyhow
> supposed to be use on main thread often.
> (I'm not sure, so, just asking here)

Not really. It's used only of DeviceStorageFileSystem... yeah.

> >+  MOZ_ASSERT((!mPromise) || NS_IsMainThread(),
> >+             "mPromise should be released on main thread!");
> So what guarantees mPromise is null if we aren't in the main thread?

Ok.. so with this patch, we are porting everything to pBackground, but we are not enabling the support for workers yet.
That step is done in a separate patch.

So, if we have a promise, it's because the task is in use by some Directory object and those run on the main-thread only.

> >+  InfallibleTArray<uint8_t> mArrayData;
> I know you just move mArrayData but it really needs some comment. What kind
> of data is this about?

I'm going to add a comment. The data is the content of the file.

> I know you're just changing the permission code a bit, but mPermissionName
> is rather unclear. What does that string actually contain?
> It is something and then "-" and then something else.

This is the permission name that we are going to check, if we need.
We will call AssertAppProcessPermission() in PBackground with that string.
The permission can be device-storage:music-read, for instance.
Check DeviceStorageTypeChecker::GetPermissionForType()

All the other comments will be applied before sending a new patch for review.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch 2 - PBackground

Comment 15

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

Comment on attachment 8737591 [details] [diff] [review]
patch 2 - PBackground

Given that FileSystemTaskBase is anyhow being used in multiple threads, why do we need 
FileSystemTaskIORunnable? Why couldn't FileSystemTaskBase extend nsRunnable?
FileSystemTaskIORunnable::Run seems to anyhow just call methods on mTask.
DispatchToIOThread could then be a method of FileSystemTaskBase (to make the code tiny bit simpler).



+  // This reference is set and unset in the PBackground thread only.
+  RefPtr<FileSystemTaskParentBase> mTask;
I don't see anything guaranteeing that. If some dispatch fails, the runnable is possibly deleted in random thread.
For example in case of  DispatchToIOThread(this);
The comment after that "// This is bad. We are going to leak the Task." doesn't seem to be valid.
mTask is released when the event loop releases the runnable. And at least that ends up asserting in ~FileSystemTaskParentBase
>+CreateDirectoryTask::GetPermissionAccessType(nsCString& aAccess) const
>+{
>+  aAccess.AssignLiteral("create");
>+}
Do we need all the permissions types both in *Task and TaskParent
Though, I don't still understand the permission handling.
But anyhow, should it be the *Parent stuff doing actual permission handling (using main thread)?

>  * The base class to implement a Task class.
>- * The task is used to handle the OOP (out of process) operations.
>- * The file system operations can only be performed in the parent process. When
>- * performing such a parent-process-only operation, a task will delivered the
>- * operation to the parent process if needed.
>+ * The file system operations can only be performed in the parent process. In
>+ * order to avoid duplicated code, we used PBackground for child-parent and
>+ * parent-parent communications.
>  *
>  * The following diagram illustrates the how a API call from the content page
>  * starts a task and gets call back results.
>  *
>- * The left block is the call sequence inside the child process, while the
>- * right block is the call sequence inside the parent process.
>+ * The left block is the call sequence inside any process loading content, while
>+ * the right block is the call sequence only inside the parent process.
>  *
>- * There are two types of API call. One is from the content page of the child
>- * process and we mark the steps as (1) to (8). The other is from the content
>- * page of the parent process and we mark the steps as (1') to (4').
>+ *       Page
>+ *        |
>+ *        | (1)
>+ *  ______|_____________________     |     __________________________________
>+ * |      |                     |    |    |                                  |
>+ * |      V                     |   IPC   | PBackground thread on            |
>+ * | [new FileSystemTaskBase()] |    |    | the parent process               |
>+ * |         |                  |    |    |                                  |
>+ * |         | (2)              |         |                                  |
>+ * |         V                  |   (3)   |                                  |
>+ * |    [GetRequestParams]----------------->[new FileSystemTaskParentBase()] |
>+ * |                            |         |          |                       |
>+ * |                            |    |    |          | (4)     ________________
>+ * |                            |    |    |          |        |                |
>+ * |                            |    |    |          |        | I/O Thread     |
>+ * |                            |    |    |          |        |                |
>+ * |                            |    |    |          -----------> [IOWork]     |
>+ * |                            |   IPC   |                   |     |          |
>+ * |                            |    |    |                   |     | (5)      |
>+ * |                            |    |    |          ----------------          |
>+ * |                            |    |    |          |        |________________|
>+ * |                            |    |    |          |                       |
>+ * |                            |    |    |          V                       |
>+ * |                            |    |    |     [HandleResult]               |
>+ * |                            |    |    |          |                       |
>+ * |                            |         |          | (6)                   |
>+ * |                            |   (7)   |          V                       |
>+ * |   [SetRequestResult]<------------------[GetRequestResult]               |
>+ * |       |                    |         |                                  |
>+ * |       | (8)                |    |    |                                  |
>+ * |       V                    |    |    |                                  |
>+ * |[HandlerCallback]           |   IPC   |                                  |
>+ * |_______|____________________|    |    |__________________________________|
>+ *         |                         |
>+ *         V
>+ *        Page
I would be great if this contained also permission handling, even if it was optional and for DeviceStorage only.

r- mainly because I can't see that mTask handling being safe. (and the code would be possibly simpler without that extra FileSystemTaskIORunnable class).
But please also think about the permission handling. Could we just have that all in one place?

Comment 16

[Andrea Marchesini [:baku]]

> that ends up asserting in ~FileSystemTaskParentBase

Having FileSystemTaskParentBase as nsRunnable, makes the class thread-safe.
I think I can remove all these comments, but still, if 1 of the runnable fails, the FileSystemBase object could be released in the wrong thread. There is nothing I can do to prevent this, I guess.

> Do we need all the permissions types both in *Task and TaskParent
> Though, I don't still understand the permission handling.
> But anyhow, should it be the *Parent stuff doing actual permission handling
> (using main thread)?

Yes I do. Because the permission check happens in the parent process but the UI is shown in the content.
Check: nsContentPermissionUtils::AskPermission

In the parent process I just use the permission for the assertion.

> I would be great if this contained also permission handling, even if it was
> optional and for DeviceStorage only.

Yeah... more ascii art!

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch 2 - PBackground

Comment 18

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

(In reply to Andrea Marchesini (:baku) from comment #16)
> the FileSystemBase object could be released in the wrong thread.
> There is nothing I can do to prevent this, I guess.
Well, explicitly leaking is one option. But also, if the object should be released in the background thread, if
posting the runnable to main or io thread fails, you should ensure that the object is tried to be released in background thread. (I guess dispatching to main thread shouldn't really fail, but io thread might during shutdown)

> Yes I do. Because the permission check happens in the parent process but the
> UI is shown in the content.
> Check: nsContentPermissionUtils::AskPermission
> 
> In the parent process I just use the permission for the assertion.
Hmm, would be still nice to be able to use the same strings at least in both places and not copy paste the names. Or if that is hard, #define the permission names?

Comment 19

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

Comment on attachment 8738075 [details] [diff] [review]
patch 2 - PBackground


>+ *       Page
>+ *        |
>+ *        | (1)
>+ *  ______|_______________________     |     __________________________________
>+ * |      |                       |    |    |                                  |
>+ * |      V                       |   IPC   | PBackground thread on            |
>+ * | [new FileSystemTaskBase()]   |    |    | the parent process               |
>+ * |       |                      |    |    |                                  |
>+ * |       | (2)                  |    |    |                                  |
>+ * |       V                      |    |    |                                  |
>+ * | [FileSystemPermissionRequest |    |    |                                  |
>+ * |  ::RequestForTask()]         |    |    |                                  |
>+ * |         |                    |    |    |                                  |
>+ * |         | (3)                |         |                                  |
>+ * |         V                    |   (4)   |                                  |
>+ * |    [GetRequestParams]------------------->[new FileSystemTaskParentBase()] |
>+ * |                              |         |          |                       |
>+ * |                              |    |    |          | (5)   _____________   |
>+ * |                              |    |    |          |      |             |  |
>+ * |                              |    |    |          |      | I/O Thread  |  |
>+ * |                              |    |    |          |      |             |  |
>+ * |                              |    |    |          ---------> [IOWork]  |  |
>+ * |                              |   IPC   |                 |     |       |  |
>+ * |                              |    |    |                 |     | (6)   |  |
>+ * |                              |    |    |          --------------       |  |
>+ * |                              |    |    |          |      |_____________|  |
>+ * |                              |    |    |          |                       |
>+ * |                              |    |    |          V                       |
>+ * |                              |    |    |     [HandleResult]               |
>+ * |                              |    |    |          |                       |
>+ * |                              |         |          | (7)                   |
>+ * |                              |   (8)   |          V                       |
>+ * |   [SetRequestResult]<--------------------[GetRequestResult]               |
>+ * |       |                      |         |                                  |
>+ * |       | (9)                  |    |    |                                  |
>+ * |       V                      |    |    |                                  |
>+ * |[HandlerCallback]             |   IPC   |                                  |
>+ * |_______|______________________|    |    |__________________________________|
>+ *         |                           |
>+ *         V
>+ *        Page
So this could use some hint about the permission handling on parent side.

>+class FileSystemTaskBase : public PFileSystemRequestChild
I would be nice for all the classes extending some P*Child class to also have Child in their name
Such renaming could be a separate bug, since this patch is massive enough, or at least hard enough to review.


>+class OSFileSystemParent final : public FileSystemBase
It is not clear to me why we want to inherit FileSystemBase here.
Don't we want FileSystemBaseParent or some such


>+struct FileSystemGetDirectoryListingParams
>+{
>+  nsString filesystem;
>+  nsString realPath;
>+  bool isRoot;
>+  // 'filters' could be an array rather than a semicolon separated string
>+  // (we'd then use InfallibleTArray<nsString> internally), but that is
>+  // wasteful.  E10s requires us to pass the filters over as a string anyway,
>+  // so avoiding using an array avoids serialization on the side passing the
>+  // filters.  Since an nsString can share its buffer when copied,
>+  // using that instead of InfallibleTArray<nsString> makes copying the filters
>+  // around in any given process a bit more efficient too, since copying a
>+  // single nsString is cheaper than copying InfallibleTArray member data and
>+  // each nsString that it contains.
>+  nsString filters;
(this is just some moved code, but I don't understand the comment at all. No need to change here.)


>+BackgroundParentImpl::RecvPFileSystemRequestConstructor(
>+                                               PFileSystemRequestParent* aActor,
>+                                               const FileSystemParams& aParams)
>+{
>+  AssertIsInMainProcess();
>+  AssertIsOnBackgroundThread();
>+
>+  dom::FileSystemRequestParent* actor =
>+    static_cast<dom::FileSystemRequestParent*>(aActor);
>+  if (actor->FileSystem()->PermissionCheckType() == FileSystemBase::ePermissionCheckNotRequired) {
>+    return true;
>+  }
>+
>+  RefPtr<ContentParent> parent = BackgroundParent::GetContentParent(this);
>+
>+  // If the ContentParent is null we are dealing with a same-process actor.
>+  if (!parent) {
>+    return true;
>+  }
>+
>+  const nsCString& permissionName = actor->PermissionName();
>+  MOZ_ASSERT(!permissionName.IsEmpty());
>+
>+  // At this point we should have the right permission but we do the last check
>+  // with this runnable. If the app doesn't have the permission, we kill the
>+  // child process.
>+  RefPtr<CheckPermissionRunnable> runnable =
>+    new CheckPermissionRunnable(parent.forget(),
>+                                actor->FileSystem()->PermissionCheckType(),
>+                                permissionName);
>+
>+  nsresult rv = NS_DispatchToMainThread(runnable);
I don't understand this setup. runnable is handled some time later in the main thread. What
guarantees nothing else happens in the background or io thread before the runnable?
Or is it guaranteed that the possible pending IPC messages will be processed after the runnable? Please fix or explain.
Also, the permission check handling in background thread could be documented.

>+BackgroundParentImpl::DeallocPFileSystemRequestParent(
>+                                              PFileSystemRequestParent* aDoomed)
>+{
>+  AssertIsInMainProcess();
>+  AssertIsOnBackgroundThread();
>+
>+  RefPtr<dom::FileSystemRequestParent> parent =
>+      dont_AddRef(static_cast<dom::FileSystemRequestParent*>(aDoomed));
2 spaces for indentation

almost there, but needs still at least one read.

Comment 20

[Andrea Marchesini [:baku]]

> >+class OSFileSystemParent final : public FileSystemBase
> It is not clear to me why we want to inherit FileSystemBase here.

Because the deserialization happens in FileSystemBase::DeserializeDOMPath that returns a FileSystemBase.

> Don't we want FileSystemBaseParent or some such

I don't want to change DeviceStorageFileSystem too :)

Comment 21

[Andrea Marchesini [:baku]]

Attachment: patch 2 - PBackground

Comment 22

[Andrea Marchesini [:baku]]

Attachment: patch 3 - FileSystemTaskChildBase

Comment 23

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

Comment on attachment 8738508 [details] [diff] [review]
patch 2 - PBackground

>+CreateDirectoryTask::SetSuccessRequestResult(const FileSystemResponseValue& aValue,
>+                                             ErrorResult& aRv)
> {
>   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> 
>+  FileSystemDirectoryResponse r = aValue;
>+
>+  NS_ConvertUTF16toUTF8 path(r.realPath());
Why do we need the temporary r? Couldn't you just pass use aValue.realPath() here?

> class CreateDirectoryTask final : public FileSystemTaskBase
> {
...
>+  RefPtr<Promise> mPromise;
>+  nsCOMPtr<nsIFile> mTargetPath;
>+};
Just a random comment, which applies to the old code too. I hope we don't end up keeping Promise objects alive too long by FileSystem code.


>+#define GET_PERMISSION_ACCESS_TYPE(aAccess) \
>+  if (mReplace) {                           \
>+    aAccess.AssignLiteral("write");         \
>+    return;                                 \
>+  }                                         \
>+  aAccess.AssignLiteral("create");
Could you use REMOVE_TASK_PERMISSION and CREATE_DIRECTORY_TASK_PERMISSION here so that we don't
have the literal strings defined in so many places.
And it is not clear to me whether GET_PERMISSION_ACCESS_TYPE makes the code easier to maintain. More important is to not have the string literals defined in many places

>+  // If we are here, PBackground must be up and running
could you add some comment explaining why PBackground must be up and running here?

>+CreateFileTaskParent::Create(FileSystemBase* aFileSystem,
>+                             const FileSystemCreateFileParams& aParam,
>+                             FileSystemRequestParent* aParent,
>+                             ErrorResult& aRv)
>+{
>+  MOZ_ASSERT(XRE_IsParentProcess(), "Only call from parent process!");
>+  AssertIsOnBackgroundThread();
>+  MOZ_ASSERT(aFileSystem);
>+
>+  RefPtr<CreateFileTaskParent> task =
>+    new CreateFileTaskParent(aFileSystem, aParam, aParent);
>+
>+  NS_ConvertUTF16toUTF8 path(aParam.realPath());
>+  aRv = NS_NewNativeLocalFile(path, true, getter_AddRefs(task->mTargetPath));
>+  if (NS_WARN_IF(aRv.Failed())) {
>+    return nullptr;
>+  }
>+
>+  task->mReplace = aParam.replace();
>+
>+  auto& data = aParam.data();
This hides the type pretty well. One needs to read the expression in the following 'if' in other to understand what
data might be about, or read the ipdl. So, could you use actual type and no 'auto'

> DeviceStorageFileSystem::DeviceStorageFileSystem(const nsAString& aStorageType,
>                                                  const nsAString& aStorageName)
>-  : mWindowId(0)
>+  : mStorageType(aStorageType)
>+  , mStorageName(aStorageName)
>+  , mWindowId(0)
> {
So this ctor is possibly called on background thread too?

>-  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>+  mPermissionCheckType = ePermissionCheckByTestingPref;
> 
>-  mStorageType = aStorageType;
>-  mStorageName = aStorageName;
>-
>-  mRequiresPermissionChecks =
>-    !mozilla::Preferences::GetBool("device.storage.prompt.testing", false);
>+  if (NS_IsMainThread()) {
>+    if (mozilla::Preferences::GetBool("device.storage.prompt.testing", false)) {
>+      mPermissionCheckType = ePermissionCheckNotRequired;
>+    } else {
>+      mPermissionCheckType = ePermissionCheckRequired;
>+    }
>+  }

... if so, we should assert there that we're } else { /*...in background thread*/ }

> DeviceStorageFileSystem::Init(nsDOMDeviceStorage* aDeviceStorage)
> {
>-  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>+  AssertIsOnOwningThread();
This is wrong....

>   MOZ_ASSERT(aDeviceStorage);
>+
>   nsCOMPtr<nsPIDOMWindowInner> window = aDeviceStorage->GetOwner();
... because we're addreffing CCable mainthread only object.
So, you need to keep the NS_IsMainThread() assertion.


> DeviceStorageFileSystem::GetParentObject() const
> {
>-  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>+  AssertIsOnOwningThread();
>+
>   nsGlobalWindow* window = nsGlobalWindow::GetInnerWindowWithId(mWindowId);
This is totally unsafe call on non-mainthread.
GetInnerWindowWithId doesn't use locks or anything, and actually it asserts when called on non-mainthread.

>+FileSystemRequestParent::Start()
Could we assert here that mDestroyed is false.


>+ *  ______|_______________________     |     __________________________________
>+ * |      |                       |    |    |                                  |
>+ * |      V                       |   IPC   | PBackground thread on            |
>+ * | [new FileSystemTaskBase()]   |    |    | the parent process               |
>+ * |       |                      |    |    |                                  |
>+ * |       | (2)                  |    |    |                                  |
>+ * |       V                      |    |    |                                  |
>+ * | [FileSystemPermissionRequest |    |    |                                  |
>+ * |  ::RequestForTask()]         |    |    |                                  |
>+ * |         |                    |    |    |                                  |
>+ * |         | (3)                |         |                                  |
>+ * |         V                    |   (4)   |                                  |
>+ * |    [GetRequestParams]------------------->[new FileSystemTaskParentBase()] |
So it would be nice to have some hint here that parent side may do some permission checks.

>+ *   (2) The FileSystemTaskBase object is given to
>+ *   [FileSystemPermissionRequest::RequestForTask()] that will performe a
perform

>+ *   permission check step if needed (See ePermissionCheckType enum). The real
>+ *   operaiton is done on the parent process but it's hidden by
operation

>+  bool
>+  HasError() const { return mErrorValue != NS_OK; }
Don't we want return NS_SUCCEEDED(mErrorValue);
(there are also other success code than just NS_OK)


> OSFileSystem::GetParentObject() const
> {
>-  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>+  AssertIsOnOwningThread();
As long as we support the API on main thread only, this should be NS_IsMainthread() assertion

> OSFileSystem::Unlink()
> {
>+  AssertIsOnOwningThread();
and here too

>   mParent = nullptr;
> }
> 
> void
> OSFileSystem::Traverse(nsCycleCollectionTraversalCallback &cb)
> {
>+  AssertIsOnOwningThread();
and here.
Perhaps change the assertion to something which checks that we're in main thread or in a worker?


Because of couple of wrong assertions, still r-, since either the code is wrong (==not strict enough), or I've misunderstood something

Comment 24

[Andrea Marchesini [:baku]]

Attachment: patch 2 - PBackground

Comment 25

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

Comment on attachment 8738748 [details] [diff] [review]
patch 2 - PBackground

>+void
>+CreateDirectoryTask::SetSuccessRequestResult(const FileSystemResponseValue& aValue,
>+                                             ErrorResult& aRv)
> {
>   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> 
>+  FileSystemDirectoryResponse r = aValue;
Why do you need r variable? I asked this already before.

> 
>+  // If we are here, PBackground must be up and running
>+  PBackgroundChild* actor =
>+    mozilla::ipc::BackgroundChild::GetForCurrentThread();
Could you add a comment here to why PBackground must be up and running.



>+void
>+CreateFileTask::SetSuccessRequestResult(const FileSystemResponseValue& aValue,
>+                                        ErrorResult& aRv)
> {
>   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> 
>+  FileSystemFileResponse r = aValue;
>+
>+  NS_ConvertUTF16toUTF8 path(r.realPath());
>+  aRv = NS_NewNativeLocalFile(path, true, getter_AddRefs(mTargetPath));

why the need for r variable?

>+class ErrorRunnable final : public nsRunnable
> {
> public:
>-  explicit FileSystemReleaseRunnable(RefPtr<FileSystemBase>& aDoomed)
>-    : mDoomed(nullptr)
>+  explicit ErrorRunnable(FileSystemTaskBase* aTask)
>+    : mTask(aTask)
>   {
>-    aDoomed.swap(mDoomed);
>+    MOZ_ASSERT(aTask);
>+    MOZ_ASSERT(NS_IsMainThread());
>   }
> 
>-  NS_IMETHOD Run()
>+  NS_IMETHOD
>+  Run() override
>   {
>-    mDoomed->Release();
>+    MOZ_ASSERT(NS_IsMainThread());
>+    MOZ_ASSERT(mTask->HasError());
>+
>+    mTask->HandlerCallback();
>     return NS_OK;
>   }
> 
> private:
>-  FileSystemBase* MOZ_OWNING_REF mDoomed;
>+  RefPtr<FileSystemTaskBase> mTask;
> };
Why do we even need this. Couldn't a runnable method work there?


>+ *  ______|_______________________     |     __________________________________
>+ * |      |                       |    |    |                                  |
>+ * |      V                       |   IPC   | PBackground thread on            |
>+ * | [new FileSystemTaskBase()]   |    |    | the parent process               |
>+ * |       |                      |    |    |                                  |
>+ * |       | (2)                  |    |    |                                  |
>+ * |       V                      |    |    |                                  |
>+ * | [FileSystemPermissionRequest |    |    |                                  |
>+ * |  ::RequestForTask()]         |    |    |                                  |
>+ * |         |                    |    |    |                                  |
>+ * |         | (3)                |         |                                  |
>+ * |         V                    |   (4)   |                                  |
>+ * |    [GetRequestParams]------------------->[new FileSystemTaskParentBase()] |
>+ * |                              |         |          |                       |

so this is still missing the permission check on parent side.



r+ with those fixed.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d079b46a6d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/985e569816b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9aed5af58b6

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d079b46a6d0
https://hg.mozilla.org/mozilla-central/rev/985e569816b2
https://hg.mozilla.org/mozilla-central/rev/f9aed5af58b6