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

This reverts commit 776645cc.

This CL was reverted because it caused on-device crash loops:
base::CreateSequencedTaskRunner() would fail on a DCHECK because of lack
of access to |g_thread_pool|.

Now that the libcast_media_1.0_audio target is statically linked when
possible, this is no longer an issue. See:
https://chromium-review.googlesource.com/c/chromium/src/+/1846314

Bug: internal b/135280530
Change-Id: Ib84ff58f17dec31abe3ba438a1a87a53763942af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1848576Reviewed-by: Kenneth MacKay <kmackay@chromium.org>
Commit-Queue: Jenny Wong <jyw@google.com>
Cr-Commit-Position: refs/heads/master@{#705213}

chromecast/media/cma/backend/cast_audio_json.cc

@@ -8,18 +8,37 @@
 #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/message_loop/message_pump_type.h"
-#include "base/sequenced_task_runner.h"
+#include "base/task/post_task.h"
+#include "base/task/task_traits.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
@@ -27,14 +46,6 @@ 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();
@@ -56,17 +67,11 @@ base::FilePath CastAudioJson::GetFilePathForTuning() {
 }
 
 CastAudioJsonProviderImpl::CastAudioJsonProviderImpl()
-    : 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();
-}
+    : cast_audio_watcher_(base::SequenceBound<FileWatcher>(
+          base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock(),
+                                           base::TaskPriority::LOWEST}))) {}
 
-CastAudioJsonProviderImpl::~CastAudioJsonProviderImpl() {
-  StopWatchingFileOnThread();
-}
+CastAudioJsonProviderImpl::~CastAudioJsonProviderImpl() = default;
 
 std::unique_ptr<base::Value> CastAudioJsonProviderImpl::GetCastAudioConfig() {
   std::string contents;
@@ -76,35 +81,18 @@ std::unique_ptr<base::Value> CastAudioJsonProviderImpl::GetCastAudioConfig() {
 
 void CastAudioJsonProviderImpl::SetTuningChangedCallback(
     TuningChangedCallback 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();
+  cast_audio_watcher_.Post(FROM_HERE, &FileWatcher::SetTuningChangedCallback,
+                           std::move(callback));
 }
 
-void CastAudioJsonProviderImpl::OnTuningFileChanged(const base::FilePath& path,
-                                                    bool error) {
-  DCHECK(task_runner_->RunsTasksInCurrentSequence());
-  DCHECK(callback_);
+CastAudioJsonProviderImpl::FileWatcher::FileWatcher() = default;
+CastAudioJsonProviderImpl::FileWatcher::~FileWatcher() = default;
 
-  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;
+void CastAudioJsonProviderImpl::FileWatcher::SetTuningChangedCallback(
+    TuningChangedCallback callback) {
+  watcher_.Watch(
+      CastAudioJson::GetFilePathForTuning(), false /* recursive */,
+      base::BindRepeating(&ReadFileRunCallback, std::move(callback)));
 }
 
 }  // namespace media

chromecast/media/cma/backend/cast_audio_json.h

@@ -9,15 +9,11 @@
 
 #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/thread.h"
+#include "base/threading/sequence_bound.h"
 #include "base/values.h"
 
-namespace base {
-class FilePathWatcher;
-class SequencedTaskRunner;
-}  // namespace base
-
 namespace chromecast {
 namespace media {
 
@@ -48,9 +44,8 @@ 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 the same
-  // thread on which |SetTuningChangedCallback| is called.
-  // |callback| will never be called after ~CastAudioJsonProvider() is called.
+  // |callback| will always be called from the same thread, but not necessarily
+  // the same thread on which |SetTuningChangedCallback| is called.
   virtual void SetTuningChangedCallback(TuningChangedCallback callback) = 0;
 };
 
@@ -60,17 +55,22 @@ 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;
 
-  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_;
+  base::SequenceBound<FileWatcher> cast_audio_watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(CastAudioJsonProviderImpl);
 };