Bug 1842007 - Part 2: Replace DomPromiseListener use with Promise method in DoubleBufferQueueImpl r=dom-storage-reviewers,janv,asuth

AddCallbacksWithCycleCollectedArgs prevents potential cycle. (I'm assuming `this` and `newPage` cannot make cycle here)

Also making FileSystemDirectoryIterator::Impl as refcountable to safely capture `this`.

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

dom/fs/api/FileSystemDirectoryHandle.h

@@ -46,7 +46,7 @@ class FileSystemDirectoryHandle final : public FileSystemHandle {
   FileSystemHandleKind Kind() const override;
 
   struct IteratorData {
-    UniquePtr<FileSystemDirectoryIterator::Impl> mImpl;
+    RefPtr<FileSystemDirectoryIterator::Impl> mImpl;
   };
 
   void InitAsyncIteratorData(IteratorData& aData,

dom/fs/api/FileSystemDirectoryIterator.cpp

@@ -23,8 +23,8 @@ NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(FileSystemDirectoryIterator, mGlobal);
 
 FileSystemDirectoryIterator::FileSystemDirectoryIterator(
     nsIGlobalObject* aGlobal, RefPtr<FileSystemManager>& aManager,
-    UniquePtr<Impl> aImpl)
-    : mGlobal(aGlobal), mManager(aManager), mImpl(std::move(aImpl)) {}
+    RefPtr<Impl>& aImpl)
+    : mGlobal(aGlobal), mManager(aManager), mImpl(aImpl) {}
 
 // WebIDL Boilerplate
 

dom/fs/api/FileSystemDirectoryIterator.h

@@ -31,9 +31,13 @@ class FileSystemDirectoryIterator : public nsISupports, public nsWrapperCache {
  public:
   class Impl {
    public:
+    NS_INLINE_DECL_REFCOUNTING(Impl)
+
     virtual already_AddRefed<Promise> Next(nsIGlobalObject* aGlobal,
                                            RefPtr<FileSystemManager>& aManager,
                                            ErrorResult& aError) = 0;
+
+   protected:
     virtual ~Impl() = default;
   };
 
@@ -42,7 +46,7 @@ class FileSystemDirectoryIterator : public nsISupports, public nsWrapperCache {
 
   explicit FileSystemDirectoryIterator(nsIGlobalObject* aGlobal,
                                        RefPtr<FileSystemManager>& aManager,
-                                       UniquePtr<Impl> aImpl);
+                                       RefPtr<Impl>& aImpl);
 
   // WebIDL Boilerplate
   nsIGlobalObject* GetParentObject() const;
@@ -61,7 +65,7 @@ class FileSystemDirectoryIterator : public nsISupports, public nsWrapperCache {
   RefPtr<FileSystemManager> mManager;
 
  private:
-  UniquePtr<Impl> mImpl;
+  RefPtr<Impl> mImpl;
 };
 
 }  // namespace dom

dom/fs/child/FileSystemDirectoryIteratorFactory.cpp

@@ -16,6 +16,7 @@
 #include "mozilla/dom/FileSystemLog.h"
 #include "mozilla/dom/FileSystemManager.h"
 #include "mozilla/dom/IterableIterator.h"
+#include "mozilla/dom/Promise-inl.h"
 #include "mozilla/dom/Promise.h"
 #include "mozilla/dom/PromiseNativeHandler.h"
 
@@ -117,9 +118,9 @@ class DoubleBufferQueueImpl
     return promise.forget();
   }
 
-  ~DoubleBufferQueueImpl() = default;
-
  protected:
+  ~DoubleBufferQueueImpl() override = default;
+
   void next(nsIGlobalObject* aGlobal, RefPtr<FileSystemManager>& aManager,
             RefPtr<Promise> aResult, ErrorResult& aError) {
     LOG_VERBOSE(("next"));
@@ -138,33 +139,38 @@ class DoubleBufferQueueImpl
 
       auto newPage = MakeRefPtr<FileSystemEntryMetadataArray>();
 
-      RefPtr<DomPromiseListener> listener = new DomPromiseListener(
-          [global = nsCOMPtr<nsIGlobalObject>(aGlobal),
-           manager = RefPtr<FileSystemManager>(aManager), newPage, aResult,
-           this](JSContext* aCx, JS::Handle<JS::Value> aValue) mutable {
-            MOZ_ASSERT(0u == mWithinPageIndex);
+      promise->AddCallbacksWithCycleCollectedArgs(
+          [self = RefPtr(this), newPage](
+              JSContext*, JS::Handle<JS::Value>, ErrorResult&,
+              RefPtr<FileSystemManager>& aManager, RefPtr<Promise>& aResult) {
+            MOZ_ASSERT(0u == self->mWithinPageIndex);
             MOZ_ASSERT(newPage->Length() <= PageSize);
 
-            const size_t startPos = mCurrentPageIsLastPage ? 0u : PageSize;
-            if (mData.Length() < 2 * PageSize) {
-              mData.InsertElementsAt(startPos, newPage->Elements(),
-                                     newPage->Length());
+            const size_t startPos =
+                self->mCurrentPageIsLastPage ? 0u : PageSize;
+            if (self->mData.Length() < 2 * PageSize) {
+              self->mData.InsertElementsAt(startPos, newPage->Elements(),
+                                           newPage->Length());
             } else {
-              mData.ReplaceElementsAt(startPos, newPage->Length(),
-                                      newPage->Elements(), newPage->Length());
+              self->mData.ReplaceElementsAt(startPos, newPage->Length(),
+                                            newPage->Elements(),
+                                            newPage->Length());
             }
-            MOZ_ASSERT(mData.Length() <= 2 * PageSize);
-            mWithinPageEnd = newPage->Length();
+            MOZ_ASSERT(self->mData.Length() <= 2 * PageSize);
+            self->mWithinPageEnd = newPage->Length();
 
             Maybe<DataType> value;
             if (0 != newPage->Length()) {
-              nextInternal(value);
+              self->nextInternal(value);
             }
 
-            ResolveValue(global, manager, value, aResult);
+            self->ResolveValue(aResult->GetGlobalObject(), aManager, value,
+                               aResult);
           },
-          [aResult](nsresult aRv) { aResult->MaybeReject(aRv); });
-      promise->AppendNativeHandler(listener);
+          [](JSContext*, JS::Handle<JS::Value> aValue, ErrorResult&,
+             RefPtr<FileSystemManager>&,
+             RefPtr<Promise>& aResult) { aResult->MaybeReject(aValue); },
+          aManager, aResult);
 
       FileSystemRequestHandler{}.GetEntries(aManager, mEntryId, mPageNumber,
                                             promise, newPage, aError);
@@ -218,20 +224,21 @@ using UnderlyingQueue = DoubleBufferQueueImpl<ValueResolver<Type>>;
 
 }  // namespace
 
-UniquePtr<mozilla::dom::FileSystemDirectoryIterator::Impl>
+already_AddRefed<mozilla::dom::FileSystemDirectoryIterator::Impl>
 FileSystemDirectoryIteratorFactory::Create(
     const FileSystemEntryMetadata& aMetadata,
     IterableIteratorBase::IteratorType aType) {
   if (IterableIteratorBase::Entries == aType) {
-    return MakeUnique<UnderlyingQueue<IterableIteratorBase::Entries>>(
+    return MakeAndAddRef<UnderlyingQueue<IterableIteratorBase::Entries>>(
         aMetadata);
   }
 
   if (IterableIteratorBase::Values == aType) {
-    return MakeUnique<UnderlyingQueue<IterableIteratorBase::Values>>(aMetadata);
+    return MakeAndAddRef<UnderlyingQueue<IterableIteratorBase::Values>>(
+        aMetadata);
   }
 
-  return MakeUnique<UnderlyingQueue<IterableIteratorBase::Keys>>(aMetadata);
+  return MakeAndAddRef<UnderlyingQueue<IterableIteratorBase::Keys>>(aMetadata);
 }
 
 }  // namespace mozilla::dom::fs

dom/fs/child/FileSystemDirectoryIteratorFactory.h

@@ -14,9 +14,9 @@ namespace mozilla::dom::fs {
 class FileSystemEntryMetadata;
 
 struct FileSystemDirectoryIteratorFactory {
-  static UniquePtr<mozilla::dom::FileSystemDirectoryIterator::Impl> Create(
-      const FileSystemEntryMetadata& aMetadata,
-      IterableIteratorBase::IteratorType aType);
+  static already_AddRefed<mozilla::dom::FileSystemDirectoryIterator::Impl>
+  Create(const FileSystemEntryMetadata& aMetadata,
+         IterableIteratorBase::IteratorType aType);
 };
 
 }  // namespace mozilla::dom::fs

dom/fs/test/gtest/api/TestFileSystemDirectoryHandle.cpp

@@ -89,7 +89,7 @@ TEST_F(TestFileSystemDirectoryHandle, isNextPromiseReturned) {
 
   ASSERT_TRUE(dirHandle);
 
-  auto mockIter = MakeUnique<MockFileSystemDirectoryIteratorImpl>();
+  auto mockIter = MakeRefPtr<MockFileSystemDirectoryIteratorImpl>();
   IgnoredErrorResult error;
   EXPECT_CALL(*mockIter, Next(_, _, _))
       .WillOnce(::testing::Return(Promise::Create(mGlobal, error)));