[Extensions] Ensure File API testing values are reset to defaults.

File API tests that use global variables to invoke certain behaviors
for the file chooser were not always cleaning up those variables,
or they would clean them up too late, leading to flakes with
use-after-free or unexpected values being set.

This CL move these setting these variables into RAII-style objects
that set them to the desired values, then reset them on destruction.


Bug: 1199424
Change-Id: I8c049439a6d438f915623b4cf85ee1dbbc9f4f55
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2845093
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#893960}

apps/app_restore_service_browsertest.cc

@@ -125,8 +125,8 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, FileAccessIsSavedToPrefs) {
   ASSERT_TRUE(
       base::CreateTemporaryFileInDir(temp_directory.GetPath(), &temp_file));
 
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &temp_file);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      temp_file);
   FileSystemChooseEntryFunction::RegisterTempExternalFileSystemForTest(
       "temp", temp_directory.GetPath());
 
@@ -166,8 +166,8 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, MAYBE_FileAccessIsRestored) {
   ASSERT_TRUE(
       base::CreateTemporaryFileInDir(temp_directory.GetPath(), &temp_file));
 
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &temp_file);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      temp_file);
   FileSystemChooseEntryFunction::RegisterTempExternalFileSystemForTest(
       "temp", temp_directory.GetPath());
 

chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc

@@ -140,7 +140,6 @@ class FileSystemApiTestForDrive : public PlatformAppBrowserTest {
   }
 
   void TearDown() override {
-    FileSystemChooseEntryFunction::StopSkippingPickerForTest();
     PlatformAppBrowserTest::TearDown();
   }
 
@@ -328,8 +327,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
                        FileSystemApiOpenExistingFileTest) {
   base::FilePath test_file =
       GetDriveMountPoint().AppendASCII("root/open_existing.txt");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_file);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_file);
   ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_existing",
                                {.launch_as_platform_app = true}))
       << message_;
@@ -339,8 +338,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
                        FileSystemApiOpenExistingFileWithWriteTest) {
   base::FilePath test_file =
       GetDriveMountPoint().AppendASCII("root/open_existing.txt");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_file);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_file);
   ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_existing_with_write",
                                {.launch_as_platform_app = true}))
       << message_;
@@ -352,7 +351,7 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
       GetDriveMountPoint().AppendASCII("root/open_existing.txt");
   ASSERT_TRUE(base::PathService::OverrideAndCreateIfNeeded(
       chrome::DIR_USER_DOCUMENTS, test_file.DirName(), true, false));
-  FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest();
+  FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest picker;
   ASSERT_TRUE(
       RunExtensionTest("api_test/file_system/open_multiple_with_suggested_name",
                        {.launch_as_platform_app = true}))
@@ -368,8 +367,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
   std::vector<base::FilePath> test_files;
   test_files.push_back(test_file1);
   test_files.push_back(test_file2);
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest(
-      &test_files);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest picker(
+      test_files);
   ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_multiple_existing",
                                {.launch_as_platform_app = true}))
       << message_;
@@ -379,8 +378,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
                        FileSystemApiOpenDirectoryTest) {
   base::FilePath test_directory =
       GetDriveMountPoint().AppendASCII("root/subdir");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_directory);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_directory);
   ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_directory",
                                {.launch_as_platform_app = true}))
       << message_;
@@ -390,8 +389,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
                        FileSystemApiOpenDirectoryWithWriteTest) {
   base::FilePath test_directory =
       GetDriveMountPoint().AppendASCII("root/subdir");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_directory);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_directory);
   ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_directory_with_write",
                                {.launch_as_platform_app = true}))
       << message_;
@@ -401,8 +400,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
                        FileSystemApiOpenDirectoryWithoutPermissionTest) {
   base::FilePath test_directory =
       GetDriveMountPoint().AppendASCII("root/subdir");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_directory);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_directory);
   ASSERT_TRUE(
       RunExtensionTest("api_test/file_system/open_directory_without_permission",
                        {.launch_as_platform_app = true}))
@@ -413,8 +412,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
                        FileSystemApiOpenDirectoryWithOnlyWritePermissionTest) {
   base::FilePath test_directory =
       GetDriveMountPoint().AppendASCII("root/subdir");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_directory);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_directory);
   ASSERT_TRUE(
       RunExtensionTest("api_test/file_system/open_directory_with_only_write",
                        {.launch_as_platform_app = true}))
@@ -425,8 +424,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
                        FileSystemApiSaveNewFileTest) {
   base::FilePath test_file =
       GetDriveMountPoint().AppendASCII("root/save_new.txt");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_file);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_file);
   ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_new",
                                {.launch_as_platform_app = true}))
       << message_;
@@ -436,8 +435,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
                        FileSystemApiSaveExistingFileTest) {
   base::FilePath test_file =
       GetDriveMountPoint().AppendASCII("root/save_existing.txt");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_file);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_file);
   ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_existing",
                                {.launch_as_platform_app = true}))
       << message_;
@@ -447,8 +446,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
     FileSystemApiSaveNewFileWithWriteTest) {
   base::FilePath test_file =
       GetDriveMountPoint().AppendASCII("root/save_new.txt");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_file);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_file);
   ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_new_with_write",
                                {.launch_as_platform_app = true}))
       << message_;
@@ -458,8 +457,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
     FileSystemApiSaveExistingFileWithWriteTest) {
   base::FilePath test_file =
       GetDriveMountPoint().AppendASCII("root/save_existing.txt");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_file);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_file);
   ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_existing_with_write",
                                {.launch_as_platform_app = true}))
       << message_;

chrome/browser/extensions/api/image_writer_private/image_writer_private_apitest.cc

@@ -71,7 +71,6 @@ class ImageWriterPrivateApiTest : public ExtensionApiTest {
     ExtensionApiTest::TearDownInProcessBrowserTestFixture();
     test_utils_.TearDown();
     RemovableStorageProvider::ClearDeviceListForTesting();
-    FileSystemChooseEntryFunction::StopSkippingPickerForTest();
   }
 
 
@@ -89,8 +88,8 @@ IN_PROC_BROWSER_TEST_F(ImageWriterPrivateApiTest, TestWriteFromFile) {
       "test_temp", test_utils_.GetTempDir());
 
   base::FilePath selected_image(test_utils_.GetImagePath());
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &selected_image);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      selected_image);
 
 #if !BUILDFLAG(IS_CHROMEOS_ASH)
   auto set_up_utility_client_callbacks = [](FakeImageWriterClient* client) {

chrome/browser/extensions/native_bindings_apitest.cc

@@ -138,8 +138,8 @@ IN_PROC_BROWSER_TEST_F(NativeBindingsApiTest, FileSystemApiGetDisplayPath) {
   FileSystemChooseEntryFunction::RegisterTempExternalFileSystemForTest(
       "test_root", test_dir);
   base::FilePath test_file = test_dir.AppendASCII("text.txt");
-  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-      &test_file);
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
+      test_file);
   ASSERT_TRUE(RunExtensionTest("native_bindings/instance_of",
                                {.launch_as_platform_app = true}))
       << message_;

extensions/browser/api/file_system/file_system_api.cc

@@ -102,11 +102,20 @@ namespace {
 
 bool g_skip_picker_for_test = false;
 bool g_use_suggested_path_for_test = false;
-base::FilePath* g_path_to_be_picked_for_test;
-std::vector<base::FilePath>* g_paths_to_be_picked_for_test;
+const base::FilePath* g_path_to_be_picked_for_test = nullptr;
+const std::vector<base::FilePath>* g_paths_to_be_picked_for_test = nullptr;
 bool g_skip_directory_confirmation_for_test = false;
 bool g_allow_directory_access_for_test = false;
 
+void ResetTestValuesToDefaults() {
+  g_skip_picker_for_test = false;
+  g_use_suggested_path_for_test = false;
+  g_path_to_be_picked_for_test = nullptr;
+  g_paths_to_be_picked_for_test = nullptr;
+  g_skip_directory_confirmation_for_test = false;
+  g_allow_directory_access_for_test = false;
+}
+
 // Expand the mime-types and extensions provided in an AcceptOption, returning
 // them within the passed extension vector. Returns false if no valid types
 // were found.
@@ -422,59 +431,54 @@ void FileSystemChooseEntryFunction::ShowPicker(
   }
 }
 
-// static
-void FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
-    base::FilePath* path) {
-  g_skip_picker_for_test = true;
-  g_use_suggested_path_for_test = false;
-  g_path_to_be_picked_for_test = path;
-  g_paths_to_be_picked_for_test = NULL;
-}
+FileSystemChooseEntryFunction::SkipPickerBaseForTest*
+    FileSystemChooseEntryFunction::SkipPickerBaseForTest::g_picker = nullptr;
 
-// static
-void FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest(
-    std::vector<base::FilePath>* paths) {
-  g_skip_picker_for_test = true;
-  g_use_suggested_path_for_test = false;
-  g_paths_to_be_picked_for_test = paths;
+FileSystemChooseEntryFunction::SkipPickerBaseForTest::SkipPickerBaseForTest() {
+  CHECK(!g_picker);
+  g_picker = this;
 }
 
-// static
-void FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest() {
-  g_skip_picker_for_test = true;
-  g_use_suggested_path_for_test = true;
-  g_path_to_be_picked_for_test = NULL;
-  g_paths_to_be_picked_for_test = NULL;
+FileSystemChooseEntryFunction::SkipPickerBaseForTest::~SkipPickerBaseForTest() {
+  DCHECK_EQ(this, g_picker);
+  ResetTestValuesToDefaults();
+  g_picker = nullptr;
 }
 
-// static
-void FileSystemChooseEntryFunction::SkipPickerAndAlwaysCancelForTest() {
+FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest::
+    SkipPickerAndAlwaysSelectPathForTest(const base::FilePath& path,
+                                         bool skip_dir_confirmation,
+                                         bool allow_directory_access)
+    : path_(path) {
   g_skip_picker_for_test = true;
-  g_use_suggested_path_for_test = false;
-  g_path_to_be_picked_for_test = NULL;
-  g_paths_to_be_picked_for_test = NULL;
+  g_path_to_be_picked_for_test = &path_;
+  g_skip_directory_confirmation_for_test = skip_dir_confirmation;
+  g_allow_directory_access_for_test = allow_directory_access;
 }
 
-// static
-void FileSystemChooseEntryFunction::StopSkippingPickerForTest() {
-  g_skip_picker_for_test = false;
-}
+FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest::
+    ~SkipPickerAndAlwaysSelectPathForTest() = default;
 
-// static
-void FileSystemChooseEntryFunction::SkipDirectoryConfirmationForTest() {
-  g_skip_directory_confirmation_for_test = true;
-  g_allow_directory_access_for_test = true;
+FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest::
+    SkipPickerAndAlwaysSelectPathsForTest(
+        const std::vector<base::FilePath>& paths)
+    : paths_(paths) {
+  g_skip_picker_for_test = true;
+  g_paths_to_be_picked_for_test = &paths_;
 }
 
-// static
-void FileSystemChooseEntryFunction::AutoCancelDirectoryConfirmationForTest() {
-  g_skip_directory_confirmation_for_test = true;
-  g_allow_directory_access_for_test = false;
+FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest::
+    ~SkipPickerAndAlwaysSelectPathsForTest() = default;
+
+FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest::
+    SkipPickerAndSelectSuggestedPathForTest() {
+  g_skip_picker_for_test = true;
+  g_use_suggested_path_for_test = true;
 }
 
-// static
-void FileSystemChooseEntryFunction::StopSkippingDirectoryConfirmationForTest() {
-  g_skip_directory_confirmation_for_test = false;
+FileSystemChooseEntryFunction::SkipPickerAndAlwaysCancelForTest::
+    SkipPickerAndAlwaysCancelForTest() {
+  g_skip_picker_for_test = true;
 }
 
 // static

extensions/browser/api/file_system/file_system_api.h

@@ -117,17 +117,54 @@ class FileSystemIsWritableEntryFunction : public ExtensionFunction {
 
 class FileSystemChooseEntryFunction : public FileSystemEntryFunction {
  public:
-  // Allow picker UI to be skipped in testing.
-  static void SkipPickerAndAlwaysSelectPathForTest(base::FilePath* path);
-  static void SkipPickerAndAlwaysSelectPathsForTest(
-      std::vector<base::FilePath>* paths);
-  static void SkipPickerAndSelectSuggestedPathForTest();
-  static void SkipPickerAndAlwaysCancelForTest();
-  static void StopSkippingPickerForTest();
-  // Allow directory access confirmation UI to be skipped in testing.
-  static void SkipDirectoryConfirmationForTest();
-  static void AutoCancelDirectoryConfirmationForTest();
-  static void StopSkippingDirectoryConfirmationForTest();
+  class SkipPickerBaseForTest {
+   protected:
+    SkipPickerBaseForTest();
+    ~SkipPickerBaseForTest();
+
+   private:
+    // Nested pickers are not allowed, so track the singleton
+    // instance.
+    static SkipPickerBaseForTest* g_picker;
+  };
+
+  // Various classes to to allow the picker UI to be skipped in testing.
+  // Upon destruction, the affected global variables are reset to their
+  // default values;
+  class SkipPickerAndAlwaysSelectPathForTest : public SkipPickerBaseForTest {
+   public:
+    explicit SkipPickerAndAlwaysSelectPathForTest(
+        const base::FilePath& path,
+        bool skip_dir_confirmation = false,
+        bool allow_directory_access = false);
+    ~SkipPickerAndAlwaysSelectPathForTest();
+
+   private:
+    const base::FilePath path_;
+  };
+
+  class SkipPickerAndAlwaysSelectPathsForTest : public SkipPickerBaseForTest {
+   public:
+    explicit SkipPickerAndAlwaysSelectPathsForTest(
+        const std::vector<base::FilePath>& paths);
+    ~SkipPickerAndAlwaysSelectPathsForTest();
+
+   private:
+    const std::vector<base::FilePath> paths_;
+  };
+
+  class SkipPickerAndSelectSuggestedPathForTest : public SkipPickerBaseForTest {
+   public:
+    SkipPickerAndSelectSuggestedPathForTest();
+    ~SkipPickerAndSelectSuggestedPathForTest() = default;
+  };
+
+  class SkipPickerAndAlwaysCancelForTest : public SkipPickerBaseForTest {
+   public:
+    SkipPickerAndAlwaysCancelForTest();
+    ~SkipPickerAndAlwaysCancelForTest() = default;
+  };
+
   // Call this with the directory for test file paths. On Chrome OS, accessed
   // path needs to be explicitly registered for smooth integration with Google
   // Drive support.