Changed methods of rename session to avoid use Unretained(this)

This CL makes the calls to GetSessionList and DoRenameSession safe as
was noted in CL 929754.

Bug: 830913
Change-Id: I7ad22b5712e032f256bd46ae9f04c28255861af7
Reviewed-on: https://chromium-review.googlesource.com/996472
Commit-Queue: Martin Rodriguez <rodmartin@google.com>
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549532}

chrome/browser/ui/webui/policy_tool_ui_browsertest.cc

@@ -24,6 +24,19 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
 
+namespace {
+
+const base::FilePath::CharType kPolicyToolSessionsDir[] =
+    FILE_PATH_LITERAL("Policy sessions");
+
+const base::FilePath::CharType kPolicyToolDefaultSessionName[] =
+    FILE_PATH_LITERAL("policy");
+
+const base::FilePath::CharType kPolicyToolSessionExtension[] =
+    FILE_PATH_LITERAL("json");
+
+}  // namespace
+
 class PolicyToolUITest : public InProcessBrowserTest {
  public:
   PolicyToolUITest();
@@ -70,15 +83,15 @@ base::FilePath PolicyToolUITest::GetSessionsDir() {
   base::FilePath profile_dir;
   EXPECT_TRUE(PathService::Get(chrome::DIR_USER_DATA, &profile_dir));
   return profile_dir.AppendASCII(TestingProfile::kTestUserProfileDir)
-      .Append(PolicyToolUIHandler::kPolicyToolSessionsDir);
+      .Append(kPolicyToolSessionsDir);
 }
 
 base::FilePath::StringType PolicyToolUITest::GetDefaultSessionName() {
-  return PolicyToolUIHandler::kPolicyToolDefaultSessionName;
+  return kPolicyToolDefaultSessionName;
 }
 
 base::FilePath::StringType PolicyToolUITest::GetSessionExtension() {
-  return PolicyToolUIHandler::kPolicyToolSessionExtension;
+  return kPolicyToolSessionExtension;
 }
 
 base::FilePath PolicyToolUITest::GetSessionPath(

chrome/browser/ui/webui/policy_tool_ui_handler.cc

@@ -15,19 +15,47 @@
 #include "components/strings/grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 
-// static
-const base::FilePath::CharType PolicyToolUIHandler::kPolicyToolSessionsDir[] =
+namespace {
+
+const base::FilePath::CharType kPolicyToolSessionsDir[] =
     FILE_PATH_LITERAL("Policy sessions");
 
-// static
-const base::FilePath::CharType
-    PolicyToolUIHandler::kPolicyToolDefaultSessionName[] =
-        FILE_PATH_LITERAL("policy");
+const base::FilePath::CharType kPolicyToolDefaultSessionName[] =
+    FILE_PATH_LITERAL("policy");
 
-// static
-const base::FilePath::CharType
-    PolicyToolUIHandler::kPolicyToolSessionExtension[] =
-        FILE_PATH_LITERAL("json");
+const base::FilePath::CharType kPolicyToolSessionExtension[] =
+    FILE_PATH_LITERAL("json");
+
+// Returns the current list of all sessions sorted by last access time in
+// decreasing order.
+base::ListValue GetSessionsList(const base::FilePath& sessions_dir) {
+  base::FilePath::StringType sessions_pattern =
+      FILE_PATH_LITERAL("*.") +
+      base::FilePath::StringType(kPolicyToolSessionExtension);
+  base::FileEnumerator enumerator(sessions_dir, /*recursive=*/false,
+                                  base::FileEnumerator::FILES,
+                                  sessions_pattern);
+  // A vector of session names and their last access times.
+  using Session = std::pair<base::Time, base::FilePath::StringType>;
+  std::vector<Session> sessions;
+  for (base::FilePath name = enumerator.Next(); !name.empty();
+       name = enumerator.Next()) {
+    base::File::Info info;
+    base::GetFileInfo(name, &info);
+    sessions.push_back(
+        {info.last_accessed, name.BaseName().RemoveExtension().value()});
+  }
+  // Sort the sessions by the the time of last access in decreasing order.
+  std::sort(sessions.begin(), sessions.end(), std::greater<Session>());
+
+  // Convert sessions to the list containing only names.
+  base::ListValue session_names;
+  for (const Session& session : sessions)
+    session_names.GetList().push_back(base::Value(session.second));
+  return session_names;
+}
+
+}  // namespace
 
 PolicyToolUIHandler::PolicyToolUIHandler() : callback_weak_ptr_factory_(this) {}
 
@@ -73,35 +101,8 @@ base::FilePath PolicyToolUIHandler::GetSessionPath(
   return sessions_dir_.Append(name).AddExtension(kPolicyToolSessionExtension);
 }
 
-base::ListValue PolicyToolUIHandler::GetSessionsList() {
-  base::FilePath::StringType sessions_pattern =
-      FILE_PATH_LITERAL("*.") +
-      base::FilePath::StringType(kPolicyToolSessionExtension);
-  base::FileEnumerator enumerator(sessions_dir_, /*recursive=*/false,
-                                  base::FileEnumerator::FILES,
-                                  sessions_pattern);
-  // A vector of session names and their last access times.
-  using Session = std::pair<base::Time, base::FilePath::StringType>;
-  std::vector<Session> sessions;
-  for (base::FilePath name = enumerator.Next(); !name.empty();
-       name = enumerator.Next()) {
-    base::File::Info info;
-    base::GetFileInfo(name, &info);
-    sessions.push_back(
-        {info.last_accessed, name.BaseName().RemoveExtension().value()});
-  }
-  // Sort the sessions by the the time of last access in decreasing order.
-  std::sort(sessions.begin(), sessions.end(), std::greater<Session>());
-
-  // Convert sessions to the list containing only names.
-  base::ListValue session_names;
-  for (const Session& session : sessions)
-    session_names.GetList().push_back(base::Value(session.second));
-  return session_names;
-}
-
 void PolicyToolUIHandler::SetDefaultSessionName() {
-  base::ListValue sessions = GetSessionsList();
+  base::ListValue sessions = GetSessionsList(sessions_dir_);
   if (sessions.empty()) {
     // If there are no sessions, fallback to the default session name.
     session_name_ = kPolicyToolDefaultSessionName;
@@ -180,8 +181,7 @@ void PolicyToolUIHandler::OnFileRead(const std::string& contents) {
                            base::Value(session_name_));
     base::PostTaskWithTraitsAndReplyWithResult(
         FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
-        base::BindOnce(&PolicyToolUIHandler::GetSessionsList,
-                       base::Unretained(this)),
+        base::BindOnce(&GetSessionsList, sessions_dir_),
         base::BindOnce(&PolicyToolUIHandler::OnSessionsListReceived,
                        callback_weak_ptr_factory_.GetWeakPtr()));
   }
@@ -234,15 +234,14 @@ void PolicyToolUIHandler::HandleLoadSession(const base::ListValue* args) {
   ImportFile();
 }
 
+// static
 PolicyToolUIHandler::SessionErrors PolicyToolUIHandler::DoRenameSession(
-    const base::FilePath::StringType& old_session_name,
-    const base::FilePath::StringType& new_session_name) {
-  const base::FilePath old_session_path = GetSessionPath(old_session_name);
-  const base::FilePath new_session_path = GetSessionPath(new_session_name);
-
-  // Check if the session files exist. If |old_session_name| doesn't exist, it
-  // means that is not a valid session. If |new_session_name| exists, it means
-  // that we can't do the rename because that will cause a file overwrite.
+    const base::FilePath& old_session_path,
+    const base::FilePath& new_session_path) {
+  // Check if a session files with the new name exist. If |old_session_path|
+  // doesn't exist, it means that is not a valid session. If |new_session_path|
+  // exists, it means that we can't do the rename because that will cause a file
+  // overwrite.
   if (!PathExists(old_session_path))
     return SessionErrors::kSessionNameNotExist;
   if (PathExists(new_session_path))
@@ -257,8 +256,7 @@ void PolicyToolUIHandler::OnSessionRenamed(
   if (result == SessionErrors::kNone) {
     base::PostTaskWithTraitsAndReplyWithResult(
         FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
-        base::BindOnce(&PolicyToolUIHandler::GetSessionsList,
-                       base::Unretained(this)),
+        base::BindOnce(&GetSessionsList, sessions_dir_),
         base::BindOnce(&PolicyToolUIHandler::OnSessionsListReceived,
                        callback_weak_ptr_factory_.GetWeakPtr()));
     CallJavascriptFunction("policy.Page.closeRenameSessionDialog");
@@ -299,10 +297,12 @@ void PolicyToolUIHandler::HandleRenameSession(const base::ListValue* args) {
   // will be created with the old name and with an empty dictionary in it.
   session_name_.clear();
   base::PostTaskWithTraitsAndReplyWithResult(
-      FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_BLOCKING},
+      FROM_HERE,
+      {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
+       base::TaskShutdownBehavior::BLOCK_SHUTDOWN},
       base::BindOnce(&PolicyToolUIHandler::DoRenameSession,
-                     base::Unretained(this), old_session_name,
-                     new_session_name),
+                     GetSessionPath(old_session_name),
+                     GetSessionPath(new_session_name)),
       base::BindOnce(&PolicyToolUIHandler::OnSessionRenamed,
                      callback_weak_ptr_factory_.GetWeakPtr()));
 }

chrome/browser/ui/webui/policy_tool_ui_handler.h

@@ -28,9 +28,6 @@ class PolicyToolUIHandler : public PolicyUIHandler {
     kRenamedSessionError,
   };
 
-  static const base::FilePath::CharType kPolicyToolSessionsDir[];
-  static const base::FilePath::CharType kPolicyToolDefaultSessionName[];
-  static const base::FilePath::CharType kPolicyToolSessionExtension[];
   // Reads the current session file (based on the session_name_) and sends the
   // contents to the UI.
   void ImportFile();
@@ -53,9 +50,8 @@ class PolicyToolUIHandler : public PolicyUIHandler {
   std::string ReadOrCreateFileCallback();
   void OnFileRead(const std::string& contents);
 
-  SessionErrors DoRenameSession(
-      const base::FilePath::StringType& old_session_name,
-      const base::FilePath::StringType& new_session_name);
+  static SessionErrors DoRenameSession(const base::FilePath& old_session_path,
+                                       const base::FilePath& new_session_path);
 
   void OnSessionRenamed(SessionErrors result);
 
@@ -67,10 +63,6 @@ class PolicyToolUIHandler : public PolicyUIHandler {
 
   base::FilePath GetSessionPath(const base::FilePath::StringType& name) const;
 
-  // Returns the current list of all sessions sorted by last access time in
-  // decreasing order.
-  base::ListValue GetSessionsList();
-
   void OnSessionsListReceived(base::ListValue list);
 
   void SetDefaultSessionName();