Include profile name in AppUserModelId for default profile.

Migrate pinned PWA shortcuts as well.

This makes pinned icons for the default profile on Windows have
the profile name in the AppUserModelId, which prevents weird behaviors
when other profiles are added/removed.

If shortcuts are migrated, shortcuts without a profile will be migrated
to point to the default profile.

Originally submitted as 1658991, which was reverted because of an issue
with pinned PWA shortcuts.

Bug: 593414, 328738
Change-Id: I68bc7083e233cf6dd3cfd56359245df59fd0e4a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1686222Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: David Bienvenu <davidbienvenu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677305}

chrome/browser/shell_integration_win.cc

@@ -43,6 +43,7 @@
 #include "chrome/browser/policy/policy_path_parser.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
+#include "chrome/browser/web_applications/components/web_app_shortcut_win.h"
 #include "chrome/browser/win/settings_app_monitor.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths_internal.h"
@@ -68,16 +69,6 @@ base::string16 GetProfileIdFromPath(const base::FilePath& profile_path) {
   if (profile_path.empty())
     return base::string16();
 
-  base::FilePath default_user_data_dir;
-  // Return empty string if profile_path is in default user data
-  // dir and is the default profile.
-  if (chrome::GetDefaultUserDataDirectory(&default_user_data_dir) &&
-      profile_path.DirName() == default_user_data_dir &&
-      profile_path.BaseName().value() ==
-          base::ASCIIToUTF16(chrome::kInitialProfile)) {
-    return base::string16();
-  }
-
   // Get joined basenames of user data dir and profile.
   base::string16 basenames = profile_path.DirName().BaseName().value() +
       L"." + profile_path.BaseName().value();
@@ -144,21 +135,6 @@ base::string16 GetExpectedAppId(const base::CommandLine& command_line,
   return win::GetAppModelIdForProfile(app_name, profile_path);
 }
 
-void MigrateTaskbarPinsCallback() {
-  // Get full path of chrome.
-  base::FilePath chrome_exe;
-  if (!base::PathService::Get(base::FILE_EXE, &chrome_exe))
-    return;
-
-  base::FilePath pins_path;
-  if (!base::PathService::Get(base::DIR_TASKBAR_PINS, &pins_path)) {
-    NOTREACHED();
-    return;
-  }
-
-  win::MigrateShortcutsInPathInternal(chrome_exe, pins_path);
-}
-
 // Windows treats a given scheme as an Internet scheme only if its registry
 // entry has a "URL Protocol" key. Check this, otherwise we allow ProgIDs to be
 // used as custom protocols which leads to security bugs.
@@ -733,9 +709,28 @@ void MigrateTaskbarPins() {
   // This needs to happen (e.g. so that the appid is fixed and the
   // run-time Chrome icon is merged with the taskbar shortcut), but it is not an
   // urgent task.
+  base::FilePath pins_path;
+  if (!base::PathService::Get(base::DIR_TASKBAR_PINS, &pins_path)) {
+    NOTREACHED();
+    return;
+  }
+
   base::CreateCOMSTATaskRunnerWithTraits(
       {base::MayBlock(), base::TaskPriority::BEST_EFFORT})
-      ->PostTask(FROM_HERE, base::BindOnce(&MigrateTaskbarPinsCallback));
+      ->PostTask(FROM_HERE,
+                 base::BindOnce(&MigrateTaskbarPinsCallback, pins_path));
+}
+
+void MigrateTaskbarPinsCallback(const base::FilePath& pins_path) {
+  // Get full path of chrome.
+  base::FilePath chrome_exe;
+  if (!base::PathService::Get(base::FILE_EXE, &chrome_exe))
+    return;
+
+  win::MigrateShortcutsInPathInternal(chrome_exe, pins_path);
+
+  // Migrate any pinned PWA shortcuts.
+  win::MigrateShortcutsInPathInternal(web_app::GetChromeProxyPath(), pins_path);
 }
 
 void GetIsPinnedToTaskbarState(
@@ -796,6 +791,8 @@ int MigrateShortcutsInPathInternal(const base::FilePath& chrome_exe,
     // |updated_properties|.
     base::win::ShortcutProperties updated_properties;
 
+    base::string16 current_app_id;
+
     // Validate the existing app id for the shortcut.
     Microsoft::WRL::ComPtr<IPropertyStore> property_store;
     propvariant.Reset();
@@ -813,7 +810,8 @@ int MigrateShortcutsInPathInternal(const base::FilePath& chrome_exe,
             updated_properties.set_app_id(expected_app_id);
           break;
         case VT_LPWSTR:
-          if (expected_app_id != base::string16(propvariant.get().pwszVal))
+          current_app_id = base::string16(propvariant.get().pwszVal);
+          if (expected_app_id != current_app_id)
             updated_properties.set_app_id(expected_app_id);
           break;
         default:
@@ -827,7 +825,7 @@ int MigrateShortcutsInPathInternal(const base::FilePath& chrome_exe,
     // |default_chromium_model_id|).
     base::string16 default_chromium_model_id(
         ShellUtil::GetBrowserModelId(is_per_user_install));
-    if (expected_app_id == default_chromium_model_id) {
+    if (current_app_id == default_chromium_model_id) {
       propvariant.Reset();
       if (property_store->GetValue(PKEY_AppUserModel_IsDualMode,
                                    propvariant.Receive()) != S_OK) {

chrome/browser/shell_integration_win.h

@@ -76,9 +76,12 @@ void GetIsPinnedToTaskbarState(
     const IsPinnedToTaskbarCallback& result_callback);
 
 // Migrates existing chrome taskbar pins by tagging them with correct app id.
-// see http://crbug.com/28104
+// see http://crbug.com/28104. Migrates taskbar pins via a task.
 void MigrateTaskbarPins();
 
+// Callback for MigrateTaskbarPins(). Exposed for testing.
+void MigrateTaskbarPinsCallback(const base::FilePath& pins_path);
+
 // Migrates all shortcuts in |path| which point to |chrome_exe| such that they
 // have the appropriate AppUserModelId. Also clears the legacy dual_mode
 // property from shortcuts with the default chrome app id.

chrome/browser/shell_integration_win_unittest.cc

@@ -18,6 +18,7 @@
 #include "base/test/test_shortcut_win.h"
 #include "base/win/scoped_com_initializer.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
+#include "chrome/browser/web_applications/components/web_app_shortcut_win.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths_internal.h"
 #include "chrome/install_static/install_util.h"
@@ -65,6 +66,9 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     non_default_user_data_dir_and_profile_chrome_app_id_ =
         GetChromiumModelIdForProfile(
             non_default_user_data_dir_.Append(non_default_profile_));
+    default_profile_ = L"Default";
+    default_profile_chrome_app_id_ = GetChromiumModelIdForProfile(
+        default_user_data_dir.Append(default_profile_));
 
     extension_id_ = L"chromiumexampleappidforunittests";
     base::string16 app_name =
@@ -73,8 +77,6 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     extension_app_id_ = GetAppModelIdForProfile(app_name, default_profile_path);
     non_default_profile_extension_app_id_ = GetAppModelIdForProfile(
         app_name, default_user_data_dir.Append(non_default_profile_));
-
-    CreateShortcuts();
   }
 
   // Creates a test shortcut corresponding to |shortcut_properties| and resets
@@ -116,13 +118,13 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     // Shortcut 2 points to chrome.exe, but already has the right appid and thus
     // should only be migrated if dual_mode is desired.
     temp_properties.set_target(chrome_exe_);
-    temp_properties.set_app_id(chrome_app_id_);
+    temp_properties.set_app_id(default_profile_chrome_app_id_);
     ASSERT_NO_FATAL_FAILURE(
         AddTestShortcutAndResetProperties(&temp_properties));
 
     // Shortcut 3 is like shortcut 1, but it's appid is a prefix of the expected
     // appid instead of being totally different.
-    base::string16 chrome_app_id_is_prefix(chrome_app_id_);
+    base::string16 chrome_app_id_is_prefix(default_profile_chrome_app_id_);
     chrome_app_id_is_prefix.push_back(L'1');
     temp_properties.set_target(chrome_exe_);
     temp_properties.set_app_id(chrome_app_id_is_prefix);
@@ -131,7 +133,8 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
 
     // Shortcut 4 is like shortcut 1, but it's appid is of the same size as the
     // expected appid.
-    base::string16 same_size_as_chrome_app_id(chrome_app_id_.size(), L'1');
+    base::string16 same_size_as_chrome_app_id(
+        default_profile_chrome_app_id_.size(), L'1');
     temp_properties.set_target(chrome_exe_);
     temp_properties.set_app_id(same_size_as_chrome_app_id);
     ASSERT_NO_FATAL_FAILURE(
@@ -191,9 +194,9 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     ASSERT_NO_FATAL_FAILURE(
         AddTestShortcutAndResetProperties(&temp_properties));
 
-    // Shortcut 11 points to chrome.exe, already has the right appid, and has
-    // dual_mode set and thus should only be migrated if dual_mode is being
-    // cleared.
+    // Shortcut 11 points to chrome.exe, has the chrome appid, and has
+    // dual_mode set and thus should be migrated to the
+    // default_profile_chrome_app_id, and dual_mode should be cleared.
     temp_properties.set_target(chrome_exe_);
     temp_properties.set_app_id(chrome_app_id_);
     temp_properties.set_dual_mode(true);
@@ -224,6 +227,12 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
   // Chrome's AppUserModelId.
   base::string16 chrome_app_id_;
 
+  // Chrome's AppUserModelId for the default profile.
+  base::string16 default_profile_chrome_app_id_;
+
+  // The Default profile.
+  base::string16 default_profile_;
+
   // A profile that isn't the Default profile.
   base::string16 non_default_profile_;
 
@@ -256,22 +265,26 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
 }  // namespace
 
 TEST_F(ShellIntegrationWinMigrateShortcutTest, ClearDualModeAndAdjustAppIds) {
-  // 9 shortcuts should have their app id updated below and shortcut 11 should
-  // be migrated away from dual_mode for a total of 10 shortcuts migrated.
-  EXPECT_EQ(10,
+  CreateShortcuts();
+  // 11 shortcuts should have their app id updated below including shortcut 11,
+  // which should also be migrated away from dual_mode.
+  EXPECT_EQ(11,
             MigrateShortcutsInPathInternal(chrome_exe_, temp_dir_.GetPath()));
 
-  // Shortcut 1, 3, 4, 5, 6, 7, 8, 9, and 10 should have had their app_id fixed.
-  shortcuts_[1].properties.set_app_id(chrome_app_id_);
-  shortcuts_[3].properties.set_app_id(chrome_app_id_);
-  shortcuts_[4].properties.set_app_id(chrome_app_id_);
-  shortcuts_[5].properties.set_app_id(chrome_app_id_);
+  // Shortcut 1, 3, 4, 5, 6, 7, 8, 9, 10, 11 and 12 should have had their app_id
+  // fixed.
+  shortcuts_[1].properties.set_app_id(default_profile_chrome_app_id_);
+  shortcuts_[3].properties.set_app_id(default_profile_chrome_app_id_);
+  shortcuts_[4].properties.set_app_id(default_profile_chrome_app_id_);
+  shortcuts_[5].properties.set_app_id(default_profile_chrome_app_id_);
   shortcuts_[6].properties.set_app_id(non_default_profile_chrome_app_id_);
   shortcuts_[7].properties.set_app_id(non_default_user_data_dir_chrome_app_id_);
   shortcuts_[8].properties.set_app_id(
       non_default_user_data_dir_and_profile_chrome_app_id_);
   shortcuts_[9].properties.set_app_id(extension_app_id_);
   shortcuts_[10].properties.set_app_id(non_default_profile_extension_app_id_);
+  shortcuts_[11].properties.set_app_id(default_profile_chrome_app_id_);
+  shortcuts_[12].properties.set_app_id(default_profile_chrome_app_id_);
 
   // No shortcut should still have the dual_mode property.
   for (size_t i = 0; i < shortcuts_.size(); ++i)
@@ -287,6 +300,24 @@ TEST_F(ShellIntegrationWinMigrateShortcutTest, ClearDualModeAndAdjustAppIds) {
             MigrateShortcutsInPathInternal(chrome_exe_, temp_dir_.GetPath()));
 }
 
+// Test that a chrome_proxy.exe shortcut (PWA) has its app_id migrated
+// to include the default profile name.
+TEST_F(ShellIntegrationWinMigrateShortcutTest, MigrateChromeProxyTest) {
+  // Create shortcut to chrome_proxy_exe in executable directory,
+  // using the default profile, with the AppModelId not containing the
+  // profile name. "chrome_proxy.exe" won't exist, but it appears that
+  // creating the shortcut succeeds.
+  base::win::ShortcutProperties temp_properties;
+  temp_properties.set_target(web_app::GetChromeProxyPath());
+  temp_properties.set_app_id(L"Dumbo");
+  ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(&temp_properties));
+
+  MigrateTaskbarPinsCallback(temp_dir_.GetPath());
+  // Verify that the migrated shortcut now contains the default profile name.
+  shortcuts_[0].properties.set_app_id(default_profile_chrome_app_id_);
+  base::win::ValidateShortcut(shortcuts_[0].path, shortcuts_[0].properties);
+}
+
 TEST(ShellIntegrationWinTest, GetAppModelIdForProfileTest) {
   const base::string16 base_app_id(install_static::GetBaseAppId());
 
@@ -294,12 +325,13 @@ TEST(ShellIntegrationWinTest, GetAppModelIdForProfileTest) {
   base::FilePath empty_path;
   EXPECT_EQ(base_app_id, GetAppModelIdForProfile(base_app_id, empty_path));
 
-  // Default profile path should get chrome::kBrowserAppID
+  // Default profile path should get chrome::kBrowserAppID joined with
+  // profile info.
   base::FilePath default_user_data_dir;
   chrome::GetDefaultUserDataDirectory(&default_user_data_dir);
   base::FilePath default_profile_path =
       default_user_data_dir.AppendASCII(chrome::kInitialProfile);
-  EXPECT_EQ(base_app_id,
+  EXPECT_EQ(base_app_id + L".UserData.Default",
             GetAppModelIdForProfile(base_app_id, default_profile_path));
 
   // Non-default profile path should get chrome::kBrowserAppID joined with

chrome/browser/web_applications/components/web_app_shortcut_win.cc

@@ -44,12 +44,6 @@ constexpr base::FilePath::CharType kIconChecksumFileExt[] =
 constexpr base::FilePath::CharType kChromeProxyExecutable[] =
     FILE_PATH_LITERAL("chrome_proxy.exe");
 
-base::FilePath GetChromeProxyPath() {
-  base::FilePath chrome_dir;
-  CHECK(base::PathService::Get(base::DIR_EXE, &chrome_dir));
-  return chrome_dir.Append(kChromeProxyExecutable);
-}
-
 // Calculates checksum of an icon family using MD5.
 // The checksum is derived from all of the icons in the family.
 void GetImageCheckSum(const gfx::ImageFamily& image, base::MD5Digest* digest) {
@@ -195,7 +189,7 @@ bool CreateShortcutsInPaths(const base::FilePath& web_app_path,
     return false;
   }
 
-  base::FilePath chrome_proxy_path = GetChromeProxyPath();
+  base::FilePath chrome_proxy_path = web_app::GetChromeProxyPath();
 
   // Working directory.
   base::FilePath working_dir(chrome_proxy_path.DirName());
@@ -346,7 +340,7 @@ void CreateIconAndSetRelaunchDetails(
                                                     shortcut_info.extension_id,
                                                     shortcut_info.profile_path);
 
-  command_line.SetProgram(GetChromeProxyPath());
+  command_line.SetProgram(web_app::GetChromeProxyPath());
   ui::win::SetRelaunchDetailsForWindow(command_line.GetCommandLineString(),
                                        shortcut_info.title, hwnd);
 
@@ -361,6 +355,12 @@ void CreateIconAndSetRelaunchDetails(
 
 namespace web_app {
 
+base::FilePath GetChromeProxyPath() {
+  base::FilePath chrome_dir;
+  CHECK(base::PathService::Get(base::DIR_EXE, &chrome_dir));
+  return chrome_dir.Append(kChromeProxyExecutable);
+}
+
 namespace internals {
 
 void OnShortcutInfoLoadedForSetRelaunchDetails(

chrome/browser/web_applications/components/web_app_shortcut_win.h

@@ -11,6 +11,8 @@
 
 namespace web_app {
 
+base::FilePath GetChromeProxyPath();
+
 namespace internals {
 
 // Returns the Windows user-level shortcut paths that are specified in