MacPWAs: Fix duplicate bundle creation

The root cause of this bug is that the call to
LSCopyApplicationURLsForBundleIdentifier that is made in
WebAppShortcutCreator::GetAppBundlesByIdUnsorted will not necessarily
find app shims that were created just moments ago (likely there is an
asynchronous indexing that is going on).

To fix this, make WebAppShortcutCreator::GetAppBundlesByIdUnsorted read
through all of the app shims in the path under ~/Applications where
app shims are created, and see if it finds an application that matches
the one we are looking for, based on its Info.plist.

Rather than add another place where Info.plists are manually parsed,
merge all code that reads Info.plists from app bundles, and put all
of the accessors for this data in a single BundleInfoPlist class.
Note that some of the moved code is not well understood by this
author (in particular, IsForCurrentUserDataDir and GetFullProfilePath).

This BundleInfoPlist will be used more extensively in the updated code
to delete zombie bundles and to provide more robust renaming.

Bug: 937703
Change-Id: Ia292840debd9c74024d707f784dceab78443a536
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1500851
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638225}

chrome/browser/web_applications/components/web_app_shortcut_mac.mm

@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include <algorithm>
+#include <list>
 #include <map>
 #include <utility>
 
@@ -203,43 +204,123 @@ NSString* GetPlistPath(const base::FilePath& bundle_path) {
       bundle_path.Append("Contents").Append("Info.plist"));
 }
 
-NSMutableDictionary* ReadPlist(NSString* plist_path) {
-  return [NSMutableDictionary dictionaryWithContentsOfFile:plist_path];
-}
-
-bool HasExistingExtensionShimForDifferentProfile(
-    const base::FilePath& destination_directory,
-    const std::string& extension_id,
-    const base::FilePath& profile_dir) {
-  // Check if there any any other shims for the same extension.
-  base::FileEnumerator enumerator(destination_directory, false /* recursive */,
+// Data and helpers for an Info.plist under a given bundle path.
+class BundleInfoPlist {
+ public:
+  // Retrieve info from all app shims found in |apps_path|.
+  static std::list<BundleInfoPlist> GetAllInPath(
+      const base::FilePath& apps_path,
+      bool recursive) {
+    std::list<BundleInfoPlist> bundles;
+    base::FileEnumerator enumerator(apps_path, recursive,
                                     base::FileEnumerator::DIRECTORIES);
     for (base::FilePath shim_path = enumerator.Next(); !shim_path.empty();
          shim_path = enumerator.Next()) {
-    NSDictionary* plist = ReadPlist(GetPlistPath(shim_path));
-    std::string plist_extension_id = base::SysNSStringToUTF8(
-        [plist valueForKey:app_mode::kCrAppModeShortcutIDKey]);
-    base::FilePath plist_profile_dir(base::SysNSStringToUTF8(
-        [plist valueForKey:app_mode::kCrAppModeProfileDirKey]));
-    if (plist_extension_id == extension_id && plist_profile_dir != profile_dir)
-      return true;
+      bundles.emplace_back(shim_path);
+    }
+    return bundles;
   }
 
-  return false;
-}
+  // Retrieve info from the specified app shim in |bundle_path|.
+  explicit BundleInfoPlist(const base::FilePath& bundle_path)
+      : bundle_path_(bundle_path) {
+    NSString* plist_path = GetPlistPath(bundle_path_);
+    plist_.reset([NSDictionary dictionaryWithContentsOfFile:plist_path],
+                 base::scoped_policy::RETAIN);
+  }
+  ~BundleInfoPlist() = default;
 
-// Takes the path to an app bundle and checks that the CrAppModeUserDataDir in
-// the Info.plist starts with the current user_data_dir. This uses starts with
-// instead of equals because the CrAppModeUserDataDir could be the user_data_dir
-// or the |app_data_dir_|.
-bool HasSameUserDataDir(const base::FilePath& bundle_path) {
-  NSDictionary* plist = ReadPlist(GetPlistPath(bundle_path));
+  const base::FilePath& bundle_path() const { return bundle_path_; }
+
+  // Checks that the CrAppModeUserDataDir in the Info.plist is, or is a subpath
+  // of the current user_data_dir. This uses starts with instead of equals
+  // because the CrAppModeUserDataDir could be the user_data_dir or the
+  // |app_data_dir_|.
+  bool IsForCurrentUserDataDir() const {
     base::FilePath user_data_dir;
     base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
     DCHECK(!user_data_dir.empty());
-  return base::StartsWith(base::SysNSStringToUTF8([plist
-                              valueForKey:app_mode::kCrAppModeUserDataDirKey]),
+    return base::StartsWith(
+        base::SysNSStringToUTF8(
+            [plist_ valueForKey:app_mode::kCrAppModeUserDataDirKey]),
         user_data_dir.value(), base::CompareCase::SENSITIVE);
+  }
+
+  // Checks if kCrAppModeProfileDirKey corresponds to the specified profile
+  // path.
+  bool IsForProfile(const base::FilePath& test_profile_path) const {
+    std::string profile_path = base::SysNSStringToUTF8(
+        [plist_ valueForKey:app_mode::kCrAppModeProfileDirKey]);
+    return profile_path == test_profile_path.BaseName().value();
+  }
+
+  // Return the full profile path (as a subpath of the user_data_dir).
+  base::FilePath GetFullProfilePath() const {
+    // Figure out the profile_path. Since the user_data_dir could contain the
+    // path to the web app data dir.
+    base::FilePath user_data_dir = base::mac::NSStringToFilePath(
+        [plist_ valueForKey:app_mode::kCrAppModeUserDataDirKey]);
+    base::FilePath profile_base_name = base::mac::NSStringToFilePath(
+        [plist_ valueForKey:app_mode::kCrAppModeProfileDirKey]);
+    if (user_data_dir.DirName().DirName().BaseName() == profile_base_name)
+      return user_data_dir.DirName().DirName();
+    return user_data_dir.Append(profile_base_name);
+  }
+
+  std::string GetExtensionId() const {
+    return base::SysNSStringToUTF8(
+        [plist_ valueForKey:app_mode::kCrAppModeShortcutIDKey]);
+  }
+  std::string GetProfileName() const {
+    return base::SysNSStringToUTF8(
+        [plist_ valueForKey:app_mode::kCrAppModeProfileNameKey]);
+  }
+  GURL GetURL() const {
+    return GURL(base::SysNSStringToUTF8(
+        [plist_ valueForKey:app_mode::kCrAppModeShortcutURLKey]));
+  }
+  base::string16 GetTitle() const {
+    return base::SysNSStringToUTF16(
+        [plist_ valueForKey:app_mode::kCrAppModeShortcutNameKey]);
+  }
+  base::Version GetVersion() const {
+    NSString* version_string =
+        [plist_ valueForKey:app_mode::kCrBundleVersionKey];
+    if (!version_string) {
+      // Older bundles have the Chrome version in the following key.
+      version_string =
+          [plist_ valueForKey:app_mode::kCFBundleShortVersionStringKey];
+    }
+    return base::Version(base::SysNSStringToUTF8(version_string));
+  }
+  std::string GetBundleId() const {
+    return base::SysNSStringToUTF8(
+        [plist_ valueForKey:base::mac::CFToNSCast(kCFBundleIdentifierKey)]);
+  }
+
+ private:
+  // The path of the app bundle from this this info was read.
+  base::FilePath bundle_path_;
+
+  // Data read from the Info.plist.
+  base::scoped_nsobject<NSDictionary> plist_;
+
+  DISALLOW_COPY_AND_ASSIGN(BundleInfoPlist);
+};
+
+bool HasExistingExtensionShimForDifferentProfile(
+    const base::FilePath& destination_directory,
+    const std::string& extension_id,
+    const base::FilePath& profile_dir) {
+  std::list<BundleInfoPlist> bundles_info = BundleInfoPlist::GetAllInPath(
+      destination_directory, false /* recursive */);
+  for (const auto& info : bundles_info) {
+    if (info.GetExtensionId() == extension_id &&
+        !info.IsForProfile(profile_dir)) {
+      return true;
+    }
+  }
+  return false;
 }
 
 void LaunchShimOnFileThread(web_app::LaunchShimUpdateBehavior update_behavior,
@@ -485,57 +566,17 @@ bool UpdateAppShortcutsSubdirLocalizedName(
   return true;
 }
 
-bool IsShimForProfile(const base::FilePath& bundle_path,
-                      const std::string& profile_base_name) {
-  NSDictionary* plist = ReadPlist(GetPlistPath(bundle_path));
-  std::string profile_dir = base::SysNSStringToUTF8(
-      [plist valueForKey:app_mode::kCrAppModeProfileDirKey]);
-  return profile_dir == profile_base_name;
-}
-
-std::vector<base::FilePath> GetAllAppBundlesInPath(
-    const base::FilePath& internal_shortcut_path,
-    const std::string& profile_base_name) {
-  std::vector<base::FilePath> bundle_paths;
-
-  base::FileEnumerator enumerator(internal_shortcut_path, true /* recursive */,
-                                  base::FileEnumerator::DIRECTORIES);
-  for (base::FilePath bundle_path = enumerator.Next(); !bundle_path.empty();
-       bundle_path = enumerator.Next()) {
-    if (IsShimForProfile(bundle_path, profile_base_name))
-      bundle_paths.push_back(bundle_path);
-  }
-
-  return bundle_paths;
-}
-
 std::unique_ptr<web_app::ShortcutInfo> BuildShortcutInfoFromBundle(
     const base::FilePath& bundle_path) {
-  NSDictionary* plist = ReadPlist(GetPlistPath(bundle_path));
-
+  BundleInfoPlist bundle_info(bundle_path);
   std::unique_ptr<web_app::ShortcutInfo> shortcut_info(
       new web_app::ShortcutInfo);
-  shortcut_info->extension_id = base::SysNSStringToUTF8(
-      [plist valueForKey:app_mode::kCrAppModeShortcutIDKey]);
+  shortcut_info->extension_id = bundle_info.GetExtensionId();
   shortcut_info->is_platform_app = true;
-  shortcut_info->url = GURL(base::SysNSStringToUTF8(
-      [plist valueForKey:app_mode::kCrAppModeShortcutURLKey]));
-  shortcut_info->title = base::SysNSStringToUTF16(
-      [plist valueForKey:app_mode::kCrAppModeShortcutNameKey]);
-  shortcut_info->profile_name = base::SysNSStringToUTF8(
-      [plist valueForKey:app_mode::kCrAppModeProfileNameKey]);
-
-  // Figure out the profile_path. Since the user_data_dir could contain the
-  // path to the web app data dir.
-  base::FilePath user_data_dir = base::mac::NSStringToFilePath(
-      [plist valueForKey:app_mode::kCrAppModeUserDataDirKey]);
-  base::FilePath profile_base_name = base::mac::NSStringToFilePath(
-      [plist valueForKey:app_mode::kCrAppModeProfileDirKey]);
-  if (user_data_dir.DirName().DirName().BaseName() == profile_base_name)
-    shortcut_info->profile_path = user_data_dir.DirName().DirName();
-  else
-    shortcut_info->profile_path = user_data_dir.Append(profile_base_name);
-
+  shortcut_info->url = bundle_info.GetURL();
+  shortcut_info->title = bundle_info.GetTitle();
+  shortcut_info->profile_name = bundle_info.GetProfileName();
+  shortcut_info->profile_path = bundle_info.GetFullProfilePath();
   return shortcut_info;
 }
 
@@ -545,14 +586,7 @@ namespace web_app {
 
 std::unique_ptr<web_app::ShortcutInfo> RecordAppShimErrorAndBuildShortcutInfo(
     const base::FilePath& bundle_path) {
-  NSDictionary* plist = ReadPlist(GetPlistPath(bundle_path));
-  NSString* version_string = [plist valueForKey:app_mode::kCrBundleVersionKey];
-  if (!version_string) {
-    // Older bundles have the Chrome version in the following key.
-    version_string =
-        [plist valueForKey:app_mode::kCFBundleShortVersionStringKey];
-  }
-  base::Version full_version(base::SysNSStringToUTF8(version_string));
+  base::Version full_version = BundleInfoPlist(bundle_path).GetVersion();
   uint32_t major_version = 0;
   if (full_version.IsValid())
     major_version = full_version.components()[0];
@@ -803,7 +837,8 @@ bool WebAppShortcutCreator::UpdatePlist(const base::FilePath& app_path) const {
           app_mode::kShortcutBrowserBundleIDPlaceholder, nil];
 
   NSString* plist_path = GetPlistPath(app_path);
-  NSMutableDictionary* plist = ReadPlist(plist_path);
+  NSMutableDictionary* plist =
+      [NSMutableDictionary dictionaryWithContentsOfFile:plist_path];
   NSArray* keys = [plist allKeys];
 
   // 1. Fill in variables.
@@ -873,8 +908,7 @@ bool WebAppShortcutCreator::UpdateDisplayName(
   NSString* bundle_name = base::SysUTF16ToNSString(info_->title);
   NSString* display_name = base::SysUTF16ToNSString(info_->title);
   if (HasExistingExtensionShimForDifferentProfile(
-          GetApplicationsDirname(), info_->extension_id,
-          info_->profile_path.BaseName())) {
+          GetApplicationsDirname(), info_->extension_id, info_->profile_path)) {
     display_name = [bundle_name
         stringByAppendingString:base::SysUTF8ToNSString(
                                     " (" + info_->profile_name + ")")];
@@ -912,8 +946,9 @@ bool WebAppShortcutCreator::UpdateIcon(const base::FilePath& app_path) const {
 
 std::vector<base::FilePath> WebAppShortcutCreator::GetAppBundlesByIdUnsorted()
     const {
+  const std::string bundle_id = GetBundleIdentifier();
   base::ScopedCFTypeRef<CFStringRef> bundle_id_cf(
-      base::SysUTF8ToCFStringRef(GetBundleIdentifier()));
+      base::SysUTF8ToCFStringRef(bundle_id));
 
   // Retrieve the URLs found by LaunchServices.
   base::scoped_nsobject<NSArray> urls(base::mac::CFToNSCast(
@@ -924,9 +959,22 @@ std::vector<base::FilePath> WebAppShortcutCreator::GetAppBundlesByIdUnsorted()
   for (NSURL* url : urls.get()) {
     NSString* path_string = [url path];
     base::FilePath path([path_string fileSystemRepresentation]);
-    if (HasSameUserDataDir(path))
+    if (BundleInfoPlist(path).IsForCurrentUserDataDir())
       paths.push_back(path);
   }
+
+  // LaunchServices can fail to locate a recently-created bundle. Search
+  // for an app in the applications folder to handle this case.
+  // https://crbug.com/937703
+  if (paths.empty()) {
+    std::list<BundleInfoPlist> bundles_info = BundleInfoPlist::GetAllInPath(
+        GetApplicationsDirname(), false /* recursive */);
+    for (const auto& info : bundles_info) {
+      if (info.IsForCurrentUserDataDir() && info.GetBundleId() == bundle_id)
+        paths.push_back(info.bundle_path());
+    }
+  }
+
   return paths;
 }
 
@@ -1063,15 +1111,18 @@ void UpdatePlatformShortcuts(const base::FilePath& app_data_path,
 void DeleteAllShortcutsForProfile(const base::FilePath& profile_path) {
   base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
                                                 base::BlockingType::MAY_BLOCK);
-  const std::string profile_base_name = profile_path.BaseName().value();
-  std::vector<base::FilePath> bundles = GetAllAppBundlesInPath(
-      profile_path.Append(chrome::kWebAppDirname), profile_base_name);
+  std::list<BundleInfoPlist> bundles_info = BundleInfoPlist::GetAllInPath(
+      profile_path.Append(chrome::kWebAppDirname), true /* recursive */);
+  for (const auto& info : bundles_info) {
+    if (!info.IsForCurrentUserDataDir())
+      continue;
+    if (!info.IsForProfile(profile_path))
+      continue;
 
-  for (std::vector<base::FilePath>::const_iterator it = bundles.begin();
-       it != bundles.end(); ++it) {
     std::unique_ptr<web_app::ShortcutInfo> shortcut_info =
-        BuildShortcutInfoFromBundle(*it);
-    WebAppShortcutCreator shortcut_creator(it->DirName(), shortcut_info.get());
+        BuildShortcutInfoFromBundle(info.bundle_path());
+    WebAppShortcutCreator shortcut_creator(info.bundle_path().DirName(),
+                                           shortcut_info.get());
     shortcut_creator.DeleteShortcuts();
   }
 }