FontAccess: Fix blink-side character decoding

This change fixes a bug where glyphs were being decoded erroneously,
with the code assuming latin-1 encoding.

Unfortunately, due to limited ability to change system languages, or
or to override these configurations, only the Windows implementation
has test coverage.

Change-Id: Ife4057db3af686a7cb476ad4503174aa1e89b180
Fixed: 1139805
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2504595
Commit-Queue: Olivier Yiptong <oyiptong@chromium.org>
Auto-Submit: Olivier Yiptong <oyiptong@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822581}

content/browser/font_access/font_access_manager_impl.cc

@@ -16,6 +16,7 @@
 #include "content/public/browser/render_process_host.h"
 #include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/mojom/frame/lifecycle.mojom-shared.h"
+#include "third_party/blink/public/mojom/permissions/permission_status.mojom-shared.h"
 
 namespace content {
 
@@ -44,6 +45,12 @@ void FontAccessManagerImpl::EnumerateLocalFonts(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
 #if defined(PLATFORM_HAS_LOCAL_FONT_ENUMERATION_IMPL)
+  if (skip_privacy_checks_for_testing_) {
+    DidRequestPermission(std::move(callback),
+                         blink::mojom::PermissionStatus::GRANTED);
+    return;
+  }
+
   const BindingContext& context = receivers_.current_context();
 
   RenderFrameHostImpl* rfh = RenderFrameHostImpl::FromID(context.frame_id);

content/browser/font_access/font_access_manager_impl.h

@@ -39,6 +39,10 @@ class CONTENT_EXPORT FontAccessManagerImpl
   // blink.mojom.FontAccessManager:
   void EnumerateLocalFonts(EnumerateLocalFontsCallback callback) override;
 
+  void SkipPrivacyChecksForTesting(bool skip) {
+    skip_privacy_checks_for_testing_ = skip;
+  }
+
  private:
   void DidRequestPermission(EnumerateLocalFontsCallback callback,
                             blink::mojom::PermissionStatus status);
@@ -47,6 +51,8 @@ class CONTENT_EXPORT FontAccessManagerImpl
   scoped_refptr<base::SequencedTaskRunner> ipc_task_runner_;
   scoped_refptr<base::TaskRunner> results_task_runner_;
 
+  bool skip_privacy_checks_for_testing_ = false;
+
   SEQUENCE_CHECKER(sequence_checker_);
 };
 

content/browser/font_access/font_access_manager_impl_browsertest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/font_access/font_access_manager_impl.h"
+
+#include "base/test/bind_test_util.h"
+#include "base/test/scoped_feature_list.h"
+#include "build/build_config.h"
+#include "content/browser/font_access/font_access_test_utils.h"
+#include "content/browser/font_access/font_enumeration_cache.h"
+#include "content/browser/storage_partition_impl.h"
+#include "content/public/browser/storage_partition.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/shell/browser/shell.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/common/features.h"
+
+namespace content {
+
+class FontAccessManagerImplBrowserTest : public ContentBrowserTest {
+ public:
+  FontAccessManagerImplBrowserTest() {
+    scoped_feature_list_.InitAndEnableFeature(blink::features::kFontAccess);
+  }
+
+  void SetUpOnMainThread() override {
+    enumeration_cache_ = FontEnumerationCache::GetInstance();
+    enumeration_cache_->ResetStateForTesting();
+  }
+
+  void TearDownOnMainThread() override {
+    font_access_manager()->SkipPrivacyChecksForTesting(false);
+  }
+
+  RenderFrameHost* main_rfh() {
+    return shell()->web_contents()->GetMainFrame();
+  }
+
+  FontAccessManagerImpl* font_access_manager() {
+    auto* storage_partition =
+        static_cast<StoragePartitionImpl*>(main_rfh()->GetStoragePartition());
+    return storage_partition->GetFontAccessManager();
+  }
+
+  void OverrideFontAccessLocale(const std::string& locale) {
+    enumeration_cache_->OverrideLocaleForTesting(locale);
+    enumeration_cache_->ResetStateForTesting();
+  }
+
+ protected:
+  base::test::ScopedFeatureList scoped_feature_list_;
+  FontEnumerationCache* enumeration_cache_;
+};
+
+#if defined(PLATFORM_HAS_LOCAL_FONT_ENUMERATION_IMPL)
+
+IN_PROC_BROWSER_TEST_F(FontAccessManagerImplBrowserTest, EnumerationTest) {
+  ASSERT_TRUE(NavigateToURL(shell(), GetTestUrl(nullptr, "simple_page.html")));
+  font_access_manager()->SkipPrivacyChecksForTesting(true);
+
+  int result = EvalJs(shell(),
+                      "(async () => {"
+                      "  let count = 0;"
+                      "  for await (const item of navigator.fonts.query()) {"
+                      "    count++;"
+                      "  }"
+                      "  return count;"
+                      "})()")
+                   .ExtractInt();
+  ASSERT_GT(result, 0) << "Expected at least one font. Got: " << result;
+}
+
+#if defined(OS_WIN)
+IN_PROC_BROWSER_TEST_F(FontAccessManagerImplBrowserTest, LocaleTest) {
+  ASSERT_TRUE(NavigateToURL(shell(), GetTestUrl(nullptr, "simple_page.html")));
+  font_access_manager()->SkipPrivacyChecksForTesting(true);
+
+  OverrideFontAccessLocale("zh-cn");
+
+  std::string result =
+      EvalJs(shell(),
+             "(async () => {"
+             "  let fullName = '';"
+             "  for await (const item of navigator.fonts.query()) {"
+             "    if (item.postscriptName == 'MicrosoftYaHei') {"
+             "      fullName = item.fullName;"
+             "      break;"
+             "    }"
+             "  }"
+             "  return fullName;"
+             "})()")
+          .ExtractString();
+  std::string ms_yahei_utf8 = "微软雅黑";
+  ASSERT_EQ(result, ms_yahei_utf8)
+      << "Expected:" << ms_yahei_utf8 << " Got:" << result;
+}
+#endif
+
+#endif
+
+}  // namespace content

content/browser/font_access/font_access_manager_impl_unittest.cc

@@ -7,11 +7,11 @@
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
+#include "content/browser/font_access/font_access_test_utils.h"
 #include "content/browser/font_access/font_enumeration_cache.h"
 #include "content/browser/permissions/permission_controller_impl.h"
 #include "content/browser/renderer_host/frame_tree_node.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
-#include "content/public/test/mock_permission_manager.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/test/test_render_frame_host.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -28,51 +28,6 @@ using blink::mojom::FontEnumerationStatus;
 
 namespace content {
 
-namespace {
-
-using PermissionCallback =
-    base::OnceCallback<void(blink::mojom::PermissionStatus)>;
-
-class TestPermissionManager : public MockPermissionManager {
- public:
-  TestPermissionManager() = default;
-  ~TestPermissionManager() override = default;
-
-  int RequestPermission(PermissionType permissions,
-                        RenderFrameHost* render_frame_host,
-                        const GURL& requesting_origin,
-                        bool user_gesture,
-                        PermissionCallback callback) override {
-    EXPECT_EQ(permissions, PermissionType::FONT_ACCESS);
-    EXPECT_TRUE(user_gesture);
-    request_callback_.Run(std::move(callback));
-    return 0;
-  }
-
-  blink::mojom::PermissionStatus GetPermissionStatusForFrame(
-      PermissionType permission,
-      RenderFrameHost* render_frame_host,
-      const GURL& requesting_origin) override {
-    return permission_status_for_frame_;
-  }
-
-  void SetRequestCallback(
-      base::RepeatingCallback<void(PermissionCallback)> request_callback) {
-    request_callback_ = std::move(request_callback);
-  }
-
-  void SetPermissionStatusForFrame(blink::mojom::PermissionStatus status) {
-    permission_status_for_frame_ = status;
-  }
-
- private:
-  base::RepeatingCallback<void(PermissionCallback)> request_callback_;
-  blink::mojom::PermissionStatus permission_status_for_frame_ =
-      blink::mojom::PermissionStatus::ASK;
-};
-
-}  // namespace
-
 class FontAccessManagerImplTest : public RenderViewHostImplTestHarness {
  public:
   FontAccessManagerImplTest() {
@@ -101,21 +56,21 @@ class FontAccessManagerImplTest : public RenderViewHostImplTestHarness {
     TestBrowserContext* browser_context =
         static_cast<TestBrowserContext*>(main_rfh()->GetBrowserContext());
     browser_context->SetPermissionControllerDelegate(
-        std::make_unique<TestPermissionManager>());
+        std::make_unique<TestFontAccessPermissionManager>());
     permission_controller_ =
         std::make_unique<PermissionControllerImpl>(browser_context);
   }
 
   void TearDown() override { RenderViewHostImplTestHarness::TearDown(); }
 
-  TestPermissionManager* test_permission_manager() {
-    return static_cast<TestPermissionManager*>(
+  TestFontAccessPermissionManager* test_permission_manager() {
+    return static_cast<TestFontAccessPermissionManager*>(
         main_rfh()->GetBrowserContext()->GetPermissionControllerDelegate());
   }
 
   void AutoGrantPermission() {
-    test_permission_manager()->SetRequestCallback(
-        base::BindRepeating([](PermissionCallback callback) {
+    test_permission_manager()->SetRequestCallback(base::BindRepeating(
+        [](TestFontAccessPermissionManager::PermissionCallback callback) {
           std::move(callback).Run(blink::mojom::PermissionStatus::GRANTED);
         }));
     test_permission_manager()->SetPermissionStatusForFrame(
@@ -123,8 +78,8 @@ class FontAccessManagerImplTest : public RenderViewHostImplTestHarness {
   }
 
   void AutoDenyPermission() {
-    test_permission_manager()->SetRequestCallback(
-        base::BindRepeating([](PermissionCallback callback) {
+    test_permission_manager()->SetRequestCallback(base::BindRepeating(
+        [](TestFontAccessPermissionManager::PermissionCallback callback) {
           std::move(callback).Run(blink::mojom::PermissionStatus::DENIED);
         }));
     test_permission_manager()->SetPermissionStatusForFrame(
@@ -134,8 +89,8 @@ class FontAccessManagerImplTest : public RenderViewHostImplTestHarness {
   void AskGrantPermission() {
     test_permission_manager()->SetPermissionStatusForFrame(
         blink::mojom::PermissionStatus::ASK);
-    test_permission_manager()->SetRequestCallback(
-        base::BindRepeating([](PermissionCallback callback) {
+    test_permission_manager()->SetRequestCallback(base::BindRepeating(
+        [](TestFontAccessPermissionManager::PermissionCallback callback) {
           std::move(callback).Run(blink::mojom::PermissionStatus::GRANTED);
         }));
   }
@@ -143,8 +98,8 @@ class FontAccessManagerImplTest : public RenderViewHostImplTestHarness {
   void AskDenyPermission() {
     test_permission_manager()->SetPermissionStatusForFrame(
         blink::mojom::PermissionStatus::ASK);
-    test_permission_manager()->SetRequestCallback(
-        base::BindRepeating([](PermissionCallback callback) {
+    test_permission_manager()->SetRequestCallback(base::BindRepeating(
+        [](TestFontAccessPermissionManager::PermissionCallback callback) {
           std::move(callback).Run(blink::mojom::PermissionStatus::DENIED);
         }));
   }

content/browser/font_access/font_access_test_utils.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/font_access/font_access_test_utils.h"
+
+namespace content {
+
+TestFontAccessPermissionManager::TestFontAccessPermissionManager() = default;
+TestFontAccessPermissionManager::~TestFontAccessPermissionManager() = default;
+
+int TestFontAccessPermissionManager::RequestPermission(
+    PermissionType permissions,
+    RenderFrameHost* render_frame_host,
+    const GURL& requesting_origin,
+    bool user_gesture,
+    PermissionCallback callback) {
+  EXPECT_EQ(permissions, PermissionType::FONT_ACCESS);
+  EXPECT_TRUE(user_gesture);
+  request_callback_.Run(std::move(callback));
+  return 0;
+}
+
+blink::mojom::PermissionStatus
+TestFontAccessPermissionManager::GetPermissionStatusForFrame(
+    PermissionType permission,
+    RenderFrameHost* render_frame_host,
+    const GURL& requesting_origin) {
+  return permission_status_for_frame_;
+}
+
+}  // namespace content

content/browser/font_access/font_access_test_utils.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_FONT_ACCESS_FONT_ACCESS_TEST_UTILS_H_
+#define CONTENT_BROWSER_FONT_ACCESS_FONT_ACCESS_TEST_UTILS_H_
+
+#include "content/public/test/mock_permission_manager.h"
+
+namespace content {
+
+class TestFontAccessPermissionManager : public MockPermissionManager {
+ public:
+  TestFontAccessPermissionManager();
+  ~TestFontAccessPermissionManager() override;
+
+  using PermissionCallback =
+      base::OnceCallback<void(blink::mojom::PermissionStatus)>;
+
+  int RequestPermission(PermissionType permissions,
+                        RenderFrameHost* render_frame_host,
+                        const GURL& requesting_origin,
+                        bool user_gesture,
+                        PermissionCallback callback) override;
+
+  blink::mojom::PermissionStatus GetPermissionStatusForFrame(
+      PermissionType permission,
+      RenderFrameHost* render_frame_host,
+      const GURL& requesting_origin) override;
+
+  void SetRequestCallback(
+      base::RepeatingCallback<void(PermissionCallback)> request_callback) {
+    request_callback_ = std::move(request_callback);
+  }
+
+  void SetPermissionStatusForFrame(blink::mojom::PermissionStatus status) {
+    permission_status_for_frame_ = status;
+  }
+
+ private:
+  base::RepeatingCallback<void(PermissionCallback)> request_callback_;
+  blink::mojom::PermissionStatus permission_status_for_frame_ =
+      blink::mojom::PermissionStatus::ASK;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_FONT_ACCESS_FONT_ACCESS_TEST_UTILS_H_

content/browser/font_access/font_enumeration_cache.cc

@@ -5,6 +5,7 @@
 #include "content/browser/font_access/font_enumeration_cache.h"
 
 #include "base/feature_list.h"
+#include "base/memory/read_only_shared_memory_region.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
 #include "content/public/browser/browser_task_traits.h"

content/browser/font_access/font_enumeration_cache.h

@@ -7,6 +7,7 @@
 
 #include "base/deferred_sequenced_task_runner.h"
 #include "base/memory/read_only_shared_memory_region.h"
+#include "base/optional.h"
 #include "base/sequence_checker.h"
 #include "base/synchronization/atomic_flag.h"
 #include "base/time/time.h"
@@ -47,6 +48,11 @@ class CONTENT_EXPORT FontEnumerationCache {
   // region is ready.
   bool IsFontEnumerationCacheReady();
 
+  // This will set an override for the system locale setting. Unfortunately,
+  // only the Windows platform is supported at this time.
+  void OverrideLocaleForTesting(const std::string& locale) {
+    locale_override_ = locale;
+  }
   void ResetStateForTesting();
 
  protected:
@@ -90,6 +96,8 @@ class CONTENT_EXPORT FontEnumerationCache {
 
   FontEnumerationStatus status_ = FontEnumerationStatus::kOk;
 
+  base::Optional<std::string> locale_override_;
+
   SEQUENCE_CHECKER(sequence_checker_);
 };
 

content/browser/font_access/font_enumeration_cache_win.cc

@@ -50,15 +50,14 @@ base::Optional<std::string> GetLocalizedString(
 
 std::unique_ptr<content::FontEnumerationCacheWin::FamilyDataResult>
 ExtractNamesFromFamily(Microsoft::WRL::ComPtr<IDWriteFontCollection> collection,
-                       uint32_t family_index) {
+                       uint32_t family_index,
+                       const std::string& locale) {
   auto family_result =
       std::make_unique<content::FontEnumerationCacheWin::FamilyDataResult>();
   family_result->fonts =
       std::vector<blink::FontEnumerationTable_FontMetadata>();
   family_result->exit_hresult = S_OK;
 
-  std::string locale = base::i18n::GetConfiguredLocale();
-
   Microsoft::WRL::ComPtr<IDWriteFontFamily> family;
   Microsoft::WRL::ComPtr<IDWriteLocalizedStrings> family_names;
 
@@ -253,6 +252,9 @@ void FontEnumerationCacheWin::PrepareFontEnumerationCache() {
         outstanding_family_results_, 1, 5000, 50);
   }
 
+  std::string locale =
+      locale_override_.value_or(base::i18n::GetConfiguredLocale());
+
   for (UINT32 family_index = 0; family_index < outstanding_family_results_;
        ++family_index) {
     // Specify base::ThreadPolicy::MUST_USE_FOREGROUND because a priority
@@ -262,7 +264,8 @@ void FontEnumerationCacheWin::PrepareFontEnumerationCache() {
         FROM_HERE,
         {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
          base::ThreadPolicy::MUST_USE_FOREGROUND},
-        base::BindOnce(&ExtractNamesFromFamily, collection_, family_index),
+        base::BindOnce(&ExtractNamesFromFamily, collection_, family_index,
+                       locale),
         base::BindOnce(
             &FontEnumerationCacheWin::AppendFontDataAndFinalizeIfNeeded,
             // Safe because this is an initialized singleton.

content/test/BUILD.gn

@@ -62,6 +62,8 @@ static_library("test_support") {
     "../browser/file_system_access/mock_native_file_system_permission_context.h",
     "../browser/file_system_access/mock_native_file_system_permission_grant.cc",
     "../browser/file_system_access/mock_native_file_system_permission_grant.h",
+    "../browser/font_access/font_access_test_utils.cc",
+    "../browser/font_access/font_access_test_utils.h",
     "../browser/media/session/mock_media_session_player_observer.cc",
     "../browser/media/session/mock_media_session_player_observer.h",
     "../browser/media/session/mock_media_session_service_impl.cc",
@@ -1483,6 +1485,7 @@ test("content_browsertests") {
       "../browser/file_system_access/file_system_chooser_browsertest.cc",
       "../browser/file_system_access/native_file_system_drag_drop_browsertest.cc",
       "../browser/file_system_access/native_file_system_file_writer_impl_browsertest.cc",
+      "../browser/font_access/font_access_manager_impl_browsertest.cc",
       "../browser/host_zoom_map_impl_browsertest.cc",
       "../browser/serial/serial_browsertest.cc",
       "../browser/speech/speech_recognition_browsertest.cc",

third_party/blink/renderer/modules/font_access/font_iterator.cc

@@ -15,6 +15,7 @@
 #include "third_party/blink/renderer/modules/font_access/font_metadata.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
 #include "third_party/blink/renderer/platform/wtf/functional.h"
+#include "third_party/blink/renderer/platform/wtf/text/string_impl.h"
 
 namespace blink {
 
@@ -117,9 +118,10 @@ void FontIterator::DidGetEnumerationResponse(
 
   table.ParseFromArray(mapping.memory(), static_cast<int>(mapping.size()));
   for (const auto& element : table.fonts()) {
-    auto entry = FontEnumerationEntry{String(element.postscript_name().c_str()),
-                                      String(element.full_name().c_str()),
-                                      String(element.family().c_str())};
+    auto entry = FontEnumerationEntry{
+        String::FromUTF8(element.postscript_name().c_str()),
+        String::FromUTF8(element.full_name().c_str()),
+        String::FromUTF8(element.family().c_str())};
     entries_.push_back(FontMetadata::Create(std::move(entry)));
   }