Do not perform font scanning when cache directory not configured

Fixes flakiness in unit testing due to DWriteFontLookupTableBuilder
spuriously trying to index fonts when it should not.

In order to test DWriteFontLookupTableBuilder a cache directory is
configured for persisting the built table. The same happens when a full
browser runs with a profile - in which case a subdirectory of the
USER_DATA directory is used. Do not scan fonts if a directory is not
configured.

This situation happens in unit tests in tests other than the
DWriteFontLookupTableBuilder unit tests, as ContentClient and
BrowserClient do not return a path for GetFontLookupTableCacheDir() in
testing. It is sufficient to test DWriteFontLookupTableBuilder in its
own unittests. If local unique font matching on Windows 7 is required in
a unit test, a cache directory can be configured using
DWriteFontLookupTableBuilder::GetInstance()->
    SetCacheDirectoryForTesting(...)

Thanks to etienneb@ for spotting this problem and initial fix proposals.

Bug: 996167
Change-Id: Iccac27225d7be7fc464a645e7e7689eba3680efa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1769437Reviewed-by: Etienne Bergeron <etienneb@chromium.org>
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Auto-Submit: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691074}

content/browser/renderer_host/dwrite_font_lookup_table_builder_win.cc

@@ -95,9 +95,10 @@ bool EnsureCacheDirectory(base::FilePath cache_directory) {
   // If the directory does not exist already, ensure that the parent directory
   // exists, which is usually the User Data directory. If it exists, we can try
   // creating the cache directory.
-  return base::DirectoryExists(cache_directory) ||
-         (base::DirectoryExists(cache_directory.DirName()) &&
-          CreateDirectory(cache_directory));
+  return !cache_directory.empty() &&
+         (base::DirectoryExists(cache_directory) ||
+          (base::DirectoryExists(cache_directory.DirName()) &&
+           CreateDirectory(cache_directory)));
 }
 
 }  // namespace
@@ -115,6 +116,10 @@ DWriteFontLookupTableBuilder::FontFileWithUniqueNames::FontFileWithUniqueNames(
 
 DWriteFontLookupTableBuilder::DWriteFontLookupTableBuilder()
     : font_indexing_timeout_(kFontIndexingTimeoutDefault) {
+  InitializeCacheDirectoryFromProfile();
+}
+
+void DWriteFontLookupTableBuilder::InitializeCacheDirectoryFromProfile() {
   // In FontUniqueNameBrowserTest the DWriteFontLookupTableBuilder is
   // instantiated to configure the cache directory for testing explicitly before
   // GetContentClient() is available. Catch this case here. It is safe to not
@@ -122,7 +127,7 @@ DWriteFontLookupTableBuilder::DWriteFontLookupTableBuilder()
   // detected by TableCacheFilePath and the LoadFromFile and PersistToFile
   // methods.
   cache_directory_ =
-      GetContentClient()
+      GetContentClient() && GetContentClient()->browser()
           ? GetContentClient()->browser()->GetFontLookupTableCacheDir()
           : base::FilePath();
 }
@@ -131,6 +136,9 @@ DWriteFontLookupTableBuilder::~DWriteFontLookupTableBuilder() = default;
 
 base::ReadOnlySharedMemoryRegion
 DWriteFontLookupTableBuilder::DuplicateMemoryRegion() {
+  DCHECK(!TableCacheFilePath().empty())
+      << "Ensure that a cache_directory_ is set (see "
+         "InitializeCacheDirectoryFromProfile())";
   DCHECK(FontUniqueNameTableReady());
   return font_table_memory_.region.Duplicate();
 }
@@ -152,10 +160,6 @@ void DWriteFontLookupTableBuilder::InitializeDirectWrite() {
     return;
   }
 
-  // QueryInterface for IDWriteFactory2. It's ok for this to fail if we are
-  // running an older version of DirectWrite (earlier than Win8.1).
-  factory.As<IDWriteFactory2>(&factory2_);
-
   // QueryInterface for IDwriteFactory3, needed for MatchUniqueFont on Windows
   // 10. May fail on older versions, in which case, unique font matching must be
   // done through indexing system fonts using DWriteFontLookupTableBuilder.
@@ -315,10 +319,18 @@ void DWriteFontLookupTableBuilder::
     InitializeDirectWrite();
   }
 
-  // Nothing to do if we have API to directly lookup local fonts by unique name.
+  // Nothing to do if we have API to directly lookup local fonts by unique name
+  // (as on Windows 10, IDWriteFactory3 available).
   if (HasDWriteUniqueFontLookups())
     return;
 
+  // Do not schedule indexing if we do not have a profile or temporary directory
+  // to store the cached table. This prevents repetitive and redundant scanning
+  // when the ContentBrowserClient did not provide a cache directory, as is the
+  // case in content_unittests.
+  if (TableCacheFilePath().empty())
+    return;
+
   start_time_table_ready_ = base::TimeTicks::Now();
 
   scoped_refptr<base::SequencedTaskRunner> results_collection_task_runner =
@@ -656,6 +668,15 @@ void DWriteFontLookupTableBuilder::ResetLookupTableForTesting() {
   font_table_built_.Reset();
 }
 
+void DWriteFontLookupTableBuilder::ResetStateForTesting() {
+  ResetLookupTableForTesting();
+  // Recreate fFactory3 if available, to reset
+  // OverrideDWriteVersionChecksForTesting().
+  direct_write_initialized_ = false;
+  InitializeDirectWrite();
+  InitializeCacheDirectoryFromProfile();
+}
+
 void DWriteFontLookupTableBuilder::ResumeFromHangForTesting() {
   hang_event_for_testing_->Signal();
 }

content/browser/renderer_host/dwrite_font_lookup_table_builder_win.h

@@ -78,10 +78,15 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder {
   void SetSlowDownIndexingForTestingWithTimeout(SlowDownMode slowdown_mode,
                                                 base::TimeDelta new_timeout);
 
-  // Needed to trigger rebuilding the lookup table, when testing using
-  // slowed-down indexing. Otherwise, the test methods would use the already
-  // cached lookup table.
+  // Reset timeout overrides and empty table. Needed to trigger rebuilding the
+  // lookup table, when testing using slowed-down indexing. Otherwise, the test
+  // methods would use the already cached lookup table.
   void ResetLookupTableForTesting();
+
+  // Resets other overrides such as the DWrite version check override and cache
+  // directory back to its default values.
+  void ResetStateForTesting();
+
   // Signals hang_event_for_testing_ which is used in testing hanging one of the
   // font name retrieval tasks.
   void ResumeFromHangForTesting();
@@ -102,8 +107,9 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder {
   // repeated rebuilding of the font table lookup structure.
   void SetCachingEnabledForTesting(bool caching_enabled);
 
-  // Disables DCHECKs that ensure DWriteFontLookupTableBuilder is only run pre
-  // Windows 10, used for testing only to allow running the tests on Windows 10.
+  // Disable DCHECKs that ensure DWriteFontLookupTableBuilder is only
+  // run pre Windows 10, used for testing only to allow running the tests on
+  // Windows 10.
   void OverrideDWriteVersionChecksForTesting();
 
  private:
@@ -132,6 +138,11 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder {
   // specified at construction time.
   bool PersistToFile();
 
+  // Initialize the cache directory from the user profile directory if
+  // DWriteFontLookupTableBuilder is executed in an environment where the
+  // profile is accessible.
+  void InitializeCacheDirectoryFromProfile();
+
   // Load from cache or construct the font unique name lookup table. If the
   // cache is up to date, do not schedule a run to scan all Windows-enumerated
   // fonts.
@@ -193,7 +204,6 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder {
   base::TimeTicks start_time_table_ready_;
   base::TimeTicks start_time_table_build_;
   base::FilePath cache_directory_;
-  std::string persistence_hash_;
 
   bool caching_enabled_ = true;
   base::Optional<base::WaitableEvent> hang_event_for_testing_;

content/browser/renderer_host/dwrite_font_lookup_table_builder_win_unittest.cc

@@ -51,6 +51,10 @@ class DWriteFontLookupTableBuilderTest : public testing::Test {
         scoped_temp_dir_.GetPath());
   }
 
+  void TearDown() override {
+    font_lookup_table_builder_->ResetStateForTesting();
+  }
+
   void TestMatchFonts() {
     base::ReadOnlySharedMemoryRegion font_table_memory =
         font_lookup_table_builder_->DuplicateMemoryRegion();

content/browser/renderer_host/dwrite_font_proxy_impl_win_unittest.cc

@@ -95,10 +95,11 @@ class DWriteFontProxyLocalMatchingTest : public DWriteFontProxyImplUnitTest {
 class DWriteFontProxyTableMatchingTest
     : public DWriteFontProxyLocalMatchingTest {
  public:
-  DWriteFontProxyTableMatchingTest() {
+  void SetUp() override {
     DWriteFontLookupTableBuilder* table_builder_instance =
         DWriteFontLookupTableBuilder::GetInstance();
-    DCHECK(scoped_temp_dir_.CreateUniqueTempDir());
+    bool temp_dir_success = scoped_temp_dir_.CreateUniqueTempDir();
+    ASSERT_TRUE(temp_dir_success);
     table_builder_instance->OverrideDWriteVersionChecksForTesting();
     table_builder_instance->SetCacheDirectoryForTesting(
         scoped_temp_dir_.GetPath());
@@ -106,6 +107,12 @@ class DWriteFontProxyTableMatchingTest
     table_builder_instance->SchedulePrepareFontUniqueNameTableIfNeeded();
   }
 
+  void TearDown() override {
+    DWriteFontLookupTableBuilder* table_builder_instance =
+        DWriteFontLookupTableBuilder::GetInstance();
+    table_builder_instance->ResetStateForTesting();
+  }
+
  private:
   base::ScopedTempDir scoped_temp_dir_;
 };

third_party/blink/common/font_unique_name_lookup/font_table_persistence.cc

@@ -14,7 +14,8 @@ namespace font_table_persistence {
 
 bool LoadFromFile(base::FilePath file_path,
                   base::MappedReadOnlyRegion* name_table_region) {
-  DCHECK(!file_path.empty());
+  if (file_path.empty())
+    return false;
 
   // Reset to empty to ensure IsValid() is false if reading fails.
   *name_table_region = base::MappedReadOnlyRegion();
@@ -74,8 +75,11 @@ bool LoadFromFile(base::FilePath file_path,
 
 bool PersistToFile(const base::MappedReadOnlyRegion& name_table_region,
                    base::FilePath file_path) {
-  DCHECK(name_table_region.mapping.IsValid() &&
-         name_table_region.mapping.size() && !file_path.empty());
+  DCHECK(name_table_region.mapping.IsValid());
+  DCHECK(name_table_region.mapping.size());
+
+  if (file_path.empty())
+    return false;
 
   base::File table_cache_file(file_path, base::File::FLAG_CREATE_ALWAYS |
                                              base::File::Flags::FLAG_WRITE);