arc: Update app icons on Android framework update.

This detects framework version change and invalidate icons for installed
packages.
This also fixes flakiness in unit test when installing app icons and
deleting invalidated icons could happen from different concurrent threads.

TEST=Locally, unit test
BUG=959028

Change-Id: Ifb2b9a857a07275df401fc9df988cf931219ba4a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1610845Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Commit-Queue: Yury Khmel <khmel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659752}

chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc

@@ -14,6 +14,7 @@
 #include "base/files/file_util.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/sequenced_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task/post_task.h"
@@ -51,6 +52,7 @@
 namespace {
 
 constexpr char kActivity[] = "activity";
+constexpr char kFrameworkPackageName[] = "android";
 constexpr char kIconResourceId[] = "icon_resource_id";
 constexpr char kIconVersion[] = "icon_version";
 constexpr char kInstallTime[] = "install_time";
@@ -231,6 +233,8 @@ void ArcAppListPrefs::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterDictionaryPref(arc::prefs::kArcApps);
   registry->RegisterDictionaryPref(arc::prefs::kArcPackages);
+  registry->RegisterIntegerPref(arc::prefs::kArcFrameworkVersion,
+                                -1 /* default_value */);
   registry->RegisterDictionaryPref(
       arc::prefs::kArcSetNotificationsEnabledDeferred);
   ArcDefaultAppList::RegisterProfilePrefs(registry);
@@ -350,6 +354,9 @@ ArcAppListPrefs::ArcAppListPrefs(
     : profile_(profile),
       prefs_(profile->GetPrefs()),
       app_connection_holder_for_testing_(app_connection_holder_for_testing),
+      file_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
+          {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
       weak_ptr_factory_(this) {
   VLOG(1) << "ARC app list prefs created";
   DCHECK(profile);
@@ -1254,6 +1261,20 @@ void ArcAppListPrefs::AddOrUpdatePackagePrefs(
     package_dict->RemoveKey(kPermissions);
   }
 
+  // TODO (crbug.com/xxxxx): Remove in M78. This is required to force updating
+  // icons for all packages in case framework version is changed. Prior to this
+  // change |InvalidatePackageIcons| for framework did not refresh all packages.
+  if (package_name == kFrameworkPackageName) {
+    const int last_framework_version =
+        profile_->GetPrefs()->GetInteger(arc::prefs::kArcFrameworkVersion);
+    if (last_framework_version != package.package_version) {
+      InvalidatePackageIcons(package_name);
+      profile_->GetPrefs()->SetInteger(arc::prefs::kArcFrameworkVersion,
+                                       package.package_version);
+    }
+    return;
+  }
+
   if (old_package_version == -1 ||
       old_package_version == package.package_version) {
     return;
@@ -1381,14 +1402,22 @@ void ArcAppListPrefs::InvalidateAppIcons(const std::string& app_id) {
 }
 
 void ArcAppListPrefs::InvalidatePackageIcons(const std::string& package_name) {
+  if (package_name == kFrameworkPackageName) {
+    VLOG(1)
+        << "Android framework was changed, refreshing icons for all packages";
+    for (const auto& package_name_to_invalidate : GetPackagesFromPrefs()) {
+      if (package_name_to_invalidate != kFrameworkPackageName)
+        InvalidatePackageIcons(package_name_to_invalidate);
+    }
+  }
   for (const std::string& app_id : GetAppsForPackage(package_name))
     InvalidateAppIcons(app_id);
 }
 
 void ArcAppListPrefs::ScheduleAppFolderDeletion(const std::string& app_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  base::PostTaskWithTraits(
-      FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
+  file_task_runner_->PostTask(
+      FROM_HERE,
       base::BindOnce(&DeleteAppFolderFromFileThread, GetAppPath(app_id)));
 }
 
@@ -1740,8 +1769,8 @@ void ArcAppListPrefs::InstallIcon(const std::string& app_id,
                                   const ArcAppIconDescriptor& descriptor,
                                   const std::vector<uint8_t>& content_png) {
   const base::FilePath icon_path = GetIconPath(app_id, descriptor);
-  base::PostTaskWithTraitsAndReplyWithResult(
-      FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(), FROM_HERE,
       base::BindOnce(&InstallIconFromFileThread, icon_path, content_png),
       base::BindOnce(&ArcAppListPrefs::OnIconInstalled,
                      weak_ptr_factory_.GetWeakPtr(), app_id, descriptor));

chrome/browser/ui/app_list/arc/arc_app_list_prefs.h

@@ -17,6 +17,7 @@
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/optional.h"
@@ -40,6 +41,10 @@ template <typename InstanceType, typename HostType>
 class ConnectionHolder;
 }  // namespace arc
 
+namespace base {
+class SequencedTaskRunner;
+}  // namespace base
+
 namespace content {
 class BrowserContext;
 }  // namespace content
@@ -549,6 +554,8 @@ class ArcAppListPrefs : public KeyedService,
   base::OneShotTimer detect_default_app_availability_timeout_;
   // Set of currently installing apps_.
   std::unordered_set<std::string> apps_installations_;
+  // To execute file operations in sequence.
+  scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
 
   arc::ArcPackageSyncableService* sync_service_ = nullptr;
 

chrome/browser/ui/app_list/arc/arc_app_unittest.cc

@@ -78,6 +78,10 @@
 namespace {
 
 constexpr char kTestPackageName[] = "fake.package.name2";
+constexpr char kFrameworkPackageName[] = "android";
+
+constexpr int kFrameworkNycVersion = 25;
+constexpr int kFrameworkPiVersion = 28;
 
 class FakeAppIconLoaderDelegate : public AppIconLoaderDelegate {
  public:
@@ -142,15 +146,17 @@ void WaitForIconCreation(ArcAppListPrefs* prefs,
   } while (!base::PathExists(icon_path));
 }
 
-void WaitForIconUpdates(Profile* profile,
-                        const std::string& app_id,
-                        size_t expected_updates) {
+void WaitForIconUpdates(Profile* profile, const std::string& app_id) {
   FakeAppIconLoaderDelegate delegate;
   ArcAppIconLoader icon_loader(
       profile, app_list::AppListConfig::instance().grid_icon_dimension(),
       &delegate);
+
+  // |FetchImage| ensures all supported factors are loaded.
+  const std::vector<ui::ScaleFactor>& scale_factors =
+      ui::GetSupportedScaleFactors();
   icon_loader.FetchImage(app_id);
-  delegate.WaitForIconUpdates(expected_updates);
+  delegate.WaitForIconUpdates(scale_factors.size());
 }
 
 enum class ArcState {
@@ -453,11 +459,17 @@ class ArcAppModelBuilderTest : public extensions::ExtensionServiceTestBase,
   }
 
   arc::mojom::ArcPackageInfoPtr CreatePackage(const std::string& package_name) {
+    return CreatePackageWithVersion(package_name, 1 /* package_version */);
+  }
+
+  arc::mojom::ArcPackageInfoPtr CreatePackageWithVersion(
+      const std::string& package_name,
+      int package_version) {
     base::flat_map<arc::mojom::AppPermission, bool> permissions;
     permissions.insert(std::make_pair(arc::mojom::AppPermission::CAMERA, 0));
     permissions.insert(std::make_pair(arc::mojom::AppPermission::LOCATION, 1));
     return arc::mojom::ArcPackageInfo::New(
-        package_name, 1 /* package_version */, 1 /* last_backup_android_id */,
+        package_name, package_version, 1 /* last_backup_android_id */,
         1 /* last_backup_time */, true /* sync */, false /* system */,
         false /* vpn_provider */, nullptr /* web_app_info */, permissions);
   }
@@ -572,17 +584,14 @@ class ArcAppModelIconTest : public ArcAppModelBuilderRecreate {
     DCHECK(prefs);
 
     app_instance()->SendRefreshAppList({app});
-    auto package = CreatePackage(app.package_name);
-    package->package_version = package_version;
-    AddPackage(std::move(package));
+    AddPackage(CreatePackageWithVersion(app.package_name, package_version));
     return app_id;
   }
 
   // Simulates package of the test app is updated.
   void UpdatePackage(int package_version) {
     const arc::mojom::AppInfo app = test_app();
-    auto package = CreatePackage(app.package_name);
-    package->package_version = package_version;
+    auto package = CreatePackageWithVersion(app.package_name, package_version);
     app_instance()->SendPackageAppListRefreshed(package->package_name, {app});
     app_instance()->SendPackageModified(std::move(package));
   }
@@ -602,7 +611,7 @@ class ArcAppModelIconTest : public ArcAppModelBuilderRecreate {
     ASSERT_EQ(GetAppListIconDimensionForScaleFactor(ui::SCALE_FACTOR_200P),
               icon_requests[1]->dimension());
 
-    WaitForIconUpdates(profile_.get(), app_id, 2);
+    WaitForIconUpdates(profile_.get(), app_id);
   }
 
   arc::mojom::AppInfo test_app() const { return fake_apps()[0]; }
@@ -734,8 +743,8 @@ TEST_P(ArcAppModelBuilderTest, ArcPackagePref) {
   RemovePackage(kTestPackageName);
   ValidateHavePackages(fake_packages());
 
-  auto package = CreatePackage(kTestPackageName);
-  package->package_version = 2;
+  auto package =
+      CreatePackageWithVersion(kTestPackageName, 2 /* package_version */);
   package->last_backup_android_id = 2;
   package->last_backup_time = 2;
   AddPackage(package);
@@ -1122,9 +1131,10 @@ TEST_P(ArcAppModelBuilderTest, RequestShortcutIcons) {
   std::set<int> expected_dimensions;
   ArcAppItem* app_item = FindArcItem(ArcAppTest::GetAppId(shortcut));
   ASSERT_NE(nullptr, app_item);
+  WaitForIconUpdates(profile_.get(), app_item->id());
+
   const std::vector<ui::ScaleFactor>& scale_factors =
       ui::GetSupportedScaleFactors();
-  WaitForIconUpdates(profile_.get(), app_item->id(), scale_factors.size());
   for (auto& scale_factor : scale_factors) {
     expected_dimensions.insert(
         GetAppListIconDimensionForScaleFactor(scale_factor));
@@ -1224,7 +1234,7 @@ TEST_P(ArcAppModelBuilderTest, InstallIcon) {
   // This initiates async loading.
   app_item->icon().GetRepresentation(scale);
 
-  WaitForIconUpdates(profile_.get(), app_id, 1);
+  WaitForIconUpdates(profile_.get(), app_id);
 
   // Validate that icons are installed, have right content and icon is
   // refreshed for ARC app item.
@@ -1260,7 +1270,7 @@ TEST_P(ArcAppModelBuilderTest, RemoveAppCleanUpFolder) {
   const base::FilePath app_path = prefs->GetAppPath(app_id);
 
   // Now send generated icon for the app.
-  WaitForIconUpdates(profile_.get(), app_id, 1);
+  WaitForIconUpdates(profile_.get(), app_id);
   EXPECT_TRUE(IsIconCreated(prefs, app_id, scale_factor));
 
   // Send empty app list. This will delete app and its folder.
@@ -2021,10 +2031,7 @@ TEST_P(ArcAppModelIconTest, IconInvalidation) {
   ASSERT_TRUE(prefs);
 
   const std::string app_id = StartApp(1 /* package_version */);
-  prefs->MaybeRequestIcon(app_id,
-                          GetAppListIconDescriptor(ui::SCALE_FACTOR_100P));
-
-  WaitForIconUpdates(profile_.get(), app_id, 1);
+  WaitForIconUpdates(profile_.get(), app_id);
 
   // Simulate ARC restart.
   RestartArc();
@@ -2046,6 +2053,49 @@ TEST_P(ArcAppModelIconTest, IconInvalidation) {
   EXPECT_TRUE(app_instance()->icon_requests().empty());
 }
 
+TEST_P(ArcAppModelIconTest, IconInvalidationOnFrameworkUpdate) {
+  ArcAppListPrefs* const prefs = ArcAppListPrefs::Get(profile_.get());
+  ASSERT_TRUE(prefs);
+
+  const arc::mojom::AppInfo app = test_app();
+  const std::string app_id = ArcAppTest::GetAppId(app);
+  app_instance()->SendRefreshAppList({app});
+
+  std::vector<arc::mojom::ArcPackageInfoPtr> packages;
+  packages.emplace_back(CreatePackage(app.package_name));
+  packages.emplace_back(
+      CreatePackageWithVersion(kFrameworkPackageName, kFrameworkNycVersion));
+  app_instance()->SendRefreshPackageList(std::move(packages));
+
+  WaitForIconUpdates(profile_.get(), app_id);
+
+  RestartArc();
+
+  app_instance()->SendRefreshAppList({app});
+
+  // Framework is the same, no update.
+  packages.emplace_back(CreatePackage(app.package_name));
+  packages.emplace_back(
+      CreatePackageWithVersion(kFrameworkPackageName, kFrameworkNycVersion));
+  app_instance()->SendRefreshPackageList(std::move(packages));
+
+  EXPECT_TRUE(app_instance()->icon_requests().empty());
+
+  RestartArc();
+
+  app_instance()->SendRefreshAppList({app});
+
+  // Framework was updated, app icons should be updated even app's package is
+  // the same.
+  packages.emplace_back(CreatePackage(app.package_name));
+  packages.emplace_back(
+      CreatePackageWithVersion(kFrameworkPackageName, kFrameworkPiVersion));
+  app_instance()->SendRefreshPackageList(std::move(packages));
+
+  EXPECT_FALSE(app_instance()->icon_requests().empty());
+  EnsureIconsUpdated();
+}
+
 // This verifies that app icons are invalidated in case icon version was
 // changed which means ARC sends icons using updated processing.
 TEST_P(ArcAppModelIconTest, IconInvalidationOnIconVersionUpdate) {

components/arc/arc_prefs.cc

@@ -75,6 +75,9 @@ const char kArcProvisioningInitiatedFromOobe[] =
 const char kArcFastAppReinstallStarted[] = "arc.fast.app.reinstall.started";
 // A preference to keep list of Play Fast App Reinstall packages.
 const char kArcFastAppReinstallPackages[] = "arc.fast.app.reinstall.packages";
+// A preference to keep the current Android framework version. Note, that value
+// is only available after first packages update.
+const char kArcFrameworkVersion[] = "arc.framework.version";
 // A preference that holds the list of apps that the admin requested to be
 // push-installed.
 const char kArcPushInstallAppsRequested[] = "arc.push_install.requested";

components/arc/arc_prefs.h

@@ -23,6 +23,7 @@ ARC_EXPORT extern const char kArcDataRemoveRequested[];
 ARC_EXPORT extern const char kArcEnabled[];
 ARC_EXPORT extern const char kArcFastAppReinstallPackages[];
 ARC_EXPORT extern const char kArcFastAppReinstallStarted[];
+ARC_EXPORT extern const char kArcFrameworkVersion[];
 ARC_EXPORT extern const char kArcHasAccessToRemovableMedia[];
 ARC_EXPORT extern const char kArcInitialSettingsPending[];
 ARC_EXPORT extern const char kArcLocationServiceEnabled[];