chore: DirectoryLister memory management improvement (#18634)

* chore: small memory management improvement

Co-authored-by: Charles Kerr <ckerr@github.com>

* chore: fix code style

* use start-from-one ref count, check ref_counted.h for motivations
* reuse list_base_dir_
* net::DirectorLister offloads directory enumeration to a different
  task sequence in its implementation, use of sequence runner on
  our end is unnecessary
* Don't manually `Release` in `WebContentsDestroyed`, content::FileSelectListener
  already handles this case.

shell/browser/web_dialog_helper.cc

@@ -21,6 +21,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "native_mate/dictionary.h"
+#include "net/base/directory_lister.h"
 #include "net/base/mime_util.h"
 #include "shell/browser/atom_browser_context.h"
 #include "shell/browser/native_window.h"
@@ -36,8 +37,10 @@ namespace {
 
 class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
                          public content::WebContentsObserver,
-                         public electron::DirectoryListerHelperDelegate {
+                         public net::DirectoryLister::DirectoryListerDelegate {
  public:
+  REQUIRE_ADOPTION_FOR_REFCOUNTED_TYPE();
+
   FileSelectHelper(content::RenderFrameHost* render_frame_host,
                    std::unique_ptr<content::FileSelectListener> listener,
                    blink::mojom::FileChooserParams::Mode mode)
@@ -73,28 +76,46 @@ class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
             base::BindOnce(&FileSelectHelper::OnSaveDialogDone, this)))));
   }
 
-  void OnDirectoryListerDone(std::vector<FileChooserFileInfoPtr> file_info,
-                             base::FilePath base_dir) override {
-    OnFilesSelected(std::move(file_info), base_dir);
-    Release();
-  }
-
  private:
   friend class base::RefCounted<FileSelectHelper>;
 
   ~FileSelectHelper() override {}
 
-  void EnumerateDirectory(base::FilePath base_dir) {
+  // net::DirectoryLister::DirectoryListerDelegate
-    auto* lister = new net::DirectoryLister(
+  void OnListFile(
-        base_dir, net::DirectoryLister::NO_SORT_RECURSIVE,
+      const net::DirectoryLister::DirectoryListerData& data) override {
-        new electron::DirectoryListerHelper(base_dir, this));
+    // We don't want to return directory paths, only file paths
-    lister->Start();
+    if (data.info.IsDirectory())
+      return;
+
+    lister_paths_.push_back(data.path);
+  }
+
+  // net::DirectoryLister::DirectoryListerDelegate
+  void OnListDone(int error) override {
+    std::vector<FileChooserFileInfoPtr> file_info;
+    for (const auto& path : lister_paths_)
+      file_info.push_back(FileChooserFileInfo::NewNativeFile(
+          blink::mojom::NativeFileInfo::New(path, base::string16())));
+
+    OnFilesSelected(std::move(file_info), lister_base_dir_);
+    Release();
+  }
+
+  void EnumerateDirectory() {
+    // Ensure that this fn is only called once
+    DCHECK(!lister_);
+    DCHECK(!lister_base_dir_.empty());
+    DCHECK(lister_paths_.empty());
+
+    lister_.reset(new net::DirectoryLister(
+        lister_base_dir_, net::DirectoryLister::NO_SORT_RECURSIVE, this));
+    lister_->Start();
     // It is difficult for callers to know how long to keep a reference to
     // this instance.  We AddRef() here to keep the instance alive after we
     // return to the caller.  Once the directory lister is complete we
-    // Release() in OnDirectoryListerDone() and at that point we run
+    // Release() & at that point we run OnFilesSelected() which will
-    // OnFilesSelected() which will deref the last reference held by the
+    // deref the last reference held by the listener.
-    // listener.
     AddRef();
   }
 
@@ -102,7 +123,6 @@ class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
     std::vector<FileChooserFileInfoPtr> file_info;
     bool canceled = true;
     result.Get("canceled", &canceled);
-    base::FilePath base_dir;
     // For certain file chooser modes (kUploadFolder) we need to do some async
     // work before calling back to the listener.  In that particular case the
     // listener is called from the directory enumerator.
@@ -114,12 +134,8 @@ class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
         // If we are uploading a folder we need to enumerate its contents
         if (mode_ == FileChooserParams::Mode::kUploadFolder &&
             paths.size() >= 1) {
-          base_dir = paths[0];
+          lister_base_dir_ = paths[0];
-
+          EnumerateDirectory();
-          // Actually enumerate soemwhere off-thread
-          base::SequencedTaskRunnerHandle::Get()->PostTask(
-              FROM_HERE, base::BindOnce(&FileSelectHelper::EnumerateDirectory,
-                                        this, base_dir));
         } else {
           for (auto& path : paths) {
             file_info.push_back(FileChooserFileInfo::NewNativeFile(
@@ -140,7 +156,7 @@ class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
     }
 
     if (ready_to_call_listener)
-      OnFilesSelected(std::move(file_info), base_dir);
+      OnFilesSelected(std::move(file_info), lister_base_dir_);
   }
 
   void OnSaveDialogDone(mate::Dictionary result) {
@@ -187,6 +203,11 @@ class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
   content::RenderFrameHost* render_frame_host_;
   std::unique_ptr<content::FileSelectListener> listener_;
   blink::mojom::FileChooserParams::Mode mode_;
+
+  // DirectoryLister-specific members
+  std::unique_ptr<net::DirectoryLister> lister_;
+  base::FilePath lister_base_dir_;
+  std::vector<base::FilePath> lister_paths_;
 };
 
 file_dialog::Filters GetFileTypesFromAcceptType(
@@ -227,7 +248,7 @@ file_dialog::Filters GetFileTypesFromAcceptType(
       valid_type_count++;
   }
 
-  // If no valid exntesion is added, return empty filters.
+  // If no valid extension is added, return empty filters.
   if (extensions.empty())
     return filters;
 
@@ -259,30 +280,6 @@ file_dialog::Filters GetFileTypesFromAcceptType(
 
 namespace electron {
 
-DirectoryListerHelper::DirectoryListerHelper(
-    base::FilePath base,
-    DirectoryListerHelperDelegate* helper)
-    : base_dir_(base), delegate_(helper) {}
-DirectoryListerHelper::~DirectoryListerHelper() {}
-
-void DirectoryListerHelper::OnListFile(
-    const net::DirectoryLister::DirectoryListerData& data) {
-  // We don't want to return directory paths, only file paths
-  if (data.info.IsDirectory())
-    return;
-
-  paths_.push_back(data.path);
-}
-void DirectoryListerHelper::OnListDone(int error) {
-  std::vector<FileChooserFileInfoPtr> file_info;
-  for (auto path : paths_)
-    file_info.push_back(FileChooserFileInfo::NewNativeFile(
-        blink::mojom::NativeFileInfo::New(path, base::string16())));
-
-  delegate_->OnDirectoryListerDone(std::move(file_info), base_dir_);
-  delete this;
-}
-
 WebDialogHelper::WebDialogHelper(NativeWindow* window, bool offscreen)
     : window_(window), offscreen_(offscreen), weak_factory_(this) {}
 
@@ -298,8 +295,8 @@ void WebDialogHelper::RunFileChooser(
   settings.parent_window = window_;
   settings.title = base::UTF16ToUTF8(params.title);
 
-  scoped_refptr<FileSelectHelper> file_select_helper(new FileSelectHelper(
+  auto file_select_helper = base::MakeRefCounted<FileSelectHelper>(
-      render_frame_host, std::move(listener), params.mode));
+      render_frame_host, std::move(listener), params.mode);
   if (params.mode == FileChooserParams::Mode::kSave) {
     settings.default_path = params.default_file_name;
     file_select_helper->ShowSaveDialog(settings);

shell/browser/web_dialog_helper.h

@@ -9,7 +9,6 @@
 #include <vector>
 
 #include "base/memory/weak_ptr.h"
-#include "net/base/directory_lister.h"
 #include "third_party/blink/public/mojom/choosers/file_chooser.mojom.h"
 
 namespace base {
@@ -24,32 +23,6 @@ class WebContents;
 
 namespace electron {
 
-class DirectoryListerHelperDelegate {
- public:
-  virtual void OnDirectoryListerDone(
-      std::vector<blink::mojom::FileChooserFileInfoPtr> file_info,
-      base::FilePath base_dir) = 0;
-};
-
-class DirectoryListerHelper
-    : public net::DirectoryLister::DirectoryListerDelegate {
- public:
-  DirectoryListerHelper(base::FilePath base,
-                        DirectoryListerHelperDelegate* helper);
-  ~DirectoryListerHelper() override;
-
- private:
-  void OnListFile(
-      const net::DirectoryLister::DirectoryListerData& data) override;
-  void OnListDone(int error) override;
-
-  base::FilePath base_dir_;
-  DirectoryListerHelperDelegate* delegate_;
-  std::vector<base::FilePath> paths_;
-
-  DISALLOW_COPY_AND_ASSIGN(DirectoryListerHelper);
-};
-
 class NativeWindow;
 
 class WebDialogHelper {