Revert "[Chromecast] Fix thread safety issues in CastAudioJson."

This reverts commit f98214f3.

Reason for revert: CP for unblocking chromium merge sheriff

Original change's description:
> [Chromecast] Fix thread safety issues in CastAudioJson.
>
> Bug: internal 135280530
> Change-Id: I611e61735b4537e5db330cb6c78fe05fcb193170
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1706786
> Commit-Queue: Jenny Wong <jyw@chromium.org>
> Reviewed-by: Kenneth MacKay <kmackay@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#684198}

TBR=jyw@chromium.org,kmackay@chromium.org,mont@chromium.org

Bug: internal 135280530
Bug: b/140290037
Change-Id: I000c7e6b3e38d7a23781ed6b5480615cdd1d9a49
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1761698Reviewed-by: Michael Spang <spang@chromium.org>
Commit-Queue: Michael Spang <spang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692073}

chromecast/media/cma/backend/cast_audio_json.cc

@@ -8,37 +8,18 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/files/file_path_watcher.h"
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/location.h"
 #include "base/logging.h"
-#include "base/task/post_task.h"
-#include "base/task/task_traits.h"
+#include "base/message_loop/message_pump_type.h"
+#include "base/sequenced_task_runner.h"
 #include "build/build_config.h"
 
 namespace chromecast {
 namespace media {
 
-namespace {
-
-void ReadFileRunCallback(CastAudioJsonProvider::TuningChangedCallback callback,
-                         const base::FilePath& path,
-                         bool error) {
-  DCHECK(callback);
-
-  std::string contents;
-  base::ReadFileToString(path, &contents);
-  std::unique_ptr<base::Value> value =
-      base::JSONReader::ReadDeprecated(contents);
-  if (value) {
-    callback.Run(std::move(value));
-    return;
-  }
-  LOG(ERROR) << "Unable to parse JSON in " << path;
-}
-
-}  // namespace
-
 #if defined(OS_FUCHSIA)
 const char kCastAudioJsonFilePath[] = "/system/data/cast_audio.json";
 #else
@@ -46,6 +27,14 @@ const char kCastAudioJsonFilePath[] = "/etc/cast_audio.json";
 #endif
 const char kCastAudioJsonFileName[] = "cast_audio.json";
 
+#define ENSURE_OWN_THREAD(method, ...)                                     \
+  if (!task_runner_->RunsTasksInCurrentSequence()) {                       \
+    task_runner_->PostTask(                                                \
+        FROM_HERE, base::BindOnce(&CastAudioJsonProviderImpl::method,      \
+                                  base::Unretained(this), ##__VA_ARGS__)); \
+    return;                                                                \
+  }
+
 // static
 base::FilePath CastAudioJson::GetFilePath() {
   base::FilePath tuning_path = CastAudioJson::GetFilePathForTuning();
@@ -67,11 +56,17 @@ base::FilePath CastAudioJson::GetFilePathForTuning() {
 }
 
 CastAudioJsonProviderImpl::CastAudioJsonProviderImpl()
-    : cast_audio_watcher_(base::SequenceBound<FileWatcher>(
-          base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock(),
-                                           base::TaskPriority::LOWEST}))) {}
+    : thread_("cast_audio_json_provider"),
+      cast_audio_watcher_(std::make_unique<base::FilePathWatcher>()) {
+  base::Thread::Options options;
+  options.message_pump_type = base::MessagePumpType::IO;
+  thread_.StartWithOptions(options);
+  task_runner_ = thread_.task_runner();
+}
 
-CastAudioJsonProviderImpl::~CastAudioJsonProviderImpl() = default;
+CastAudioJsonProviderImpl::~CastAudioJsonProviderImpl() {
+  StopWatchingFileOnThread();
+}
 
 std::unique_ptr<base::Value> CastAudioJsonProviderImpl::GetCastAudioConfig() {
   std::string contents;
@@ -81,18 +76,35 @@ std::unique_ptr<base::Value> CastAudioJsonProviderImpl::GetCastAudioConfig() {
 
 void CastAudioJsonProviderImpl::SetTuningChangedCallback(
     TuningChangedCallback callback) {
-  cast_audio_watcher_.Post(FROM_HERE, &FileWatcher::SetTuningChangedCallback,
-                           std::move(callback));
+  ENSURE_OWN_THREAD(SetTuningChangedCallback, std::move(callback));
+
+  CHECK(!callback_);
+  callback_ = callback;
+  cast_audio_watcher_->Watch(
+      CastAudioJson::GetFilePathForTuning(), false /* recursive */,
+      base::BindRepeating(&CastAudioJsonProviderImpl::OnTuningFileChanged,
+                          base::Unretained(this)));
+}
+
+void CastAudioJsonProviderImpl::StopWatchingFileOnThread() {
+  ENSURE_OWN_THREAD(StopWatchingFileOnThread);
+  cast_audio_watcher_.reset();
 }
 
-CastAudioJsonProviderImpl::FileWatcher::FileWatcher() = default;
-CastAudioJsonProviderImpl::FileWatcher::~FileWatcher() = default;
+void CastAudioJsonProviderImpl::OnTuningFileChanged(const base::FilePath& path,
+                                                    bool error) {
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(callback_);
 
-void CastAudioJsonProviderImpl::FileWatcher::SetTuningChangedCallback(
-    TuningChangedCallback callback) {
-  watcher_.Watch(
-      CastAudioJson::GetFilePathForTuning(), false /* recursive */,
-      base::BindRepeating(&ReadFileRunCallback, std::move(callback)));
+  std::string contents;
+  base::ReadFileToString(path, &contents);
+  std::unique_ptr<base::Value> value =
+      base::JSONReader::ReadDeprecated(contents);
+  if (value) {
+    callback_.Run(std::move(value));
+    return;
+  }
+  LOG(ERROR) << "Unable to parse JSON in " << path;
 }
 
 }  // namespace media

chromecast/media/cma/backend/cast_audio_json.h

@@ -9,11 +9,15 @@
 
 #include "base/callback.h"
 #include "base/files/file_path.h"
-#include "base/files/file_path_watcher.h"
 #include "base/memory/scoped_refptr.h"
-#include "base/threading/sequence_bound.h"
+#include "base/threading/thread.h"
 #include "base/values.h"
 
+namespace base {
+class FilePathWatcher;
+class SequencedTaskRunner;
+}  // namespace base
+
 namespace chromecast {
 namespace media {
 
@@ -44,8 +48,9 @@ class CastAudioJsonProvider {
   virtual std::unique_ptr<base::Value> GetCastAudioConfig() = 0;
 
   // |callback| will be called when a new cast_audio config is available.
-  // |callback| will always be called from the same thread, but not necessarily
-  // the same thread on which |SetTuningChangedCallback| is called.
+  // |callback| will always be called from the same thread, but not the same
+  // thread on which |SetTuningChangedCallback| is called.
+  // |callback| will never be called after ~CastAudioJsonProvider() is called.
   virtual void SetTuningChangedCallback(TuningChangedCallback callback) = 0;
 };
 
@@ -55,22 +60,17 @@ class CastAudioJsonProviderImpl : public CastAudioJsonProvider {
   ~CastAudioJsonProviderImpl() override;
 
  private:
-  class FileWatcher {
-   public:
-    FileWatcher();
-    ~FileWatcher();
-
-    void SetTuningChangedCallback(TuningChangedCallback callback);
-
-   private:
-    base::FilePathWatcher watcher_;
-  };
-
   // CastAudioJsonProvider implementation:
   std::unique_ptr<base::Value> GetCastAudioConfig() override;
   void SetTuningChangedCallback(TuningChangedCallback callback) override;
 
-  base::SequenceBound<FileWatcher> cast_audio_watcher_;
+  void StopWatchingFileOnThread();
+  void OnTuningFileChanged(const base::FilePath& path, bool error);
+
+  TuningChangedCallback callback_;
+  base::Thread thread_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  std::unique_ptr<base::FilePathWatcher> cast_audio_watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(CastAudioJsonProviderImpl);
 };