Bug 1751681 - Cleanup FileSystemHandle serialization/deserialization; r=dom-storage-reviewers,jesup

This patch also cleanups the StorageManager public interface a bit.

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

dom/fs/api/FileSystemHandle.cpp

@@ -18,7 +18,6 @@
 #include "mozilla/dom/Promise.h"
 #include "mozilla/dom/StorageManager.h"
 #include "mozilla/dom/StructuredCloneHolder.h"
-#include "mozilla/dom/StructuredCloneTags.h"
 #include "mozilla/dom/quota/QuotaCommon.h"
 #include "mozilla/ipc/PBackgroundSharedTypes.h"
 #include "nsJSPrincipals.h"
@@ -67,7 +66,6 @@ bool ConstructHandleMetadata(JSContext* aCx, nsIGlobalObject* aGlobal,
 
   LOG_VERBOSE(("Deserializing %s", NS_ConvertUTF16toUTF8(name).get()));
 
-  LOG_VERBOSE(("Metadata: name=%s", NS_ConvertUTF16toUTF8(name).get()));
   aMetadata = fs::FileSystemEntryMetadata(entryId, name, aDirectory);
   return true;
 }
@@ -211,9 +209,10 @@ already_AddRefed<Promise> FileSystemHandle::Move(const fs::EntryId& aParentId,
 already_AddRefed<FileSystemHandle> FileSystemHandle::ReadStructuredClone(
     JSContext* aCx, nsIGlobalObject* aGlobal,
     JSStructuredCloneReader* aReader) {
-  uint32_t kind = static_cast<uint32_t>(FileSystemHandleKind::EndGuard_);
   LOG_VERBOSE(("Reading File/DirectoryHandle"));
 
+  uint32_t kind = static_cast<uint32_t>(FileSystemHandleKind::EndGuard_);
+
   if (!JS_ReadBytes(aReader, reinterpret_cast<void*>(&kind),
                     sizeof(uint32_t))) {
     return nullptr;
@@ -237,6 +236,7 @@ already_AddRefed<FileSystemHandle> FileSystemHandle::ReadStructuredClone(
 bool FileSystemHandle::WriteStructuredClone(
     JSContext* aCx, JSStructuredCloneWriter* aWriter) const {
   LOG_VERBOSE(("Writing File/DirectoryHandle"));
+  MOZ_ASSERT(mMetadata.entryId().Length() == 32);
 
   auto kind = static_cast<uint32_t>(Kind());
   if (NS_WARN_IF(!JS_WriteBytes(aWriter, static_cast<void*>(&kind),
@@ -244,7 +244,6 @@ bool FileSystemHandle::WriteStructuredClone(
     return false;
   }
 
-  MOZ_ASSERT(mMetadata.entryId().Length() == 32);
   if (NS_WARN_IF(!JS_WriteBytes(
           aWriter, static_cast<const void*>(mMetadata.entryId().get()),
           mMetadata.entryId().Length()))) {
@@ -266,17 +265,22 @@ bool FileSystemHandle::WriteStructuredClone(
 already_AddRefed<FileSystemFileHandle> FileSystemHandle::ConstructFileHandle(
     JSContext* aCx, nsIGlobalObject* aGlobal,
     JSStructuredCloneReader* aReader) {
-  using namespace mozilla::dom::fs;
+  LOG(("Reading FileHandle"));
 
-  FileSystemEntryMetadata metadata;
+  fs::FileSystemEntryMetadata metadata;
   if (!ConstructHandleMetadata(aCx, aGlobal, aReader, /* aDirectory */ false,
                                metadata)) {
     return nullptr;
   }
 
+  RefPtr<StorageManager> storageManager = aGlobal->GetStorageManager();
+  if (!storageManager) {
+    return nullptr;
+  }
+
   // Note that the actor may not exist or may not be connected yet.
   RefPtr<FileSystemManager> fileSystemManager =
-      FileSystemManager::GetManagerForGlobal(aGlobal);
+      storageManager->GetFileSystemManager();
 
   RefPtr<FileSystemFileHandle> fsHandle =
       new FileSystemFileHandle(aGlobal, fileSystemManager, metadata);
@@ -290,17 +294,21 @@ FileSystemHandle::ConstructDirectoryHandle(JSContext* aCx,
                                            nsIGlobalObject* aGlobal,
                                            JSStructuredCloneReader* aReader) {
   LOG(("Reading DirectoryHandle"));
-  using namespace mozilla::dom::fs;
 
-  FileSystemEntryMetadata metadata;
+  fs::FileSystemEntryMetadata metadata;
   if (!ConstructHandleMetadata(aCx, aGlobal, aReader, /* aDirectory */ true,
                                metadata)) {
     return nullptr;
   }
 
+  RefPtr<StorageManager> storageManager = aGlobal->GetStorageManager();
+  if (!storageManager) {
+    return nullptr;
+  }
+
   // Note that the actor may not exist or may not be connected yet.
   RefPtr<FileSystemManager> fileSystemManager =
-      FileSystemManager::GetManagerForGlobal(aGlobal);
+      storageManager->GetFileSystemManager();
 
   RefPtr<FileSystemDirectoryHandle> fsHandle =
       new FileSystemDirectoryHandle(aGlobal, fileSystemManager, metadata);

dom/fs/api/FileSystemManager.cpp

@@ -10,11 +10,8 @@
 #include "fs/FileSystemRequestHandler.h"
 #include "mozilla/ErrorResult.h"
 #include "mozilla/dom/FileSystemManagerChild.h"
-#include "mozilla/dom/Navigator.h"
 #include "mozilla/dom/Promise.h"
 #include "mozilla/dom/StorageManager.h"
-#include "mozilla/dom/WorkerNavigator.h"
-#include "mozilla/dom/WorkerScope.h"
 #include "mozilla/dom/quota/QuotaCommon.h"
 #include "mozilla/dom/quota/ResultExtensions.h"
 
@@ -32,6 +29,7 @@ FileSystemManager::FileSystemManager(nsIGlobalObject* aGlobal,
                                      RefPtr<StorageManager> aStorageManager)
     : FileSystemManager(aGlobal, std::move(aStorageManager),
                         MakeRefPtr<FileSystemBackgroundRequestHandler>()) {}
+
 FileSystemManager::~FileSystemManager() { MOZ_ASSERT(mShutdown); }
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(FileSystemManager)
@@ -92,21 +90,6 @@ void FileSystemManager::BeginRequest(
       ->Track(mCreateFileSystemManagerChildPromiseRequestHolder);
 }
 
-/* static */
-FileSystemManager* FileSystemManager::GetManagerForGlobal(
-    nsIGlobalObject* aGlobal) {
-  nsCOMPtr<nsPIDOMWindowInner> inner = aGlobal->AsInnerWindow();
-  if (inner) {
-    return inner->Navigator()->Storage()->GetFileSystemManager();
-  }
-  WorkerPrivate* worker = GetCurrentThreadWorkerPrivate();
-  if (!worker) {
-    return nullptr;
-  }
-  RefPtr<WorkerNavigator> navigator = worker->GlobalScope()->Navigator();
-  return navigator->Storage()->GetFileSystemManager();
-}
-
 already_AddRefed<Promise> FileSystemManager::GetDirectory(ErrorResult& aError) {
   MOZ_ASSERT(mGlobal);
 

dom/fs/api/FileSystemManager.h

@@ -67,8 +67,6 @@ class FileSystemManager : public nsISupports {
 
   already_AddRefed<Promise> GetDirectory(ErrorResult& aError);
 
-  static FileSystemManager* GetManagerForGlobal(nsIGlobalObject* aGlobal);
-
  private:
   virtual ~FileSystemManager();
 

dom/fs/child/FileSystemRequestHandler.cpp

@@ -55,10 +55,6 @@ void HandleFailedStatus(nsresult aError, const RefPtr<Promise>& aPromise) {
     case NS_ERROR_DOM_INVALID_MODIFICATION_ERR:
       aPromise->MaybeRejectWithInvalidModificationError("Invalid modification");
       break;
-    case NS_ERROR_DOM_SECURITY_ERR:
-      aPromise->MaybeRejectWithSecurityError(
-          "Security error when calling GetDirectory");
-      break;
     default:
       if (NS_FAILED(aError)) {
         aPromise->MaybeRejectWithUnknownError("Unknown failure");
@@ -346,7 +342,7 @@ void FileSystemRequestHandler::GetRootHandle(
     ErrorResult& aError) {
   MOZ_ASSERT(aManager);
   MOZ_ASSERT(aPromise);
-  LOG(("GetRoot"));
+  LOG(("GetRootHandle"));
 
   if (aManager->IsShutdown()) {
     aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
@@ -371,7 +367,7 @@ void FileSystemRequestHandler::GetDirectoryHandle(
   MOZ_ASSERT(aManager);
   MOZ_ASSERT(!aDirectory.parentId().IsEmpty());
   MOZ_ASSERT(aPromise);
-  LOG(("getDirectoryHandle"));
+  LOG(("GetDirectoryHandle"));
 
   if (aManager->IsShutdown()) {
     aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
@@ -403,7 +399,7 @@ void FileSystemRequestHandler::GetFileHandle(
   MOZ_ASSERT(aManager);
   MOZ_ASSERT(!aFile.parentId().IsEmpty());
   MOZ_ASSERT(aPromise);
-  LOG(("getFileHandle"));
+  LOG(("GetFileHandle"));
 
   if (aManager->IsShutdown()) {
     aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
@@ -547,7 +543,7 @@ void FileSystemRequestHandler::RemoveEntry(
   MOZ_ASSERT(aManager);
   MOZ_ASSERT(!aEntry.parentId().IsEmpty());
   MOZ_ASSERT(aPromise);
-  LOG(("removeEntry"));
+  LOG(("RemoveEntry"));
 
   if (aManager->IsShutdown()) {
     aError.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);

dom/quota/StorageManager.cpp

@@ -732,6 +732,43 @@ StorageManager::StorageManager(nsIGlobalObject* aGlobal) : mOwner(aGlobal) {
 
 StorageManager::~StorageManager() = default;
 
+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(StorageManager)
+  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
+  NS_INTERFACE_MAP_ENTRY(nsISupports)
+NS_INTERFACE_MAP_END
+
+NS_IMPL_CYCLE_COLLECTING_ADDREF(StorageManager)
+NS_IMPL_CYCLE_COLLECTING_RELEASE(StorageManager)
+
+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(StorageManager, mOwner,
+                                      mFileSystemManager)
+
+void StorageManager::Shutdown() {
+  if (mFileSystemManager) {
+    mFileSystemManager->Shutdown();
+    mFileSystemManager = nullptr;
+  }
+}
+
+already_AddRefed<FileSystemManager> StorageManager::GetFileSystemManager() {
+  if (!mFileSystemManager) {
+    MOZ_ASSERT(mOwner);
+
+    mFileSystemManager = MakeRefPtr<FileSystemManager>(mOwner, this);
+  }
+
+  return do_AddRef(mFileSystemManager);
+}
+
+// WebIDL Boilerplate
+
+JSObject* StorageManager::WrapObject(JSContext* aCx,
+                                     JS::Handle<JSObject*> aGivenProto) {
+  return StorageManager_Binding::Wrap(aCx, this, aGivenProto);
+}
+
+// WebIDL Interface
+
 already_AddRefed<Promise> StorageManager::Persisted(ErrorResult& aRv) {
   MOZ_ASSERT(mOwner);
 
@@ -757,47 +794,7 @@ already_AddRefed<Promise> StorageManager::Estimate(ErrorResult& aRv) {
 }
 
 already_AddRefed<Promise> StorageManager::GetDirectory(ErrorResult& aRv) {
-  if (!mFileSystemManager) {
+  return RefPtr(GetFileSystemManager())->GetDirectory(aRv);
-    MOZ_ASSERT(mOwner);
-
-    mFileSystemManager = MakeRefPtr<FileSystemManager>(mOwner, this);
-  }
-
-  return mFileSystemManager->GetDirectory(aRv);
-}
-
-FileSystemManager* StorageManager::GetFileSystemManager() {
-  if (!mFileSystemManager) {
-    MOZ_ASSERT(mOwner);
-
-    mFileSystemManager = MakeRefPtr<FileSystemManager>(mOwner, this);
-    ErrorResult result;
-  }
-
-  return mFileSystemManager;
-}
-
-void StorageManager::Shutdown() {
-  if (mFileSystemManager) {
-    mFileSystemManager->Shutdown();
-    mFileSystemManager = nullptr;
-  }
-}
-
-NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(StorageManager, mOwner,
-                                      mFileSystemManager)
-
-NS_IMPL_CYCLE_COLLECTING_ADDREF(StorageManager)
-NS_IMPL_CYCLE_COLLECTING_RELEASE(StorageManager)
-
-NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(StorageManager)
-  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
-  NS_INTERFACE_MAP_ENTRY(nsISupports)
-NS_INTERFACE_MAP_END
-
-JSObject* StorageManager::WrapObject(JSContext* aCx,
-                                     JS::Handle<JSObject*> aGivenProto) {
-  return StorageManager_Binding::Wrap(aCx, this, aGivenProto);
 }
 
 }  // namespace mozilla::dom

dom/quota/StorageManager.h

@@ -34,9 +34,20 @@ class StorageManager final : public nsISupports, public nsWrapperCache {
  public:
   explicit StorageManager(nsIGlobalObject* aGlobal);
 
+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
+  NS_DECL_CYCLE_COLLECTION_WRAPPERCACHE_CLASS(StorageManager)
+
+  void Shutdown();
+
+  already_AddRefed<FileSystemManager> GetFileSystemManager();
+
+  // WebIDL Boilerplate
   nsIGlobalObject* GetParentObject() const { return mOwner; }
 
-  // WebIDL
+  JSObject* WrapObject(JSContext* aCx,
+                       JS::Handle<JSObject*> aGivenProto) override;
+
+  // WebIDL Interface
   already_AddRefed<Promise> Persisted(ErrorResult& aRv);
 
   already_AddRefed<Promise> Persist(ErrorResult& aRv);
@@ -45,17 +56,6 @@ class StorageManager final : public nsISupports, public nsWrapperCache {
 
   already_AddRefed<Promise> GetDirectory(ErrorResult& aRv);
 
-  FileSystemManager* GetFileSystemManager();
-
-  void Shutdown();
-
-  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
-  NS_DECL_CYCLE_COLLECTION_WRAPPERCACHE_CLASS(StorageManager)
-
-  // nsWrapperCache
-  virtual JSObject* WrapObject(JSContext* aCx,
-                               JS::Handle<JSObject*> aGivenProto) override;
-
  private:
   ~StorageManager();