Fix potential use-after-free/race condition in AmbientPhotoCache.

The old API was accepting a raw pointer to a protobuf message and
modifying the object asynchronously on a worker thread. We don't know
for sure that this is the exact reason for the crash, but this API
can be a problem if a) the protobuf message is destroyed before
the worker thread writes to it (i.e. the ambient-mode enabled pref
changes) b) AmbientPhotoController modifies the protobuf message
concurrently on the UI thread. Both seem extremely unlikely, but this
is a low volume crash that's been around since M98 so maybe it's
happening on a few devices.

Since this is the only visible explanation for a crash like this
and it seems like an overall improvement to the API,
we'll make this change and see if the crash goes away.

Bug: b:240194606
Change-Id: Icc47b78702e67e6a52be9b99fe49be6ea7681dd1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3786842
Commit-Queue: Eric Sum <esum@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1028513}

ash/ambient/ambient_photo_cache.cc

@@ -233,26 +233,27 @@ class AmbientPhotoCacheImpl : public AmbientPhotoCache {
         std::move(callback));
   }
 
-  void ReadPhotoCache(int cache_index,
-                      ambient::PhotoCacheEntry* cache_entry,
-                      base::OnceCallback<void()> callback) override {
-    task_runner_->PostTaskAndReply(
+  void ReadPhotoCache(
+      int cache_index,
+      base::OnceCallback<void(::ambient::PhotoCacheEntry)> callback) override {
+    task_runner_->PostTaskAndReplyWithResult(
         FROM_HERE,
         base::BindOnce(
-            [](int cache_index, const base::FilePath& root_path,
-               ambient::PhotoCacheEntry* cache_entry) {
+            [](int cache_index, const base::FilePath& root_path) {
               auto cache_path = GetCachePath(cache_index, root_path);
 
               // Read the existing cache.
               const char* path_str = cache_path.value().c_str();
               std::fstream input(path_str, std::ios::in | std::ios::binary);
-              if (!input || !cache_entry->ParseFromIstream(&input)) {
+              ambient::PhotoCacheEntry cache_entry;
+              if (!input || !cache_entry.ParseFromIstream(&input)) {
                 LOG(ERROR) << "Unable to read photo cache";
-                *cache_entry = ambient::PhotoCacheEntry();
+                cache_entry = ::ambient::PhotoCacheEntry();
                 base::DeleteFile(cache_path);
               }
+              return cache_entry;
             },
-            cache_index, root_directory_, cache_entry),
+            cache_index, root_directory_),
         std::move(callback));
   }
 

ash/ambient/ambient_photo_cache.h

@@ -63,9 +63,10 @@ class ASH_EXPORT AmbientPhotoCache {
 
   // Read the photo cache at |cache_index| and call |callback| when complete.
   // If a particular cache fails to be read, |cache_entry| will be empty.
-  virtual void ReadPhotoCache(int cache_index,
-                              ::ambient::PhotoCacheEntry* cache_entry,
-                              base::OnceCallback<void()> callback) = 0;
+  virtual void ReadPhotoCache(
+      int cache_index,
+      base::OnceCallback<void(::ambient::PhotoCacheEntry cache_entry)>
+          callback) = 0;
 
   // Erase all stored files from disk.
   virtual void Clear() = 0;

ash/ambient/ambient_photo_cache_unittest.cc

@@ -18,6 +18,7 @@
 #include "base/files/file_util.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
+#include "base/test/bind.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -106,9 +107,12 @@ TEST_F(AmbientPhotoCacheTest, ReadsBackWrittenFiles) {
     // Read the files back using photo cache.
     ambient::PhotoCacheEntry cache_read;
     photo_cache()->ReadPhotoCache(
-        cache_index, &cache_read,
-        base::BindOnce([](base::OnceClosure done) { std::move(done).Run(); },
-                       loop.QuitClosure()));
+        cache_index,
+        base::BindLambdaForTesting(
+            [&cache_read, &loop](ambient::PhotoCacheEntry cache_entry_in) {
+              cache_read = std::move(cache_entry_in);
+              loop.Quit();
+            }));
     loop.Run();
 
     EXPECT_EQ(cache_read.primary_photo().image(), "image");
@@ -127,9 +131,12 @@ TEST_F(AmbientPhotoCacheTest, SetsDataToEmptyStringWhenFilesMissing) {
     base::RunLoop loop;
     ambient::PhotoCacheEntry cache_read;
     photo_cache()->ReadPhotoCache(
-        /*cache_index=*/1, &cache_read,
-        base::BindOnce([](base::OnceClosure done) { std::move(done).Run(); },
-                       loop.QuitClosure()));
+        /*cache_index=*/1,
+        base::BindLambdaForTesting(
+            [&cache_read, &loop](ambient::PhotoCacheEntry cache_entry_in) {
+              cache_read = std::move(cache_entry_in);
+              loop.Quit();
+            }));
     loop.Run();
 
     EXPECT_TRUE(cache_read.primary_photo().image().empty());

ash/ambient/ambient_photo_controller.cc

@@ -328,9 +328,9 @@ void AmbientPhotoController::TryReadPhotoFromCache() {
              << backup_cache_index_for_display_;
     // Try to read a backup image.
     backup_photo_cache_->ReadPhotoCache(
-        /*cache_index=*/backup_cache_index_for_display_, &cache_entry_,
-        base::BindOnce(&AmbientPhotoController::OnAllPhotoRawDataAvailable,
-                       weak_factory_.GetWeakPtr(), /*from_downloading=*/false));
+        /*cache_index=*/backup_cache_index_for_display_,
+        base::BindOnce(&AmbientPhotoController::OnPhotoCacheReadComplete,
+                       weak_factory_.GetWeakPtr()));
 
     backup_cache_index_for_display_++;
     if (backup_cache_index_for_display_ == GetBackupPhotoUrls().size())
@@ -347,9 +347,15 @@ void AmbientPhotoController::TryReadPhotoFromCache() {
 
   DVLOG(3) << "Read from cache index: " << current_cache_index;
   photo_cache_->ReadPhotoCache(
-      current_cache_index, &cache_entry_,
-      base::BindOnce(&AmbientPhotoController::OnAllPhotoRawDataAvailable,
-                     weak_factory_.GetWeakPtr(), /*from_downloading=*/false));
+      current_cache_index,
+      base::BindOnce(&AmbientPhotoController::OnPhotoCacheReadComplete,
+                     weak_factory_.GetWeakPtr()));
+}
+
+void AmbientPhotoController::OnPhotoCacheReadComplete(
+    ::ambient::PhotoCacheEntry cache_entry) {
+  cache_entry_ = std::move(cache_entry);
+  OnAllPhotoRawDataAvailable(/*from_downloading=*/false);
 }
 
 void AmbientPhotoController::OnPhotoRawDataDownloaded(

ash/ambient/ambient_photo_controller.h

@@ -169,6 +169,8 @@ class ASH_EXPORT AmbientPhotoController : public AmbientViewDelegateObserver {
 
   void TryReadPhotoFromCache();
 
+  void OnPhotoCacheReadComplete(::ambient::PhotoCacheEntry cache_entry);
+
   void OnPhotoRawDataDownloaded(bool is_related_image,
                                 base::RepeatingClosure on_done,
                                 std::string&& data);

ash/ambient/test/ambient_ash_test_base.cc

@@ -114,17 +114,16 @@ class TestAmbientPhotoCacheImpl : public AmbientPhotoCache {
     std::move(callback).Run();
   }
 
-  void ReadPhotoCache(int cache_index,
-                      ::ambient::PhotoCacheEntry* cache_entry,
-                      base::OnceCallback<void()> callback) override {
+  void ReadPhotoCache(
+      int cache_index,
+      base::OnceCallback<void(::ambient::PhotoCacheEntry)> callback) override {
     auto it = files_.find(cache_index);
     if (it == files_.end()) {
-      std::move(callback).Run();
+      std::move(callback).Run(::ambient::PhotoCacheEntry());
       return;
     }
 
-    *cache_entry = it->second;
-    std::move(callback).Run();
+    std::move(callback).Run(it->second);
   }
 
   void Clear() override {