From c86e2842a5e54a3c860898dbccdcd802877edcef Mon Sep 17 00:00:00 2001 From: David Bertoni Date: Fri, 18 Jun 2021 21:38:09 +0000 Subject: [PATCH] [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 Reviewed-by: Devlin Commit-Queue: David Bertoni Cr-Commit-Position: refs/heads/master@{#893960} --- apps/app_restore_service_browsertest.cc | 8 +- .../api/file_system/file_system_apitest.cc | 157 +++++++++--------- .../file_system_apitest_chromeos.cc | 47 +++--- .../image_writer_private_apitest.cc | 5 +- .../extensions/native_bindings_apitest.cc | 4 +- .../api/file_system/file_system_api.cc | 88 +++++----- .../browser/api/file_system/file_system_api.h | 59 +++++-- 7 files changed, 202 insertions(+), 166 deletions(-) diff --git a/apps/app_restore_service_browsertest.cc b/apps/app_restore_service_browsertest.cc index 015e7029246d8f..bc84191ed1a7f2 100644 --- a/apps/app_restore_service_browsertest.cc +++ b/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()); diff --git a/chrome/browser/extensions/api/file_system/file_system_apitest.cc b/chrome/browser/extensions/api/file_system/file_system_apitest.cc index 1137d977c5bd71..3e289ae0424dc2 100644 --- a/chrome/browser/extensions/api/file_system/file_system_apitest.cc +++ b/chrome/browser/extensions/api/file_system/file_system_apitest.cc @@ -87,7 +87,6 @@ class FileSystemApiTest : public PlatformAppBrowserTest { } void TearDown() override { - FileSystemChooseEntryFunction::StopSkippingPickerForTest(); PlatformAppBrowserTest::TearDown(); } @@ -157,8 +156,8 @@ class FileSystemApiTest : public PlatformAppBrowserTest { IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiGetDisplayPath) { base::FilePath test_file = test_root_folder_.AppendASCII("gold.txt"); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/get_display_path", {.launch_as_platform_app = true})) << message_; @@ -173,8 +172,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiGetDisplayPathPrettify) { } base::FilePath test_file = test_root_folder_.AppendASCII("gold.txt"); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/get_display_path_prettify", {.launch_as_platform_app = true})) << message_; @@ -196,8 +195,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, EXPECT_TRUE(base::CopyFile(source, test_file)); } - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE( RunExtensionTest("api_test/file_system/get_display_path_prettify_mac", {.launch_as_platform_app = true})) @@ -208,8 +207,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenExistingFileTest) { base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_existing", {.launch_as_platform_app = true})) << message_; @@ -220,8 +219,7 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenExistingFileUsingPreviousPathTest) { base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction:: - SkipPickerAndSelectSuggestedPathForTest(); + FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest picker; { AppLoadObserver observer( profile(), @@ -243,8 +241,7 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, ASSERT_TRUE(base::PathService::OverrideAndCreateIfNeeded( chrome::DIR_USER_DOCUMENTS, test_file.DirName(), false, false)); } - FileSystemChooseEntryFunction:: - SkipPickerAndSelectSuggestedPathForTest(); + FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest picker; { AppLoadObserver observer( profile(), @@ -269,8 +266,7 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, ASSERT_TRUE(base::PathService::OverrideAndCreateIfNeeded( chrome::DIR_USER_DOCUMENTS, test_file.DirName(), false, false)); } - FileSystemChooseEntryFunction:: - SkipPickerAndSelectSuggestedPathForTest(); + FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest picker; ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_existing", {.launch_as_platform_app = true})) << message_; @@ -285,7 +281,7 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenMultipleSuggested) { ASSERT_TRUE(base::PathService::OverrideAndCreateIfNeeded( chrome::DIR_USER_DOCUMENTS, test_file.DirName(), false, false)); } - FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest(); + FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest picker; ASSERT_TRUE( RunExtensionTest("api_test/file_system/open_multiple_with_suggested_name", {.launch_as_platform_app = true})) @@ -300,8 +296,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, names.push_back("open_existing2.txt"); std::vector test_files = TempFilePaths(names, true); ASSERT_EQ(2u, test_files.size()); - 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_; @@ -311,8 +307,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenDirectoryTest) { base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); base::FilePath test_directory = test_file.DirName(); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_directory); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_directory); ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_directory", {.launch_as_platform_app = true})) << message_; @@ -324,8 +320,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); base::FilePath test_directory = test_file.DirName(); - 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_; @@ -337,8 +333,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); base::FilePath test_directory = test_file.DirName(); - 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})) @@ -351,8 +347,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); base::FilePath test_directory = test_file.DirName(); - 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})) @@ -363,17 +359,17 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, #if defined(OS_WIN) || defined(OS_POSIX) IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenDirectoryOnGraylistAndAllowTest) { - FileSystemChooseEntryFunction::SkipDirectoryConfirmationForTest(); base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); base::FilePath test_directory = test_file.DirName(); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_directory, /*skip_dir_confirmation=*/true, + /*allow_directory_access=*/true); { base::ScopedAllowBlockingForTesting allow_blocking; ASSERT_TRUE(base::PathService::OverrideAndCreateIfNeeded( kGraylistedPath, test_directory, false, false)); } - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_directory); ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_directory", {.launch_as_platform_app = true})) << message_; @@ -382,17 +378,18 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenDirectoryOnGraylistTest) { - FileSystemChooseEntryFunction::AutoCancelDirectoryConfirmationForTest(); base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); base::FilePath test_directory = test_file.DirName(); + // If a dialog is erroneously displayed, auto cancel it, so that the test + // fails. + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_directory, /*skip_dir_confirmation=*/true); { base::ScopedAllowBlockingForTesting allow_blocking; ASSERT_TRUE(base::PathService::OverrideAndCreateIfNeeded( kGraylistedPath, test_directory, false, false)); } - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_directory); ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_directory_cancel", {.launch_as_platform_app = true})) << message_; @@ -401,18 +398,19 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenDirectoryContainingGraylistTest) { - FileSystemChooseEntryFunction::AutoCancelDirectoryConfirmationForTest(); base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); base::FilePath test_directory = test_file.DirName(); base::FilePath parent_directory = test_directory.DirName(); + // If a dialog is erroneously displayed, auto cancel it, so that the test + // fails. + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + parent_directory, /*skip_dir_confirmation=*/true); { base::ScopedAllowBlockingForTesting allow_blocking; ASSERT_TRUE(base::PathService::OverrideAndCreateIfNeeded( kGraylistedPath, test_directory, false, false)); } - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &parent_directory); ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_directory_cancel", {.launch_as_platform_app = true})) << message_; @@ -422,20 +420,19 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, // Test that choosing a subdirectory of a path does not require confirmation. IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenDirectorySubdirectoryOfGraylistTest) { - // If a dialog is erroneously displayed, auto cancel it, so that the test - // fails. - FileSystemChooseEntryFunction::AutoCancelDirectoryConfirmationForTest(); base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); base::FilePath test_directory = test_file.DirName(); base::FilePath parent_directory = test_directory.DirName(); + // If a dialog is erroneously displayed, auto cancel it, so that the test + // fails. + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_directory, /*skip_dir_confirmation=*/true); { base::ScopedAllowBlockingForTesting allow_blocking; ASSERT_TRUE(base::PathService::OverrideAndCreateIfNeeded( kGraylistedPath, parent_directory, false, false)); } - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_directory); ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_directory", {.launch_as_platform_app = true})) << message_; @@ -447,8 +444,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiInvalidChooseEntryTypeTest) { base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/invalid_choose_file_type", {.launch_as_platform_app = true})) << message_; @@ -459,8 +456,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenExistingFileWithWriteTest) { base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); - 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_; @@ -471,8 +468,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenWritableExistingFileTest) { base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_writable_existing", {.launch_as_platform_app = true})) << message_; @@ -483,8 +480,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenWritableExistingFileWithWriteTest) { base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE( RunExtensionTest("api_test/file_system/open_writable_existing_with_write", {.launch_as_platform_app = true})) @@ -499,8 +496,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, names.push_back("open_existing2.txt"); std::vector test_files = TempFilePaths(names, true); ASSERT_EQ(2u, test_files.size()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest( - &test_files); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest picker( + test_files); ASSERT_TRUE(RunExtensionTest( "api_test/file_system/open_multiple_writable_existing_with_write", {.launch_as_platform_app = true})) @@ -508,7 +505,7 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, } IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenCancelTest) { - FileSystemChooseEntryFunction::SkipPickerAndAlwaysCancelForTest(); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysCancelForTest picker; ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_cancel", {.launch_as_platform_app = true})) << message_; @@ -524,8 +521,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiOpenBackgroundTest) { IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiSaveNewFileTest) { base::FilePath test_file = TempFilePath("save_new.txt", false); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_new", {.launch_as_platform_app = true})) << message_; @@ -535,8 +532,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiSaveNewFileTest) { IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiSaveExistingFileTest) { base::FilePath test_file = TempFilePath("save_existing.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_existing", {.launch_as_platform_app = true})) << message_; @@ -547,8 +544,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiSaveNewFileWithWriteTest) { base::FilePath test_file = TempFilePath("save_new.txt", false); ASSERT_FALSE(test_file.empty()); - 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_; @@ -559,8 +556,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiSaveExistingFileWithWriteTest) { base::FilePath test_file = TempFilePath("save_existing.txt", true); ASSERT_FALSE(test_file.empty()); - 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_; @@ -573,15 +570,15 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiSaveMultipleFilesTest) { names.push_back("save2.txt"); std::vector test_files = TempFilePaths(names, false); ASSERT_EQ(2u, test_files.size()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest( - &test_files); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest picker( + test_files); ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_multiple", {.launch_as_platform_app = true})) << message_; } IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiSaveCancelTest) { - FileSystemChooseEntryFunction::SkipPickerAndAlwaysCancelForTest(); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysCancelForTest picker; ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_cancel", {.launch_as_platform_app = true})) << message_; @@ -596,8 +593,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiSaveBackgroundTest) { IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiGetWritableTest) { base::FilePath test_file = TempFilePath("writable.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/get_writable_file_entry", {.launch_as_platform_app = true})) << message_; @@ -607,8 +604,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiGetWritableWithWriteTest) { base::FilePath test_file = TempFilePath("writable.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest( "api_test/file_system/get_writable_file_entry_with_write", {.launch_as_platform_app = true})) @@ -619,8 +616,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiGetWritableRootEntryTest) { base::FilePath test_file = TempFilePath("writable.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/get_writable_root_entry", {.launch_as_platform_app = true})) << message_; @@ -629,8 +626,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiIsWritableTest) { base::FilePath test_file = TempFilePath("writable.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/is_writable_file_entry", {.launch_as_platform_app = true})) << message_; @@ -640,8 +637,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiIsWritableWithWritePermissionTest) { base::FilePath test_file = TempFilePath("writable.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE( RunExtensionTest("api_test/file_system/is_writable_file_entry_with_write", {.launch_as_platform_app = true})) @@ -651,8 +648,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiRetainEntry) { base::FilePath test_file = TempFilePath("writable.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); ASSERT_TRUE(RunExtensionTest("api_test/file_system/retain_entry", {.launch_as_platform_app = true})) << message_; @@ -669,8 +666,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiRetainDirectoryEntry) { base::FilePath test_file = TempFilePath("open_existing.txt", true); ASSERT_FALSE(test_file.empty()); base::FilePath test_directory = test_file.DirName(); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_directory); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_directory); ASSERT_TRUE(RunExtensionTest("api_test/file_system/retain_directory", {.launch_as_platform_app = true})) << message_; @@ -686,8 +683,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiRetainDirectoryEntry) { IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiRestoreEntry) { base::FilePath test_file = TempFilePath("writable.txt", true); ASSERT_FALSE(test_file.empty()); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); AppLoadObserver observer( profile(), base::BindRepeating(AddSavedEntry, test_file, false, apps::SavedFilesService::Get(profile()))); @@ -700,8 +697,8 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTest, FileSystemApiRestoreDirectoryEntry) { base::FilePath test_file = TempFilePath("writable.txt", true); ASSERT_FALSE(test_file.empty()); base::FilePath test_directory = test_file.DirName(); - FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest( - &test_file); + FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker( + test_file); AppLoadObserver observer( profile(), base::BindRepeating(AddSavedEntry, test_directory, true, apps::SavedFilesService::Get(profile()))); diff --git a/chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc b/chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc index c48ce9dd36ca48..77d07ac192e7e4 100644 --- a/chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc +++ b/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 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_; diff --git a/chrome/browser/extensions/api/image_writer_private/image_writer_private_apitest.cc b/chrome/browser/extensions/api/image_writer_private/image_writer_private_apitest.cc index 52302d656e5232..7ef2586252d11e 100644 --- a/chrome/browser/extensions/api/image_writer_private/image_writer_private_apitest.cc +++ b/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) { diff --git a/chrome/browser/extensions/native_bindings_apitest.cc b/chrome/browser/extensions/native_bindings_apitest.cc index daf35ee8870a25..cc88864b0bd027 100644 --- a/chrome/browser/extensions/native_bindings_apitest.cc +++ b/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_; diff --git a/extensions/browser/api/file_system/file_system_api.cc b/extensions/browser/api/file_system/file_system_api.cc index a1dad9081c4ff7..14b2b07e9ae0a1 100644 --- a/extensions/browser/api/file_system/file_system_api.cc +++ b/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* g_paths_to_be_picked_for_test; +const base::FilePath* g_path_to_be_picked_for_test = nullptr; +const std::vector* 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* 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& 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 diff --git a/extensions/browser/api/file_system/file_system_api.h b/extensions/browser/api/file_system/file_system_api.h index ae1588ce8536e4..0986b703d0e57b 100644 --- a/extensions/browser/api/file_system/file_system_api.h +++ b/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* 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& paths); + ~SkipPickerAndAlwaysSelectPathsForTest(); + + private: + const std::vector 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.