Fix SystemFont flaky tests

This CL is fixing a cause of flaky tests when re-ordering tests execution.

SystemFont is keeping two static callbacks that modify metrics of the
system fonts. Typically, these callbacks are set up before the SystemFont
class is instantiated. A DCHECK was ensuring this.

We are able to repro flaky test by changing tests order or
by specifying test filters (see below).

A DCHECK fails because it is possible in unittests to set the
callbacks after the SystemFont is instantiated. The problem is
that callbacks are static variables that keep states
between two tests.

I believe other fancy bugs may be hidden by this bug since system fonts
may be scaled based on the callback left by a previous test.

This CL is adding the required logic to reset the system fonts.


R=robliao@chromium.org,asvitkine@chromium.org
CC=​benck@google.com
Bug: 944227

C:\src\chromium\src>out\build\gfx_unittests --gtest_filter=FontListTest.Fonts_DeriveWithHeightUpperBound:RenderTextTest.HarfBuzz_FallbackFontsSupportGlyphs:SystemFontsWinTest.AdjustFontSize
IMPORTANT DEBUGGING NOTE: batches of tests are run inside their
own process. For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with
--single-process-tests.
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
Note: Google Test filter = FontListTest.Fonts_DeriveWithHeightUpperBound:RenderTextTest.HarfBuzz_FallbackFontsSupportGlyphs:SystemFontsWinTest.AdjustFontSize
[==========] Running 3 tests from 3 test suites.
[----------] Global test environment set-up.
[----------] 1 test from FontListTest
[ RUN      ] FontListTest.Fonts_DeriveWithHeightUpperBound
[       OK ] FontListTest.Fonts_DeriveWithHeightUpperBound (13 ms)
[----------] 1 test from FontListTest (14 ms total)

[----------] 1 test from RenderTextTest
[ RUN      ] RenderTextTest.HarfBuzz_FallbackFontsSupportGlyphs
[       OK ] RenderTextTest.HarfBuzz_FallbackFontsSupportGlyphs (32 ms)
[----------] 1 test from RenderTextTest (34 ms total)

[----------] 1 test from SystemFontsWinTest
[ RUN      ] SystemFontsWinTest.AdjustFontSize
[26180:21012:0605/182005.067:1151493921:FATAL:system_fonts_win.cc(100)] Check failed: !SystemFonts::IsInitialized().

        base::debug::CollectStackTrace [0x00007FFB78F6D7E0+48] (C:\src\chromium\src\base\debug\stack_trace_win.cc:284)
        base::debug::StackTrace::StackTrace [0x00007FFB78F6C950+80] (C:\src\chromium\src\base\debug\stack_trace.cc:206)
        base::debug::StackTrace::StackTrace [0x00007FFB78F6C8D8+40] (C:\src\chromium\src\base\debug\stack_trace.cc:203)
        logging::LogMessage::~LogMessage [0x00007FFB78FBC78F+143] (C:\src\chromium\src\base\logging.cc:613)
        gfx::win::`anonymous namespace'::SystemFonts::SetGetMinimumFontSizeCallback [0x00007FFB834E3E41+161] (C:\src\chromium\src\ui\gfx\system_fonts_win.cc:101)
        gfx::win::SetGetMinimumFontSizeCallback [0x00007FFB834E3D93+19] (C:\src\chromium\src\ui\gfx\system_fonts_win.cc:222)
        gfx::win::SystemFontsWinTest_AdjustFontSize_Test::TestBody [0x00007FF7F312886E+46] (C:\src\chromium\src\ui\gfx\system_fonts_win_unittest.cc:30)
        testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void> [0x00007FF7F31A50DF+79] (C:\src\chromium\src\third_party\googletest\src\googletest\src\gtest.cc:2507)
        testing::Test::Run [0x00007FF7F31A5049+185] (C:\src\chromium\src\third_party\googletest\src\googletest\src\gtest.cc:2529)
        testing::TestInfo::Run [0x00007FF7F31A5D11+225] (C:\src\chromium\src\third_party\googletest\src\googletest\src\gtest.cc:2701)
        testing::TestSuite::Run [0x00007FF7F31A69D6+278] (C:\src\chromium\src\third_party\googletest\src\googletest\src\gtest.cc:2827)
        testing::internal::UnitTestImpl::RunAllTests [0x00007FF7F31B0EE9+1113] (C:\src\chromium\src\third_party\googletest\src\googletest\src\gtest.cc:5284)
        testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool> [0x00007FF7F31B0A75+85] (C:\src\chromium\src\third_party\googletest\src\googletest\src\gtest.cc:2505)
        testing::UnitTest::Run [0x00007FF7F31B08BC+300] (C:\src\chromium\src\third_party\googletest\src\googletest\src\gtest.cc:4873)
        RUN_ALL_TESTS [0x00007FF7F32463F1+17] (C:\src\chromium\src\third_party\googletest\src\googletest\include\gtest\gtest.h:2453)
        base::TestSuite::Run [0x00007FF7F32459D4+164] (C:\src\chromium\src\base\test\test_suite.cc:316)
        base::internal::FunctorTraits<int (base::TestSuite::*)(),void>::Invoke<int (base::TestSuite::*)(),(anonymous namespace)::GfxTestSuite *> [0x00007FF7F2E1516D+29] (C:\src\chromium\src\base\bind_internal.h:499)
        base::internal::InvokeHelper<0,int>::MakeItSo<int (base::TestSuite::*)(),(anonymous namespace)::GfxTestSuite *> [0x00007FF7F2E150E4+52] (C:\src\chromium\src\base\bind_internal.h:599)
        base::internal::Invoker<base::internal::BindState<int (base::TestSuite::*)(),base::internal::UnretainedWrapper<(anonymous namespace)::GfxTestSuite> >,int ()>::RunImpl<int (base::TestSuite::*)(),std::__1::tuple<base::internal::UnretainedWrapper<(anonymous  [0x00007FF7F2E15068+88] (C:\src\chromium\src\base\bind_internal.h:672)
        base::internal::Invoker<base::internal::BindState<int (base::TestSuite::*)(),base::internal::UnretainedWrapper<(anonymous namespace)::GfxTestSuite> >,int ()>::RunOnce [0x00007FF7F2E14F3E+78] (C:\src\chromium\src\base\bind_internal.h:641)
        base::OnceCallback<int ()>::Run [0x00007FF7F324E611+97] (C:\src\chromium\src\base\callback.h:98)
        base::`anonymous namespace'::LaunchUnitTestsInternal [0x00007FF7F324BCE2+418] (C:\src\chromium\src\base\test\launcher\unit_test_launcher.cc:158)
        base::LaunchUnitTests [0x00007FF7F324BAFD+253] (C:\src\chromium\src\base\test\launcher\unit_test_launcher.cc:492)
        main [0x00007FF7F2E14AAA+138] (C:\src\chromium\src\ui\gfx\test\run_all_unittests.cc:81)
        invoke_main [0x00007FF7F33739E4+52] (d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:79)
        __scrt_common_main_seh [0x00007FF7F3373B1E+302] (d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
        __scrt_common_main [0x00007FF7F3373B9E+14] (d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:331)
        mainCRTStartup [0x00007FF7F3373BB9+9] (d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp:17)
        BaseThreadInitThunk [0x00007FFBB44837E4+20]

Backtrace: 
Change-Id: I23109356db89c48c89c51b1db78cbb0c04367e8a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1647192
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667235}

ui/gfx/font_unittest.cc

@@ -19,7 +19,24 @@
 namespace gfx {
 namespace {
 
-using FontTest = testing::Test;
+class FontTest : public testing::Test {
+ public:
+  FontTest() = default;
+
+ protected:
+  void SetUp() override {
+#if defined(OS_WIN)
+    // System fonts is keeping a cache of loaded system fonts. These fonts are
+    // scaled based on global callbacks configured on startup. The tests in this
+    // file are testing these callbacks and need to be sure we cleared the
+    // global state to avoid flaky tests.
+    win::ResetSystemFontsForTesting();
+#endif
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FontTest);
+};
 
 TEST_F(FontTest, DefaultFont) {
   Font cf;

ui/gfx/system_fonts_win.cc

@@ -21,18 +21,28 @@ namespace {
 
 class SystemFonts {
  public:
-  const gfx::Font& GetFont(SystemFont system_font) const {
+  const gfx::Font& GetFont(SystemFont system_font) {
+    if (!IsInitialized())
+      Initialize();
+
     auto it = system_fonts_.find(system_font);
     DCHECK(it != system_fonts_.end())
         << "System font #" << static_cast<int>(system_font) << " not found!";
     return it->second;
   }
 
-  static const SystemFonts* Instance() {
+  static SystemFonts* Instance() {
     static base::NoDestructor<SystemFonts> instance;
     return instance.get();
   }
 
+  void ResetForTesting() {
+    SystemFonts::is_initialized_ = false;
+    SystemFonts::adjust_font_callback_ = nullptr;
+    SystemFonts::get_minimum_font_size_callback_ = nullptr;
+    system_fonts_.clear();
+  }
+
   static int AdjustFontSize(int lf_height, int size_delta) {
     // Extract out the sign of |lf_height| - we'll add it back later.
     const int lf_sign = lf_height < 0 ? -1 : 1;
@@ -109,8 +119,10 @@ class SystemFonts {
  private:
   friend base::NoDestructor<SystemFonts>;
 
-  SystemFonts() {
-    TRACE_EVENT0("fonts", "gfx::SystemFonts::SystemFonts");
+  SystemFonts() {}
+
+  void Initialize() {
+    TRACE_EVENT0("fonts", "gfx::SystemFonts::Initialize");
 
     NONCLIENTMETRICS_XP metrics;
     base::win::GetNonClientMetrics(&metrics);
@@ -255,5 +267,9 @@ void AdjustLOGFONTForTesting(const FontAdjustment& font_adjustment,
   SystemFonts::AdjustLOGFONT(font_adjustment, logfont);
 }
 
+GFX_EXPORT void ResetSystemFontsForTesting() {
+  SystemFonts::Instance()->ResetForTesting();
+}
+
 }  // namespace win
 }  // namespace gfx

ui/gfx/system_fonts_win.h

@@ -53,6 +53,12 @@ GFX_EXPORT int AdjustFontSize(int lf_height, int size_delta);
 // Adjusts a LOGFONT structure for optional size scale and face override.
 GFX_EXPORT void AdjustLOGFONTForTesting(const FontAdjustment& font_adjustment,
                                         LOGFONT* logfont);
+
+// Clear the system fonts cache. SystemFonts is keeping a global state that
+// must be reset in unittests when using |GetMinimumFontSizeCallback| or
+// |SetAdjustFontCallback|.
+GFX_EXPORT void ResetSystemFontsForTesting();
+
 }  // namespace win
 }  // namespace gfx
 

ui/gfx/system_fonts_win_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <windows.h>
 
+#include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace gfx {
@@ -13,6 +14,25 @@ namespace win {
 
 namespace {
 
+class SystemFontsWinTest : public testing::Test {
+ public:
+  SystemFontsWinTest() = default;
+
+ protected:
+  void SetUp() override {
+#if defined(OS_WIN)
+    // System fonts is keeping a cache of loaded system fonts. These fonts are
+    // scaled based on global callbacks configured on startup. The tests in this
+    // file are testing these callbacks and need to be sure we cleared the
+    // global state to avoid flaky tests.
+    win::ResetSystemFontsForTesting();
+#endif
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SystemFontsWinTest);
+};
+
 LOGFONT CreateLOGFONT(const base::char16* name, LONG height) {
   LOGFONT logfont = {};
   logfont.lfHeight = height;
@@ -26,8 +46,7 @@ const base::char16 kArial[] = L"Arial";
 
 }  // namespace
 
-TEST(SystemFontsWinTest, AdjustFontSize) {
-  gfx::win::SetGetMinimumFontSizeCallback(nullptr);
+TEST_F(SystemFontsWinTest, AdjustFontSize) {
   EXPECT_EQ(10, gfx::win::AdjustFontSize(10, 0));
   EXPECT_EQ(-10, gfx::win::AdjustFontSize(-10, 0));
   EXPECT_EQ(8, gfx::win::AdjustFontSize(10, -2));
@@ -40,7 +59,7 @@ TEST(SystemFontsWinTest, AdjustFontSize) {
   EXPECT_EQ(0, gfx::win::AdjustFontSize(-10, -12));
 }
 
-TEST(SystemFontsWinTest, AdjustFontSize_MinimumSizeSpecified) {
+TEST_F(SystemFontsWinTest, AdjustFontSize_MinimumSizeSpecified) {
   gfx::win::SetGetMinimumFontSizeCallback([] { return 1; });
   EXPECT_EQ(10, gfx::win::AdjustFontSize(10, 0));
   EXPECT_EQ(-10, gfx::win::AdjustFontSize(-10, 0));
@@ -54,7 +73,7 @@ TEST(SystemFontsWinTest, AdjustFontSize_MinimumSizeSpecified) {
   EXPECT_EQ(-1, gfx::win::AdjustFontSize(-10, -12));
 }
 
-TEST(SystemFontsWinTest, AdjustLOGFONT_NoAdjustment) {
+TEST_F(SystemFontsWinTest, AdjustLOGFONT_NoAdjustment) {
   LOGFONT logfont = CreateLOGFONT(kSegoeUI, -12);
   FontAdjustment adjustment;
   AdjustLOGFONTForTesting(adjustment, &logfont);
@@ -62,7 +81,7 @@ TEST(SystemFontsWinTest, AdjustLOGFONT_NoAdjustment) {
   EXPECT_STREQ(kSegoeUI, logfont.lfFaceName);
 }
 
-TEST(SystemFontsWinTest, AdjustLOGFONT_ChangeFace) {
+TEST_F(SystemFontsWinTest, AdjustLOGFONT_ChangeFace) {
   LOGFONT logfont = CreateLOGFONT(kSegoeUI, -12);
   FontAdjustment adjustment{kArial, 1.0};
   AdjustLOGFONTForTesting(adjustment, &logfont);
@@ -70,7 +89,7 @@ TEST(SystemFontsWinTest, AdjustLOGFONT_ChangeFace) {
   EXPECT_STREQ(kArial, logfont.lfFaceName);
 }
 
-TEST(SystemFontsWinTest, AdjustLOGFONT_ScaleDown) {
+TEST_F(SystemFontsWinTest, AdjustLOGFONT_ScaleDown) {
   LOGFONT logfont = CreateLOGFONT(kSegoeUI, -12);
   FontAdjustment adjustment{L"", 0.5};
   AdjustLOGFONTForTesting(adjustment, &logfont);
@@ -84,7 +103,7 @@ TEST(SystemFontsWinTest, AdjustLOGFONT_ScaleDown) {
   EXPECT_STREQ(kSegoeUI, logfont.lfFaceName);
 }
 
-TEST(SystemFontsWinTest, AdjustLOGFONT_ScaleDownWithRounding) {
+TEST_F(SystemFontsWinTest, AdjustLOGFONT_ScaleDownWithRounding) {
   LOGFONT logfont = CreateLOGFONT(kSegoeUI, -10);
   FontAdjustment adjustment{L"", 0.85};
   AdjustLOGFONTForTesting(adjustment, &logfont);
@@ -98,7 +117,7 @@ TEST(SystemFontsWinTest, AdjustLOGFONT_ScaleDownWithRounding) {
   EXPECT_STREQ(kSegoeUI, logfont.lfFaceName);
 }
 
-TEST(SystemFontsWinTest, AdjustLOGFONT_ScaleUpWithFaceChange) {
+TEST_F(SystemFontsWinTest, AdjustLOGFONT_ScaleUpWithFaceChange) {
   LOGFONT logfont = CreateLOGFONT(kSegoeUI, -12);
   FontAdjustment adjustment{kArial, 1.5};
   AdjustLOGFONTForTesting(adjustment, &logfont);
@@ -112,7 +131,7 @@ TEST(SystemFontsWinTest, AdjustLOGFONT_ScaleUpWithFaceChange) {
   EXPECT_STREQ(kArial, logfont.lfFaceName);
 }
 
-TEST(SystemFontsWinTest, AdjustLOGFONT_ScaleUpWithRounding) {
+TEST_F(SystemFontsWinTest, AdjustLOGFONT_ScaleUpWithRounding) {
   LOGFONT logfont = CreateLOGFONT(kSegoeUI, -10);
   FontAdjustment adjustment{L"", 1.111};
   AdjustLOGFONTForTesting(adjustment, &logfont);