Bug 1792245 - Improve FileSystemManager shutdown; r=dom-storage-reviewers,jesup

Changes:
- new MozPromise requests are not created after shutdown
- existing MozPromise requests are disconnected during shutdown

Differential Revision: https://phabricator.services.mozilla.com/D158659

dom/fs/api/FileSystemManager.cpp

@@ -73,11 +73,19 @@ NS_IMPL_CYCLE_COLLECTING_ADDREF(FileSystemManager);
 NS_IMPL_CYCLE_COLLECTING_RELEASE(FileSystemManager);
 NS_IMPL_CYCLE_COLLECTION(FileSystemManager, mGlobal, mStorageManager);
 
-void FileSystemManager::Shutdown() { mBackgroundRequestHandler->Shutdown(); }
+void FileSystemManager::Shutdown() {
+  mShutdown.Flip();
+
+  mBackgroundRequestHandler->Shutdown();
+
+  mCreateFileSystemManagerChildPromiseRequestHolder.DisconnectIfExists();
+}
 
 void FileSystemManager::BeginRequest(
     std::function<void(const RefPtr<FileSystemManagerChild>&)>&& aSuccess,
     std::function<void(nsresult)>&& aFailure) {
+  MOZ_ASSERT(!mShutdown);
+
   if (mBackgroundRequestHandler->FileSystemManagerChildStrongRef()) {
     aSuccess(mBackgroundRequestHandler->FileSystemManagerChildStrongRef());
     return;
@@ -89,17 +97,21 @@ void FileSystemManager::BeginRequest(
                  [&aFailure](nsresult rv) { aFailure(rv); });
 
   mBackgroundRequestHandler->CreateFileSystemManagerChild(principalInfo)
-      ->Then(GetCurrentSerialEventTarget(), __func__,
-             [self = RefPtr<FileSystemManager>(this),
-              success = std::move(aSuccess), failure = std::move(aFailure)](
-                 const BoolPromise::ResolveOrRejectValue& aValue) {
-               if (aValue.IsResolve()) {
-                 success(self->mBackgroundRequestHandler
-                             ->FileSystemManagerChildStrongRef());
-               } else {
-                 failure(aValue.RejectValue());
-               }
-             });
+      ->Then(
+          GetCurrentSerialEventTarget(), __func__,
+          [self = RefPtr<FileSystemManager>(this),
+           success = std::move(aSuccess), failure = std::move(aFailure)](
+              const BoolPromise::ResolveOrRejectValue& aValue) {
+            self->mCreateFileSystemManagerChildPromiseRequestHolder.Complete();
+
+            if (aValue.IsResolve()) {
+              success(self->mBackgroundRequestHandler
+                          ->FileSystemManagerChildStrongRef());
+            } else {
+              failure(aValue.RejectValue());
+            }
+          })
+      ->Track(mCreateFileSystemManagerChildPromiseRequestHolder);
 }
 
 already_AddRefed<Promise> FileSystemManager::GetDirectory(ErrorResult& aError) {

dom/fs/api/FileSystemManager.h

@@ -9,7 +9,10 @@
 
 #include <functional>
 
+#include "mozilla/MozPromise.h"
 #include "mozilla/UniquePtr.h"
+#include "mozilla/dom/FlippedOnce.h"
+#include "mozilla/dom/quota/ForwardDecls.h"
 #include "nsCOMPtr.h"
 #include "nsCycleCollectionParticipant.h"
 #include "nsISupports.h"
@@ -49,6 +52,8 @@ class FileSystemManager : public nsISupports {
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_CLASS(FileSystemManager)
 
+  bool IsShutdown() const { return mShutdown; }
+
   void Shutdown();
 
   void BeginRequest(
@@ -66,6 +71,11 @@ class FileSystemManager : public nsISupports {
 
   const RefPtr<FileSystemBackgroundRequestHandler> mBackgroundRequestHandler;
   const UniquePtr<fs::FileSystemRequestHandler> mRequestHandler;
+
+  MozPromiseRequestHolder<BoolPromise>
+      mCreateFileSystemManagerChildPromiseRequestHolder;
+
+  FlippedOnce<false> mShutdown;
 };
 
 }  // namespace dom

dom/fs/child/FileSystemBackgroundRequestHandler.cpp

@@ -31,9 +31,24 @@ FileSystemBackgroundRequestHandler::~FileSystemBackgroundRequestHandler() =
     default;
 
 void FileSystemBackgroundRequestHandler::Shutdown() {
+  mShutdown.Flip();
+
   if (mFileSystemManagerChild) {
+    MOZ_ASSERT(!mCreatingFileSystemManagerChild);
+
     mFileSystemManagerChild->Shutdown();
+
     mFileSystemManagerChild = nullptr;
+
+    return;
+  }
+
+  if (mCreatingFileSystemManagerChild) {
+    MOZ_ASSERT(!mFileSystemManagerChild);
+
+    mCreateFileSystemManagerParentPromiseRequestHolder.Disconnect();
+
+    mCreatingFileSystemManagerChild = false;
   }
 }
 
@@ -46,6 +61,7 @@ RefPtr<BoolPromise>
 FileSystemBackgroundRequestHandler::CreateFileSystemManagerChild(
     const mozilla::ipc::PrincipalInfo& aPrincipalInfo) {
   MOZ_ASSERT(!mFileSystemManagerChild);
+  MOZ_ASSERT(!mShutdown);
 
   using mozilla::ipc::BackgroundChild;
   using mozilla::ipc::Endpoint;
@@ -71,9 +87,6 @@ FileSystemBackgroundRequestHandler::CreateFileSystemManagerChild(
 
     mCreatingFileSystemManagerChild = true;
 
-    // XXX do something (register with global?) so that we can Close() the actor
-    // before the event queue starts to shut down.  That will cancel all
-    // outstanding async requests (returns lambdas with errors).
     backgroundChild
         ->SendCreateFileSystemManagerParent(aPrincipalInfo,
                                             std::move(parentEndpoint))
@@ -81,6 +94,9 @@ FileSystemBackgroundRequestHandler::CreateFileSystemManagerChild(
             GetCurrentSerialEventTarget(), __func__,
             [self = RefPtr<FileSystemBackgroundRequestHandler>(this),
              child](nsresult rv) {
+              self->mCreateFileSystemManagerParentPromiseRequestHolder
+                  .Complete();
+
               self->mCreatingFileSystemManagerChild = false;
 
               if (NS_FAILED(rv)) {
@@ -95,11 +111,15 @@ FileSystemBackgroundRequestHandler::CreateFileSystemManagerChild(
             },
             [self = RefPtr<FileSystemBackgroundRequestHandler>(this)](
                 const mozilla::ipc::ResponseRejectReason&) {
+              self->mCreateFileSystemManagerParentPromiseRequestHolder
+                  .Complete();
+
               self->mCreatingFileSystemManagerChild = false;
 
               self->mCreateFileSystemManagerChildPromiseHolder.RejectIfExists(
                   NS_ERROR_FAILURE, __func__);
-            });
+            })
+        ->Track(mCreateFileSystemManagerParentPromiseRequestHolder);
   }
 
   return mCreateFileSystemManagerChildPromiseHolder.Ensure(__func__);

dom/fs/child/FileSystemBackgroundRequestHandler.h

@@ -10,6 +10,7 @@
 #include "mozilla/MozPromise.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/dom/quota/ForwardDecls.h"
+#include "mozilla/ipc/PBackgroundChild.h"
 
 template <class T>
 class RefPtr;
@@ -52,10 +53,15 @@ class FileSystemBackgroundRequestHandler {
 
   const UniquePtr<fs::FileSystemChildFactory> mChildFactory;
 
+  MozPromiseRequestHolder<
+      mozilla::ipc::PBackgroundChild::CreateFileSystemManagerParentPromise>
+      mCreateFileSystemManagerParentPromiseRequestHolder;
   MozPromiseHolder<BoolPromise> mCreateFileSystemManagerChildPromiseHolder;
 
   RefPtr<FileSystemManagerChild> mFileSystemManagerChild;
 
+  FlippedOnce<false> mShutdown;
+
   bool mCreatingFileSystemManagerChild;
 };  // class FileSystemBackgroundRequestHandler
 

dom/fs/child/FileSystemRequestHandler.cpp

@@ -312,6 +312,11 @@ void FileSystemRequestHandler::GetRootHandle(
   MOZ_ASSERT(aManager);
   MOZ_ASSERT(aPromise);
 
+  if (aManager->IsShutdown()) {
+    aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
   aManager->BeginRequest(
       [onResolve = SelectResolveCallback<FileSystemGetHandleResponse,
                                          RefPtr<FileSystemDirectoryHandle>>(
@@ -334,6 +339,11 @@ void FileSystemRequestHandler::GetDirectoryHandle(
   MOZ_ASSERT(aPromise);
   LOG(("getDirectoryHandle"));
 
+  if (aManager->IsShutdown()) {
+    aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
   if (!IsValidName(aDirectory.childName())) {
     aPromise->MaybeRejectWithTypeError("Invalid directory name");
     return;
@@ -363,6 +373,11 @@ void FileSystemRequestHandler::GetFileHandle(
   MOZ_ASSERT(aPromise);
   LOG(("getFileHandle"));
 
+  if (aManager->IsShutdown()) {
+    aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
   if (!IsValidName(aFile.childName())) {
     aPromise->MaybeRejectWithTypeError("Invalid filename");
     return;
@@ -388,6 +403,11 @@ void FileSystemRequestHandler::GetAccessHandle(
   MOZ_ASSERT(aPromise);
   LOG(("getAccessHandle"));
 
+  if (aManager->IsShutdown()) {
+    aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
   aManager->BeginRequest(
       [request = FileSystemGetAccessHandleRequest(aFile.entryId()),
        onResolve = SelectResolveCallback<FileSystemGetAccessHandleResponse,
@@ -410,6 +430,11 @@ void FileSystemRequestHandler::GetFile(
   MOZ_ASSERT(!aFile.entryId().IsEmpty());
   MOZ_ASSERT(aPromise);
 
+  if (aManager->IsShutdown()) {
+    aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
   aManager->BeginRequest(
       [request = FileSystemGetFileRequest(aFile.entryId()),
        onResolve =
@@ -432,6 +457,11 @@ void FileSystemRequestHandler::GetEntries(
   MOZ_ASSERT(!aDirectory.IsEmpty());
   MOZ_ASSERT(aPromise);
 
+  if (aManager->IsShutdown()) {
+    aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
   aManager->BeginRequest(
       [request = FileSystemGetEntriesRequest(aDirectory, aPage),
        onResolve = SelectResolveCallback<FileSystemGetEntriesResponse, bool>(
@@ -455,6 +485,11 @@ void FileSystemRequestHandler::RemoveEntry(
   MOZ_ASSERT(aPromise);
   LOG(("removeEntry"));
 
+  if (aManager->IsShutdown()) {
+    aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
   if (!IsValidName(aEntry.childName())) {
     aPromise->MaybeRejectWithTypeError("Invalid name");
     return;
@@ -482,6 +517,11 @@ void FileSystemRequestHandler::MoveEntry(
   MOZ_ASSERT(aPromise);
   LOG(("MoveEntry"));
 
+  if (aManager->IsShutdown()) {
+    aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
   // reject invalid names: empty, path separators, current & parent directories
   if (!IsValidName(aNewEntry.childName())) {
     aPromise->MaybeRejectWithTypeError("Invalid name");
@@ -510,6 +550,11 @@ void FileSystemRequestHandler::RenameEntry(
   MOZ_ASSERT(aPromise);
   LOG(("RenameEntry"));
 
+  if (aManager->IsShutdown()) {
+    aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
   // reject invalid names: empty, path separators, current & parent directories
   if (!IsValidName(aName)) {
     aPromise->MaybeRejectWithTypeError("Invalid name");
@@ -539,6 +584,11 @@ void FileSystemRequestHandler::Resolve(
   MOZ_ASSERT(!aEndpoints.childId().IsEmpty());
   MOZ_ASSERT(aPromise);
 
+  if (aManager->IsShutdown()) {
+    aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
   aManager->BeginRequest(
       [request = FileSystemResolveRequest(aEndpoints),
        onResolve =

dom/fs/test/gtest/child/TestFileSystemRequestHandler.cpp

@@ -55,6 +55,13 @@ class TestFileSystemRequestHandler : public ::testing::Test {
     return result.forget();
   }
 
+  already_AddRefed<Promise> GetSimplePromise() {
+    IgnoredErrorResult rv;
+    RefPtr<Promise> result = Promise::Create(mGlobal, rv);
+
+    return result.forget();
+  }
+
   UniquePtr<FileSystemRequestHandler> GetFileSystemRequestHandler() {
     return MakeUnique<FileSystemRequestHandler>();
   }
@@ -93,6 +100,17 @@ TEST_F(TestFileSystemRequestHandler, isGetRootHandleSuccessful) {
                      [this]() { return mListener->IsDone(); });
 }
 
+TEST_F(TestFileSystemRequestHandler, isGetRootHandleBlockedAfterShutdown) {
+  mManager->Shutdown();
+
+  IgnoredErrorResult error;
+  GetFileSystemRequestHandler()->GetRootHandle(mManager, GetSimplePromise(),
+                                               error);
+
+  ASSERT_TRUE(error.Failed());
+  ASSERT_TRUE(error.ErrorCodeIs(NS_ERROR_ILLEGAL_DURING_SHUTDOWN));
+}
+
 TEST_F(TestFileSystemRequestHandler, isGetDirectoryHandleSuccessful) {
   auto fakeResponse = [](const auto& /* aRequest */, auto&& aResolve,
                          auto&& /* aReject */) {
@@ -120,6 +138,17 @@ TEST_F(TestFileSystemRequestHandler, isGetDirectoryHandleSuccessful) {
                      [this]() { return mListener->IsDone(); });
 }
 
+TEST_F(TestFileSystemRequestHandler, isGetDirectoryHandleBlockedAfterShutdown) {
+  mManager->Shutdown();
+
+  IgnoredErrorResult error;
+  GetFileSystemRequestHandler()->GetDirectoryHandle(
+      mManager, mChild, /* aCreate */ true, GetSimplePromise(), error);
+
+  ASSERT_TRUE(error.Failed());
+  ASSERT_TRUE(error.ErrorCodeIs(NS_ERROR_ILLEGAL_DURING_SHUTDOWN));
+}
+
 TEST_F(TestFileSystemRequestHandler, isGetFileHandleSuccessful) {
   auto fakeResponse = [](const auto& /* aRequest */, auto&& aResolve,
                          auto&& /* aReject */) {
@@ -146,6 +175,17 @@ TEST_F(TestFileSystemRequestHandler, isGetFileHandleSuccessful) {
                      [this]() { return mListener->IsDone(); });
 }
 
+TEST_F(TestFileSystemRequestHandler, isGetFileHandleBlockedAfterShutdown) {
+  mManager->Shutdown();
+
+  IgnoredErrorResult error;
+  GetFileSystemRequestHandler()->GetFileHandle(
+      mManager, mChild, /* aCreate */ true, GetSimplePromise(), error);
+
+  ASSERT_TRUE(error.Failed());
+  ASSERT_TRUE(error.ErrorCodeIs(NS_ERROR_ILLEGAL_DURING_SHUTDOWN));
+}
+
 TEST_F(TestFileSystemRequestHandler, isGetFileSuccessful) {
   auto fakeResponse = [](const auto& /* aRequest */, auto&& aResolve,
                          auto&& /* aReject */) {
@@ -194,6 +234,17 @@ TEST_F(TestFileSystemRequestHandler, isGetFileSuccessful) {
                      [this]() { return mListener->IsDone(); });
 }
 
+TEST_F(TestFileSystemRequestHandler, isGetFileBlockedAfterShutdown) {
+  mManager->Shutdown();
+
+  IgnoredErrorResult error;
+  GetFileSystemRequestHandler()->GetFile(mManager, mEntry, GetSimplePromise(),
+                                         error);
+
+  ASSERT_TRUE(error.Failed());
+  ASSERT_TRUE(error.ErrorCodeIs(NS_ERROR_ILLEGAL_DURING_SHUTDOWN));
+}
+
 TEST_F(TestFileSystemRequestHandler, isGetEntriesSuccessful) {
   auto fakeResponse = [](const auto& /* aRequest */, auto&& aResolve,
                          auto&& /* aReject */) {
@@ -230,6 +281,20 @@ TEST_F(TestFileSystemRequestHandler, isGetEntriesSuccessful) {
                      [listener]() { return listener->IsDone(); });
 }
 
+TEST_F(TestFileSystemRequestHandler, isGetEntriesBlockedAfterShutdown) {
+  mManager->Shutdown();
+
+  RefPtr<FileSystemEntryMetadataArray> sink;
+
+  IgnoredErrorResult error;
+  GetFileSystemRequestHandler()->GetEntries(mManager, mEntry.entryId(),
+                                            /* aPage */ 0, GetSimplePromise(),
+                                            sink, error);
+
+  ASSERT_TRUE(error.Failed());
+  ASSERT_TRUE(error.ErrorCodeIs(NS_ERROR_ILLEGAL_DURING_SHUTDOWN));
+}
+
 TEST_F(TestFileSystemRequestHandler, isRemoveEntrySuccessful) {
   auto fakeResponse = [](const auto& /* aRequest */, auto&& aResolve,
                          auto&& /* aReject */) {
@@ -255,4 +320,15 @@ TEST_F(TestFileSystemRequestHandler, isRemoveEntrySuccessful) {
                      [this]() { return mListener->IsDone(); });
 }
 
+TEST_F(TestFileSystemRequestHandler, isRemoveEntryBlockedAfterShutdown) {
+  mManager->Shutdown();
+
+  IgnoredErrorResult error;
+  GetFileSystemRequestHandler()->RemoveEntry(
+      mManager, mChild, /* aRecursive */ true, GetSimplePromise(), error);
+
+  ASSERT_TRUE(error.Failed());
+  ASSERT_TRUE(error.ErrorCodeIs(NS_ERROR_ILLEGAL_DURING_SHUTDOWN));
+}
+
 }  // namespace mozilla::dom::fs::test