ambient: Fix potential Settings timing issue

Currently in Settings page, the request settings and update settings
calls are independently, which could make local cache |settings_| out of
sync of server side.

Two potential timing issues are:
1. Call UpdateSettings(), and immediately call
   RequestSettingsAndAlbums(). It is possible that
   RequestSettingsAndAlbums() reads stale settings on server earlier
   than the server is updated. When RequestSettingsAndAlbums() returns,
   it will overwrite local |settings_| with old value, but the server
   got updated with new value.
2. Vice versa, call RequestSettingsAndAlbums(), and then immediately
   call UpdateSettings(). RequestSettingsAndAlbums() returns with old
   value and overwrite local |settings_| with old value.

For example:
1. On album selection page (ambientMode/photos page), user makes changes
   to select different albums. It will change local cache |settings_|
   to the selected albums list. And call update settings to server.
2. Immediately, user goes back to ambientMode page, current code will
   trigger requesting settings from server.
3. It is not guarantee that update settings will update server before
   request settings, so if request settings succeeds earlier, it will
   return stale data and overwrite local |settings_|.

The major changes in this patch:
1. When request settings, if there is ongoing UpdateSettings(), we do
   not call RequestSettingsAndAlbums(). We update the UI when
   UpdateSettings() returns.
2. When update settings, if there is ongoing RequestSettingsAndAlbums(),
   we invalidate the weakPtr to prevent changing local settings, and
   call UpdateSettings().
3. If UpdateSettings() fails, will update UI with previous settings
   data.

Bug: b/169448409
Test: Added new unittests
Change-Id: Icfc156fba22087b6b438464d7ac9a711a31a390c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2458951Reviewed-by: Jimmy Gong <jimmyxgong@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816789}

chrome/browser/ui/webui/settings/chromeos/ambient_mode_handler.cc

@@ -241,7 +241,6 @@ void AmbientModeHandler::HandleSetSelectedAlbums(const base::ListValue* args) {
   }
 
   UpdateSettings();
-  // TODO(wutao): Undate the UI when success in OnUpdateSettings.
   SendTopicSource();
 }
 
@@ -314,6 +313,10 @@ void AmbientModeHandler::SendAlbumPreview(
 }
 
 void AmbientModeHandler::UpdateSettings() {
+  // Prevent fetch settings callback changing |settings_| and |personal_albums_|
+  // while updating.
+  ui_update_weak_factory_.InvalidateWeakPtrs();
+
   if (is_updating_backend_) {
     has_pending_updates_for_backend_ = true;
     return;
@@ -323,6 +326,7 @@ void AmbientModeHandler::UpdateSettings() {
   is_updating_backend_ = true;
 
   DCHECK(settings_);
+  settings_sent_for_update_ = settings_;
   ash::AmbientBackendController::Get()->UpdateSettings(
       *settings_, base::BindOnce(&AmbientModeHandler::OnUpdateSettings,
                                  backend_weak_factory_.GetWeakPtr()));
@@ -333,25 +337,72 @@ void AmbientModeHandler::OnUpdateSettings(bool success) {
 
   if (success) {
     update_settings_retry_backoff_.Reset();
+    cached_settings_ = settings_sent_for_update_;
   } else {
     update_settings_retry_backoff_.InformOfRequest(/*succeeded=*/false);
-    if (update_settings_retry_backoff_.failure_count() > kMaxRetries)
-      return;
   }
 
-  if (has_pending_updates_for_backend_ || !success) {
-    const base::TimeDelta kDelay =
-        update_settings_retry_backoff_.GetTimeUntilRelease();
-    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE,
-        base::BindOnce(&AmbientModeHandler::UpdateSettings,
-                       backend_weak_factory_.GetWeakPtr()),
-        kDelay);
-  }
+  if (MaybeScheduleNewUpdateSettings(success))
+    return;
+
+  UpdateUIWithCachedSettings(success);
+}
+
+bool AmbientModeHandler::MaybeScheduleNewUpdateSettings(bool success) {
+  // If it was unsuccessful to update settings, but have not reached
+  // |kMaxRetries|, then it will retry.
+  const bool need_retry_update_settings_at_backend =
+      !success && update_settings_retry_backoff_.failure_count() <= kMaxRetries;
+
+  // If there has pending updates or need to retry, then updates settings again.
+  const bool should_update_settings_at_backend =
+      has_pending_updates_for_backend_ || need_retry_update_settings_at_backend;
+
+  if (!should_update_settings_at_backend)
+    return false;
+
+  const base::TimeDelta kDelay =
+      update_settings_retry_backoff_.GetTimeUntilRelease();
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&AmbientModeHandler::UpdateSettings,
+                     backend_weak_factory_.GetWeakPtr()),
+      kDelay);
+  return true;
+}
+
+void AmbientModeHandler::UpdateUIWithCachedSettings(bool success) {
+  // If it was unsuccessful to update settings with |kMaxRetries|, need to
+  // restore to cached settings.
+  const bool should_restore_previous_settings =
+      !success && update_settings_retry_backoff_.failure_count() > kMaxRetries;
+
+  // Otherwise, if there has pending fetching request or need to restore
+  // cached settings, then updates the WebUi.
+  const bool should_update_web_ui =
+      has_pending_fetch_request_ || should_restore_previous_settings;
+
+  if (!should_update_web_ui)
+    return;
+
+  OnSettingsAndAlbumsFetched(last_fetch_request_topic_source_, cached_settings_,
+                             std::move(personal_albums_));
+  has_pending_fetch_request_ = false;
+  last_fetch_request_topic_source_ = base::nullopt;
 }
 
 void AmbientModeHandler::RequestSettingsAndAlbums(
     base::Optional<ash::AmbientModeTopicSource> topic_source) {
+  last_fetch_request_topic_source_ = topic_source;
+
+  // If there is an ongoing update, do not request. If update succeeds, it will
+  // update the UI with the new settings. If update fails, it will restore
+  // previous settings and update UI.
+  if (is_updating_backend_) {
+    has_pending_fetch_request_ = true;
+    return;
+  }
+
   // TODO(b/161044021): Add a helper function to get all the albums. Currently
   // only load 100 latest modified albums.
   ash::AmbientBackendController::Get()->FetchSettingsAndAlbums(
@@ -382,6 +433,7 @@ void AmbientModeHandler::OnSettingsAndAlbumsFetched(
 
   fetch_settings_retry_backoff_.Reset();
   settings_ = settings;
+  cached_settings_ = settings;
   personal_albums_ = std::move(personal_albums);
   SyncSettingsAndAlbums();
 
@@ -454,7 +506,6 @@ void AmbientModeHandler::MaybeUpdateTopicSource(
 
   settings_->topic_source = topic_source;
   UpdateSettings();
-  // TODO(wutao): Undate the UI when success in OnUpdateSettings.
   SendTopicSource();
 }
 

chrome/browser/ui/webui/settings/chromeos/ambient_mode_handler.h

@@ -79,8 +79,16 @@ class AmbientModeHandler : public ::settings::SettingsPageUIHandler {
   void UpdateSettings();
 
   // Called when the settings is updated.
+  // |success| is true when update successfully.
   void OnUpdateSettings(bool success);
 
+  // Return true if a new update needed.
+  // |success| is true when update successfully.
+  bool MaybeScheduleNewUpdateSettings(bool success);
+
+  // |success| is true when update successfully.
+  void UpdateUIWithCachedSettings(bool success);
+
   // Will be called from ambientMode/photos subpage and ambientMode subpage.
   // |topic_source| is used to request the albums in that source and identify
   // the callers:
@@ -113,13 +121,30 @@ class AmbientModeHandler : public ::settings::SettingsPageUIHandler {
 
   ash::ArtSetting* FindArtAlbumById(const std::string& album_id);
 
+  // Local settings which may contain changes from WebUI but have not sent to
+  // server.
   base::Optional<ash::AmbientSettings> settings_;
 
+  // The cached settings from the server. Should be the same as the server side.
+  // This value will be updated when RequestSettingsAndAlbums() and
+  // UpdateSettings() return successfully.
+  // If UpdateSettings() fails, will restore to this value.
+  base::Optional<ash::AmbientSettings> cached_settings_;
+
+  // A temporary settings sent to the server in UpdateSettings().
+  base::Optional<ash::AmbientSettings> settings_sent_for_update_;
+
   ash::PersonalAlbums personal_albums_;
 
   // Backoff retries for RequestSettingsAndAlbums().
   net::BackoffEntry fetch_settings_retry_backoff_;
 
+  // Whether to update UI when UpdateSettings() returns successfully.
+  bool has_pending_fetch_request_ = false;
+
+  // The topic source in the latest RequestSettingsAndAlbums().
+  base::Optional<ash::AmbientModeTopicSource> last_fetch_request_topic_source_;
+
   // Whether the Settings updating is ongoing.
   bool is_updating_backend_ = false;