Fix data race in DirectoryLister on cancellation.

The class tracked cancellation by overwriting a variable on one thread
that would then be used on the other thread to prevent a callback on the
first thread, which isn't threadsafe.  Switched to using weak ptrs.

Includes some cleanup, and expands test coverage.

BUG=436211

Review URL: https://codereview.chromium.org/777033003

Cr-Commit-Position: refs/heads/master@{#306918}

3 files changed, 264 insertions, 105 deletions

diff --git a/net/base/directory_lister.cc b/net/base/directory_lister.cc
index 841f454..f746f9b 100644
--- a/net/base/directory_lister.cc
+++ b/net/base/directory_lister.cc
@@ -5,12 +5,12 @@
 #include "net/base/directory_lister.h"
 
 #include <algorithm>
-#include <vector>
 
 #include "base/bind.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/i18n/file_util_icu.h"
+#include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/worker_pool.h"
@@ -73,22 +73,23 @@ void SortData(std::vector<DirectoryLister::DirectoryListerData>* data,
               DirectoryLister::SortType sort_type) {
   // Sort the results. See the TODO below (this sort should be removed and we
   // should do it from JS).
-  if (sort_type == DirectoryLister::DATE)
+  if (sort_type == DirectoryLister::DATE) {
     std::sort(data->begin(), data->end(), CompareDate);
-  else if (sort_type == DirectoryLister::FULL_PATH)
+  } else if (sort_type == DirectoryLister::FULL_PATH) {
     std::sort(data->begin(), data->end(), CompareFullPath);
-  else if (sort_type == DirectoryLister::ALPHA_DIRS_FIRST)
+  } else if (sort_type == DirectoryLister::ALPHA_DIRS_FIRST) {
     std::sort(data->begin(), data->end(), CompareAlphaDirsFirst);
-  else
+  } else {
     DCHECK_EQ(DirectoryLister::NO_SORT, sort_type);
+  }
 }
 
 }  // namespace
 
 DirectoryLister::DirectoryLister(const base::FilePath& dir,
                                  DirectoryListerDelegate* delegate)
-    : core_(new Core(dir, false, ALPHA_DIRS_FIRST, this)),
-      delegate_(delegate) {
+    : delegate_(delegate) {
+  core_ = new Core(dir, false, ALPHA_DIRS_FIRST, this);
   DCHECK(delegate_);
   DCHECK(!dir.value().empty());
 }
@@ -97,8 +98,8 @@ DirectoryLister::DirectoryLister(const base::FilePath& dir,
                                  bool recursive,
                                  SortType sort,
                                  DirectoryListerDelegate* delegate)
-    : core_(new Core(dir, recursive, sort, this)),
-      delegate_(delegate) {
+    : delegate_(delegate) {
+  core_ = new Core(dir, recursive, sort, this);
   DCHECK(delegate_);
   DCHECK(!dir.value().empty());
 }
@@ -108,11 +109,14 @@ DirectoryLister::~DirectoryLister() {
 }
 
 bool DirectoryLister::Start() {
-  return core_->Start();
+  return base::WorkerPool::PostTask(
+      FROM_HERE,
+      base::Bind(&Core::Start, core_),
+      true);
 }
 
 void DirectoryLister::Cancel() {
-  return core_->Cancel();
+  core_->CancelOnOriginThread();
 }
 
 DirectoryLister::Core::Core(const base::FilePath& dir,
@@ -122,29 +126,32 @@ DirectoryLister::Core::Core(const base::FilePath& dir,
     : dir_(dir),
       recursive_(recursive),
       sort_(sort),
-      lister_(lister) {
+      origin_loop_(base::MessageLoopProxy::current()),
+      lister_(lister),
+      cancelled_(0) {
   DCHECK(lister_);
 }
 
 DirectoryLister::Core::~Core() {}
 
-bool DirectoryLister::Core::Start() {
-  origin_loop_ = base::MessageLoopProxy::current();
-
-  return base::WorkerPool::PostTask(
-      FROM_HERE, base::Bind(&Core::StartInternal, this), true);
-}
+void DirectoryLister::Core::CancelOnOriginThread() {
+  DCHECK(origin_loop_->BelongsToCurrentThread());
 
-void DirectoryLister::Core::Cancel() {
-  lister_ = NULL;
+  base::subtle::NoBarrier_Store(&cancelled_, 1);
+  // Core must not call into |lister_| after cancellation, as the |lister_| may
+  // have been destroyed. Setting |lister_| to NULL ensures any such access will
+  // cause a crash.
+  lister_ = nullptr;
 }
 
-void DirectoryLister::Core::StartInternal() {
+void DirectoryLister::Core::Start() {
+  scoped_ptr<DirectoryList> directory_list(new DirectoryList());
 
   if (!base::DirectoryExists(dir_)) {
     origin_loop_->PostTask(
         FROM_HERE,
-        base::Bind(&DirectoryLister::Core::OnDone, this, ERR_FILE_NOT_FOUND));
+        base::Bind(&Core::DoneOnOriginThread, this,
+                   base::Passed(directory_list.Pass()), ERR_FILE_NOT_FOUND));
     return;
   }
 
@@ -155,12 +162,16 @@ void DirectoryLister::Core::StartInternal() {
   base::FileEnumerator file_enum(dir_, recursive_, types);
 
   base::FilePath path;
-  std::vector<DirectoryListerData> file_data;
-  while (lister_ && !(path = file_enum.Next()).empty()) {
+  while (!(path = file_enum.Next()).empty()) {
+    // Abort on cancellation. This is purely for performance reasons.
+    // Correctness guarantees are made by checks in DoneOnOriginThread.
+    if (IsCancelled())
+      return;
+
     DirectoryListerData data;
     data.info = file_enum.GetInfo();
     data.path = path;
-    file_data.push_back(data);
+    directory_list->push_back(data);
 
     /* TODO(brettw) bug 24107: It would be nice to send incremental updates.
        We gather them all so they can be sorted, but eventually the sorting
@@ -178,36 +189,40 @@ void DirectoryLister::Core::StartInternal() {
     */
   }
 
-  SortData(&file_data, sort_);
-  origin_loop_->PostTask(
-      FROM_HERE,
-      base::Bind(&DirectoryLister::Core::SendData, this, file_data));
+  SortData(directory_list.get(), sort_);
 
   origin_loop_->PostTask(
       FROM_HERE,
-      base::Bind(&DirectoryLister::Core::OnDone, this, OK));
+      base::Bind(&Core::DoneOnOriginThread, this,
+                 base::Passed(directory_list.Pass()), OK));
 }
 
-void DirectoryLister::Core::SendData(
-    const std::vector<DirectoryLister::DirectoryListerData>& data) {
-  DCHECK(origin_loop_->BelongsToCurrentThread());
-  // We need to check for cancellation (indicated by NULL'ing of |lister_|)
-  // which can happen during each callback.
-  for (size_t i = 0; lister_ && i < data.size(); ++i)
-    lister_->OnReceivedData(data[i]);
+bool DirectoryLister::Core::IsCancelled() const {
+  return !!base::subtle::NoBarrier_Load(&cancelled_);
 }
 
-void DirectoryLister::Core::OnDone(int error) {
+void DirectoryLister::Core::DoneOnOriginThread(
+    scoped_ptr<DirectoryList> directory_list, int error) const {
   DCHECK(origin_loop_->BelongsToCurrentThread());
-  if (lister_)
-    lister_->OnDone(error);
+
+  // Need to check if the operation was before first callback.
+  if (IsCancelled())
+    return;
+
+  for (const auto& lister_data : *directory_list) {
+    lister_->OnListFile(lister_data);
+    // Need to check if the operation was cancelled during the callback.
+    if (IsCancelled())
+      return;
+  }
+  lister_->OnListDone(error);
 }
 
-void DirectoryLister::OnReceivedData(const DirectoryListerData& data) {
+void DirectoryLister::OnListFile(const DirectoryListerData& data) {
   delegate_->OnListFile(data);
 }
 
-void DirectoryLister::OnDone(int error) {
+void DirectoryLister::OnListDone(int error) {
   delegate_->OnListDone(error);
 }
 
diff --git a/net/base/directory_lister.h b/net/base/directory_lister.h
index 5fbc9dc6..51387e8 100644
--- a/net/base/directory_lister.h
+++ b/net/base/directory_lister.h
@@ -7,21 +7,22 @@
 
 #include <vector>
 
+#include "base/atomicops.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
+#include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "net/base/net_export.h"
 
 namespace net {
 
-//
-// This class provides an API for listing the contents of a directory on the
-// filesystem asynchronously.  It spawns a background thread, and enumerates
-// the specified directory on that thread.  It marshalls WIN32_FIND_DATA
-// structs over to the main application thread.  The consumer of this class
-// is insulated from any of the multi-threading details.
-//
+// This class provides an API for asynchronously listing the contents of a
+// directory on the filesystem.  It runs a task on a background thread, and
+// enumerates all files in the specified directory on that thread.  Destroying
+// the lister cancels the list operation.  The DirectoryLister must only be
+// used on a thread with a MessageLoop.
 class NET_EXPORT DirectoryLister  {
  public:
   // Represents one file found.
@@ -48,6 +49,9 @@ class NET_EXPORT DirectoryLister  {
   //   directories first in name order, then files by name order
   // FULL_PATH sorts by paths as strings, ignoring files v. directories
   // DATE sorts by last modified date
+  // TODO(mmenke):  Only NO_SORT and ALPHA_DIRS_FIRST appear to be used in
+  //     production code, and there's very little testing of some of these
+  //     options.  Remove unused options, improve testing of the others.
   enum SortType {
     NO_SORT,
     DATE,
@@ -74,6 +78,15 @@ class NET_EXPORT DirectoryLister  {
   void Cancel();
 
  private:
+  typedef std::vector<DirectoryListerData> DirectoryList;
+
+  // Class responsible for retrieving and sorting the actual directory list on
+  // a worker pool thread. Created on the DirectoryLister's thread. As it's
+  // refcounted, it's destroyed when the final reference is released, which may
+  // happen on either thread.
+  //
+  // It's kept alive during the calls to Start() and DoneOnOriginThread() by the
+  // reference owned by the callback itself.
   class Core : public base::RefCountedThreadSafe<Core> {
    public:
     Core(const base::FilePath& dir,
@@ -81,9 +94,11 @@ class NET_EXPORT DirectoryLister  {
          SortType sort,
          DirectoryLister* lister);
 
-    bool Start();
+    // May only be called on a worker pool thread.
+    void Start();
 
-    void Cancel();
+    // Must be called on the origin thread.
+    void CancelOnOriginThread();
 
    private:
     friend class base::RefCountedThreadSafe<Core>;
@@ -91,28 +106,35 @@ class NET_EXPORT DirectoryLister  {
 
     ~Core();
 
-    // This method runs on a WorkerPool thread.
-    void StartInternal();
+    // Called on both threads.
+    bool IsCancelled() const;
 
-    void SendData(const std::vector<DirectoryListerData>& data);
+    // Called on origin thread.
+    void DoneOnOriginThread(scoped_ptr<DirectoryList> directory_list,
+                            int error) const;
 
-    void OnDone(int error);
+    const base::FilePath dir_;
+    const bool recursive_;
+    const SortType sort_;
+    const scoped_refptr<base::MessageLoopProxy> origin_loop_;
 
-    base::FilePath dir_;
-    bool recursive_;
-    SortType sort_;
-    scoped_refptr<base::MessageLoopProxy> origin_loop_;
-
-    // |lister_| gets set to NULL when canceled.
+    // Only used on the origin thread.
     DirectoryLister* lister_;
 
+    // Set to 1 on cancellation. Used both to abort listing files early on the
+    // worker pool thread for performance reasons and to ensure |lister_| isn't
+    // called after cancellation on the origin thread.
+    base::subtle::Atomic32 cancelled_;
+
     DISALLOW_COPY_AND_ASSIGN(Core);
   };
 
-  void OnReceivedData(const DirectoryListerData& data);
-  void OnDone(int error);
+  // Call into the corresponding DirectoryListerDelegate. Must not be called
+  // after cancellation.
+  void OnListFile(const DirectoryListerData& data);
+  void OnListDone(int error);
 
-  const scoped_refptr<Core> core_;
+  scoped_refptr<Core> core_;
   DirectoryListerDelegate* const delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(DirectoryLister);
diff --git a/net/base/directory_lister_unittest.cc b/net/base/directory_lister_unittest.cc
index 869e4f6..adc3d5e 100644
--- a/net/base/directory_lister_unittest.cc
+++ b/net/base/directory_lister_unittest.cc
@@ -4,12 +4,15 @@
 
 #include <list>
 #include <utility>
+#include <vector>
 
+#include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/i18n/file_util_icu.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/directory_lister.h"
 #include "net/base/net_errors.h"
@@ -18,29 +21,59 @@
 
 namespace net {
 
+namespace {
+
+const int kMaxDepth = 3;
+const int kBranchingFactor = 4;
+const int kFilesPerDirectory = 5;
+
 class ListerDelegate : public DirectoryLister::DirectoryListerDelegate {
  public:
-  ListerDelegate(bool recursive,
-                 bool quit_loop_after_each_file)
-      : error_(-1),
-        recursive_(recursive),
-        quit_loop_after_each_file_(quit_loop_after_each_file) {
+  explicit ListerDelegate(bool recursive)
+      : cancel_lister_on_list_file_(false),
+        cancel_lister_on_list_done_(false),
+        lister_(nullptr),
+        done_(false),
+        error_(-1),
+        recursive_(recursive) {
+  }
+
+  // When set to true, this signals that the directory list operation should be
+  // cancelled (And the run loop quit) in the first call to OnListFile.
+  void set_cancel_lister_on_list_file(bool cancel_lister_on_list_file) {
+    cancel_lister_on_list_file_ = cancel_lister_on_list_file;
+  }
+
+  // When set to true, this signals that the directory list operation should be
+  // cancelled (And the run loop quit) when OnDone is called.
+  void set_cancel_lister_on_list_done(bool cancel_lister_on_list_done) {
+    cancel_lister_on_list_done_ = cancel_lister_on_list_done;
   }
 
   void OnListFile(const DirectoryLister::DirectoryListerData& data) override {
+    ASSERT_FALSE(done_);
+
     file_list_.push_back(data.info);
     paths_.push_back(data.path);
-    if (quit_loop_after_each_file_)
-      base::MessageLoop::current()->Quit();
+    if (cancel_lister_on_list_file_) {
+      lister_->Cancel();
+      run_loop.Quit();
+    }
   }
 
   void OnListDone(int error) override {
+    ASSERT_FALSE(done_);
+
+    done_ = true;
     error_ = error;
-    base::MessageLoop::current()->Quit();
     if (recursive_)
       CheckRecursiveSort();
     else
       CheckSort();
+
+    if (cancel_lister_on_list_done_)
+      lister_->Cancel();
+    run_loop.Quit();
   }
 
   void CheckRecursiveSort() {
@@ -77,25 +110,45 @@ class ListerDelegate : public DirectoryLister::DirectoryListerDelegate {
     }
   }
 
+  void Run(DirectoryLister* lister) {
+    lister_ = lister;
+    lister_->Start();
+    run_loop.Run();
+  }
+
   int error() const { return error_; }
 
   int num_files() const { return file_list_.size(); }
 
+  bool done() const { return done_; }
+
  private:
+  bool cancel_lister_on_list_file_;
+  bool cancel_lister_on_list_done_;
+
+  // This is owned by the individual tests, rather than the ListerDelegate.
+  DirectoryLister* lister_;
+
+  base::RunLoop run_loop;
+
+  bool done_;
   int error_;
   bool recursive_;
-  bool quit_loop_after_each_file_;
+
   std::vector<base::FileEnumerator::FileInfo> file_list_;
   std::vector<base::FilePath> paths_;
 };
 
+}  // namespace
+
 class DirectoryListerTest : public PlatformTest {
  public:
-  void SetUp() override {
-    const int kMaxDepth = 3;
-    const int kBranchingFactor = 4;
-    const int kFilesPerDirectory = 5;
+  DirectoryListerTest()
+      : total_created_file_system_objects_in_temp_root_dir_(0),
+        created_file_system_objects_in_temp_root_dir_(0) {
+  }
 
+  void SetUp() override {
     // Randomly create a directory structure of depth 3 in a temporary root
     // directory.
     std::list<std::pair<base::FilePath, int> > directories;
@@ -110,12 +163,18 @@ class DirectoryListerTest : public PlatformTest {
         base::File file(file_path,
                         base::File::FLAG_CREATE | base::File::FLAG_WRITE);
         ASSERT_TRUE(file.IsValid());
+        ++total_created_file_system_objects_in_temp_root_dir_;
+        if (dir_data.first == temp_root_dir_.path())
+          ++created_file_system_objects_in_temp_root_dir_;
       }
       if (dir_data.second < kMaxDepth - 1) {
         for (int i = 0; i < kBranchingFactor; i++) {
           std::string dir_name = base::StringPrintf("child_dir_%d", i);
           base::FilePath dir_path = dir_data.first.AppendASCII(dir_name);
           ASSERT_TRUE(base::CreateDirectory(dir_path));
+          ++total_created_file_system_objects_in_temp_root_dir_;
+          if (dir_data.first == temp_root_dir_.path())
+            ++created_file_system_objects_in_temp_root_dir_;
           directories.push_back(std::make_pair(dir_path, dir_data.second + 1));
         }
       }
@@ -127,62 +186,125 @@ class DirectoryListerTest : public PlatformTest {
     return temp_root_dir_.path();
   }
 
+  int expected_list_length_recursive() const {
+    // List should include everything but the top level directory, and does not
+    // include "..".
+    return total_created_file_system_objects_in_temp_root_dir_;
+  }
+
+  int expected_list_length_non_recursive() const {
+    // List should include everything in the top level directory, and "..".
+    return created_file_system_objects_in_temp_root_dir_ + 1;
+  }
+
  private:
+  // Number of files and directories created in SetUp, excluding
+  // |temp_root_dir_| itself.  Includes all nested directories and their files.
+  int total_created_file_system_objects_in_temp_root_dir_;
+  // Number of files and directories created directly in |temp_root_dir_|.
+  int created_file_system_objects_in_temp_root_dir_;
+
   base::ScopedTempDir temp_root_dir_;
 };
 
 TEST_F(DirectoryListerTest, BigDirTest) {
-  ListerDelegate delegate(false, false);
+  ListerDelegate delegate(false);
   DirectoryLister lister(root_path(), &delegate);
-  lister.Start();
-
-  base::MessageLoop::current()->Run();
+  delegate.Run(&lister);
 
+  EXPECT_TRUE(delegate.done());
   EXPECT_EQ(OK, delegate.error());
+  EXPECT_EQ(expected_list_length_non_recursive(), delegate.num_files());
 }
 
 TEST_F(DirectoryListerTest, BigDirRecursiveTest) {
-  ListerDelegate delegate(true, false);
-  DirectoryLister lister(root_path(), true, DirectoryLister::FULL_PATH,
-                         &delegate);
-  lister.Start();
-
-  base::MessageLoop::current()->Run();
+  ListerDelegate delegate(true);
+  DirectoryLister lister(
+      root_path(), true, DirectoryLister::FULL_PATH, &delegate);
+  delegate.Run(&lister);
 
+  EXPECT_TRUE(delegate.done());
   EXPECT_EQ(OK, delegate.error());
+  EXPECT_EQ(expected_list_length_recursive(), delegate.num_files());
 }
 
-TEST_F(DirectoryListerTest, CancelTest) {
-  ListerDelegate delegate(false, true);
-  DirectoryLister lister(root_path(), &delegate);
-  lister.Start();
+TEST_F(DirectoryListerTest, EmptyDirTest) {
+  base::ScopedTempDir tempDir;
+  EXPECT_TRUE(tempDir.CreateUniqueTempDir());
 
-  base::MessageLoop::current()->Run();
+  ListerDelegate delegate(false);
+  DirectoryLister lister(tempDir.path(), &delegate);
+  delegate.Run(&lister);
 
-  int num_files = delegate.num_files();
+  EXPECT_TRUE(delegate.done());
+  EXPECT_EQ(OK, delegate.error());
+  // Contains only the parent directory ("..").
+  EXPECT_EQ(1, delegate.num_files());
+}
 
-  lister.Cancel();
+// This doesn't really test much, except make sure calling cancel before any
+// callbacks are invoked doesn't crash.  Can't wait for all tasks running on a
+// worker pool to complete, unfortunately.
+// TODO(mmenke):  See if there's a way to make this fail more reliably on
+// regression.
+TEST_F(DirectoryListerTest, BasicCancelTest) {
+  ListerDelegate delegate(false);
+  scoped_ptr<DirectoryLister> lister(new DirectoryLister(
+      root_path(), &delegate));
+  lister->Start();
+  lister->Cancel();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(delegate.done());
+  EXPECT_EQ(0, delegate.num_files());
+}
 
-  base::MessageLoop::current()->RunUntilIdle();
+TEST_F(DirectoryListerTest, CancelOnListFileTest) {
+  ListerDelegate delegate(false);
+  DirectoryLister lister(root_path(), &delegate);
+  delegate.set_cancel_lister_on_list_file(true);
+  delegate.Run(&lister);
 
-  EXPECT_EQ(num_files, delegate.num_files());
+  EXPECT_FALSE(delegate.done());
+  EXPECT_EQ(1, delegate.num_files());
 }
 
-TEST_F(DirectoryListerTest, EmptyDirTest) {
+TEST_F(DirectoryListerTest, CancelOnListDoneTest) {
+  ListerDelegate delegate(false);
+  DirectoryLister lister(root_path(), &delegate);
+  delegate.set_cancel_lister_on_list_done(true);
+  delegate.Run(&lister);
+
+  EXPECT_TRUE(delegate.done());
+  EXPECT_EQ(OK, delegate.error());
+  EXPECT_EQ(expected_list_length_non_recursive(), delegate.num_files());
+}
+
+TEST_F(DirectoryListerTest, CancelOnLastElementTest) {
   base::ScopedTempDir tempDir;
   EXPECT_TRUE(tempDir.CreateUniqueTempDir());
 
-  bool kRecursive = false;
-  bool kQuitLoopAfterEachFile = false;
-  ListerDelegate delegate(kRecursive, kQuitLoopAfterEachFile);
+  ListerDelegate delegate(false);
   DirectoryLister lister(tempDir.path(), &delegate);
-  lister.Start();
-
-  base::MessageLoop::current()->Run();
+  delegate.set_cancel_lister_on_list_file(true);
+  delegate.Run(&lister);
 
-  // Contains only the parent directory ("..")
+  EXPECT_FALSE(delegate.done());
+  // Contains only the parent directory ("..").
   EXPECT_EQ(1, delegate.num_files());
-  EXPECT_EQ(OK, delegate.error());
+}
+
+TEST_F(DirectoryListerTest, NoSuchDirTest) {
+  base::ScopedTempDir tempDir;
+  EXPECT_TRUE(tempDir.CreateUniqueTempDir());
+
+  ListerDelegate delegate(false);
+  DirectoryLister lister(
+      tempDir.path().AppendASCII("this_path_does_not_exist"), &delegate);
+  delegate.Run(&lister);
+
+  EXPECT_EQ(ERR_FILE_NOT_FOUND, delegate.error());
+  EXPECT_EQ(0, delegate.num_files());
 }
 
 }  // namespace net