feedv2: ClearAll() fixes

- ClearAll() now clears metadata, including consistency token
- ClearAll() changes are now all applied from within a task. This
  might prevent subtle bugs.

Bug: 1044139
Change-Id: I79087516e914da4705f41b162f27cecafdd38443
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2341808Reviewed-by: Ian Wells <iwells@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796100}

components/feed/core/v2/feed_stream.cc

@@ -98,7 +98,7 @@ class FeedStream::OfflineSuggestionsProvider
 RefreshResponseData FeedStream::WireResponseTranslator::TranslateWireResponse(
     feedwire::Response response,
     StreamModelUpdateRequest::Source source,
-    base::Time current_time) {
+    base::Time current_time) const {
   return ::feed::TranslateWireResponse(std::move(response), source,
                                        current_time);
 }
@@ -570,13 +570,17 @@ void FeedStream::BackgroundRefreshComplete(LoadStreamTask::Result result) {
 }
 
 void FeedStream::ClearAll() {
-  prefs::ClearClientInstanceId(*profile_prefs_);
-  delegate_->ClearAll();
   metrics_reporter_->OnClearAll(clock_->Now() - GetLastFetchTime());
 
   task_queue_.AddTask(std::make_unique<ClearAllTask>(this));
 }
 
+void FeedStream::FinishClearAll() {
+  prefs::ClearClientInstanceId(*profile_prefs_);
+  metadata_.Populate(feedstore::Metadata());
+  delegate_->ClearAll();
+}
+
 void FeedStream::UploadAction(
     feedwire::FeedAction action,
     bool upload_now,

components/feed/core/v2/feed_stream.h

@@ -77,7 +77,7 @@ class FeedStream : public FeedStreamApi,
     virtual RefreshResponseData TranslateWireResponse(
         feedwire::Response response,
         StreamModelUpdateRequest::Source source,
-        base::Time current_time);
+        base::Time current_time) const;
   };
 
   class Metadata {
@@ -229,6 +229,10 @@ class FeedStream : public FeedStreamApi,
   // is not true.
   void TriggerStreamLoad();
 
+  // Only to be called by ClearAllTask. This clears other stream data stored in
+  // memory.
+  void FinishClearAll();
+
   // Returns the model if it is loaded, or null otherwise.
   StreamModel* GetModel() { return model_.get(); }
 
@@ -236,7 +240,7 @@ class FeedStream : public FeedStreamApi,
   const base::TickClock* GetTickClock() const { return tick_clock_; }
   RequestMetadata GetRequestMetadata();
 
-  WireResponseTranslator* GetWireResponseTranslator() const {
+  const WireResponseTranslator* GetWireResponseTranslator() const {
     return wire_response_translator_;
   }
 
@@ -246,7 +250,7 @@ class FeedStream : public FeedStreamApi,
   // loading from network or storage.
   void LoadModelForTesting(std::unique_ptr<StreamModel> model);
   void SetWireResponseTranslatorForTesting(
-      WireResponseTranslator* wire_response_translator) {
+      const WireResponseTranslator* wire_response_translator) {
     wire_response_translator_ = wire_response_translator;
   }
   void SetIdleCallbackForTesting(base::RepeatingClosure idle_callback);
@@ -290,7 +294,7 @@ class FeedStream : public FeedStreamApi,
   FeedStore* store_;
   const base::Clock* clock_;
   const base::TickClock* tick_clock_;
-  WireResponseTranslator* wire_response_translator_;
+  const WireResponseTranslator* wire_response_translator_;
 
   ChromeInfo chrome_info_;
 

components/feed/core/v2/feed_stream_unittest.cc

@@ -346,7 +346,7 @@ class TestWireResponseTranslator : public FeedStream::WireResponseTranslator {
   RefreshResponseData TranslateWireResponse(
       feedwire::Response response,
       StreamModelUpdateRequest::Source source,
-      base::Time current_time) override {
+      base::Time current_time) const override {
     if (injected_response_) {
       if (injected_response_->model_update_request)
         injected_response_->model_update_request->source = source;
@@ -367,7 +367,7 @@ class TestWireResponseTranslator : public FeedStream::WireResponseTranslator {
   bool InjectedResponseConsumed() const { return !injected_response_; }
 
  private:
-  base::Optional<RefreshResponseData> injected_response_;
+  mutable base::Optional<RefreshResponseData> injected_response_;
 };
 
 class FakeRefreshTaskScheduler : public RefreshTaskScheduler {
@@ -1386,8 +1386,29 @@ TEST_F(FeedStreamTest, ClearAllWithNoSurfacesAttachedDoesNotReload) {
   WaitForIdleTaskQueue();
 
   EXPECT_EQ("loading -> 2 slices", surface.DescribeUpdates());
-  // Also check that the storage is cleared.
+}
+
+TEST_F(FeedStreamTest, ClearAllWipesAllState) {
+  // Trigger saving a consistency token, so it can be cleared later.
+  network_.consistency_token = "token-11";
+  stream_->UploadAction(MakeFeedAction(42ul), true, base::DoNothing());
+  // Trigger saving a feed stream, so it can be cleared later.
+  response_translator_.InjectResponse(MakeTypicalInitialModelState());
+  TestSurface surface(stream_.get());
+  WaitForIdleTaskQueue();
+
+  // Enqueue an action, so it can be cleared later.
+  stream_->UploadAction(MakeFeedAction(43ul), false, base::DoNothing());
+
+  // Trigger ClearAll, this should erase everything.
+  stream_->OnCacheDataCleared();
+  WaitForIdleTaskQueue();
+
+  ASSERT_EQ("loading -> 2 slices -> loading -> cant-refresh",
+            surface.DescribeUpdates());
+
   EXPECT_EQ("", DumpStoreState());
+  EXPECT_EQ("", stream_->GetMetadata()->GetConsistencyToken());
 }
 
 TEST_F(FeedStreamTest, StorePendingAction) {
@@ -1852,6 +1873,8 @@ TEST_F(FeedStreamTest, SendsClientInstanceId) {
 
   // Trigger a ClearAll to verify the instance ID changes.
   stream_->OnSignedOut();
+  WaitForIdleTaskQueue();
+
   const std::string new_instance_id =
       stream_->GetRequestMetadata().client_instance_id;
   ASSERT_NE("", new_instance_id);

components/feed/core/v2/tasks/clear_all_task.cc

@@ -23,6 +23,7 @@ void ClearAllTask::Run() {
 
 void ClearAllTask::StoreClearComplete(bool ok) {
   DLOG_IF(ERROR, !ok) << "FeedStore::ClearAll failed";
+  stream_->FinishClearAll();
   if (stream_->HasSurfaceAttached()) {
     stream_->TriggerStreamLoad();
   }