Skip profiles in GetLastOpenedProfiles that fail to initialize

chrome/browser/profiles/profile_manager_browsertest.cc

Index: chrome/browser/profiles/profile_manager_browsertest.cc
diff --git a/chrome/browser/profiles/profile_manager_browsertest.cc b/chrome/browser/profiles/profile_manager_browsertest.cc
index 06f06dc2d8fa4b9e2843c2751ce1d1f706ab6001..d7d160d1b8fff85897353e3f7f62a19cd7e48ede 100644
--- a/chrome/browser/profiles/profile_manager_browsertest.cc
+++ b/chrome/browser/profiles/profile_manager_browsertest.cc
@@ -36,6 +36,16 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #endif
 
+#if defined(OS_WIN)
+#include <AccCtrl.h>
+#include <Aclapi.h>
+#include "base/callback_forward.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
+#endif

[Peter Kasting, 2016/06/04 01:24:25]

Nit: There's a lot of stuff in this block and added below, that's all
Windows-specific.  Does it make sense to move this to a separate
_browsertest_win.cc file?

[WC Leung, 2016/06/04 04:55:22]

SGTM

+
 namespace {
 
 const ProfileManager::CreateCallback kOnProfileSwitchDoNothing;
@@ -499,3 +509,139 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, IncognitoProfile) {
                    ->GetFilePath(prefs::kSaveFileDefaultDirectory)
                    .empty());
 }
+
+#if defined(OS_WIN)
+
+bool AddAceToObjectsSecurityDescriptor(LPTSTR filename, LPTSTR trustee,

[brettw, 2016/06/03 21:47:24]

I'm OK duplicating this code. It's a bit yucky but it seems a bit difficult to
generalize and it's just a wrapper around a single syscall.

[Peter Kasting, 2016/06/04 01:24:25]

It looks to me as if these two are so close that just a few fields on the
TRUSTEE are different.  If that's true, I think you should indeed try to avoid
the code duplication.

[WC Leung, 2016/06/04 04:55:22]

I see. Then I'll do the deduplication.

+    DWORD access_rights, ACCESS_MODE access_mode, DWORD inheritance) {

[Peter Kasting, 2016/06/04 01:24:25]

Nit: All lines of args must be indented the same.

[WC Leung, 2016/06/04 04:55:21]

Acknowledged. Anyway, if that deduplication succeeds we don't need the code
here.

+  // Get a pointer to the existing DACL.
+  ACL* old_dacl = nullptr;
+  PSECURITY_DESCRIPTOR security_descriptor = nullptr;
+  DWORD result = GetNamedSecurityInfo(filename, SE_FILE_OBJECT,
+                                      DACL_SECURITY_INFORMATION,
+                                      nullptr, nullptr, &old_dacl, nullptr,
+                                      &security_descriptor);
+  if (result != ERROR_SUCCESS) {
+    LOG(WARNING) << "GetNamedSecurityInfo Error " << result;

[Peter Kasting, 2016/06/04 01:24:25]

Are you going to collect and analyze this log data?  If not, don't log; logging
statements bloat the binary and make it unclear whether we think the statement
is even reachable. (3 places)

+  } else {
+    // Initialize an EXPLICIT_ACCESS structure for the new ACE.
+    EXPLICIT_ACCESS explicit_access;
+    ZeroMemory(&explicit_access, sizeof(EXPLICIT_ACCESS));
+    explicit_access.grfAccessPermissions = access_rights;
+    explicit_access.grfAccessMode = access_mode;
+    explicit_access.grfInheritance = inheritance;
+    explicit_access.Trustee.TrusteeForm = TRUSTEE_IS_NAME;
+    explicit_access.Trustee.ptstrName = trustee;

[Peter Kasting, 2016/06/04 01:24:25]

Nit: Try to use initializer values if possible, e.g.:

  EXPLICIT_ACCESS explicit_access =
      {access_rights, access_mode, inheritance,
       {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_NAME, TRUSTEE_IS_UNKNOWN,
        trustee}};

(Not sure how clang-format would wrap/indent this)

This results in being explicit about all the fields in the TRUSTEE struct and is
more efficient to boot.

+
+    // Create a new ACL that merges the new ACE into the existing DACL.
+    ACL* new_dacl = nullptr;
+    result = SetEntriesInAcl(1, &explicit_access, old_dacl, &new_dacl);
+    if (result != ERROR_SUCCESS) {
+      LOG(WARNING) << "SetEntriesInAcl Error " << result;
+    } else {
+      // Attach the new ACL as the object's DACL.
+      result = SetNamedSecurityInfo(filename, SE_FILE_OBJECT,
+                                    DACL_SECURITY_INFORMATION,
+                                    nullptr, nullptr, new_dacl, nullptr);
+      if (result != ERROR_SUCCESS)
+        LOG(WARNING) << "SetNamedSecurityInfo Error " << result;
+    }
+
+    if (new_dacl)

[Peter Kasting, 2016/06/04 01:24:25]

Don't conditionalize these LocalFree() calls; it should be valid to free a null
pointer.

[WC Leung, 2016/06/04 04:55:22]

Noted and thanks.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa366730%28v=vs.85%2...
did confirm that it is valid to pass NULL to LocalFree.

+      LocalFree((HLOCAL)new_dacl);

[Peter Kasting, 2016/06/04 01:24:25]

Consider creating a scoping object that frees these automatically to avoid the
possibility of forgetting to free on exit/later refactoring breaking this.

This is more compelling if added to base/win as another ScopedXXX object, and
then used across the rest of the codebase where LocalFrees are currently
performed, in which case you'd want to split this to a separate CL.  The class
could be something like ScopedLocal<T> where T is e.g. ACL or
PSECURITY_DESCRIPTOR, and there should be a Receive() method as on
ScopedComPtr() to allow initialization via outparam.

Nit: No C-style casts.  I don't think you need to cast at all; the code in
acl.cc frees these types with no casting. (2 places)

[WC Leung, 2016/06/04 04:55:22]

Acknowledged.

+  }
+
+  if (security_descriptor)
+    LocalFree((HLOCAL)security_descriptor);
+
+  return result == ERROR_SUCCESS;
+}
+
+bool RemoveCreateDirectoryPermission(const base::FilePath& path) {
+  return AddAceToObjectsSecurityDescriptor(
+             const_cast<wchar_t*>(path.value().c_str()), L"EVERYONE",
+             FILE_ADD_SUBDIRECTORY, DENY_ACCESS, NO_INHERITANCE);
+}
+
+class ProfileManagerWinBrowserTest : public ProfileManagerBrowserTest {
+ public:
+  // Tests may be reported as PASS even if the test has never been run. To

[Peter Kasting, 2016/06/04 01:24:25]

I have no idea what this is talking about :(.

[WC Leung, 2016/06/04 04:55:22]

You've stumpled on a bug in the browser test. :-) The main body of the test
isn't run at all, but the test is shown to PASS.

To reproduce:
1. change "Profile 1" into "Profile 2" in line 588 (dir_to_delete variable).
2. run the tests. Now it should fail on line 597 (i.e. EXPECT_FALSE(red_flag_);)
3. remove the line EXPECT_FALSE(red_flag_);
4. run the tests. The test should still FAIL, but actually it is a PASS!

[Peter Kasting, 2016/06/04 08:01:39]

Please understand this bug completely before proceeding with this CL -- the
workaround you have here isn't OK unless we understand much more deeply what's
going on.

I wonder if this is connected with the extremely confusing design of this test
(see reply below).

[WC Leung, 2016/06/04 15:48:33]

Agree that the bug needs more investigation. The perfect way to land the CL is
to wait for the fix, so we don't need a special flag in the text fixture.

But bug 614753 is getting pushed. So I think we'd move the test into a separate
CL and commit the rest.
I think it's related to paragraph (2) in comment #20 of this CL. Somehow
chromium didn't start, but there's no crash in that case.

+  // mitigate the issue, a "red flag" is raised for suspectable tests. These

[Peter Kasting, 2016/06/04 01:24:25]

Do you mean "susceptible" or "suspicious"?

[WC Leung, 2016/06/04 04:55:22]

Susceptble. Sorry for wrong spelling.

[WC Leung, 2016/06/04 17:34:43]

Done.

+  // tests need to call DropRedFlag() to pass the test.
+  void DropRedFlag() { red_flag_ = false; }
+
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kRestoreLastSession);
+  }
+
+  bool SetUpUserDataDirectory() override {
+    std::string testname(
+        testing::UnitTest::GetInstance()->current_test_info()->name());
+    if (testname == "LastOpenedProfileMissing") {

[brettw, 2016/06/03 21:47:24]

This seems potentially very confusing since people don't expect the test
behavior to change based on the name of the test. Can you use this test harness
only for the one test you need? It looks like otherwise this class is almost a
no-op.

[WC Leung, 2016/06/04 04:55:22]

We need to do remove a profile directory and set the ACL of another directory
after the end of the PRE_ test and before the start of the main test.

And I tried to do it in the TearDown step of the PRE_ test, and sadly failed. So
sadly I have no other choices.

Anyway, in gtest even the PRE_ test counts as a separate test, so (sadly again)
the condition is necessary.

+      red_flag_ = true;
+
+      base::FilePath user_data_dir;
+      if (!PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
+        return false;
+
+      base::FilePath dir_to_delete = user_data_dir.AppendASCII("Profile 1");
+      return base::DirectoryExists(dir_to_delete) &&
+             DeleteFile(dir_to_delete, true) &&
+             RemoveCreateDirectoryPermission(user_data_dir);
+    }
+    return true;
+  }
+
+  void TearDown() override {
+    EXPECT_FALSE(red_flag_);
+    ProfileManagerBrowserTest::TearDown();
+  }
+
+ private:
+  bool red_flag_ = false;
+};

[Peter Kasting, 2016/06/04 01:24:25]

Nit: DISALLOW_COPY_AND_ASSIGN

[WC Leung, 2016/06/04 04:55:22]

Acknowledged.

[WC Leung, 2016/06/04 17:34:43]

Unable to do this in test fixtures, because compilation fails if this statement
is added.

[Peter Kasting, 2016/06/04 18:12:31]

Why?

[WC Leung, 2016/06/05 03:04:48]

Here's the compiler log:

f:\chromium\src\chrome\browser\profiles\profile_manager_browsertest.cc(558):
error C2512: 'ProfileManagerWinBrowserTest': no appropriate default constructor
available
f:\chromium\src\chrome\browser\profiles\profile_manager_browsertest.cc(517):
note: see declaration of 'ProfileManagerWinBrowserTest'

And adding "public ProfileManagerWinBrowserTest() {}" in the public section
solves the problem.

+
+IN_PROC_BROWSER_TEST_F(ProfileManagerWinBrowserTest,
+                       PRE_LastOpenedProfileMissing) {

[Peter Kasting, 2016/06/04 01:24:25]

I'm not terribly familiar with using PRE_ in tests.  Why can't the code in this
testcase be unified with the testcase below?

[WC Leung, 2016/06/04 04:55:22]

The test (PRE_ and non-PRE_ as a whole) do the following:
(1) The PRE_ test simply set up 2 extra profiles for the main test.
(2) Then the directory "Profile 1" is removed (in SetUpUserDataDirectory()) and
also |user_data_dir| is set to deny creation of new directories.
(3) Then start the main test with it's browser process.

[Peter Kasting, 2016/06/04 08:01:39]

That didn't really answer my question.  Are you saying that the main test's
browser process needs stuff from the PRE test but reads that stuff before we
have an opportunity to do SetUp() etc.?  This is all very unclear, which is why
both Brett and I seem confused.

[WC Leung, 2016/06/04 15:48:33]

Oh sorry for my bad explanation. What you're saying is close.

The purpose of the PRE test is to set up some states for the main test. After
the PRE test finish, the main test starts with a new process, but using same
user data directory as the PRE test. The "Tests Spanning Restarts" in
https://www.chromium.org/developers/testing/browser-tests has made a brief
introduction to PRE tests, but the details are not available unless we read the
source.

In this test the information shared are a few profiles and their respective
browser windows. The switch switches::kRestoreLastSession was used to make the
main test starts with the same set of windows as the PRE test.

BTW, SetUpUserDataDir is called during SetUp, and is said to be the place to do
something in the user data directory before the browser process starts.
https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test...
https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test...

In this test, some profile(s) are deleted in SetUpUserDataDir, and the user data
directory is also set to deny creation of new directories (profiles). In this
way the browser process will crash in the unpatched code.

+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  ASSERT_TRUE(profile_manager);
+
+  std::vector<base::FilePath> profile_paths;
+  // Create two additional profile.
+  for (int i = 0; i < 2; ++i) {
+    base::FilePath path = profile_manager->GenerateNextProfileDirectoryPath();
+    profile_paths.push_back(path);
+
+    base::RunLoop run_loop;
+    profile_manager->CreateProfileAsync(
+        path, base::Bind(&OnUnblockOnProfileCreation, &run_loop),
+        base::string16(), std::string(), std::string());
+
+    // Run the message loop to allow profile creation to take place; the loop is
+    // terminated by OnUnblockOnProfileCreation when the profile is created.
+    run_loop.Run();
+
+    profiles::SwitchToProfile(path, false, kOnProfileSwitchDoNothing,
+                              ProfileMetrics::SWITCH_PROFILE_ICON);
+  }
+
+  ASSERT_EQ(base::FilePath(FILE_PATH_LITERAL("Profile 1")),
+            profile_paths[0].BaseName());
+}
+
+IN_PROC_BROWSER_TEST_F(ProfileManagerWinBrowserTest,
+                       LastOpenedProfileMissing) {
+  DropRedFlag();
+
+  ProfileManager* profile_manager = g_browser_process->profile_manager();

[Peter Kasting, 2016/06/04 01:24:25]

Why do you need this variable?

[WC Leung, 2016/06/04 04:55:21]

I don't need it, sorry for that. Forgot to remove dead code that doesn't work.

+  ASSERT_TRUE(profile_manager);
+
+  BrowserList* browser_list = BrowserList::GetInstance();
+  EXPECT_EQ(2u, browser_list->size());
+  for (const Browser* browser : *browser_list) {
+    EXPECT_NE(base::FilePath(FILE_PATH_LITERAL("Profile 1")),
+              browser->profile()->GetPath().BaseName());
+  }
+}
+#endif  // defined(OS_WIN)

[Peter Kasting, 2016/06/04 01:24:25]

Nit: Blank line above this, or no blank below the opening #ifdef.

[WC Leung, 2016/06/04 04:55:22]

Acknowledged.