ozone: Fix mash_browser_tests DCHECKs in gfx::ClientNativePixmapFactory

In chrome --mash and browser_tests --mash the ash service and window service run in the same process but on different threads. There's a race between the ash service creation of gfx::ClientNativePixmapFactory and the window service's use of it. Change the gfx::ClientNativePixmapFactory to be owned by a thread-safe base::Singleton so that it gets created once by whichever service needs it first. Also eliminate an unused reference to the factory in BrowserMainLoop. Bug: 807781 Test: mash_browser_tests Change-Id: I7b77c3d6ad957c87ed947caa703b7f65161ebb04 Reviewed-on: https://chromium-review.googlesource.com/897987 Commit-Queue: James Cook <jamescook@chromium.org> Reviewed-by: Robert Kroeger <rjkroege@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#536267}
Denger-Network · Feb 13, 2018 · 66c7d1d · 66c7d1d
1 parent b24898a
commit 66c7d1d
Show file tree

Hide file tree

Showing 9 changed files with 60 additions and 62 deletions.
diff --git a/content/browser/browser_main_loop.h b/content/browser/browser_main_loop.h
@@ -76,12 +76,6 @@ namespace net {
 class NetworkChangeNotifier;
 }  // namespace net
 
-#if defined(USE_OZONE)
-namespace gfx {
-class ClientNativePixmapFactory;
-}  // namespace gfx
-#endif
-
 #if BUILDFLAG(ENABLE_MUS)
 namespace ui {
 class ImageCursorsSet;
@@ -380,9 +374,6 @@ class CONTENT_EXPORT BrowserMainLoop {
 #elif defined(OS_MACOSX) && !defined(OS_IOS)
   std::unique_ptr<media::DeviceMonitorMac> device_monitor_mac_;
 #endif
-#if defined(USE_OZONE)
-  std::unique_ptr<gfx::ClientNativePixmapFactory> client_native_pixmap_factory_;
-#endif
 
 #if BUILDFLAG(ENABLE_WEBRTC)
   std::unique_ptr<WebRtcEventLogManager> webrtc_event_log_manager_;

diff --git a/content/renderer/renderer_main.cc b/content/renderer/renderer_main.cc
@@ -84,11 +84,6 @@ static void HandleRendererErrorTestParameters(
     WaitForDebugger("Renderer");
 }
 
-#if defined(USE_OZONE)
-base::LazyInstance<std::unique_ptr<gfx::ClientNativePixmapFactory>>::
-    DestructorAtExit g_pixmap_factory = LAZY_INSTANCE_INITIALIZER;
-#endif
-
 }  // namespace
 
 // mainline routine for running as the Renderer process
@@ -154,8 +149,7 @@ int RendererMain(const MainFunctionParams& parameters) {
 #endif
 
 #if defined(USE_OZONE)
-  g_pixmap_factory.Get() = ui::CreateClientNativePixmapFactoryOzone();
-  gfx::ClientNativePixmapFactory::SetInstance(g_pixmap_factory.Get().get());
+  ui::CreateClientNativePixmapFactoryOzone();
 #endif
 
   // This function allows pausing execution using the --renderer-startup-dialog

diff --git a/services/ui/service.cc b/services/ui/service.cc
@@ -59,6 +59,8 @@
 #include "services/ui/display/screen_manager_forwarding.h"
 #include "ui/events/ozone/layout/keyboard_layout_engine.h"
 #include "ui/events/ozone/layout/keyboard_layout_engine_manager.h"
+#include "ui/gfx/client_native_pixmap_factory.h"
+#include "ui/ozone/public/client_native_pixmap_factory_ozone.h"
 #include "ui/ozone/public/ozone_platform.h"
 #include "ui/ozone/public/ozone_switches.h"
 #endif
@@ -257,12 +259,7 @@ void Service::OnStart() {
   ui::KeyboardLayoutEngineManager::GetKeyboardLayoutEngine()
       ->SetCurrentLayoutByName("us");
 
-  if (running_standalone_) {
-    client_native_pixmap_factory_ = ui::CreateClientNativePixmapFactoryOzone();
-    gfx::ClientNativePixmapFactory::SetInstance(
-        client_native_pixmap_factory_.get());
-  }
-
+  ui::CreateClientNativePixmapFactoryOzone();
   DCHECK(gfx::ClientNativePixmapFactory::GetInstance());
 #endif
 

diff --git a/services/ui/service.h b/services/ui/service.h
@@ -39,10 +39,6 @@
 #include "services/ui/ws/user_id.h"
 #include "services/ui/ws/window_server_delegate.h"
 
-#if defined(USE_OZONE)
-#include "ui/ozone/public/client_native_pixmap_factory_ozone.h"
-#endif
-
 #if defined(OS_CHROMEOS)
 #include "services/ui/public/interfaces/arc.mojom.h"
 #endif  // defined(OS_CHROMEOS)
@@ -207,10 +203,6 @@ class Service : public service_manager::Service,
 
   bool test_config_;
 
-#if defined(USE_OZONE)
-  std::unique_ptr<gfx::ClientNativePixmapFactory> client_native_pixmap_factory_;
-#endif
-
 #if defined(OS_CHROMEOS)
   std::unique_ptr<InputDeviceController> input_device_controller_;
 #endif

diff --git a/ui/aura/env.cc b/ui/aura/env.cc
@@ -25,6 +25,7 @@
 #include "ui/events/platform/platform_event_source.h"
 
 #if defined(USE_OZONE)
+#include "ui/gfx/client_native_pixmap_factory.h"
 #include "ui/ozone/public/client_native_pixmap_factory_ozone.h"
 #include "ui/ozone/public/ozone_platform.h"
 #include "ui/ozone/public/ozone_switches.h"
@@ -164,23 +165,20 @@ Env::Env(Mode mode)
       is_touch_down_(false),
       get_last_mouse_location_from_mus_(mode_ == Mode::MUS),
       input_state_lookup_(InputStateLookup::Create()),
-#if defined(USE_OZONE)
-      native_pixmap_factory_(ui::CreateClientNativePixmapFactoryOzone()),
-#endif
       context_factory_(nullptr),
       context_factory_private_(nullptr) {
   DCHECK(lazy_tls_ptr.Pointer()->Get() == NULL);
   lazy_tls_ptr.Pointer()->Set(this);
 }
 
 void Env::Init() {
+#if defined(USE_OZONE)
+  ui::CreateClientNativePixmapFactoryOzone();
+  DCHECK(gfx::ClientNativePixmapFactory::GetInstance());
+#endif
   if (mode_ == Mode::MUS) {
     EnableMusOSExchangeDataProvider();
     EnableMusOverrideInputInjector();
-#if defined(USE_OZONE)
-    // Required by all Aura-using clients of services/ui
-    gfx::ClientNativePixmapFactory::SetInstance(native_pixmap_factory_.get());
-#endif
     return;
   }
 
@@ -197,7 +195,6 @@ void Env::Init() {
   params.using_mojo = command_line->HasSwitch(switches::kEnableDrmMojo);
 
   ui::OzonePlatform::InitializeForUI(params);
-  gfx::ClientNativePixmapFactory::SetInstance(native_pixmap_factory_.get());
 #endif
   if (!ui::PlatformEventSource::GetInstance())
     event_source_ = ui::PlatformEventSource::CreateDefault();

diff --git a/ui/aura/env.h b/ui/aura/env.h
@@ -26,12 +26,6 @@ namespace base {
 class UnguessableToken;
 }
 
-#if defined(USE_OZONE)
-namespace gfx {
-class ClientNativePixmapFactory;
-}
-#endif
-
 namespace mojo {
 template <typename MojoInterface>
 class InterfacePtr;
@@ -214,12 +208,6 @@ class AURA_EXPORT Env : public ui::EventTarget,
   std::unique_ptr<InputStateLookup> input_state_lookup_;
   std::unique_ptr<ui::PlatformEventSource> event_source_;
 
-#if defined(USE_OZONE)
-  // Factory for pixmaps that can use be transported from the client to the GPU
-  // process using a low-level ozone-provided platform specific mechanism.
-  std::unique_ptr<gfx::ClientNativePixmapFactory> native_pixmap_factory_;
-#endif
-
   ui::ContextFactory* context_factory_;
   ui::ContextFactoryPrivate* context_factory_private_;
 

diff --git a/ui/ozone/gl/gl_image_ozone_native_pixmap_unittest.cc b/ui/ozone/gl/gl_image_ozone_native_pixmap_unittest.cc
@@ -9,6 +9,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/buffer_types.h"
 #include "ui/gfx/client_native_pixmap.h"
+#include "ui/gfx/client_native_pixmap_factory.h"
 #include "ui/gl/gl_image_native_pixmap.h"
 #include "ui/gl/test/gl_image_test_template.h"
 #include "ui/ozone/public/client_native_pixmap_factory_ozone.h"
@@ -25,8 +26,11 @@ template <gfx::BufferUsage usage, gfx::BufferFormat format>
 class GLImageNativePixmapTestDelegate : public GLImageTestDelegateBase {
  public:
   GLImageNativePixmapTestDelegate() {
-    client_pixmap_factory_ = ui::CreateClientNativePixmapFactoryOzone();
+    ui::CreateClientNativePixmapFactoryOzone();
   }
+
+  ~GLImageNativePixmapTestDelegate() override = default;
+
   scoped_refptr<GLImage> CreateSolidColorImage(const gfx::Size& size,
                                                const uint8_t color[4]) const {
     ui::SurfaceFactoryOzone* surface_factory =
@@ -37,8 +41,9 @@ class GLImageNativePixmapTestDelegate : public GLImageTestDelegateBase {
     DCHECK(pixmap);
     if (usage == gfx::BufferUsage::GPU_READ_CPU_READ_WRITE ||
         usage == gfx::BufferUsage::SCANOUT_CAMERA_READ_WRITE) {
-      auto client_pixmap = client_pixmap_factory_->ImportFromHandle(
-          pixmap->ExportHandle(), size, usage);
+      auto client_pixmap =
+          gfx::ClientNativePixmapFactory::GetInstance()->ImportFromHandle(
+              pixmap->ExportHandle(), size, usage);
       bool mapped = client_pixmap->Map();
       EXPECT_TRUE(mapped);
 
@@ -72,7 +77,7 @@ class GLImageNativePixmapTestDelegate : public GLImageTestDelegateBase {
   }
 
  private:
-  std::unique_ptr<gfx::ClientNativePixmapFactory> client_pixmap_factory_;
+  DISALLOW_COPY_AND_ASSIGN(GLImageNativePixmapTestDelegate);
 };
 
 using GLImageScanoutType = testing::Types<

diff --git a/ui/ozone/public/client_native_pixmap_factory_ozone.cc b/ui/ozone/public/client_native_pixmap_factory_ozone.cc
@@ -4,17 +4,50 @@
 
 #include "ui/ozone/public/client_native_pixmap_factory_ozone.h"
 
+#include <memory>
+
+#include "base/memory/singleton.h"
 #include "base/trace_event/trace_event.h"
+#include "ui/gfx/client_native_pixmap_factory.h"
 #include "ui/ozone/platform_object.h"
 #include "ui/ozone/platform_selection.h"
 
 namespace ui {
+namespace {
+
+// Thread-safe owner of the gfx::ClientNativePixmapFactory. Not a LazyInstance
+// because it uses PlatformObject<>::Create() for factory construction.
+// TODO(jamescook|spang): This exists to solve a startup race for chrome --mash
+// http://crbug.com/807781. Removing the factory entirely would be better,
+// with something like http://crrev.com/c/899949.
+class PixmapFactorySingleton {
+ public:
+  static PixmapFactorySingleton* GetInstance() {
+    return base::Singleton<PixmapFactorySingleton>::get();
+  }
+
+ private:
+  friend struct base::DefaultSingletonTraits<PixmapFactorySingleton>;
+
+  PixmapFactorySingleton() {
+    TRACE_EVENT1("ozone", "CreateClientNativePixmapFactoryOzone", "platform",
+                 GetOzonePlatformName());
+    pixmap_factory_ = PlatformObject<gfx::ClientNativePixmapFactory>::Create();
+    gfx::ClientNativePixmapFactory::SetInstance(pixmap_factory_.get());
+  }
+
+  std::unique_ptr<gfx::ClientNativePixmapFactory> pixmap_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(PixmapFactorySingleton);
+};
+
+}  // namespace
 
-std::unique_ptr<gfx::ClientNativePixmapFactory>
-CreateClientNativePixmapFactoryOzone() {
-  TRACE_EVENT1("ozone", "CreateClientNativePixmapFactoryOzone", "platform",
-               GetOzonePlatformName());
-  return PlatformObject<gfx::ClientNativePixmapFactory>::Create();
+void CreateClientNativePixmapFactoryOzone() {
+  // Multiple threads may race to create the factory (e.g. when the UI service
+  // and ash service are running in the same process). Create the object as a
+  // side effect of a thread-safe singleton.
+  PixmapFactorySingleton::GetInstance();
 }
 
 }  // namespace ui
diff --git a/ui/ozone/public/client_native_pixmap_factory_ozone.h b/ui/ozone/public/client_native_pixmap_factory_ozone.h
@@ -5,13 +5,14 @@
 #ifndef UI_OZONE_PUBLIC_CLIENT_NATIVE_PIXMAP_FACTORY_OZONE_H_
 #define UI_OZONE_PUBLIC_CLIENT_NATIVE_PIXMAP_FACTORY_OZONE_H_
 
-#include "ui/gfx/client_native_pixmap_factory.h"
 #include "ui/ozone/ozone_export.h"
 
 namespace ui {
 
-OZONE_EXPORT std::unique_ptr<gfx::ClientNativePixmapFactory>
-CreateClientNativePixmapFactoryOzone();
+// Creates a factory for pixmaps that can use be transported from the client to
+// the GPU process using a low-level ozone-provided platform specific mechanism.
+// The factory is installed as the gfx::ClientNativePixmapFactory instance.
+OZONE_EXPORT void CreateClientNativePixmapFactoryOzone();
 
 }  // namespace ui