Handle app reinstallation.

This Cl modifies AppActivityRegistry to
correctly handle corner cases where applications
are re-installed by doing the following:
1. If the running active time of an app is non-zero
restore the application when initializing AppActivityRegistry
even if the application is uninstalled.

2. Only remove entries in CleanRegistry if the application
is uninstalled, has no unreported activities and its running
active time is zero.

Bug: 1060340
Change-Id: Idbefe82a686eaa56d31488d305f93c379d7b2bda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2097303
Commit-Queue: Yilkal Abe <yilkal@chromium.org>
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750234}

chrome/browser/chromeos/child_accounts/time_limits/app_activity_registry.cc

@@ -199,8 +199,11 @@ void AppActivityRegistry::OnAppInstalled(const AppId& app_id) {
     // First send the system notifications for the application.
     SendSystemNotificationsForApp(app_id);
 
-    if (GetAppState(app_id) == AppState::kLimitReached)
+    if (GetAppState(app_id) == AppState::kLimitReached) {
       NotifyLimitReached(app_id, /* was_active */ false);
+    } else if (GetAppState(app_id) == AppState::kUninstalled) {
+      OnAppReinstalled(app_id);
+    }
   }
 }
 
@@ -215,9 +218,17 @@ void AppActivityRegistry::OnAppAvailable(const AppId& app_id) {
   if (!base::Contains(activity_registry_, app_id))
     return;
 
-  if (GetAppState(app_id) == AppState::kLimitReached)
+  AppState prev_state = GetAppState(app_id);
+
+  if (prev_state == AppState::kLimitReached)
     return;
 
+  // This may happen in the scenario where the application is uninstalled and
+  // reinstalled in the same session.
+  if (prev_state == AppState::kUninstalled) {
+    OnAppReinstalled(app_id);
+  }
+
   SetAppState(app_id, AppState::kAvailable);
 }
 
@@ -496,6 +507,12 @@ bool AppActivityRegistry::SetAppLimit(
     const AppId& app_id,
     const base::Optional<AppLimit>& app_limit) {
   DCHECK(base::Contains(activity_registry_, app_id));
+
+  // If an application is not installed but present in the registry return
+  // early.
+  if (!IsAppInstalled(app_id))
+    return false;
+
   AppDetails& details = activity_registry_.at(app_id);
 
   // Limit 'data' are considered equal if only the |last_updated_| is different.
@@ -549,6 +566,12 @@ bool AppActivityRegistry::SetAppLimit(
   return updated;
 }
 
+void AppActivityRegistry::SetAppWhitelisted(const AppId& app_id) {
+  if (!base::Contains(activity_registry_, app_id))
+    return;
+  SetAppState(app_id, AppState::kAlwaysAvailable);
+}
+
 void AppActivityRegistry::OnChromeAppActivityChanged(
     ChromeAppActivityState state,
     base::Time timestamp) {
@@ -686,8 +709,7 @@ void AppActivityRegistry::CleanRegistry(base::Time timestamp) {
     info->RemoveActiveTimeEarlierThan(timestamp);
     info->UpdateAppActivityPreference(&entry, /* replace */ true);
 
-    if (info->app_state() == AppState::kUninstalled &&
-        info->active_times().size() == 0) {
+    if (info->ShouldRemoveApp()) {
       // Remove entry in |activity_registry_| if it is present.
       activity_registry_.erase(info->app_id());
 
@@ -704,6 +726,20 @@ void AppActivityRegistry::CleanRegistry(base::Time timestamp) {
   *list_value = base::ListValue(std::move(list_storage));
 }
 
+void AppActivityRegistry::OnAppReinstalled(const AppId& app_id) {
+  DCHECK(base::Contains(activity_registry_, app_id));
+  AppDetails& details = activity_registry_.at(app_id);
+  if (details.IsLimitReached()) {
+    SetAppState(app_id, AppState::kLimitReached);
+  } else {
+    SetAppState(app_id, AppState::kAvailable);
+  }
+
+  // Notify observers.
+  for (auto& observer : app_state_observers_)
+    observer.OnAppInstalled(app_id);
+}
+
 void AppActivityRegistry::Add(const AppId& app_id) {
   activity_registry_[app_id].activity = AppActivity(AppState::kAvailable);
   activity_registry_[app_id].received_app_installed_ = true;
@@ -1017,8 +1053,9 @@ void AppActivityRegistry::InitializeAppActivities() {
   for (const auto& app_info : applications_info) {
     DCHECK(!base::Contains(activity_registry_, app_info.app_id()));
 
-    // Don't restore uninstalled application's data.
-    if (app_info.app_state() == AppState::kUninstalled)
+    // Don't restore uninstalled application's if its running active time is
+    // zero.
+    if (!app_info.ShouldRestoreApp())
       continue;
 
     activity_registry_[app_info.app_id()].activity =

chrome/browser/chromeos/child_accounts/time_limits/app_activity_registry.h

@@ -155,6 +155,9 @@ class AppActivityRegistry : public AppServiceWrapper::EventListener {
   bool SetAppLimit(const AppId& app_id,
                    const base::Optional<AppLimit>& app_limit);
 
+  // Sets the app identified with |app_id| as being always available.
+  void SetAppWhitelisted(const AppId& app_id);
+
   // Reset time has been reached at |timestamp|.
   void OnResetTimeReached(base::Time timestamp);
 
@@ -228,6 +231,10 @@ class AppActivityRegistry : public AppServiceWrapper::EventListener {
     std::vector<SystemNotification> pending_notifications_;
   };
 
+  // OnAppReinstalled is called when an application has been uninstalled and
+  // then installed again before being removed from app registry.
+  void OnAppReinstalled(const AppId& app_id);
+
   // Removes data older than |timestamp| from the registry.
   // Removes entries for uninstalled apps if there is no more relevant activity
   // data left.

chrome/browser/chromeos/child_accounts/time_limits/app_activity_registry_unittest.cc

@@ -715,10 +715,26 @@ TEST_F(AppActivityRegistryTest, RemoveUninstalledApplications) {
           value,
           /* include_app_activity_array */ true);
 
-  // Two apps left. They are Chrome, and kApp2.
-  EXPECT_EQ(app_infos.size(), 2u);
-  for (const auto& entry : app_infos) {
+  EXPECT_EQ(app_infos.size(), 3u);
+  for (const auto& entry : app_infos)
     EXPECT_EQ(entry.active_times().size(), 0u);
+
+  // kApp1 will still be present since it still has activity.
+  registry().OnResetTimeReached(base::Time::Now());
+  registry().SaveAppActivity();
+  registry().OnSuccessfullyReported(base::Time::Now());
+
+  const base::Value* new_value =
+      profile().GetPrefs()->GetList(prefs::kPerAppTimeLimitsAppActivities);
+
+  const std::vector<PersistedAppInfo> final_app_infos =
+      PersistedAppInfo::PersistedAppInfosFromList(
+          new_value,
+          /* include_app_activity_array */ false);
+
+  // Two apps left. They are Chrome, and kApp2.
+  EXPECT_EQ(final_app_infos.size(), 2u);
+  for (const auto& entry : final_app_infos) {
     EXPECT_NE(entry.app_id(), kApp1);
   }
 }
@@ -975,5 +991,52 @@ TEST_F(AppActivityRegistryTest, AvoidRedundantCallsToPauseApp) {
   registry().OnAppDestroyed(kApp1, new_app1_window, base::Time::Now());
 }
 
+TEST_F(AppActivityRegistryTest, AppReinstallations) {
+  AppStateObserverMock state_observer_mock;
+  registry().AddAppStateObserver(&state_observer_mock);
+
+  AppLimit app1_limit(AppRestriction::kTimeLimit, base::TimeDelta::FromHours(1),
+                      base::Time::Now());
+
+  SetAppLimit(kApp1, app1_limit);
+
+  EXPECT_CALL(state_observer_mock,
+              OnAppLimitReached(kApp1, app1_limit.daily_limit().value(),
+                                /* was_active */ true))
+      .Times(1);
+
+  // Application will reach its time limits.
+  CreateAppActivityForApp(kApp1, base::TimeDelta::FromHours(2));
+  registry().OnAppUninstalled(kApp1);
+  registry().SaveAppActivity();
+
+  // Now let's reinstantiate the registry.
+  ReInitializeRegistry();
+  registry().AddAppStateObserver(&state_observer_mock);
+
+  EXPECT_CALL(state_observer_mock, OnAppInstalled(kApp1)).Times(1);
+
+  // The child user reinstalls the application.
+  registry().OnAppInstalled(kApp1);
+  registry().OnAppAvailable(kApp1);
+
+  // Let's set the time limit.
+  EXPECT_CALL(state_observer_mock,
+              OnAppLimitReached(kApp1, app1_limit.daily_limit().value(),
+                                /* was_active */ false))
+      .Times(1);
+  registry().SetAppLimit(kApp1, app1_limit);
+
+  // Reinstalled within the same session.
+  registry().OnAppUninstalled(kApp1);
+  EXPECT_CALL(state_observer_mock, OnAppInstalled(kApp1)).Times(1);
+  EXPECT_CALL(state_observer_mock,
+              OnAppLimitReached(kApp1, app1_limit.daily_limit().value(),
+                                /* was_active */ false))
+      .Times(1);
+
+  registry().OnAppAvailable(kApp1);
+}
+
 }  // namespace app_time
 }  // namespace chromeos

chrome/browser/chromeos/child_accounts/time_limits/app_time_controller.cc

@@ -414,6 +414,16 @@ void AppTimeController::OnAppLimitRemoved(const AppId& app_id) {
 }
 
 void AppTimeController::OnAppInstalled(const AppId& app_id) {
+  const base::Value* whitelist_policy = pref_registrar_->prefs()->GetDictionary(
+      prefs::kPerAppTimeLimitsWhitelistPolicy);
+  if (whitelist_policy && whitelist_policy->is_dict()) {
+    AppTimeLimitsWhitelistPolicyWrapper wrapper(whitelist_policy);
+    if (base::Contains(wrapper.GetWhitelistAppList(), app_id))
+      app_registry_->SetAppWhitelisted(app_id);
+  } else {
+    LOG(WARNING) << " Invalid PerAppTimeLimitWhitelist policy";
+  }
+
   const base::Value* policy =
       pref_registrar_->prefs()->GetDictionary(prefs::kPerAppTimeLimitsPolicy);
 

chrome/browser/chromeos/child_accounts/time_limits/persisted_app_info.cc

@@ -283,5 +283,16 @@ void PersistedAppInfo::RemoveActiveTimeEarlierThan(base::Time timestamp) {
   }
 }
 
+bool PersistedAppInfo::ShouldRestoreApp() const {
+  bool is_installed = app_state() != AppState::kUninstalled;
+  bool has_active_running_time =
+      active_running_time() > base::TimeDelta::FromSeconds(0);
+  return is_installed || has_active_running_time;
+}
+
+bool PersistedAppInfo::ShouldRemoveApp() const {
+  return !ShouldRestoreApp() && active_times().empty();
+}
+
 }  // namespace app_time
 }  // namespace chromeos

chrome/browser/chromeos/child_accounts/time_limits/persisted_app_info.h

@@ -48,6 +48,9 @@ class PersistedAppInfo {
 
   void RemoveActiveTimeEarlierThan(base::Time timestamp);
 
+  bool ShouldRestoreApp() const;
+  bool ShouldRemoveApp() const;
+
   const AppId& app_id() const { return app_id_; }
   AppState app_state() const { return app_state_; }
   base::TimeDelta active_running_time() const { return active_running_time_; }