Restrict cross-thread access to gfx::Image and gfx::Font in ResourceB…

…undle

Since gfx::Image and gfx::Font are not thread safe, cached images and
fonts in ResourceBundle can be used only on UI thread, regardless of
the protection by ResourceBundle::images_and_fonts_lock_.

This CL removes the lock and adds DCHECK to limit the access to UI thread.

BUG=688072, 468010

Review-Url: https://codereview.chromium.org/2699323002
Cr-Commit-Position: refs/heads/master@{#452407}

ui/base/resource/resource_bundle.cc

@@ -402,17 +402,18 @@ std::string ResourceBundle::ReloadLocaleResources(
 }
 
 gfx::ImageSkia* ResourceBundle::GetImageSkiaNamed(int resource_id) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+
   const gfx::ImageSkia* image = GetImageNamed(resource_id).ToImageSkia();
   return const_cast<gfx::ImageSkia*>(image);
 }
 
 gfx::Image& ResourceBundle::GetImageNamed(int resource_id) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+
   // Check to see if the image is already in the cache.
-  {
-    base::AutoLock lock_scope(*images_and_fonts_lock_);
-    if (images_.count(resource_id))
-      return images_[resource_id];
-  }
+  if (images_.count(resource_id))
+    return images_[resource_id];
 
   gfx::Image image;
   if (delegate_)
@@ -450,12 +451,6 @@ gfx::Image& ResourceBundle::GetImageNamed(int resource_id) {
   }
 
   // The load was successful, so cache the image.
-  base::AutoLock lock_scope(*images_and_fonts_lock_);
-
-  // Another thread raced the load and has already cached the image.
-  if (images_.count(resource_id))
-    return images_[resource_id];
-
   images_[resource_id] = image;
   return images_[resource_id];
 }
@@ -586,7 +581,7 @@ const gfx::FontList& ResourceBundle::GetFontListWithDelta(
     int size_delta,
     gfx::Font::FontStyle style,
     gfx::Font::Weight weight) {
-  base::AutoLock lock_scope(*images_and_fonts_lock_);
+  DCHECK(sequence_checker_.CalledOnValidSequence());
 
   const FontKey styled_key(size_delta, style, weight);
 
@@ -622,10 +617,12 @@ const gfx::FontList& ResourceBundle::GetFontListWithDelta(
 const gfx::Font& ResourceBundle::GetFontWithDelta(int size_delta,
                                                   gfx::Font::FontStyle style,
                                                   gfx::Font::Weight weight) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   return GetFontListWithDelta(size_delta, style, weight).GetPrimaryFont();
 }
 
 const gfx::FontList& ResourceBundle::GetFontList(FontStyle legacy_style) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   gfx::Font::Weight font_weight = gfx::Font::Weight::NORMAL;
   if (legacy_style == BoldFont || legacy_style == MediumBoldFont)
     font_weight = gfx::Font::Weight::BOLD;
@@ -651,11 +648,12 @@ const gfx::FontList& ResourceBundle::GetFontList(FontStyle legacy_style) {
 }
 
 const gfx::Font& ResourceBundle::GetFont(FontStyle style) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   return GetFontList(style).GetPrimaryFont();
 }
 
 void ResourceBundle::ReloadFonts() {
-  base::AutoLock lock_scope(*images_and_fonts_lock_);
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   InitDefaultFontList();
   font_cache_.clear();
 }
@@ -678,7 +676,6 @@ bool ResourceBundle::IsScaleFactorSupported(ScaleFactor scale_factor) {
 
 ResourceBundle::ResourceBundle(Delegate* delegate)
     : delegate_(delegate),
-      images_and_fonts_lock_(new base::Lock),
       locale_resources_data_lock_(new base::Lock),
       max_scale_factor_(SCALE_FACTOR_100P) {
 }
@@ -862,7 +859,7 @@ bool ResourceBundle::LoadBitmap(int resource_id,
 }
 
 gfx::Image& ResourceBundle::GetEmptyImage() {
-  base::AutoLock lock(*images_and_fonts_lock_);
+  DCHECK(sequence_checker_.CalledOnValidSequence());
 
   if (empty_image_.IsEmpty()) {
     // The placeholder bitmap is bright red so people notice the problem.

ui/base/resource/resource_bundle.h

@@ -17,6 +17,7 @@
 #include "base/files/memory_mapped_file.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/sequence_checker.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_piece.h"
 #include "build/build_config.h"
@@ -393,9 +394,6 @@ class UI_BASE_EXPORT ResourceBundle {
   // be NULL.
   Delegate* delegate_;
 
-  // Protects |images_| and font-related members.
-  std::unique_ptr<base::Lock> images_and_fonts_lock_;
-
   // Protects |locale_resources_data_|.
   std::unique_ptr<base::Lock> locale_resources_data_lock_;
 
@@ -416,7 +414,7 @@ class UI_BASE_EXPORT ResourceBundle {
   // The various font lists used, as a map from a signed size delta from the
   // platform base font size, plus style, to the FontList. Cached to avoid
   // repeated GDI creation/destruction and font derivation.
-  // Must be accessed only while holding |images_and_fonts_lock_|.
+  // Must be accessed only from UI thread.
   std::map<FontKey, gfx::FontList> font_cache_;
 
   base::FilePath overridden_pak_path_;
@@ -425,6 +423,8 @@ class UI_BASE_EXPORT ResourceBundle {
 
   bool is_test_resources_ = false;
 
+  base::SequenceChecker sequence_checker_;
+
   DISALLOW_COPY_AND_ASSIGN(ResourceBundle);
 };
 

ui/base/resource/resource_bundle_ios.mm

@@ -90,13 +90,12 @@
 }
 
 gfx::Image& ResourceBundle::GetNativeImageNamed(int resource_id) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+
   // Check to see if the image is already in the cache.
-  {
-    base::AutoLock lock(*images_and_fonts_lock_);
-    ImageMap::iterator found = images_.find(resource_id);
-    if (found != images_.end()) {
-      return found->second;
-    }
+  ImageMap::iterator found = images_.find(resource_id);
+  if (found != images_.end()) {
+    return found->second;
   }
 
   gfx::Image image;
@@ -166,7 +165,7 @@
     image = gfx::Image(ui_image, base::scoped_policy::RETAIN);
   }
 
-  base::AutoLock lock(*images_and_fonts_lock_);
+  DCHECK(sequence_checker_.CalledOnValidSequence());
 
   // Another thread raced the load and has already cached the image.
   if (images_.count(resource_id))

ui/base/resource/resource_bundle_mac.mm

@@ -90,18 +90,18 @@
 }
 
 gfx::Image& ResourceBundle::GetNativeImageNamed(int resource_id) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   // Check to see if the image is already in the cache.
-  {
-    base::AutoLock lock(*images_and_fonts_lock_);
-    if (images_.count(resource_id)) {
-      if (!images_[resource_id].HasRepresentation(gfx::Image::kImageRepCocoa)) {
-        DLOG(WARNING) << "ResourceBundle::GetNativeImageNamed() is returning a"
+  auto found = images_.find(resource_id);
+  if (found != images_.end()) {
+    if (!found->second.HasRepresentation(gfx::Image::kImageRepCocoa)) {
+      DLOG(WARNING)
+          << "ResourceBundle::GetNativeImageNamed() is returning a"
           << " cached gfx::Image that isn't backed by an NSImage. The image"
           << " will be converted, rather than going through the NSImage loader."
           << " resource_id = " << resource_id;
-      }
-      return images_[resource_id];
     }
+    return found->second;
   }
 
   gfx::Image image;
@@ -160,7 +160,7 @@
     image = gfx::Image(ns_image.release());
   }
 
-  base::AutoLock lock(*images_and_fonts_lock_);
+  DCHECK(sequence_checker_.CalledOnValidSequence());
 
   // Another thread raced the load and has already cached the image.
   if (images_.count(resource_id))