Ensure that NativeViewport destructs correctly on Linux.

First, PlatformEventSource is a singleton so it shouldn't be created by PlatformViewportX11. Once we have multiple windows, this will be very clear. Until then, we also have UAFs on destruction of PlatformViewportX11 because PlatformEventSource could be lower in the stack calling a PlatformViewportX11 method which cause both of these objects to be deleted.

Second, ensure that child windows of the PlatformViewport aren't used after the parent window is destructed, otherwise we get X errors. This is done by ensuring that NativeViewportImpl, which owns the PlatformViewport, destructs OnscreenContextProvider first. OnscreenContextProvider will in turn destroy all windows in the CommandBufferDriver that it created.

Lastly, make NativeViewportImpl destroy itself when its platform window is closed (i.e. because the user closed the window). The old implementation in that method didn't do anything.

This is split off https://codereview.chromium.org/1139673003

BUG=484234

Review URL: https://codereview.chromium.org/1130353008

Cr-Commit-Position: refs/heads/master@{#330394}

components/gles2/command_buffer_driver.cc

@@ -57,27 +57,29 @@ CommandBufferDriver::CommandBufferDriver(
     : CommandBufferDriver(gfx::kNullAcceleratedWidget,
                           share_group,
                           mailbox_manager,
-                          sync_point_manager) {
+                          sync_point_manager,
+                          base::Callback<void(CommandBufferDriver*)>()) {
 }
 
 CommandBufferDriver::CommandBufferDriver(
     gfx::AcceleratedWidget widget,
     gfx::GLShareGroup* share_group,
     gpu::gles2::MailboxManager* mailbox_manager,
-    gpu::SyncPointManager* sync_point_manager)
+    gpu::SyncPointManager* sync_point_manager,
+    const base::Callback<void(CommandBufferDriver*)>& destruct_callback)
     : client_(nullptr),
       widget_(widget),
       share_group_(share_group),
       mailbox_manager_(mailbox_manager),
       sync_point_manager_(sync_point_manager),
+      destruct_callback_(destruct_callback),
       weak_factory_(this) {
 }
 
 CommandBufferDriver::~CommandBufferDriver() {
-  if (decoder_) {
-    bool have_context = decoder_->MakeCurrent();
-    decoder_->Destroy(have_context);
-  }
+  DestroyDecoder();
+  if (!destruct_callback_.is_null())
+    destruct_callback_.Run(this);
 }
 
 void CommandBufferDriver::Initialize(
@@ -174,6 +176,8 @@ void CommandBufferDriver::SetGetBuffer(int32_t buffer) {
 }
 
 void CommandBufferDriver::Flush(int32_t put_offset) {
+  if (!context_)
+    return;
   if (!context_->MakeCurrent(surface_.get())) {
     DLOG(WARNING) << "Context lost";
     OnContextLost(gpu::error::kUnknown);
@@ -182,6 +186,13 @@ void CommandBufferDriver::Flush(int32_t put_offset) {
   command_buffer_->Flush(put_offset);
 }
 
+void CommandBufferDriver::DestroyWindow() {
+  DestroyDecoder();
+  surface_ = nullptr;
+  context_ = nullptr;
+  destruct_callback_.Reset();
+}
+
 void CommandBufferDriver::MakeProgress(int32_t last_get_offset) {
   // TODO(piman): handle out-of-order.
   sync_client_->DidMakeProgress(
@@ -247,4 +258,12 @@ void CommandBufferDriver::OnUpdateVSyncParameters(
   client_->UpdateVSyncParameters(timebase, interval);
 }
 
+void CommandBufferDriver::DestroyDecoder() {
+  if (decoder_) {
+    bool have_context = decoder_->MakeCurrent();
+    decoder_->Destroy(have_context);
+    decoder_.reset();
+  }
+}
+
 }  // namespace gles2

components/gles2/command_buffer_driver.h

@@ -8,6 +8,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/timer/timer.h"
 #include "components/gpu/public/interfaces/command_buffer.mojom.h"
@@ -33,6 +34,10 @@ class GLSurface;
 
 namespace gles2 {
 
+// This class receives CommandBuffer messages on the same thread as the native
+// viewport. It is usually destructed on that thread, however if the native
+// viewport app is destroyed before CommandBufferImpl, then the latter's failed
+// PostTask will end up deleting this class on the control thread.
 class CommandBufferDriver {
  public:
   class Client {
@@ -47,10 +52,12 @@ class CommandBufferDriver {
                       gpu::gles2::MailboxManager* mailbox_manager,
                       gpu::SyncPointManager* sync_point_manager);
   // Onscreen.
-  CommandBufferDriver(gfx::AcceleratedWidget widget,
-                      gfx::GLShareGroup* share_group,
-                      gpu::gles2::MailboxManager* mailbox_manager,
-                      gpu::SyncPointManager* sync_point_manager);
+  CommandBufferDriver(
+      gfx::AcceleratedWidget widget,
+      gfx::GLShareGroup* share_group,
+      gpu::gles2::MailboxManager* mailbox_manager,
+      gpu::SyncPointManager* sync_point_manager,
+      const base::Callback<void(CommandBufferDriver*)>& destruct_callback);
   ~CommandBufferDriver();
 
   void set_client(scoped_ptr<Client> client) { client_ = client.Pass(); }
@@ -67,6 +74,10 @@ class CommandBufferDriver {
   void DestroyTransferBuffer(int32_t id);
   void Echo(const mojo::Callback<void()>& callback);
 
+  // Called at shutdown to destroy the X window. This is needed when the parent
+  // window is being destroyed. Otherwise X calls for this window will fail.
+  void DestroyWindow();
+
  private:
   bool DoInitialize(mojo::ScopedSharedBufferHandle shared_state);
   void OnResize(gfx::Size size, float scale_factor);
@@ -76,6 +87,7 @@ class CommandBufferDriver {
   void OnContextLost(uint32_t reason);
   void OnUpdateVSyncParameters(const base::TimeTicks timebase,
                                const base::TimeDelta interval);
+  void DestroyDecoder();
 
   scoped_ptr<Client> client_;
   mojo::CommandBufferSyncClientPtr sync_client_;
@@ -93,6 +105,8 @@ class CommandBufferDriver {
   scoped_refptr<base::SingleThreadTaskRunner> context_lost_task_runner_;
   base::Callback<void(int32_t)> context_lost_callback_;
 
+  base::Callback<void(CommandBufferDriver*)> destruct_callback_;
+
   base::WeakPtrFactory<CommandBufferDriver> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CommandBufferDriver);

components/native_viewport/native_viewport_application_delegate.cc

@@ -10,6 +10,7 @@
 #include "mojo/application/public/cpp/application_connection.h"
 #include "mojo/application/public/cpp/application_impl.h"
 #include "ui/events/event_switches.h"
+#include "ui/events/platform/platform_event_source.h"
 #include "ui/gl/gl_surface.h"
 
 namespace native_viewport {
@@ -23,6 +24,7 @@ NativeViewportApplicationDelegate::~NativeViewportApplicationDelegate() {
 
 void NativeViewportApplicationDelegate::Initialize(
     mojo::ApplicationImpl* application) {
+  event_source_ = ui::PlatformEventSource::CreateDefault();
   tracing_.Initialize(application);
 
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();

components/native_viewport/native_viewport_application_delegate.h

@@ -17,6 +17,10 @@ class ApplicationConnection;
 class ApplicationImpl;
 }
 
+namespace ui {
+class PlatformEventSource;
+}
+
 namespace native_viewport {
 
 class NativeViewportApplicationDelegate
@@ -42,6 +46,7 @@ class NativeViewportApplicationDelegate
               mojo::InterfaceRequest<mojo::Gpu> request) override;
 
   scoped_refptr<gles2::GpuState> gpu_state_;
+  scoped_ptr<ui::PlatformEventSource> event_source_;
   bool is_headless_;
   mojo::TracingImpl tracing_;
 

components/native_viewport/native_viewport_impl.cc

@@ -22,7 +22,7 @@ NativeViewportImpl::NativeViewportImpl(
     const scoped_refptr<gles2::GpuState>& gpu_state,
     mojo::InterfaceRequest<mojo::NativeViewport> request)
     : is_headless_(is_headless),
-      context_provider_(gpu_state),
+      context_provider_(new OnscreenContextProvider(gpu_state)),
       sent_metrics_(false),
       metrics_(mojo::ViewportMetrics::New()),
       binding_(this, request.Pass()),
@@ -31,6 +31,11 @@ NativeViewportImpl::NativeViewportImpl(
 }
 
 NativeViewportImpl::~NativeViewportImpl() {
+  // Destroy before |platform_viewport_| because this will destroy
+  // CommandBufferDriver objects that contain child windows. Otherwise if this
+  // class destroys its window first, X errors will occur.
+  context_provider_.reset();
+
   // Destroy the NativeViewport early on as it may call us back during
   // destruction and we want to be in a known state.
   platform_viewport_.reset();
@@ -76,7 +81,7 @@ void NativeViewportImpl::SetSize(mojo::SizePtr size) {
 
 void NativeViewportImpl::GetContextProvider(
     mojo::InterfaceRequest<mojo::ContextProvider> request) {
-  context_provider_.Bind(request.Pass());
+  context_provider_->Bind(request.Pass());
 }
 
 void NativeViewportImpl::SetEventDispatcher(
@@ -102,7 +107,7 @@ void NativeViewportImpl::OnAcceleratedWidgetAvailable(
     gfx::AcceleratedWidget widget,
     float device_pixel_ratio) {
   metrics_->device_pixel_ratio = device_pixel_ratio;
-  context_provider_.SetAcceleratedWidget(widget);
+  context_provider_->SetAcceleratedWidget(widget);
   // TODO: The metrics here might not match the actual window size on android
   // where we don't know the actual size until the first OnMetricsChanged call.
   create_callback_.Run(metrics_.Clone());
@@ -111,7 +116,7 @@ void NativeViewportImpl::OnAcceleratedWidgetAvailable(
 }
 
 void NativeViewportImpl::OnAcceleratedWidgetDestroyed() {
-  context_provider_.SetAcceleratedWidget(gfx::kNullAcceleratedWidget);
+  context_provider_->SetAcceleratedWidget(gfx::kNullAcceleratedWidget);
 }
 
 bool NativeViewportImpl::OnEvent(mojo::EventPtr event) {
@@ -150,8 +155,7 @@ bool NativeViewportImpl::OnEvent(mojo::EventPtr event) {
 }
 
 void NativeViewportImpl::OnDestroyed() {
-  // This will signal a connection error and cause us to delete |this|.
-  binding_.Close();
+  delete this;
 }
 
 void NativeViewportImpl::OnConnectionError() {

components/native_viewport/native_viewport_impl.h

@@ -70,7 +70,7 @@ class NativeViewportImpl : public mojo::NativeViewport,
 
   bool is_headless_;
   scoped_ptr<PlatformViewport> platform_viewport_;
-  OnscreenContextProvider context_provider_;
+  scoped_ptr<OnscreenContextProvider> context_provider_;
   bool sent_metrics_;
   mojo::ViewportMetricsPtr metrics_;
   CreateCallback create_callback_;

components/native_viewport/onscreen_context_provider.cc

@@ -4,6 +4,7 @@
 
 #include "components/native_viewport/onscreen_context_provider.h"
 
+#include "base/bind.h"
 #include "components/gles2/command_buffer_driver.h"
 #include "components/gles2/command_buffer_impl.h"
 #include "components/gles2/gpu_state.h"
@@ -12,10 +13,14 @@ namespace native_viewport {
 
 OnscreenContextProvider::OnscreenContextProvider(
     const scoped_refptr<gles2::GpuState>& state)
-    : state_(state), widget_(gfx::kNullAcceleratedWidget), binding_(this) {
+    : state_(state),
+      widget_(gfx::kNullAcceleratedWidget),
+      binding_(this) {
 }
 
 OnscreenContextProvider::~OnscreenContextProvider() {
+  for (const auto& driver : command_buffers_)
+    driver->DestroyWindow();
 }
 
 void OnscreenContextProvider::Bind(
@@ -45,14 +50,25 @@ void OnscreenContextProvider::Create(
 
 void OnscreenContextProvider::CreateAndReturnCommandBuffer() {
   mojo::CommandBufferPtr cb;
+  scoped_ptr<gles2::CommandBufferDriver> command_buffer_driver(
+      new gles2::CommandBufferDriver(
+          widget_, state_->share_group(), state_->mailbox_manager(),
+          state_->sync_point_manager(),
+          base::Bind(&OnscreenContextProvider::CommandBufferDestroyed,
+                     base::Unretained(this))));
+  command_buffers_.insert(command_buffer_driver.get());
+
   new gles2::CommandBufferImpl(
       GetProxy(&cb), pending_listener_.Pass(), state_->control_task_runner(),
       state_->sync_point_manager(),
-      make_scoped_ptr(new gles2::CommandBufferDriver(
-          widget_, state_->share_group(), state_->mailbox_manager(),
-          state_->sync_point_manager())));
+      command_buffer_driver.Pass());
   pending_create_callback_.Run(cb.Pass());
   pending_create_callback_.reset();
 }
 
+void OnscreenContextProvider::CommandBufferDestroyed(
+    gles2::CommandBufferDriver* command_buffer) {
+  command_buffers_.erase(command_buffer);
+}
+
 }  // namespace mojo

components/native_viewport/onscreen_context_provider.h

@@ -5,12 +5,15 @@
 #ifndef COMPONENTS_NATIVE_VIEWPORT_ONSCREEN_CONTEXT_PROVIDER_H_
 #define COMPONENTS_NATIVE_VIEWPORT_ONSCREEN_CONTEXT_PROVIDER_H_
 
+#include <set>
+
 #include "base/memory/ref_counted.h"
 #include "components/gpu/public/interfaces/context_provider.mojom.h"
 #include "components/gpu/public/interfaces/viewport_parameter_listener.mojom.h"
 #include "ui/gfx/native_widget_types.h"
 
 namespace gles2 {
+class CommandBufferDriver;
 class GpuState;
 }
 
@@ -31,12 +34,14 @@ class OnscreenContextProvider : public mojo::ContextProvider {
               const CreateCallback& callback) override;
 
   void CreateAndReturnCommandBuffer();
+  void CommandBufferDestroyed(gles2::CommandBufferDriver* command_buffer);
 
   scoped_refptr<gles2::GpuState> state_;
   gfx::AcceleratedWidget widget_;
   mojo::ViewportParameterListenerPtr pending_listener_;
   CreateCallback pending_create_callback_;
   mojo::Binding<mojo::ContextProvider> binding_;
+  std::set<gles2::CommandBufferDriver*> command_buffers_;
 
   DISALLOW_COPY_AND_ASSIGN(OnscreenContextProvider);
 };

components/native_viewport/platform_viewport_x11.cc

@@ -12,7 +12,6 @@
 #include "ui/events/event.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/platform/platform_event_dispatcher.h"
-#include "ui/events/platform/platform_event_source.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/platform_window/platform_window.h"
 #include "ui/platform_window/platform_window_delegate.h"
@@ -45,11 +44,8 @@ class PlatformViewportX11 : public PlatformViewport,
  private:
   // Overridden from PlatformViewport:
   void Init(const gfx::Rect& bounds) override {
-    CHECK(!event_source_);
     CHECK(!platform_window_);
 
-    event_source_ = ui::PlatformEventSource::CreateDefault();
-
     metrics_ = mojo::ViewportMetrics::New();
     // TODO(sky): make density real.
     metrics_->device_pixel_ratio = 1.f;
@@ -154,7 +150,6 @@ class PlatformViewportX11 : public PlatformViewport,
 
   void OnActivationChanged(bool active) override {}
 
-  scoped_ptr<ui::PlatformEventSource> event_source_;
   scoped_ptr<ui::PlatformWindow> platform_window_;
   Delegate* delegate_;
   mojo::ViewportMetricsPtr metrics_;