Bug 1948102: Refactor GetFilesHelper MozPromise behavior r=baku

Adds PromiseAdapter to abstract dom::Promise and MozPromise as one interface.
Also adds a test for the MozPromise behavior.

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

dom/filesystem/GetFilesHelper.cpp

@@ -24,43 +24,84 @@ namespace mozilla::dom {
 class GetFilesHelper::ReleaseRunnable final : public Runnable {
  public:
   static void MaybeReleaseOnMainThread(
-      nsTArray<RefPtr<Promise>>&& aPromises,
+      nsTArray<PromiseAdapter>&& aPromises,
-      nsTArray<GetFilesHelper::MozPromiseAndGlobal>&& aMozPromises,
       nsTArray<RefPtr<GetFilesCallback>>&& aCallbacks) {
     if (NS_IsMainThread()) {
       return;
     }
 
-    RefPtr<ReleaseRunnable> runnable = new ReleaseRunnable(
+    RefPtr<ReleaseRunnable> runnable =
-        std::move(aPromises), std::move(aMozPromises), std::move(aCallbacks));
+        new ReleaseRunnable(std::move(aPromises), std::move(aCallbacks));
     FileSystemUtils::DispatchRunnable(nullptr, runnable.forget());
   }
 
   NS_IMETHOD
   Run() override {
     MOZ_ASSERT(NS_IsMainThread());
-
     mPromises.Clear();
-    mMozPromises.Clear();
     mCallbacks.Clear();
-
     return NS_OK;
   }
 
  private:
-  ReleaseRunnable(nsTArray<RefPtr<Promise>>&& aPromises,
+  ReleaseRunnable(nsTArray<PromiseAdapter>&& aPromises,
-                  nsTArray<GetFilesHelper::MozPromiseAndGlobal>&& aMozPromises,
                   nsTArray<RefPtr<GetFilesCallback>>&& aCallbacks)
       : Runnable("dom::ReleaseRunnable"),
         mPromises(std::move(aPromises)),
-        mMozPromises{std::move(aMozPromises)},
         mCallbacks(std::move(aCallbacks)) {}
 
-  nsTArray<RefPtr<Promise>> mPromises;
+  nsTArray<PromiseAdapter> mPromises;
-  nsTArray<GetFilesHelper::MozPromiseAndGlobal> mMozPromises;
   nsTArray<RefPtr<GetFilesCallback>> mCallbacks;
 };
 
+///////////////////////////////////////////////////////////////////////////////
+// PromiseAdapter
+
+GetFilesHelper::PromiseAdapter::PromiseAdapter(
+    MozPromiseAndGlobal&& aMozPromise)
+    : mPromise(std::move(aMozPromise)) {}
+GetFilesHelper::PromiseAdapter::PromiseAdapter(Promise* aDomPromise)
+    : mPromise(RefPtr{aDomPromise}) {}
+GetFilesHelper::PromiseAdapter::~PromiseAdapter() { Clear(); }
+
+void GetFilesHelper::PromiseAdapter::Clear() {
+  mPromise = AsVariant(RefPtr<Promise>(nullptr));
+}
+
+nsIGlobalObject* GetFilesHelper::PromiseAdapter::GetGlobalObject() {
+  return mPromise.match(
+      [](RefPtr<Promise>& aDomPromise) {
+        return aDomPromise ? aDomPromise->GetGlobalObject() : nullptr;
+      },
+      [](MozPromiseAndGlobal& aMozPromiseAndGlobal) {
+        return aMozPromiseAndGlobal.mGlobal.get();
+      });
+}
+
+void GetFilesHelper::PromiseAdapter::Resolve(nsTArray<RefPtr<File>>&& aFiles) {
+  mPromise.match(
+      [&aFiles](RefPtr<Promise>& aDomPromise) {
+        if (aDomPromise) {
+          aDomPromise->MaybeResolve(Sequence(std::move(aFiles)));
+        }
+      },
+      [&aFiles](MozPromiseAndGlobal& aMozPromiseAndGlobal) {
+        aMozPromiseAndGlobal.mMozPromise->Resolve(std::move(aFiles), __func__);
+      });
+}
+
+void GetFilesHelper::PromiseAdapter::Reject(nsresult aError) {
+  mPromise.match(
+      [&aError](RefPtr<Promise>& aDomPromise) {
+        if (aDomPromise) {
+          aDomPromise->MaybeReject(aError);
+        }
+      },
+      [&aError](MozPromiseAndGlobal& aMozPromiseAndGlobal) {
+        aMozPromiseAndGlobal.mMozPromise->Reject(aError, __func__);
+      });
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // GetFilesHelper Base class
 
@@ -127,36 +168,31 @@ GetFilesHelper::GetFilesHelper(bool aRecursiveFlag)
       mCanceled(false) {}
 
 GetFilesHelper::~GetFilesHelper() {
-  ReleaseRunnable::MaybeReleaseOnMainThread(
+  ReleaseRunnable::MaybeReleaseOnMainThread(std::move(mPromises),
-      std::move(mPromises), std::move(mMozPromises), std::move(mCallbacks));
+                                            std::move(mCallbacks));
 }
 
-void GetFilesHelper::AddPromise(Promise* aPromise) {
+void GetFilesHelper::AddPromiseInternal(PromiseAdapter&& aPromise) {
-  MOZ_ASSERT(aPromise);
-
   // Still working.
   if (!mListingCompleted) {
-    mPromises.AppendElement(aPromise);
+    mPromises.AppendElement(std::move(aPromise));
     return;
   }
 
   MOZ_ASSERT(mPromises.IsEmpty());
-  ResolveOrRejectPromise(aPromise);
+  ResolveOrRejectPromise(std::move(aPromise));
+}
+
+void GetFilesHelper::AddPromise(Promise* aPromise) {
+  MOZ_ASSERT(aPromise);
+  AddPromiseInternal(PromiseAdapter(aPromise));
 }
 
 void GetFilesHelper::AddMozPromise(MozPromiseType* aPromise,
                                    nsIGlobalObject* aGlobal) {
   MOZ_ASSERT(aPromise);
   MOZ_ASSERT(aGlobal);
-
+  AddPromiseInternal(PromiseAdapter(MozPromiseAndGlobal{aPromise, aGlobal}));
-  // Still working.
-  if (!mListingCompleted) {
-    mMozPromises.AppendElement(MozPromiseAndGlobal{aPromise, aGlobal});
-    return;
-  }
-
-  MOZ_ASSERT(mMozPromises.IsEmpty());
-  ResolveOrRejectMozPromise({aPromise, aGlobal});
 }
 
 void GetFilesHelper::AddCallback(GetFilesCallback* aCallback) {
@@ -174,7 +210,6 @@ void GetFilesHelper::AddCallback(GetFilesCallback* aCallback) {
 
 void GetFilesHelper::Unlink() {
   mPromises.Clear();
-  mMozPromises.Clear();
   mCallbacks.Clear();
 
   {
@@ -185,9 +220,26 @@ void GetFilesHelper::Unlink() {
   Cancel();
 }
 
-void GetFilesHelper::Traverse(nsCycleCollectionTraversalCallback& cb) {
+void GetFilesHelper::Traverse(nsCycleCollectionTraversalCallback& aCb) {
-  GetFilesHelper* tmp = this;
+  for (auto&& promiseAdapter : mPromises) {
-  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPromises);
+    promiseAdapter.Traverse(aCb);
+  }
+}
+
+void GetFilesHelper::PromiseAdapter::Traverse(
+    nsCycleCollectionTraversalCallback& aCb) {
+  mPromise.match(
+      [&aCb](RefPtr<Promise>& aDomPromise) {
+        if (aDomPromise) {
+          NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(aCb, "mDomPromise");
+          aCb.NoteNativeChild(aDomPromise,
+                              NS_CYCLE_COLLECTION_PARTICIPANT(Promise));
+        }
+      },
+      [&aCb](MozPromiseAndGlobal& aMozPromiseAndGlobal) {
+        NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(aCb, "mGlobal");
+        aCb.NoteXPCOMChild(aMozPromiseAndGlobal.mGlobal);
+      });
 }
 
 void GetFilesHelper::Work(ErrorResult& aRv) {
@@ -235,12 +287,7 @@ void GetFilesHelper::OperationCompleted() {
   // Let's process the pending promises.
   auto promises = std::move(mPromises);
   for (uint32_t i = 0; i < promises.Length(); ++i) {
-    ResolveOrRejectPromise(promises[i]);
+    ResolveOrRejectPromise(std::move(promises[i]));
-  }
-
-  auto mozPromises = std::move(mMozPromises);
-  for (uint32_t i = 0; i < mozPromises.Length(); ++i) {
-    ResolveOrRejectMozPromise(mozPromises[i]);
   }
 
   // Let's process the pending callbacks.
@@ -359,53 +406,16 @@ nsresult GetFilesHelperBase::ExploreDirectory(const nsAString& aDOMPath,
   return NS_OK;
 }
 
-void GetFilesHelper::ResolveOrRejectPromise(Promise* aPromise) {
+void GetFilesHelper::ResolveOrRejectPromise(PromiseAdapter&& aPromise) {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(mListingCompleted);
-  MOZ_ASSERT(aPromise);
-
-  Sequence<RefPtr<File>> files;
-
-  if (NS_SUCCEEDED(mErrorResult)) {
-    for (uint32_t i = 0; i < mTargetBlobImplArray.Length(); ++i) {
-      RefPtr<File> domFile =
-          File::Create(aPromise->GetParentObject(), mTargetBlobImplArray[i]);
-      if (NS_WARN_IF(!domFile)) {
-        mErrorResult = NS_ERROR_FAILURE;
-        files.Clear();
-        break;
-      }
-
-      if (!files.AppendElement(domFile, fallible)) {
-        mErrorResult = NS_ERROR_OUT_OF_MEMORY;
-        files.Clear();
-        break;
-      }
-    }
-  }
-
-  // Error propagation.
-  if (NS_FAILED(mErrorResult)) {
-    aPromise->MaybeReject(mErrorResult);
-    return;
-  }
-
-  aPromise->MaybeResolve(files);
-}
-
-void GetFilesHelper::ResolveOrRejectMozPromise(
-    GetFilesHelper::MozPromiseAndGlobal aPromise) {
-  MOZ_ASSERT(NS_IsMainThread());
-  MOZ_ASSERT(mListingCompleted);
-  MOZ_ASSERT(aPromise.mMozPromise);
-  MOZ_ASSERT(aPromise.mGlobal);
 
   nsTArray<RefPtr<File>> files;
 
   if (NS_SUCCEEDED(mErrorResult)) {
     for (uint32_t i = 0; i < mTargetBlobImplArray.Length(); ++i) {
       RefPtr<File> domFile =
-          File::Create(aPromise.mGlobal, mTargetBlobImplArray[i]);
+          File::Create(aPromise.GetGlobalObject(), mTargetBlobImplArray[i]);
       if (NS_WARN_IF(!domFile)) {
         mErrorResult = NS_ERROR_FAILURE;
         files.Clear();
@@ -422,11 +432,11 @@ void GetFilesHelper::ResolveOrRejectMozPromise(
 
   // Error propagation.
   if (NS_FAILED(mErrorResult)) {
-    aPromise.mMozPromise->Reject(mErrorResult, __func__);
+    aPromise.Reject(mErrorResult);
     return;
   }
 
-  aPromise.mMozPromise->Resolve(std::move(files), __func__);
+  aPromise.Resolve(std::move(files));
 }
 
 void GetFilesHelper::RunCallback(GetFilesCallback* aCallback) {

dom/filesystem/GetFilesHelper.h

@@ -106,14 +106,31 @@ class GetFilesHelper : public Runnable, public GetFilesHelperBase {
 
   void OperationCompleted();
 
-  void ResolveOrRejectPromise(Promise* aPromise);
-
   struct MozPromiseAndGlobal {
     RefPtr<MozPromiseType> mMozPromise;
     RefPtr<nsIGlobalObject> mGlobal;
   };
 
-  void ResolveOrRejectMozPromise(MozPromiseAndGlobal aPromise);
+  class PromiseAdapter {
+   public:
+    explicit PromiseAdapter(MozPromiseAndGlobal&& aMozPromise);
+    explicit PromiseAdapter(Promise* aDomPromise);
+    ~PromiseAdapter();
+
+    void Clear();
+    void Traverse(nsCycleCollectionTraversalCallback& cb);
+
+    nsIGlobalObject* GetGlobalObject();
+    void Resolve(nsTArray<RefPtr<File>>&& aFiles);
+    void Reject(nsresult aError);
+
+   private:
+    using PromiseVariant = Variant<RefPtr<Promise>, MozPromiseAndGlobal>;
+    PromiseVariant mPromise;
+  };
+
+  void AddPromiseInternal(PromiseAdapter&& aPromise);
+  void ResolveOrRejectPromise(PromiseAdapter&& aPromise);
 
   void RunCallback(GetFilesCallback* aCallback);
 
@@ -123,8 +140,8 @@ class GetFilesHelper : public Runnable, public GetFilesHelperBase {
   // Error code to propagate.
   nsresult mErrorResult;
 
-  nsTArray<RefPtr<Promise>> mPromises;
+  nsTArray<PromiseAdapter> mPromises;
-  nsTArray<MozPromiseAndGlobal> mMozPromises;
+
   nsTArray<RefPtr<GetFilesCallback>> mCallbacks;
 
   Mutex mMutex MOZ_UNANNOTATED;

dom/filesystem/tests/gtest/TestGetFilesHelper.cpp

@@ -63,15 +63,8 @@ struct BoolStruct {
 class FilesCallback : public GetFilesCallback {
  public:
   FilesCallback(std::atomic<bool>& aGotResponse,
-                const nsTArray<nsTArray<const char*>>& aExpectedPathSegments)
+                const nsTArray<nsString>& aExpectedPaths)
-      : mGotResponse(aGotResponse) {
+      : mGotResponse(aGotResponse), mExpectedPaths(aExpectedPaths.Clone()) {}
-    for (const auto& pathSegments : aExpectedPathSegments) {
-      auto file = MakeFileFromPathSegments(pathSegments);
-      nsString expectedPath;
-      MOZ_ALWAYS_SUCCEEDS(file->GetPath(expectedPath));
-      mExpectedPaths.AppendElement(expectedPath);
-    }
-  }
 
   // -------------------
   // GetFilesCallback
@@ -94,13 +87,46 @@ class FilesCallback : public GetFilesCallback {
   nsTArray<nsString> mExpectedPaths;
 };
 
+nsTArray<nsString> GetExpectedPaths(
+    const nsTArray<nsTArray<const char*>>& aPathSegmentsArray) {
+  nsTArray<nsString> expectedPaths(aPathSegmentsArray.Length());
+  for (const auto& pathSegments : aPathSegmentsArray) {
+    auto file = MakeFileFromPathSegments(pathSegments);
+    nsString expectedPath;
+    MOZ_ALWAYS_SUCCEEDS(file->GetPath(expectedPath));
+    expectedPaths.AppendElement(expectedPath);
+  }
+  return expectedPaths;
+}
+
 void ExpectGetFilesHelperResponse(
     RefPtr<GetFilesHelper> aHelper,
     const nsTArray<nsTArray<const char*>>& aPathSegmentsArray) {
-  std::atomic<bool> gotResponse = false;
+  nsTArray<nsString> expectedPaths = GetExpectedPaths(aPathSegmentsArray);
+
+  std::atomic<bool> gotCallbackResponse = false;
+  std::atomic<bool> gotMozPromiseResponse = false;
   RefPtr<FilesCallback> callback =
-      MakeRefPtr<FilesCallback>(gotResponse, aPathSegmentsArray);
+      MakeRefPtr<FilesCallback>(gotCallbackResponse, expectedPaths);
   aHelper->AddCallback(callback);
+  auto mozPromise = MakeRefPtr<GetFilesHelper::MozPromiseType>(__func__);
+  aHelper->AddMozPromise(mozPromise,
+                         xpc::NativeGlobal(xpc::PrivilegedJunkScope()));
+  mozPromise->Then(
+      GetMainThreadSerialEventTarget(), __func__,
+      [&gotMozPromiseResponse,
+       &expectedPaths](const nsTArray<RefPtr<mozilla::dom::File>>& aFiles) {
+        EXPECT_EQ(aFiles.Length(), expectedPaths.Length());
+        for (const auto& file : aFiles) {
+          nsString path;
+          ErrorResult error;
+          file->GetMozFullPathInternal(path, error);
+          ASSERT_EQ(error.StealNSResult(), NS_OK);
+          ASSERT_TRUE(expectedPaths.Contains(path));
+        }
+        gotMozPromiseResponse = true;
+      },
+      []() { FAIL() << "MozPromise got rejected!"; });
   // Make timedOut a RefPtr so if we get a response after this function
   // has finished we can safely check that (and don't start accessing stack
   // values that don't exist anymore)
@@ -108,17 +134,20 @@ void ExpectGetFilesHelperResponse(
 
   RefPtr<CancelableRunnable> timer =
       NS_NewCancelableRunnableFunction("GetFilesHelper timeout", [&] {
-        if (!gotResponse.load()) {
+        if (!gotCallbackResponse.load() || !gotMozPromiseResponse.load()) {
           timedOut->mValue = true;
         }
       });
   constexpr uint32_t kTimeout = 10000;
   NS_DelayedDispatchToCurrentThread(do_AddRef(timer), kTimeout);
   mozilla::SpinEventLoopUntil(
-      "Waiting for GetFilesHelper result"_ns,
+      "Waiting for GetFilesHelper result"_ns, [&, timedOut]() {
-      [&, timedOut]() { return gotResponse.load() || timedOut->mValue; });
+        return (gotCallbackResponse.load() && gotMozPromiseResponse.load()) ||
+               timedOut->mValue;
+      });
   timer->Cancel();
-  EXPECT_TRUE(gotResponse);
+  EXPECT_TRUE(gotCallbackResponse);
+  EXPECT_TRUE(gotMozPromiseResponse);
   EXPECT_FALSE(timedOut->mValue);
 }