Update GpuMemoryAblationExperiment initialization and context handling

When running Pinpoint, and CQ Dry Runs of the GpuMemoryAblationExperiment
being enabled-by-default, a few issues were uncovered:
  - GrContexts cannot always be initialized (crbug.com/1103780)
  - Nested changes to GLContext during initialization fails
    (crbug.com/1104316)
  - Nested deletion crashes in mutually-exclusive ways
    (crbug.com/1106926)

This change looks to address these by restructuring how GpuMemoryAblationExperiment
performs its initialization and context switches.
  - The feature will now disable itself for Mock/Stub GL impls. This will
    prevent attempting to create GrContext in tests where it will always
    fail.
  - For initialization and allocation a ui::ScopedMakeContext will be
    used. This allows resetting of previous state so that GLContexts can
    properly initialize.
  - Deletion will now be done on a PostTask. While some deletion errors
    can be addressed by resetting the context. Resetting the context
    itself can lead to accessing partially deleted objects. Setting a
    a null Surface instead of a scoped context would address these cases,
    but do not address deletion paths that need their context set the
    entire time.

This update to initialization and context handling did help uncover other
issues (1107497, 1107502, 1107509, 1107512) however those are either
platforms we are not running Pinpoint on, or tied to specific test
suites. The root causes of these failures will be investigated
separately.

Bug: 1103780, 1104316, 1106926

Change-Id: Ibf3c9a3a560912f5505f99870b848878a99da5ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300183
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Jonathan Backer <backer@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792434}

gpu/ipc/service/gpu_channel_manager.cc

@@ -101,9 +101,11 @@ void FormatAllocationSourcesForTracing(
 }  // namespace
 
 GpuChannelManager::GpuPeakMemoryMonitor::GpuPeakMemoryMonitor(
-    GpuChannelManager* channel_manager)
+    GpuChannelManager* channel_manager,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : ablation_experiment_(
-          std::make_unique<GpuMemoryAblationExperiment>(channel_manager)),
+          std::make_unique<GpuMemoryAblationExperiment>(channel_manager,
+                                                        task_runner)),
       weak_factory_(this) {}
 
 GpuChannelManager::GpuPeakMemoryMonitor::~GpuPeakMemoryMonitor() = default;
@@ -299,7 +301,7 @@ GpuChannelManager::GpuChannelManager(
       vulkan_context_provider_(vulkan_context_provider),
       metal_context_provider_(metal_context_provider),
       dawn_context_provider_(dawn_context_provider),
-      peak_memory_monitor_(this) {
+      peak_memory_monitor_(this, task_runner) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(task_runner->BelongsToCurrentThread());
   DCHECK(io_task_runner);
@@ -755,7 +757,7 @@ scoped_refptr<SharedContextState> GpuChannelManager::GetSharedContextState(
 
   // GpuMemoryAblationExperiment needs a context to use Skia for Gpu
   // allocations.
-  need_gr_context |= base::FeatureList::IsEnabled(kGPUMemoryAblationFeature);
+  need_gr_context |= GpuMemoryAblationExperiment::ExperimentSupported();
 
   if (need_gr_context) {
     if (gpu_preferences_.gr_context_type == gpu::GrContextType::kGL) {

gpu/ipc/service/gpu_channel_manager.h

@@ -206,7 +206,9 @@ class GPU_IPC_SERVICE_EXPORT GpuChannelManager
   class GPU_IPC_SERVICE_EXPORT GpuPeakMemoryMonitor
       : public MemoryTracker::Observer {
    public:
-    explicit GpuPeakMemoryMonitor(GpuChannelManager* channel_manager);
+    GpuPeakMemoryMonitor(
+        GpuChannelManager* channel_manager,
+        scoped_refptr<base::SingleThreadTaskRunner> task_runner);
     ~GpuPeakMemoryMonitor() override;
 
     base::flat_map<GpuPeakMemoryAllocationSource, uint64_t> GetPeakMemoryUsage(

gpu/ipc/service/gpu_memory_ablation_experiment.cc

@@ -7,6 +7,7 @@
 #include <algorithm>
 
 #include "base/bind.h"
+#include "base/sequenced_task_runner.h"
 #include "base/time/time.h"
 #include "base/trace_event/common/trace_event_common.h"
 #include "components/viz/common/features.h"
@@ -20,7 +21,9 @@
 #include "gpu/ipc/service/gpu_memory_buffer_factory.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/gl/gl_context.h"
 #include "ui/gl/gl_implementation.h"
+#include "ui/gl/gl_surface.h"
 
 namespace gpu {
 
@@ -49,29 +52,75 @@ constexpr gfx::Size kLargeSize(256 * 8, 256 * 8);
 constexpr viz::ResourceFormat kFormat = viz::ResourceFormat::RGBA_8888;
 constexpr uint32_t kUsage = SHARED_IMAGE_USAGE_DISPLAY;
 
+bool GpuMemoryAblationExperiment::ExperimentSupported() {
+  if (!base::FeatureList::IsEnabled(kGPUMemoryAblationFeature))
+    return false;
+  gl::GLImplementation gl_impl = gl::GetGLImplementation();
+  // Mock and Stub implementations are used in tests. It is not possible to
+  // create a valid GrContext with these. A disabled implementation is used by
+  // the GPU Info Collection Process. This also has no graphical output
+  // possible.
+  if (gl_impl == gl::kGLImplementationNone ||
+      gl_impl == gl::kGLImplementationMockGL ||
+      gl_impl == gl::kGLImplementationStubGL ||
+      gl_impl == gl::kGLImplementationDisabled) {
+    return false;
+  }
+  return true;
+}
+
 GpuMemoryAblationExperiment::GpuMemoryAblationExperiment(
-    GpuChannelManager* channel_manager)
-    : enabled_(base::FeatureList::IsEnabled(kGPUMemoryAblationFeature)),
-      channel_manager_(channel_manager) {}
+    GpuChannelManager* channel_manager,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : channel_manager_(channel_manager), task_runner_(task_runner) {
+  if (!GpuMemoryAblationExperiment::ExperimentSupported())
+    init_status_ = Status::DISABLED;
+  if (base::FeatureList::IsEnabled(kGPUMemoryAblationGPUSmall)) {
+    size_ = kSmallSize;
+  } else if (base::FeatureList::IsEnabled(kGPUMemoryAblationGPUMedium)) {
+    size_ = kMediumSize;
+  } else if (base::FeatureList::IsEnabled(kGPUMemoryAblationGPULarge)) {
+    size_ = kLargeSize;
+  }
+}
 
-GpuMemoryAblationExperiment::~GpuMemoryAblationExperiment() = default;
+GpuMemoryAblationExperiment::~GpuMemoryAblationExperiment() {
+  // Some unittests don't properly clean up. Clean up our allocations if
+  // necessary.
+  if (mailboxes_.empty())
+    return;
+  bool have_context = context_state_->MakeCurrent(context_state_->surface());
+  factory_->DestroyAllSharedImages(have_context);
+}
 
 void GpuMemoryAblationExperiment::OnMemoryAllocated(uint64_t old_size,
                                                     uint64_t new_size) {
-  if (!enabled_)
+  if (init_status_ == Status::DISABLED) {
     return;
-  if (!init_) {
-    InitGpu(channel_manager_);
+  } else if (init_status_ == Status::UNINITIALIZED) {
+    if (InitGpu(channel_manager_)) {
+      init_status_ = Status::ENABLED;
+    } else {
+      init_status_ = Status::DISABLED;
+      context_state_ = nullptr;
+      return;
+    }
   }
   // TODO(jonross): Investigate why there are 0 size allocations.
   if (new_size > old_size) {
     // TODO(jonross): Impl CPU ablation
-    if (gpu_enabled_)
-      AllocateGpuMemory();
+    AllocateGpuMemory();
   } else if (old_size > new_size) {
     // TODO(jonross): Impl CPU ablation
-    if (gpu_enabled_ && !mailboxes_.empty()) {
-      DeleteGpuMemory();
+    if (!mailboxes_.empty()) {
+      // We need to perform this as a PostTask. Though the delete calls are
+      // similarly nested to the allocations, attempting to restore the
+      // previous context will attempt to restore bindings or surfaces which
+      // were in the process of being deleted. (https://crbug.com/1106926)
+      task_runner_->PostTask(
+          FROM_HERE,
+          base::BindOnce(&GpuMemoryAblationExperiment::DeleteGpuMemory,
+                         weak_factory_.GetWeakPtr()));
     }
   }
 }
@@ -105,7 +154,9 @@ void GpuMemoryAblationExperiment::StopSequence(uint32_t sequence_num) {
 void GpuMemoryAblationExperiment::AllocateGpuMemory() {
   // We can't successfully create an image without a context, so do not even
   // perform the initial allocations.
-  if (!MakeContextCurrent())
+  std::unique_ptr<ui::ScopedMakeCurrent> scoped_current =
+      ScopedMakeContextCurrent();
+  if (!scoped_current)
     return;
   base::Time start = base::Time::Now();
 
@@ -146,7 +197,13 @@ void GpuMemoryAblationExperiment::DeleteGpuMemory() {
   auto mailbox = mailboxes_.front();
   // We can't successfully destroy the image if we cannot get the context,
   // however we still need to cleanup our internal state.
-  if (MakeContextCurrent())
+  //
+  // Unlike in initialization and allocating memory, we cannot use an
+  // ui::ScopedMakeCurrent here. Though we use a PostTask to separate the
+  // delete calls from the nesting, attempting to restore the previous context
+  // will attempt to restore bindings or surfaces which were in the process of
+  // being deleted. (https://crbug.com/1106926)
+  if (context_state_->MakeCurrent(nullptr))
     factory_->DestroySharedImage(mailbox);
 
   mailboxes_.erase(mailboxes_.begin());
@@ -156,26 +213,16 @@ void GpuMemoryAblationExperiment::DeleteGpuMemory() {
     it.second.deallocs_ += delta;
 }
 
-void GpuMemoryAblationExperiment::InitGpu(GpuChannelManager* channel_manager) {
-  // GPU Info Collection Process can be created, with no graphical output
-  // possible. Don't init there, as all future image operations will fail.
-  if (gl::GetGLImplementation() == gl::kGLImplementationDisabled)
-    return;
-
-  if (base::FeatureList::IsEnabled(kGPUMemoryAblationGPUSmall)) {
-    size_ = kSmallSize;
-  } else if (base::FeatureList::IsEnabled(kGPUMemoryAblationGPUMedium)) {
-    size_ = kMediumSize;
-  } else if (base::FeatureList::IsEnabled(kGPUMemoryAblationGPULarge)) {
-    size_ = kLargeSize;
-  }
-
+bool GpuMemoryAblationExperiment::InitGpu(GpuChannelManager* channel_manager) {
   ContextResult result;
   context_state_ = channel_manager->GetSharedContextState(&result);
-  if (result != ContextResult::kSuccess || !MakeContextCurrent()) {
-    context_state_ = nullptr;
-    return;
-  }
+  if (result != ContextResult::kSuccess)
+    return false;
+
+  std::unique_ptr<ui::ScopedMakeCurrent> scoped_current =
+      ScopedMakeContextCurrent();
+  if (!scoped_current)
+    return false;
 
   gpu::GpuMemoryBufferFactory* gmb_factory =
       channel_manager->gpu_memory_buffer_factory();
@@ -190,12 +237,17 @@ void GpuMemoryAblationExperiment::InitGpu(GpuChannelManager* channel_manager) {
 
   rep_factory_ = std::make_unique<SharedImageRepresentationFactory>(
       channel_manager->shared_image_manager(), this);
-  gpu_enabled_ = true;
-  init_ = true;
+  return true;
 }
 
-bool GpuMemoryAblationExperiment::MakeContextCurrent() {
-  return context_state_->MakeCurrent(nullptr);
+std::unique_ptr<ui::ScopedMakeCurrent>
+GpuMemoryAblationExperiment::ScopedMakeContextCurrent() {
+  std::unique_ptr<ui::ScopedMakeCurrent> scoped_current =
+      std::make_unique<ui::ScopedMakeCurrent>(context_state_->context(),
+                                              context_state_->surface());
+  if (!context_state_->IsCurrent(context_state_->surface()))
+    return nullptr;
+  return scoped_current;
 }
 
 // MemoryTracker:

gpu/ipc/service/gpu_memory_ablation_experiment.h

@@ -19,6 +19,11 @@
 #include "gpu/command_buffer/service/memory_tracking.h"
 #include "gpu/ipc/service/gpu_ipc_service_export.h"
 #include "ui/gfx/geometry/size.h"
+#include "ui/gl/scoped_make_current.h"
+
+namespace base {
+class SequencedTaskRunner;
+}
 
 namespace gpu {
 class GpuChannelManager;
@@ -48,7 +53,13 @@ extern const base::Feature kGPUMemoryAblationFeature;
 class GPU_IPC_SERVICE_EXPORT GpuMemoryAblationExperiment
     : public MemoryTracker {
  public:
-  explicit GpuMemoryAblationExperiment(GpuChannelManager* channel_manager);
+  // Checks that the memory ablation experiment feature is enabled. As well as
+  // that a supported gl::GLImplementation is available.
+  static bool ExperimentSupported();
+
+  GpuMemoryAblationExperiment(
+      GpuChannelManager* channel_manager,
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
   ~GpuMemoryAblationExperiment() override;
 
   // Allocates a chunk of memory in response to increases. Reported decreases
@@ -78,18 +89,32 @@ class GPU_IPC_SERVICE_EXPORT GpuMemoryAblationExperiment
     uint64_t peak_memory_ = 0u;
   };
 
+  // The initialization status of the feature. It defaults to |UNINITIALIZED|
+  // and is updated upon the success or failure of initializing the needed GPU
+  // resources.
+  enum class Status {
+    UNINITIALIZED,
+    ENABLED,
+    DISABLED,
+  };
+
   void AllocateGpuMemory();
   void DeleteGpuMemory();
 
   // Setups the Gpu resources needed to allocate Gpu RAM. These are influenced
   // by SharedImageStub. Which is not used directly as there is no external
-  // host to pair a GpuChannel with.
-  void InitGpu(GpuChannelManager* channel_manager);
-
-  // This must be called before any actions on |factory_|. If this method fails
-  // then subsequent work on the |factory_| will fail. Also influenced by
-  // SharedImageStub.
-  bool MakeContextCurrent();
+  // host to pair a GpuChannel with. Returns true if initialization succeeded.
+  bool InitGpu(GpuChannelManager* channel_manager);
+
+  // This must be called before any actions on |factory_|.
+  // This provides a ui::ScopedMakeCurrent which will reset the previous
+  // context upon deletion. As the we allocate memory in-response to other
+  // allocations, we are changing the context in a nested fashion. Several
+  // areas of code do not support this, and re-verify the current context at
+  // later points. (https://crbug.com/1104316)
+  // If this method fails then a nullptr is returned. All subsequent work on
+  // the |factory_| would also fail.
+  std::unique_ptr<ui::ScopedMakeCurrent> ScopedMakeContextCurrent();
 
   // MemoryTracker:
   void TrackMemoryAllocatedChange(int64_t delta) override;
@@ -98,11 +123,9 @@ class GPU_IPC_SERVICE_EXPORT GpuMemoryAblationExperiment
   int ClientId() const override;
   uint64_t ContextGroupTracingId() const override;
 
-  // Whether or not the entire experiment is enabled.
-  bool enabled_;
-  bool init_ = false;
-  // If |true| then a Gpu ablation was requested, and initialization succeeded.
-  bool gpu_enabled_ = false;
+  // The status of the initialization. Will be updated based on the results of
+  // initializing the necessary GPU resources.
+  Status init_status_ = Status::UNINITIALIZED;
 
   // Size of image to allocate, determined by experiment parameters.
   gfx::Size size_;
@@ -125,6 +148,7 @@ class GPU_IPC_SERVICE_EXPORT GpuMemoryAblationExperiment
   std::unique_ptr<SharedImageFactory> factory_;
   std::unique_ptr<SharedImageRepresentationFactory> rep_factory_;
   GpuChannelManager* channel_manager_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
   base::WeakPtrFactory<GpuMemoryAblationExperiment> weak_factory_{this};
 };