Unify the PlatformCursor representation of the hidden cursor

This CL makes WaylandCursorFactory and BitmapCursorFactoryOzone not use
nullptr as the hidden cursor, but a BitmapCursorOzone of type kNone.
This unifies the representation of all cursor factories, where the
platform cursor will only be nullptr if it isn't available. This allows
to use stronger checks throughout the code.

Additionally, X11CursorFactory loads the invisible cursor lazily,
similarly to the rest of the default cursors.

Lastly, BitmapCursorFactoryOzone now stores its default cursors in
a map, as all other factories do. This works as a cache, but also
allows to unref the cursors on destruction, as default cursors are owned
by the factory. Before this CL, the cursors created on Lacros and the
kNone cursor were being leaked.

Bug: 1085358
Change-Id: I0659294c6a5d241bc6fecbcdbf2966d76b9b9531
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2735395
Reviewed-by: Michael Spang <spang@chromium.org>
Reviewed-by: Maksim Sisov <msisov@igalia.com>
Commit-Queue: Henrique Ferreiro <hferreiro@igalia.com>
Cr-Commit-Position: refs/heads/master@{#868048}

ui/base/cursor/cursor_factory.cc

@@ -34,10 +34,9 @@ CursorFactory* CursorFactory::GetInstance() {
   return g_instance;
 }
 
-base::Optional<PlatformCursor> CursorFactory::GetDefaultCursor(
-    mojom::CursorType type) {
+PlatformCursor CursorFactory::GetDefaultCursor(mojom::CursorType type) {
   NOTIMPLEMENTED();
-  return base::nullopt;
+  return nullptr;
 }
 
 PlatformCursor CursorFactory::CreateImageCursor(mojom::CursorType type,
@@ -119,7 +118,7 @@ std::vector<std::string> CursorNamesFromType(mojom::CursorType type) {
     case mojom::CursorType::kWestResize:
       return {"w-resize", "left_side"};
     case mojom::CursorType::kNone:
-      return {"none"};
+      return {};
     case mojom::CursorType::kGrab:
       return {"openhand", "grab"};
     case mojom::CursorType::kGrabbing:

ui/base/cursor/cursor_factory.h

@@ -8,7 +8,6 @@
 #include <vector>
 
 #include "base/component_export.h"
-#include "base/optional.h"
 #include "build/build_config.h"
 #include "ui/base/cursor/mojom/cursor_type.mojom-forward.h"
 
@@ -36,10 +35,8 @@ class COMPONENT_EXPORT(UI_BASE_CURSOR_BASE) CursorFactory {
   // Return the default cursor of the specified type. The types are listed in
   // ui/base/cursor/cursor.h. Default cursors are managed by the implementation
   // and must live indefinitely; there's no way to know when to free them.
-  // nullptr may be a valid value for the hidden cursor. When a default cursor
-  // is not available, base::nullopt is returned.
-  virtual base::Optional<PlatformCursor> GetDefaultCursor(
-      mojom::CursorType type);
+  // When a default cursor is not available, nullptr is returned.
+  virtual PlatformCursor GetDefaultCursor(mojom::CursorType type);
 
   // Return an image cursor for the specified |type| with a |bitmap| and
   // |hotspot|. Image cursors are referenced counted and have an initial

ui/base/cursor/cursor_loader.cc

@@ -99,24 +99,24 @@ PlatformCursor CursorLoader::CursorFromType(mojom::CursorType type) {
   // Check if there's a default platform cursor available.
   // For the none cursor, we also need to use the platform factory to take
   // into account the different ways of creating an invisible cursor.
+  PlatformCursor cursor;
   if (use_platform_cursors_ || type == mojom::CursorType::kNone) {
-    base::Optional<PlatformCursor> default_cursor =
-        factory_->GetDefaultCursor(type);
-    if (default_cursor)
-      return *default_cursor;
+    cursor = factory_->GetDefaultCursor(type);
+    if (cursor)
+      return cursor;
     LOG(ERROR) << "Failed to load a platform cursor of type " << type;
   }
 
   // Loads the default Aura cursor bitmap for the cursor type. Falls back on
   // pointer cursor if this fails.
-  PlatformCursor platform = LoadCursorFromAsset(type);
-  if (!platform && type != mojom::CursorType::kPointer) {
-    platform = CursorFromType(mojom::CursorType::kPointer);
-    factory_->RefImageCursor(platform);
-    image_cursors_[type] = platform;
+  cursor = LoadCursorFromAsset(type);
+  if (!cursor && type != mojom::CursorType::kPointer) {
+    cursor = CursorFromType(mojom::CursorType::kPointer);
+    factory_->RefImageCursor(cursor);
+    image_cursors_[type] = cursor;
   }
-  DCHECK(platform) << "Failed to load a bitmap for the pointer cursor.";
-  return platform;
+  DCHECK(cursor) << "Failed to load a bitmap for the pointer cursor.";
+  return cursor;
 }
 
 PlatformCursor CursorLoader::LoadCursorFromAsset(mojom::CursorType type) {

ui/base/cursor/cursor_loader_unittest.cc

@@ -4,7 +4,6 @@
 
 #include "ui/base/cursor/cursor_loader.h"
 
-#include "base/memory/scoped_refptr.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/cursor/cursor.h"
@@ -50,7 +49,10 @@ TEST(CursorLoaderTest, InvisibleCursor) {
 #if defined(USE_OZONE) && !defined(USE_X11)
 TEST(CursorLoaderTest, InvisibleCursor) {
   BitmapCursorFactoryOzone cursor_factory;
-  EXPECT_EQ(LoadInvisibleCursor(), nullptr);
+  auto* invisible_cursor =
+      static_cast<BitmapCursorOzone*>(LoadInvisibleCursor());
+  ASSERT_NE(invisible_cursor, nullptr);
+  EXPECT_EQ(invisible_cursor->type(), mojom::CursorType::kNone);
 }
 #endif
 
@@ -61,7 +63,8 @@ TEST(CursorLoaderTest, InvisibleCursor) {
   // invisible cursor in X11.
   auto* invisible_cursor =
       cursor_factory.CreateImageCursor({}, SkBitmap(), gfx::Point());
-  EXPECT_EQ(LoadInvisibleCursor(), invisible_cursor);
+  ASSERT_NE(invisible_cursor, nullptr);
+  EXPECT_EQ(invisible_cursor, LoadInvisibleCursor());
 
   // Release our refcount on the cursor
   cursor_factory.UnrefImageCursor(invisible_cursor);

ui/base/cursor/ozone/bitmap_cursor_factory_ozone.cc

@@ -150,20 +150,25 @@ scoped_refptr<BitmapCursorOzone> BitmapCursorFactoryOzone::GetBitmapCursor(
   return base::WrapRefCounted(ToBitmapCursorOzone(platform_cursor));
 }
 
-base::Optional<PlatformCursor> BitmapCursorFactoryOzone::GetDefaultCursor(
+PlatformCursor BitmapCursorFactoryOzone::GetDefaultCursor(
     mojom::CursorType type) {
-  if (type == mojom::CursorType::kNone)
-    return nullptr;  // nullptr is used for the hidden cursor.
+  if (!default_cursors_.count(type)) {
+    if (type == mojom::CursorType::kNone
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
-  if (UseDefaultCursorForType(type)) {
-    // Lacros uses server-side cursors for most types. These cursors don't need
-    // to load bitmap images on the client.
-    BitmapCursorOzone* cursor = new BitmapCursorOzone(type);
-    cursor->AddRef();  // Balanced by UnrefImageCursor.
-    return ToPlatformCursor(cursor);
+        || UseDefaultCursorForType(type)
+#endif
+    ) {
+      // Lacros uses server-side cursors for most types. These cursors don't
+      // need to load bitmap images on the client.
+      // Similarly, the hidden cursor doesn't use any bitmap.
+      default_cursors_[type] = base::MakeRefCounted<BitmapCursorOzone>(type);
+    } else {
+      return nullptr;
+    }
   }
-#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
-  return base::nullopt;
+
+  // Returns owned default cursor for this type.
+  return default_cursors_[type].get();
 }
 
 PlatformCursor BitmapCursorFactoryOzone::CreateImageCursor(

ui/base/cursor/ozone/bitmap_cursor_factory_ozone.h

@@ -5,11 +5,13 @@
 #ifndef UI_BASE_CURSOR_OZONE_BITMAP_CURSOR_FACTORY_OZONE_H_
 #define UI_BASE_CURSOR_OZONE_BITMAP_CURSOR_FACTORY_OZONE_H_
 
+#include <map>
 #include <vector>
 
 #include "base/component_export.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/time/time.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/cursor/cursor.h"
@@ -82,9 +84,8 @@ class COMPONENT_EXPORT(UI_BASE_CURSOR) BitmapCursorFactoryOzone
   static scoped_refptr<BitmapCursorOzone> GetBitmapCursor(
       PlatformCursor platform_cursor);
 
-  // CursorFactoryOzone:
-  base::Optional<PlatformCursor> GetDefaultCursor(
-      mojom::CursorType type) override;
+  // CursorFactory:
+  PlatformCursor GetDefaultCursor(mojom::CursorType type) override;
   PlatformCursor CreateImageCursor(mojom::CursorType type,
                                    const SkBitmap& bitmap,
                                    const gfx::Point& hotspot) override;
@@ -96,6 +97,9 @@ class COMPONENT_EXPORT(UI_BASE_CURSOR) BitmapCursorFactoryOzone
   void UnrefImageCursor(PlatformCursor cursor) override;
 
  private:
+  std::map<mojom::CursorType, scoped_refptr<BitmapCursorOzone>>
+      default_cursors_;
+
   DISALLOW_COPY_AND_ASSIGN(BitmapCursorFactoryOzone);
 };
 

ui/base/cursor/ozone/bitmap_cursor_factory_ozone_unittest.cc

@@ -4,52 +4,50 @@
 
 #include "ui/base/cursor/ozone/bitmap_cursor_factory_ozone.h"
 
-#include "base/optional.h"
 #include "build/chromeos_buildflags.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/cursor/cursor.h"
 #include "ui/base/cursor/mojom/cursor_type.mojom-shared.h"
 
 namespace ui {
 
+using mojom::CursorType;
+
 TEST(BitmapCursorFactoryOzoneTest, InvisibleCursor) {
   BitmapCursorFactoryOzone cursor_factory;
 
-  base::Optional<PlatformCursor> cursor =
-      cursor_factory.GetDefaultCursor(mojom::CursorType::kNone);
-  // The invisible cursor should be nullptr, not base::nullopt.
-  ASSERT_TRUE(cursor.has_value());
-  EXPECT_EQ(cursor, nullptr);
+  PlatformCursor cursor = cursor_factory.GetDefaultCursor(CursorType::kNone);
+  // The invisible cursor should be a BitmapCursorOzone of type kNone, not
+  // nullptr.
+  ASSERT_NE(cursor, nullptr);
+  EXPECT_EQ(static_cast<BitmapCursorOzone*>(cursor)->type(), CursorType::kNone);
 }
 
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
 TEST(BitmapCursorFactoryOzoneTest, LacrosUsesDefaultCursorsForCommonTypes) {
   BitmapCursorFactoryOzone factory;
 
   // Verify some common cursor types.
-  base::Optional<PlatformCursor> cursor =
-      factory.GetDefaultCursor(mojom::CursorType::kPointer);
-  ASSERT_TRUE(cursor.has_value());
+  PlatformCursor cursor = factory.GetDefaultCursor(CursorType::kPointer);
   EXPECT_NE(cursor, nullptr);
-  factory.UnrefImageCursor(cursor.value());
+  EXPECT_EQ(static_cast<BitmapCursorOzone*>(cursor)->type(),
+            CursorType::kPointer);
 
-  cursor = factory.GetDefaultCursor(mojom::CursorType::kHand);
-  ASSERT_TRUE(cursor.has_value());
+  cursor = factory.GetDefaultCursor(CursorType::kHand);
   EXPECT_NE(cursor, nullptr);
-  factory.UnrefImageCursor(cursor.value());
+  EXPECT_EQ(static_cast<BitmapCursorOzone*>(cursor)->type(), CursorType::kHand);
 
-  cursor = factory.GetDefaultCursor(mojom::CursorType::kIBeam);
-  ASSERT_TRUE(cursor.has_value());
+  cursor = factory.GetDefaultCursor(CursorType::kIBeam);
   EXPECT_NE(cursor, nullptr);
-  factory.UnrefImageCursor(cursor.value());
+  EXPECT_EQ(static_cast<BitmapCursorOzone*>(cursor)->type(),
+            CursorType::kIBeam);
 }
 
 TEST(BitmapCursorFactoryOzoneTest, LacrosCustomCursor) {
   BitmapCursorFactoryOzone factory;
-  base::Optional<PlatformCursor> cursor =
-      factory.GetDefaultCursor(mojom::CursorType::kCustom);
+  PlatformCursor cursor = factory.GetDefaultCursor(CursorType::kCustom);
   // Custom cursors don't have a default platform cursor.
-  EXPECT_FALSE(cursor.has_value());
+  EXPECT_EQ(cursor, nullptr);
 }
 #endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
 

ui/base/cursor/win/win_cursor_factory.cc

@@ -10,7 +10,6 @@
 
 #include "base/memory/scoped_refptr.h"
 #include "base/notreached.h"
-#include "base/optional.h"
 #include "base/win/scoped_gdi_object.h"
 #include "base/win/windows_types.h"
 #include "ui/base/cursor/cursor.h"
@@ -132,8 +131,7 @@ WinCursorFactory::WinCursorFactory() = default;
 
 WinCursorFactory::~WinCursorFactory() = default;
 
-base::Optional<PlatformCursor> WinCursorFactory::GetDefaultCursor(
-    mojom::CursorType type) {
+PlatformCursor WinCursorFactory::GetDefaultCursor(mojom::CursorType type) {
   if (!default_cursors_.count(type)) {
     // Using a dark 1x1 bit bmp for the kNone cursor may still cause DWM to do
     // composition work unnecessarily. Better to totally remove it from the
@@ -146,7 +144,7 @@ base::Optional<PlatformCursor> WinCursorFactory::GetDefaultCursor(
       if (!hcursor)
         hcursor = LoadCursorFromResourcesDataDLL(id);
       if (!hcursor)
-        return base::nullopt;
+        return nullptr;
     }
     default_cursors_[type] = base::MakeRefCounted<WinCursor>(hcursor);
   }

ui/base/cursor/win/win_cursor_factory.h

@@ -29,8 +29,7 @@ class COMPONENT_EXPORT(UI_BASE_CURSOR) WinCursorFactory : public CursorFactory {
   ~WinCursorFactory() override;
 
   // CursorFactory:
-  base::Optional<PlatformCursor> GetDefaultCursor(
-      mojom::CursorType type) override;
+  PlatformCursor GetDefaultCursor(mojom::CursorType type) override;
   PlatformCursor CreateImageCursor(mojom::CursorType type,
                                    const SkBitmap& bitmap,
                                    const gfx::Point& hotspot) override;

ui/base/x/x11_cursor_factory.cc

@@ -33,33 +33,24 @@ scoped_refptr<X11Cursor> CreateInvisibleCursor(XCursorLoader* cursor_loader) {
 }  // namespace
 
 X11CursorFactory::X11CursorFactory()
-    : cursor_loader_(std::make_unique<XCursorLoader>(x11::Connection::Get())),
-      invisible_cursor_(CreateInvisibleCursor(cursor_loader_.get())) {}
+    : cursor_loader_(std::make_unique<XCursorLoader>(x11::Connection::Get())) {}
 
 X11CursorFactory::~X11CursorFactory() = default;
 
-base::Optional<PlatformCursor> X11CursorFactory::GetDefaultCursor(
-    mojom::CursorType type) {
-  auto cursor = GetDefaultCursorInternal(type);
-  if (!cursor)
-    return base::nullopt;
-  return ToPlatformCursor(cursor.get());
-}
-
 PlatformCursor X11CursorFactory::CreateImageCursor(mojom::CursorType type,
                                                    const SkBitmap& bitmap,
                                                    const gfx::Point& hotspot) {
   // There is a problem with custom cursors that have no custom data. The
   // resulting SkBitmap is empty and X crashes when creating a zero size cursor
   // image. Return invisible cursor here instead.
   if (bitmap.drawsNothing()) {
-    // The result of |invisible_cursor_| is owned by the caller, and will be
+    // The result of `CreateImageCursor()` is owned by the caller, and will be
     // Unref()ed by code far away. (Usually in web_cursor.cc in content, among
     // others.) If we don't manually add another reference before we cast this
-    // to a void*, we can end up with |invisible_cursor_| being freed out from
-    // under us.
-    invisible_cursor_->AddRef();
-    return ToPlatformCursor(invisible_cursor_.get());
+    // to a void*, we can end up with the cursor being freed out from under us.
+    auto* invisible_cursor = GetDefaultCursor(mojom::CursorType::kNone);
+    RefImageCursor(invisible_cursor);
+    return invisible_cursor;
   }
 
   auto cursor = cursor_loader_->CreateCursor(bitmap, hotspot);
@@ -104,19 +95,17 @@ void X11CursorFactory::OnCursorThemeSizeChanged(int cursor_theme_size) {
   ClearThemeCursors();
 }
 
-scoped_refptr<X11Cursor> X11CursorFactory::GetDefaultCursorInternal(
-    mojom::CursorType type) {
-  if (type == mojom::CursorType::kNone)
-    return invisible_cursor_;
-
+PlatformCursor X11CursorFactory::GetDefaultCursor(mojom::CursorType type) {
   if (!default_cursors_.count(type)) {
     // Try to load a predefined X11 cursor.
     default_cursors_[type] =
-        cursor_loader_->LoadCursor(CursorNamesFromType(type));
+        type == mojom::CursorType::kNone
+            ? CreateInvisibleCursor(cursor_loader_.get())
+            : cursor_loader_->LoadCursor(CursorNamesFromType(type));
   }
 
   // Returns owned default cursor for this type.
-  return default_cursors_[type];
+  return default_cursors_[type].get();
 }
 
 void X11CursorFactory::ClearThemeCursors() {

ui/base/x/x11_cursor_factory.h

@@ -31,9 +31,8 @@ class COMPONENT_EXPORT(UI_BASE_X) X11CursorFactory
   X11CursorFactory& operator=(const X11CursorFactory&) = delete;
   ~X11CursorFactory() override;
 
-  // CursorFactoryOzone:
-  base::Optional<PlatformCursor> GetDefaultCursor(
-      mojom::CursorType type) override;
+  // CursorFactory:
+  PlatformCursor GetDefaultCursor(mojom::CursorType type) override;
   PlatformCursor CreateImageCursor(mojom::CursorType type,
                                    const SkBitmap& bitmap,
                                    const gfx::Point& hotspot) override;
@@ -54,13 +53,6 @@ class COMPONENT_EXPORT(UI_BASE_X) X11CursorFactory
 
   std::unique_ptr<XCursorLoader> cursor_loader_;
 
-  // Loads/caches default cursor or returns cached version.
-  scoped_refptr<X11Cursor> GetDefaultCursorInternal(mojom::CursorType type);
-
-  // Holds a single instance of the invisible cursor. X11 has no way to hide
-  // the cursor so an invisible cursor mimics that.
-  scoped_refptr<X11Cursor> invisible_cursor_;
-
   std::map<mojom::CursorType, scoped_refptr<X11Cursor>> default_cursors_;
 
   base::ScopedObservation<CursorThemeManager, CursorThemeManagerObserver>

ui/base/x/x11_cursor_factory_unittest.cc

@@ -20,6 +20,7 @@ TEST(X11CursorFactoryTest, InvisibleRefcount) {
   // CreateImageCursor should return an incremented refcount.
   auto* invisible_cursor = static_cast<X11Cursor*>(
       factory.CreateImageCursor({}, SkBitmap(), gfx::Point()));
+  ASSERT_NE(invisible_cursor, nullptr);
   ASSERT_FALSE(invisible_cursor->HasOneRef());
 
   // Release our refcount on the cursor

ui/ozone/platform/drm/BUILD.gn

@@ -133,6 +133,7 @@ source_set("gbm") {
     "//ui/base",
     "//ui/base/cursor",
     "//ui/base/cursor:cursor_base",
+    "//ui/base/cursor/mojom:cursor_type",
     "//ui/base/ime",
     "//ui/display",
     "//ui/display/types",