Actually check for valid ARC app shelf ids before DCHECKing validity.

Bug: 771980
Change-Id: I600e523b71c8c4261c47b12a477cc2d394a222bf
Reviewed-on: https://chromium-review.googlesource.com/944715
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540742}

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

@@ -394,15 +394,16 @@ bool ShowPackageInfo(const std::string& package_name,
 bool IsArcItem(content::BrowserContext* context, const std::string& id) {
   DCHECK(context);
 
-  // Some unit tests use empty id.
-  if (id.empty())
+  // Some unit tests use empty ids, some app ids are not valid ARC app ids.
+  const ArcAppShelfId arc_app_shelf_id = ArcAppShelfId::FromString(id);
+  if (!arc_app_shelf_id.valid())
     return false;
 
   const ArcAppListPrefs* const arc_prefs = ArcAppListPrefs::Get(context);
   if (!arc_prefs)
     return false;
 
-  return arc_prefs->IsRegistered(ArcAppShelfId::FromString(id).app_id());
+  return arc_prefs->IsRegistered(arc_app_shelf_id.app_id());
 }
 
 std::string GetLaunchIntent(const std::string& package_name,

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

@@ -5,6 +5,8 @@
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
 #include "chrome/browser/ui/ash/launcher/arc_app_shelf_id.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -24,14 +26,7 @@ std::string GetPlayStoreInitialLaunchIntent() {
 
 }  // namespace
 
-class ArcAppUtilsTest : public testing::Test {
- public:
-  ArcAppUtilsTest() = default;
-  ~ArcAppUtilsTest() override = default;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ArcAppUtilsTest);
-};
+using ArcAppUtilsTest = testing::Test;
 
 TEST_F(ArcAppUtilsTest, LaunchIntent) {
   const std::string launch_intent = GetPlayStoreInitialLaunchIntent();
@@ -78,3 +73,12 @@ TEST_F(ArcAppUtilsTest, ShelfGroupId) {
   EXPECT_FALSE(shelf_id2.has_shelf_group_id());
   EXPECT_EQ(shelf_id2.app_id(), arc::kPlayStoreAppId);
 }
+
+// Tests that IsArcItem does not crash or DCHECK with invalid crx file ids.
+TEST_F(ArcAppUtilsTest, IsArcItemDoesNotCrashWithInvalidCrxFileIds) {
+  // TestingProfile checks CurrentlyOn(cotnent::BrowserThread::UI).
+  content::TestBrowserThreadBundle thread_bundle;
+  TestingProfile testing_profile;
+  EXPECT_FALSE(arc::IsArcItem(&testing_profile, std::string()));
+  EXPECT_FALSE(arc::IsArcItem(&testing_profile, "ShelfWindowWatcher0"));
+}

chrome/browser/ui/ash/launcher/arc_app_shelf_id.cc

@@ -21,6 +21,8 @@ constexpr char kShelfGroupPrefix[] = "shelf_group:";
 
 }  // namespace
 
+ArcAppShelfId::ArcAppShelfId() = default;
+
 ArcAppShelfId::ArcAppShelfId(const std::string& shelf_group_id,
                              const std::string& app_id)
     : shelf_group_id_(shelf_group_id), app_id_(app_id) {
@@ -33,14 +35,15 @@ ArcAppShelfId::~ArcAppShelfId() = default;
 
 // static
 ArcAppShelfId ArcAppShelfId::FromString(const std::string& id) {
-  if (!base::StartsWith(id, kShelfGroupPrefix, base::CompareCase::SENSITIVE))
+  if (base::StartsWith(id, kShelfGroupPrefix, base::CompareCase::SENSITIVE)) {
+    const std::vector<std::string> parts = base::SplitString(
+        id, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+    if (parts.size() == 3u && crx_file::id_util::IdIsValid(parts[2]))
+      return ArcAppShelfId(parts[1], parts[2]);
+  } else if (crx_file::id_util::IdIsValid(id)) {
     return ArcAppShelfId(std::string(), id);
-
-  const std::vector<std::string> parts = base::SplitString(
-      id, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
-  DCHECK_EQ(3u, parts.size());
-
-  return ArcAppShelfId(parts[1], parts[2]);
+  }
+  return ArcAppShelfId();
 }
 
 // static

chrome/browser/ui/ash/launcher/arc_app_shelf_id.h

@@ -7,19 +7,21 @@
 
 #include <string>
 
-// Represents ARC app shelf id that consists from app id and optional shelf
-// group id.
 namespace arc {
 
+// An ARC app shelf id consisting of an app id (must be a valid crx file id)
+// and an optional shelf group id. Ids with empty app ids are invalid.
 class ArcAppShelfId {
  public:
+  ArcAppShelfId();
   ArcAppShelfId(const std::string& shelf_group_id, const std::string& app_id);
   ArcAppShelfId(const ArcAppShelfId& other);
   ~ArcAppShelfId();
 
-  // Returns id from string representation which has syntax
-  // "shelf_group:some_group_id:some_app_id". In case suffix shelf_group is
-  // absent then id is treated as app id.
+  // Returns an id from a string with syntax "shelf_group:group_id:app_id".
+  // If the shelf_group prefix is absent then the input is treated as an app id.
+  // In either case, if the app_id is not a valid crx file id, then the returned
+  // ArcAppShelfId is empty and considered invalid.
   static ArcAppShelfId FromString(const std::string& id);
 
   // Constructs id from app id and optional shelf_group_id encoded into the
@@ -38,6 +40,8 @@ class ArcAppShelfId {
 
   const std::string& app_id() const { return app_id_; }
 
+  bool valid() const { return !app_id_.empty(); }
+
  private:
   const std::string shelf_group_id_;
   const std::string app_id_;

chrome/browser/ui/ash/launcher/arc_app_shelf_id_unittest.cc

@@ -17,14 +17,7 @@ constexpr char kTestIntentWithShelfGroup[] =
 constexpr char kTestIntentWithoutShelfGroup[] = "#Intent;S.other=tmp;end";
 }  // namespace
 
-class ArcAppShelfIdTest : public testing::Test {
- public:
-  ArcAppShelfIdTest() = default;
-  ~ArcAppShelfIdTest() override = default;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ArcAppShelfIdTest);
-};
+using ArcAppShelfIdTest = testing::Test;
 
 TEST_F(ArcAppShelfIdTest, BaseTestAAA) {
   const arc::ArcAppShelfId shelf_id1(kTestShelfGroupId, kTestAppId);
@@ -62,3 +55,19 @@ TEST_F(ArcAppShelfIdTest, BaseTestAAA) {
   EXPECT_FALSE(shelf_id7.has_shelf_group_id());
   EXPECT_EQ(shelf_id7.app_id(), kTestAppId);
 }
+
+// Tests valid and invalid input to ArcAppShelfId::FromString.
+TEST_F(ArcAppShelfIdTest, FromString) {
+  // A string containing just a valid crx id string yields a valid id.
+  EXPECT_TRUE(arc::ArcAppShelfId::FromString(kTestAppId).valid());
+  // A serialized string with a group id and valid crx id yields a valid id.
+  const arc::ArcAppShelfId id(kTestShelfGroupId, kTestAppId);
+  EXPECT_TRUE(arc::ArcAppShelfId::FromString(id.ToString()).valid());
+
+  // An empty string yields an invalid id.
+  EXPECT_FALSE(arc::ArcAppShelfId::FromString(std::string()).valid());
+  // A string with just a group id yields an invalid id.
+  EXPECT_FALSE(arc::ArcAppShelfId::FromString(kTestShelfGroupId).valid());
+  // A string with arbitrary text (possibly a non-crx id) yields an invalid id.
+  EXPECT_FALSE(arc::ArcAppShelfId::FromString("ShelfWindowWatcher0").valid());
+}