Remove PostTask from browser_window_property_manager_browsertest_win.cc

This patch changes the tests to just wait for tasks to finish before
running their validation instead of relying on the timing of particular
threads. Also fixed a property leak on shutdown that was probably
causing their flakiness, so they are re-enabled.

Bug: 396344, 689520
Change-Id: I9f04d515279ec10f6353e0e6c096ca76bda8ed1c
Reviewed-on: https://chromium-review.googlesource.com/575758Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491918}

base/win/win_util.cc

@@ -43,6 +43,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/win/registry.h"
+#include "base/win/scoped_co_mem.h"
 #include "base/win/scoped_comptr.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/scoped_propvariant.h"
@@ -63,7 +64,19 @@ bool SetPropVariantValueForPropertyStore(
   HRESULT result = property_store->SetValue(property_key, property_value.get());
   if (result == S_OK)
     result = property_store->Commit();
-  return SUCCEEDED(result);
+  if (SUCCEEDED(result))
+    return true;
+#if DCHECK_IS_ON()
+  ScopedCoMem<OLECHAR> guidString;
+  ::StringFromCLSID(property_key.fmtid, &guidString);
+  if (HRESULT_FACILITY(result) == FACILITY_WIN32)
+    ::SetLastError(HRESULT_CODE(result));
+  // See third_party/perl/c/i686-w64-mingw32/include/propkey.h for GUID and
+  // PID definitions.
+  DPLOG(ERROR) << "Failed to set property with GUID " << guidString << " PID "
+               << property_key.pid;
+#endif
+  return false;
 }
 
 void __cdecl ForceCrashOnSigAbort(int) {

chrome/browser/ui/views/frame/browser_window_property_manager_browsertest_win.cc

@@ -3,20 +3,18 @@
 // found in the LICENSE file.
 
 #include <objbase.h>
-#include <shlobj.h>  // Must be before propkey.
 #include <propkey.h>
 #include <shellapi.h>
-#include <stddef.h>
+#include <shlobj.h>
+#include <wrl/client.h>
 
 #include <string>
 
 #include "base/command_line.h"
 #include "base/macros.h"
-#include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/win/scoped_comptr.h"
 #include "base/win/scoped_propvariant.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
@@ -47,12 +45,8 @@ typedef ExtensionBrowserTest BrowserWindowPropertyManagerTest;
 
 namespace {
 
-// An observer that resumes test code after a new profile is initialized by
-// quitting the message loop it's blocked on.
-void UnblockOnProfileCreation(Profile* profile,
-                              Profile::CreateStatus status) {
-  if (status == Profile::CREATE_STATUS_INITIALIZED)
-    base::RunLoop::QuitCurrentWhenIdleDeprecated();
+std::wstring AddIdToIconPath(const std::wstring& path) {
+  return path + L",0";
 }
 
 // Checks that the relaunch name, relaunch command and app icon for the given
@@ -60,9 +54,12 @@ void UnblockOnProfileCreation(Profile* profile,
 void ValidateBrowserWindowProperties(
     const Browser* browser,
     const base::string16& expected_profile_name) {
+  // Let shortcut creation finish before we validate the results.
+  content::RunAllBlockingPoolTasksUntilIdle();
+
   HWND hwnd = views::HWNDForNativeWindow(browser->window()->GetNativeWindow());
 
-  base::win::ScopedComPtr<IPropertyStore> pps;
+  Microsoft::WRL::ComPtr<IPropertyStore> pps;
   HRESULT result = SHGetPropertyStoreForWindow(hwnd, IID_PPV_ARGS(&pps));
   EXPECT_TRUE(SUCCEEDED(result));
 
@@ -94,18 +91,20 @@ void ValidateBrowserWindowProperties(
   EXPECT_EQ(S_OK, pps->GetValue(PKEY_AppUserModel_RelaunchIconResource,
                                 prop_var.Receive()));
   EXPECT_EQ(VT_LPWSTR, prop_var.get().vt);
-  EXPECT_EQ(profiles::internal::GetProfileIconPath(
-                browser->profile()->GetPath()).value(),
+  EXPECT_EQ(AddIdToIconPath(profiles::internal::GetProfileIconPath(
+                                browser->profile()->GetPath())
+                                .value()),
             prop_var.get().pwszVal);
   prop_var.Reset();
-  base::RunLoop::QuitCurrentWhenIdleDeprecated();
 }
 
 void ValidateHostedAppWindowProperties(const Browser* browser,
                                        const extensions::Extension* extension) {
+  content::RunAllBlockingPoolTasksUntilIdle();
+
   HWND hwnd = views::HWNDForNativeWindow(browser->window()->GetNativeWindow());
 
-  base::win::ScopedComPtr<IPropertyStore> pps;
+  Microsoft::WRL::ComPtr<IPropertyStore> pps;
   HRESULT result = SHGetPropertyStoreForWindow(hwnd, IID_PPV_ARGS(&pps));
   EXPECT_TRUE(SUCCEEDED(result));
 
@@ -138,29 +137,12 @@ void ValidateHostedAppWindowProperties(const Browser* browser,
             pps->GetValue(PKEY_AppUserModel_RelaunchIconResource,
                           prop_var.Receive()));
   EXPECT_EQ(VT_LPWSTR, prop_var.get().vt);
-  EXPECT_EQ(web_app::internals::GetIconFilePath(
-                web_app_dir, base::UTF8ToUTF16(extension->name())).value(),
-            prop_var.get().pwszVal);
+  EXPECT_EQ(
+      AddIdToIconPath(web_app::internals::GetIconFilePath(
+                          web_app_dir, base::UTF8ToUTF16(extension->name()))
+                          .value()),
+      prop_var.get().pwszVal);
   prop_var.Reset();
-  base::RunLoop::QuitCurrentWhenIdleDeprecated();
-}
-
-void PostValidationTaskToUIThread(const base::Closure& validation_task) {
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE, validation_task);
-}
-
-// Posts a validation task to the FILE thread which bounces back to the UI
-// thread and then does validation. This is necessary because the icon profile
-// pref only gets set at the end of icon creation (which happens on the FILE
-// thread) and is set on the UI thread.
-void WaitAndValidateBrowserWindowProperties(
-    const base::Closure& validation_task) {
-  content::BrowserThread::PostTask(
-      content::BrowserThread::FILE,
-      FROM_HERE,
-      base::Bind(&PostValidationTaskToUIThread, validation_task));
-  content::RunMessageLoop();
 }
 
 }  // namespace
@@ -180,12 +162,10 @@ class BrowserTestWithProfileShortcutManager : public InProcessBrowserTest {
 };
 
 // Check that the window properties on Windows are properly set.
-// http://crbug.com/396344
 IN_PROC_BROWSER_TEST_F(BrowserTestWithProfileShortcutManager,
-                       DISABLED_WindowProperties) {
+                       WindowProperties) {
   // Single profile case. The profile name should not be shown.
-  WaitAndValidateBrowserWindowProperties(base::Bind(
-      &ValidateBrowserWindowProperties, browser(), base::string16()));
+  ValidateBrowserWindowProperties(browser(), base::string16());
 
   // If multiprofile mode is not enabled, we can't test the behavior when there
   // are multiple profiles.
@@ -197,20 +177,12 @@ IN_PROC_BROWSER_TEST_F(BrowserTestWithProfileShortcutManager,
 
   base::FilePath path_profile2 =
       profile_manager->GenerateNextProfileDirectoryPath();
-  profile_manager->CreateProfileAsync(path_profile2,
-                                      base::Bind(&UnblockOnProfileCreation),
-                                      base::string16(), std::string(),
-                                      std::string());
-
-  // Spin to allow profile creation to take place, loop is terminated
-  // by UnblockOnProfileCreation when the profile is created.
-  content::RunMessageLoop();
-
+  profile_manager->CreateProfileAsync(
+      path_profile2, ProfileManager::CreateCallback(), base::string16(),
+      std::string(), std::string());
   // The default profile's name should be part of the relaunch name.
-  WaitAndValidateBrowserWindowProperties(base::Bind(
-      &ValidateBrowserWindowProperties,
-      browser(),
-      base::UTF8ToUTF16(browser()->profile()->GetProfileUserName())));
+  ValidateBrowserWindowProperties(
+      browser(), base::UTF8ToUTF16(browser()->profile()->GetProfileUserName()));
 
   // The second profile's name should be part of the relaunch name.
   Browser* profile2_browser =
@@ -218,14 +190,10 @@ IN_PROC_BROWSER_TEST_F(BrowserTestWithProfileShortcutManager,
   ProfileAttributesEntry* entry;
   ASSERT_TRUE(profile_manager->GetProfileAttributesStorage().
               GetProfileAttributesWithPath(path_profile2, &entry));
-  WaitAndValidateBrowserWindowProperties(
-      base::Bind(&ValidateBrowserWindowProperties,
-                 profile2_browser,
-                 entry->GetName()));
+  ValidateBrowserWindowProperties(profile2_browser, entry->GetName());
 }
 
-// http://crbug.com/396344
-IN_PROC_BROWSER_TEST_F(BrowserWindowPropertyManagerTest, DISABLED_HostedApp) {
+IN_PROC_BROWSER_TEST_F(BrowserWindowPropertyManagerTest, HostedApp) {
   // Load an app.
   const extensions::Extension* extension =
       LoadExtension(test_data_dir_.AppendASCII("app/"));
@@ -248,7 +216,5 @@ IN_PROC_BROWSER_TEST_F(BrowserWindowPropertyManagerTest, DISABLED_HostedApp) {
   ASSERT_TRUE(app_browser);
   ASSERT_TRUE(app_browser != browser());
 
-  WaitAndValidateBrowserWindowProperties(
-      base::Bind(&ValidateHostedAppWindowProperties, app_browser,
-                 base::RetainedRef(extension)));
+  ValidateHostedAppWindowProperties(app_browser, extension);
 }

ui/base/win/shell.cc

@@ -6,8 +6,10 @@
 
 #include <dwmapi.h>
 #include <shlobj.h>  // Must be before propkey.
+
 #include <propkey.h>
 #include <shellapi.h>
+#include <wrl/client.h>
 
 #include "base/command_line.h"
 #include "base/debug/alias.h"
@@ -17,7 +19,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
-#include "base/win/scoped_comptr.h"
 #include "base/win/win_util.h"
 #include "ui/base/ui_base_switches.h"
 
@@ -98,7 +99,7 @@ bool OpenFolderViaShell(const base::FilePath& full_path) {
 bool PreventWindowFromPinning(HWND hwnd) {
   DCHECK(hwnd);
 
-  base::win::ScopedComPtr<IPropertyStore> pps;
+  Microsoft::WRL::ComPtr<IPropertyStore> pps;
   if (FAILED(
           SHGetPropertyStoreForWindow(hwnd, IID_PPV_ARGS(pps.GetAddressOf()))))
     return false;
@@ -117,7 +118,7 @@ void SetAppDetailsForWindow(const base::string16& app_id,
                             HWND hwnd) {
   DCHECK(hwnd);
 
-  base::win::ScopedComPtr<IPropertyStore> pps;
+  Microsoft::WRL::ComPtr<IPropertyStore> pps;
   if (FAILED(
           SHGetPropertyStoreForWindow(hwnd, IID_PPV_ARGS(pps.GetAddressOf()))))
     return;
@@ -167,7 +168,7 @@ void SetRelaunchDetailsForWindow(const base::string16& relaunch_command,
 void ClearWindowPropertyStore(HWND hwnd) {
   DCHECK(hwnd);
 
-  base::win::ScopedComPtr<IPropertyStore> pps;
+  Microsoft::WRL::ComPtr<IPropertyStore> pps;
   if (FAILED(
           SHGetPropertyStoreForWindow(hwnd, IID_PPV_ARGS(pps.GetAddressOf()))))
     return;
@@ -177,13 +178,18 @@ void ClearWindowPropertyStore(HWND hwnd) {
     return;
 
   PROPVARIANT empty_property_variant = {};
-  for (DWORD i = 0; i < property_count; i++) {
+  for (DWORD i = property_count; i > 0; i--) {
     PROPERTYKEY key;
-    if (SUCCEEDED(pps->GetAt(i, &key)))
+    if (SUCCEEDED(pps->GetAt(i - 1, &key))) {
+      // Removes the value from |pps|'s array.
       pps->SetValue(key, empty_property_variant);
+    }
   }
+  if (FAILED(pps->Commit()))
+    return;
 
-  pps->Commit();
+  // Verify none of the keys are leaking.
+  DCHECK(FAILED(pps->GetCount(&property_count)) || property_count == 0);
 }
 
 bool IsAeroGlassEnabled() {