Introduce a timer to evict cached AboutResource data.

AboutResource has an internal caching mechanism, that is only refreshed when
UpdateAboutResource is called. With the introduction of team drives
UpdateAboutResource is no longer called to get the largest change id, which
means that the cached version can become stale. Here we introduce a timer that
will periodically evict that cached version, so that calls to GetAboutResource
will get fresh data from the server.

Bug: 874551
Change-Id: Ib0ed943b2abec95c6d32be17228dd429ffedaea8
Reviewed-on: https://chromium-review.googlesource.com/1189684
Commit-Queue: Stuart Langley <slangley@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586191}

components/drive/about_resource_loader_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/command_line.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/drive/chromeos/drive_test_util.h"
 #include "components/drive/chromeos/file_cache.h"
@@ -18,15 +19,30 @@
 #include "components/drive/service/fake_drive_service.h"
 #include "components/drive/service/test_util.h"
 #include "components/prefs/testing_pref_service.h"
-#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace drive {
 namespace internal {
 
+namespace {
+
+struct DestroyHelper {
+  template <typename T>
+  void operator()(T* object) const {
+    if (object) {
+      object->Destroy();
+    }
+  }
+};
+
+}  // namespace
+
 class AboutResourceLoaderTest : public testing::Test {
  protected:
   void SetUp() override {
+    task_runner_ = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
+        base::TestMockTimeTaskRunner::Type::kBoundToThread);
+
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
 
     pref_service_ = std::make_unique<TestingPrefServiceSimple>();
@@ -39,23 +55,33 @@ class AboutResourceLoaderTest : public testing::Test {
 
     scheduler_ = std::make_unique<JobScheduler>(
         pref_service_.get(), logger_.get(), drive_service_.get(),
-        base::ThreadTaskRunnerHandle::Get().get(), nullptr);
-    metadata_storage_.reset(new ResourceMetadataStorage(
-        temp_dir_.GetPath(), base::ThreadTaskRunnerHandle::Get().get()));
+        task_runner_.get(), nullptr);
+    metadata_storage_.reset(
+        new ResourceMetadataStorage(temp_dir_.GetPath(), task_runner_.get()));
     ASSERT_TRUE(metadata_storage_->Initialize());
 
     cache_.reset(new FileCache(metadata_storage_.get(), temp_dir_.GetPath(),
-                               base::ThreadTaskRunnerHandle::Get().get(),
+                               task_runner_.get(),
                                nullptr /* free_disk_space_getter */));
     ASSERT_TRUE(cache_->Initialize());
 
-    metadata_.reset(
-        new ResourceMetadata(metadata_storage_.get(), cache_.get(),
-                             base::ThreadTaskRunnerHandle::Get().get()));
+    metadata_.reset(new ResourceMetadata(metadata_storage_.get(), cache_.get(),
+                                         task_runner_.get()));
     ASSERT_EQ(FILE_ERROR_OK, metadata_->Initialize());
 
-    about_resource_loader_ =
-        std::make_unique<AboutResourceLoader>(scheduler_.get());
+    about_resource_loader_ = std::make_unique<AboutResourceLoader>(
+        scheduler_.get(), task_runner_->GetMockTickClock());
+  }
+
+  void TearDown() override {
+    // We need to manually reset the objects that implement the Destroy idiom,
+    // that deletes the object on the |task_runner_|. This is simpler than
+    // introducing custom deleters that capture the |task_runner_| and
+    // invoke RunUntilIdle().
+    metadata_.reset();
+    cache_.reset();
+    metadata_storage_.reset();
+    task_runner_->RunUntilIdle();
   }
 
   // Adds a new file to the root directory of the service.
@@ -68,21 +94,20 @@ class AboutResourceLoaderTest : public testing::Test {
         title,
         false,  // shared_with_me
         google_apis::test_util::CreateCopyResultCallback(&error, &entry));
-    base::RunLoop().RunUntilIdle();
+    task_runner_->RunUntilIdle();
     EXPECT_EQ(google_apis::HTTP_CREATED, error);
     return entry;
   }
 
-  content::TestBrowserThreadBundle thread_bundle_;
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
   base::ScopedTempDir temp_dir_;
   std::unique_ptr<TestingPrefServiceSimple> pref_service_;
   std::unique_ptr<EventLogger> logger_;
   std::unique_ptr<FakeDriveService> drive_service_;
   std::unique_ptr<JobScheduler> scheduler_;
-  std::unique_ptr<ResourceMetadataStorage, test_util::DestroyHelperForTests>
-      metadata_storage_;
-  std::unique_ptr<FileCache, test_util::DestroyHelperForTests> cache_;
-  std::unique_ptr<ResourceMetadata, test_util::DestroyHelperForTests> metadata_;
+  std::unique_ptr<ResourceMetadataStorage, DestroyHelper> metadata_storage_;
+  std::unique_ptr<FileCache, DestroyHelper> cache_;
+  std::unique_ptr<ResourceMetadata, DestroyHelper> metadata_;
   std::unique_ptr<AboutResourceLoader> about_resource_loader_;
 };
 
@@ -100,7 +125,7 @@ TEST_F(AboutResourceLoaderTest, AboutResourceLoader) {
   about_resource_loader_->GetAboutResource(
       google_apis::test_util::CreateCopyResultCallback(error + 1, about + 1));
 
-  base::RunLoop().RunUntilIdle();
+  task_runner_->RunUntilIdle();
   EXPECT_EQ(google_apis::HTTP_SUCCESS, error[0]);
   EXPECT_EQ(google_apis::HTTP_SUCCESS, error[1]);
   const int64_t first_changestamp = about[0]->largest_change_id();
@@ -120,7 +145,7 @@ TEST_F(AboutResourceLoaderTest, AboutResourceLoader) {
   about_resource_loader_->GetAboutResource(
       google_apis::test_util::CreateCopyResultCallback(error + 3, about + 3));
 
-  base::RunLoop().RunUntilIdle();
+  task_runner_->RunUntilIdle();
   EXPECT_EQ(google_apis::HTTP_SUCCESS, error[2]);
   EXPECT_EQ(google_apis::HTTP_SUCCESS, error[3]);
   EXPECT_EQ(first_changestamp + 1, about[2]->largest_change_id());
@@ -138,7 +163,7 @@ TEST_F(AboutResourceLoaderTest, AboutResourceLoader) {
   about_resource_loader_->UpdateAboutResource(
       google_apis::test_util::CreateCopyResultCallback(error + 5, about + 5));
 
-  base::RunLoop().RunUntilIdle();
+  task_runner_->RunUntilIdle();
   EXPECT_EQ(google_apis::HTTP_NO_CONTENT, error[4]);
   EXPECT_EQ(google_apis::HTTP_SUCCESS, error[5]);
   EXPECT_EQ(first_changestamp + 1, about[4]->largest_change_id());
@@ -150,5 +175,29 @@ TEST_F(AboutResourceLoaderTest, AboutResourceLoader) {
   EXPECT_EQ(3, drive_service_->about_resource_load_count());
 }
 
+TEST_F(AboutResourceLoaderTest, EvictCache) {
+  google_apis::DriveApiErrorCode error;
+  std::unique_ptr<google_apis::AboutResource> about;
+
+  // No resource is cached at the beginning.
+  ASSERT_FALSE(about_resource_loader_->cached_about_resource());
+
+  // Since no resource is cached, this "Get" should trigger the update.
+  about_resource_loader_->GetAboutResource(
+      google_apis::test_util::CreateCopyResultCallback(&error, &about));
+
+  task_runner_->RunUntilIdle();
+  EXPECT_EQ(google_apis::HTTP_SUCCESS, error);
+
+  // Should have a cached resource.
+  ASSERT_TRUE(about_resource_loader_->cached_about_resource());
+
+  // Advance the timer should evict the cache.
+  task_runner_->FastForwardUntilNoTasksRemain();
+
+  // Cache should be evicted.
+  ASSERT_FALSE(about_resource_loader_->cached_about_resource());
+}
+
 }  // namespace internal
 }  // namespace drive

components/drive/chromeos/about_resource_loader.cc

@@ -13,10 +13,23 @@
 namespace drive {
 namespace internal {
 
-AboutResourceLoader::AboutResourceLoader(JobScheduler* scheduler)
+namespace {
+// The time period that we will cache the result of UpdateAboutResource.
+constexpr base::TimeDelta kCacheEvictionTimeout =
+    base::TimeDelta::FromMinutes(1);
+}  // namespace
+
+AboutResourceLoader::AboutResourceLoader(JobScheduler* scheduler,
+                                         const base::TickClock* clock)
     : scheduler_(scheduler),
       current_update_task_id_(-1),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  cache_eviction_timer_ = std::make_unique<base::RetainingOneShotTimer>(
+      FROM_HERE, kCacheEvictionTimeout,
+      base::BindRepeating(&AboutResourceLoader::EvictCachedAboutResource,
+                          base::Unretained(this)),
+      clock);
+}
 
 AboutResourceLoader::~AboutResourceLoader() = default;
 
@@ -81,11 +94,18 @@ void AboutResourceLoader::UpdateAboutResourceAfterGetAbout(
   cached_about_resource_ =
       std::make_unique<google_apis::AboutResource>(*about_resource);
 
+  cache_eviction_timer_->Reset();
+
   for (auto& callback : callbacks) {
     callback.Run(status,
                  std::make_unique<google_apis::AboutResource>(*about_resource));
   }
 }
 
+void AboutResourceLoader::EvictCachedAboutResource() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  cached_about_resource_.reset();
+}
+
 }  // namespace internal
 }  // namespace drive

components/drive/chromeos/about_resource_loader.h

@@ -12,6 +12,8 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
+#include "base/time/default_tick_clock.h"
+#include "base/timer/timer.h"
 #include "google_apis/drive/drive_common_callbacks.h"
 
 namespace drive {
@@ -23,7 +25,10 @@ namespace internal {
 // This class is responsible to load AboutResource from the server and cache it.
 class AboutResourceLoader {
  public:
-  explicit AboutResourceLoader(JobScheduler* scheduler);
+  // |clock| can be injected for testing.
+  explicit AboutResourceLoader(
+      JobScheduler* scheduler,
+      const base::TickClock* clock = base::DefaultTickClock::GetInstance());
   ~AboutResourceLoader();
 
   // Returns the cached about resource.
@@ -54,6 +59,8 @@ class AboutResourceLoader {
       google_apis::DriveApiErrorCode status,
       std::unique_ptr<google_apis::AboutResource> about_resource);
 
+  void EvictCachedAboutResource();
+
   JobScheduler* scheduler_;
   std::unique_ptr<google_apis::AboutResource> cached_about_resource_;
 
@@ -65,6 +72,8 @@ class AboutResourceLoader {
   std::map<int, std::vector<google_apis::AboutResourceCallback>>
       pending_callbacks_;
 
+  std::unique_ptr<base::RetainingOneShotTimer> cache_eviction_timer_;
+
   THREAD_CHECKER(thread_checker_);
 
   base::WeakPtrFactory<AboutResourceLoader> weak_ptr_factory_;