mac: Rework logic for excluding files from Time Machine backups.

This CL:
1) Re-enables MacUtilTest.TestExcludeFileFromBackups, which was disabled
   due to failures on a bot that does not exist anymore.
2) Extracts the logic used to check base::mac::SetFileBackupExclusion()
   into base::mac::GetFileBackupExclusion(). This logic was duplicated
   in tests.
3) Uses the approach in //sql for converting a base::FilePath into the
   CFURLRef needed by CSBackup{Is,Set}ItemExcluded. This approach does
   not rely on an autorelease pool. The conversion logic is extracted in
   base::mac::FilePathToCFURL().

Change-Id: I6ed2a3ded285e6904dccf7e21ade7ff5f68167a2
Reviewed-on: https://chromium-review.googlesource.com/c/1438035
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Auto-Submit: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626702}

base/mac/foundation_util.h

@@ -386,6 +386,13 @@ BASE_EXPORT NSString* FilePathToNSString(const FilePath& path);
 // Converts |str| to a FilePath. Returns an empty path if |str| is nil.
 BASE_EXPORT FilePath NSStringToFilePath(NSString* str);
 
+// Converts a non-null |path| to a CFURLRef. |path| must not be empty.
+//
+// This function only uses manually-owned resources, so it does not depend on an
+// NSAutoreleasePool being set up on the current thread.
+BASE_EXPORT base::ScopedCFTypeRef<CFURLRef> FilePathToCFURL(
+    const FilePath& path);
+
 #if defined(__OBJC__)
 // Converts |range| to an NSRange, returning the new range in |range_out|.
 // Returns true if conversion was successful, false if the values of |range|

base/mac/foundation_util.mm

@@ -448,6 +448,25 @@ FilePath NSStringToFilePath(NSString* str) {
   return FilePath([str fileSystemRepresentation]);
 }
 
+base::ScopedCFTypeRef<CFURLRef> FilePathToCFURL(const FilePath& path) {
+  DCHECK(!path.empty());
+
+  // The function's docs promise that it does not require an NSAutoreleasePool.
+  // A straightforward way to accomplish this is to use *Create* functions,
+  // combined with base::ScopedCFTypeRef.
+  const std::string& path_string = path.value();
+  base::ScopedCFTypeRef<CFStringRef> path_cfstring(CFStringCreateWithBytes(
+      kCFAllocatorDefault, reinterpret_cast<const UInt8*>(path_string.data()),
+      path_string.length(), kCFStringEncodingUTF8,
+      /*isExternalRepresentation=*/FALSE));
+  if (!path_cfstring)
+    return base::ScopedCFTypeRef<CFURLRef>();
+
+  return base::ScopedCFTypeRef<CFURLRef>(CFURLCreateWithFileSystemPath(
+      kCFAllocatorDefault, path_cfstring, kCFURLPOSIXPathStyle,
+      /*isDirectory=*/FALSE));
+}
+
 bool CFRangeToNSRange(CFRange range, NSRange* range_out) {
   if (base::IsValueInRangeForNumericType<decltype(range_out->location)>(
           range.location) &&

base/mac/foundation_util_unittest.mm

@@ -316,6 +316,11 @@
   EXPECT_EQ(FilePath("/a/b"), NSStringToFilePath(@"/a/b"));
 }
 
+TEST(FoundationUtilTest, FilePathToCFURL) {
+  EXPECT_NSEQ([NSURL fileURLWithPath:@"/a/b"],
+              base::mac::CFToNSCast(FilePathToCFURL(FilePath("/a/b"))));
+}
+
 TEST(FoundationUtilTest, CFRangeToNSRange) {
   NSRange range_out;
   EXPECT_TRUE(CFRangeToNSRange(CFRangeMake(10, 5), &range_out));

base/mac/mac_util.h

@@ -63,7 +63,11 @@ BASE_EXPORT void ReleaseFullScreen(FullScreenMode mode);
 BASE_EXPORT void SwitchFullScreenModes(FullScreenMode from_mode,
                                        FullScreenMode to_mode);
 
-// Excludes the file given by |file_path| from being backed up by Time Machine.
+// Returns true if the file at |file_path| is excluded from Time Machine
+// backups.
+BASE_EXPORT bool GetFileBackupExclusion(const FilePath& file_path);
+
+// Excludes the file given by |file_path| from Time Machine backups.
 BASE_EXPORT bool SetFileBackupExclusion(const FilePath& file_path);
 
 // Checks if the current application is set as a Login Item, so it will launch

base/mac/mac_util.mm

@@ -212,18 +212,20 @@ void SwitchFullScreenModes(FullScreenMode from_mode, FullScreenMode to_mode) {
   SetUIMode();
 }
 
-bool SetFileBackupExclusion(const FilePath& file_path) {
-  NSString* file_path_ns = base::mac::FilePathToNSString(file_path);
-  NSURL* file_url = [NSURL fileURLWithPath:file_path_ns];
+bool GetFileBackupExclusion(const FilePath& file_path) {
+  return CSBackupIsItemExcluded(FilePathToCFURL(file_path), nullptr);
+}
 
+bool SetFileBackupExclusion(const FilePath& file_path) {
   // When excludeByPath is true the application must be running with root
   // privileges (admin for 10.6 and earlier) but the URL does not have to
   // already exist. When excludeByPath is false the URL must already exist but
   // can be used in non-root (or admin as above) mode. We use false so that
   // non-root (or admin) users don't get their TimeMachine drive filled up with
   // unnecessary backups.
-  OSStatus os_err =
-      CSBackupSetItemExcluded(base::mac::NSToCFCast(file_url), TRUE, FALSE);
+  OSStatus os_err = CSBackupSetItemExcluded(FilePathToCFURL(file_path),
+                                            /*exclude=*/TRUE,
+                                            /*excludeByPath=*/FALSE);
   if (os_err != noErr) {
     OSSTATUS_DLOG(WARNING, os_err)
         << "Failed to set backup exclusion for file '"

base/mac/mac_util_unittest.mm

@@ -95,8 +95,7 @@
   }
 }
 
-// http://crbug.com/425745
-TEST_F(MacUtilTest, DISABLED_TestExcludeFileFromBackups) {
+TEST_F(MacUtilTest, TestExcludeFileFromBackups) {
   // The file must already exist in order to set its exclusion property.
   ScopedTempDir temp_dir_;
   ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
@@ -105,16 +104,17 @@
   // Dump something real into the file.
   ASSERT_EQ(static_cast<int>(base::size(dummy_data)),
             WriteFile(dummy_file_path, dummy_data, base::size(dummy_data)));
-  NSString* fileURLString = base::mac::FilePathToNSString(dummy_file_path);
-  NSURL* fileURL = [NSURL URLWithString:fileURLString];
   // Initial state should be non-excluded.
-  EXPECT_FALSE(CSBackupIsItemExcluded(base::mac::NSToCFCast(fileURL), NULL));
+  EXPECT_FALSE(GetFileBackupExclusion(dummy_file_path));
   // Exclude the file.
-  EXPECT_TRUE(SetFileBackupExclusion(dummy_file_path));
-  // SetFileBackupExclusion never excludes by path.
+  ASSERT_TRUE(SetFileBackupExclusion(dummy_file_path));
+  EXPECT_TRUE(GetFileBackupExclusion(dummy_file_path));
+
+  // Ensure that SetFileBackupExclusion never excludes by path.
+  base::ScopedCFTypeRef<CFURLRef> file_url =
+      base::mac::FilePathToCFURL(dummy_file_path);
   Boolean excluded_by_path = FALSE;
-  Boolean excluded =
-      CSBackupIsItemExcluded(base::mac::NSToCFCast(fileURL), &excluded_by_path);
+  Boolean excluded = CSBackupIsItemExcluded(file_url, &excluded_by_path);
   EXPECT_TRUE(excluded);
   EXPECT_FALSE(excluded_by_path);
 }

sql/sqlite_features_unittest.cc

@@ -20,11 +20,7 @@
 #include "third_party/sqlite/sqlite3.h"
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
-#include <CoreFoundation/CoreFoundation.h>
-#include <CoreServices/CoreServices.h>
-
 #include "base/mac/mac_util.h"
-#include "base/mac/scoped_cftyperef.h"
 #endif
 
 // Test that certain features are/are-not enabled in our SQLite.
@@ -305,47 +301,30 @@ TEST_F(SQLiteFeaturesTest, CachedRegexp) {
 }
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
-base::ScopedCFTypeRef<CFURLRef> CFURLRefForPath(const base::FilePath& path){
-  base::ScopedCFTypeRef<CFStringRef> urlString(
-      CFStringCreateWithFileSystemRepresentation(
-          kCFAllocatorDefault, path.value().c_str()));
-  base::ScopedCFTypeRef<CFURLRef> url(
-      CFURLCreateWithFileSystemPath(kCFAllocatorDefault, urlString,
-                                    kCFURLPOSIXPathStyle, FALSE));
-  return url;
-}
-
 // If a database file is marked to be excluded from Time Machine, verify that
 // journal files are also excluded.
 TEST_F(SQLiteFeaturesTest, TimeMachine) {
   ASSERT_TRUE(db().Execute("CREATE TABLE t (id INTEGER PRIMARY KEY)"));
   db().Close();
 
-  base::FilePath journal = sql::Database::JournalPath(db_path());
+  base::FilePath journal_path = sql::Database::JournalPath(db_path());
   ASSERT_TRUE(GetPathExists(db_path()));
-  ASSERT_TRUE(GetPathExists(journal));
-
-  base::ScopedCFTypeRef<CFURLRef> dbURL(CFURLRefForPath(db_path()));
-  base::ScopedCFTypeRef<CFURLRef> journalURL(CFURLRefForPath(journal));
+  ASSERT_TRUE(GetPathExists(journal_path));
 
   // Not excluded to start.
-  EXPECT_FALSE(CSBackupIsItemExcluded(dbURL, nullptr));
-  EXPECT_FALSE(CSBackupIsItemExcluded(journalURL, nullptr));
+  EXPECT_FALSE(base::mac::GetFileBackupExclusion(db_path()));
+  EXPECT_FALSE(base::mac::GetFileBackupExclusion(journal_path));
 
   // Exclude the main database file.
   EXPECT_TRUE(base::mac::SetFileBackupExclusion(db_path()));
 
-  Boolean excluded_by_path = FALSE;
-  EXPECT_TRUE(CSBackupIsItemExcluded(dbURL, &excluded_by_path));
-  EXPECT_FALSE(excluded_by_path);
-  EXPECT_FALSE(CSBackupIsItemExcluded(journalURL, nullptr));
+  EXPECT_TRUE(base::mac::GetFileBackupExclusion(db_path()));
+  EXPECT_FALSE(base::mac::GetFileBackupExclusion(journal_path));
 
   EXPECT_TRUE(db().Open(db_path()));
   ASSERT_TRUE(db().Execute("INSERT INTO t VALUES (1)"));
-  EXPECT_TRUE(CSBackupIsItemExcluded(dbURL, &excluded_by_path));
-  EXPECT_FALSE(excluded_by_path);
-  EXPECT_TRUE(CSBackupIsItemExcluded(journalURL, &excluded_by_path));
-  EXPECT_FALSE(excluded_by_path);
+  EXPECT_TRUE(base::mac::GetFileBackupExclusion(db_path()));
+  EXPECT_TRUE(base::mac::GetFileBackupExclusion(journal_path));
 
   // TODO(shess): In WAL mode this will touch -wal and -shm files.  -shm files
   // could be always excluded.

sql/vfs_wrapper.cc

@@ -9,17 +9,14 @@
 #include <vector>
 
 #include "base/debug/leak_annotations.h"
+#include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_piece.h"
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
-#include <CoreFoundation/CoreFoundation.h>
-#include <CoreServices/CoreServices.h>
-
 #include "base/mac/mac_util.h"
-#include "base/mac/scoped_cftyperef.h"
 #endif
 
 namespace sql {
@@ -164,18 +161,6 @@ int Unfetch(sqlite3_file *sqlite_file, sqlite3_int64 off, void *p) {
   return wrapped_file->pMethods->xUnfetch(wrapped_file, off, p);
 }
 
-#if defined(OS_MACOSX) && !defined(OS_IOS)
-// Helper to convert a POSIX path into a CoreFoundation path.
-base::ScopedCFTypeRef<CFURLRef> CFURLRefForPath(const char* path){
-  base::ScopedCFTypeRef<CFStringRef> urlString(
-      CFStringCreateWithFileSystemRepresentation(kCFAllocatorDefault, path));
-  base::ScopedCFTypeRef<CFURLRef> url(
-      CFURLCreateWithFileSystemPath(kCFAllocatorDefault, urlString,
-                                    kCFURLPOSIXPathStyle, FALSE));
-  return url;
-}
-#endif
-
 int Open(sqlite3_vfs* vfs, const char* file_name, sqlite3_file* wrapper_file,
          int desired_flags, int* used_flags) {
   sqlite3_vfs* wrapped_vfs = GetWrappedVfs(vfs);
@@ -204,14 +189,14 @@ int Open(sqlite3_vfs* vfs, const char* file_name, sqlite3_file* wrapper_file,
       SQLITE_OPEN_SUBJOURNAL | SQLITE_OPEN_MASTER_JOURNAL;
   if (file_name && (desired_flags & kJournalFlags)) {
     // https://www.sqlite.org/c3ref/vfs.html indicates that the journal path
-    // will have a "-suffix".
-    size_t dash_index = base::StringPiece(file_name).rfind('-');
+    // will have a suffix separated by "-" from the main database file name.
+    base::StringPiece file_name_string_piece(file_name);
+    size_t dash_index = file_name_string_piece.rfind('-');
     if (dash_index != base::StringPiece::npos) {
-      std::string db_name(file_name, dash_index);
-      base::ScopedCFTypeRef<CFURLRef> db_url(CFURLRefForPath(db_name.c_str()));
-      if (CSBackupIsItemExcluded(db_url, nullptr)) {
-        base::ScopedCFTypeRef<CFURLRef> journal_url(CFURLRefForPath(file_name));
-        CSBackupSetItemExcluded(journal_url, TRUE, FALSE);
+      base::StringPiece db_name(file_name, dash_index);
+      if (base::mac::GetFileBackupExclusion(base::FilePath(db_name))) {
+        base::mac::SetFileBackupExclusion(
+            base::FilePath(file_name_string_piece));
       }
     }
   }