Add DCHECK for ScheduleGpuTask

Viz for webview architecture will only support calling ScheduleTask from
viz thread at limited times (initialization, tear down, DrawAndSwap). In
order to avoid violating this requirement, add a DCHECK for the existing
ScheduleGpuTask to ensure it is only called during allowed times.

Also add a ScheduleOrRetainTask method for the cases where existing
code violates this requirement. In viz for webview, such tasks may be
delayed indefinitely, until the next DrawAndSwap or tear down.
There are only two cases, EnsureBackbuffer and ReleaseCachedResources,
that violate the requirement. Convert them to ScheduleOrRetainGpuTask.

Only enable the DCHECK for SkiaRenderer since that's the only case
relevant for viz for webview.

Bug: 805739
Change-Id: Ic9ba91d85958fbe8ff0d8e30d1bc9095fd8cf62c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1713871
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Peng Huang <penghuang@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#681860}

android_webview/browser/gfx/task_forwarding_sequence.cc

@@ -47,6 +47,12 @@ void TaskForwardingSequence::ScheduleTask(
       false /* out_of_order */);
 }
 
+void TaskForwardingSequence::ScheduleOrRetainTask(
+    base::OnceClosure task,
+    std::vector<gpu::SyncToken> sync_token_fences) {
+  ScheduleTask(std::move(task), std::move(sync_token_fences));
+}
+
 // Should not be called because tasks aren't reposted to wait for sync tokens,
 // or for yielding execution since ShouldYield() returns false.
 void TaskForwardingSequence::ContinueTask(base::OnceClosure task) {

android_webview/browser/gfx/task_forwarding_sequence.h

@@ -38,6 +38,9 @@ class TaskForwardingSequence : public gpu::SingleTaskSequence {
 
   void ScheduleTask(base::OnceClosure task,
                     std::vector<gpu::SyncToken> sync_token_fences) override;
+  void ScheduleOrRetainTask(
+      base::OnceClosure task,
+      std::vector<gpu::SyncToken> sync_token_fences) override;
 
   // Should not be called because tasks aren't reposted to wait for sync tokens,
   // or for yielding execution since ShouldYield() returns false.

components/viz/service/display/DEPS

@@ -29,6 +29,7 @@ include_rules = [
   "+ui/gl/dc_renderer_layer_params.h",
   "+ui/gl/trace_util.h",
   "+gpu/ipc/common/vulkan_ycbcr_info.h",
+  "+gpu/ipc/scheduler_sequence.h",
 ]
 
 specific_include_rules = {

components/viz/service/display/display.cc

@@ -35,6 +35,7 @@
 #include "components/viz/service/surfaces/surface.h"
 #include "components/viz/service/surfaces/surface_manager.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
+#include "gpu/ipc/scheduler_sequence.h"
 #include "services/viz/public/mojom/compositing/compositor_frame_sink.mojom.h"
 #include "ui/gfx/buffer_types.h"
 #include "ui/gfx/geometry/rect_conversions.h"
@@ -148,6 +149,10 @@ Display::Display(
 }
 
 Display::~Display() {
+#if DCHECK_IS_ON()
+  allow_schedule_gpu_task_during_destruction_.reset(
+      new gpu::ScopedAllowScheduleGpuTask);
+#endif
 #if defined(OS_ANDROID)
   // In certain cases, drivers hang when tearing down the display. Finishing
   // before teardown appears to address this. As we're during display teardown,
@@ -187,6 +192,7 @@ void Display::Initialize(DisplayClient* client,
                          bool enable_shared_images) {
   DCHECK(client);
   DCHECK(surface_manager);
+  gpu::ScopedAllowScheduleGpuTask allow_schedule_gpu_task;
   client_ = client;
   surface_manager_ = surface_manager;
   if (scheduler_)
@@ -403,6 +409,7 @@ void Display::OnContextLost() {
 
 bool Display::DrawAndSwap() {
   TRACE_EVENT0("viz", "Display::DrawAndSwap");
+  gpu::ScopedAllowScheduleGpuTask allow_schedule_gpu_task;
 
   if (!current_surface_id_.is_valid()) {
     TRACE_EVENT_INSTANT0("viz", "No root surface.", TRACE_EVENT_SCOPE_THREAD);

components/viz/service/display/display.h

@@ -37,6 +37,10 @@ namespace gfx {
 class Size;
 }
 
+namespace gpu {
+class ScopedAllowScheduleGpuTask;
+}
+
 namespace viz {
 class DirectRenderer;
 class DisplayClient;
@@ -197,6 +201,10 @@ class VIZ_SERVICE_EXPORT Display : public DisplaySchedulerClient,
   bool swapped_since_resize_ = false;
   bool output_is_secure_ = false;
 
+#if DCHECK_IS_ON()
+  std::unique_ptr<gpu::ScopedAllowScheduleGpuTask>
+      allow_schedule_gpu_task_during_destruction_;
+#endif
   std::unique_ptr<OutputSurface> output_surface_;
   SkiaOutputSurface* const skia_output_surface_;
   std::unique_ptr<DisplayScheduler> scheduler_;

components/viz/service/display_embedder/output_surface_provider_impl.cc

@@ -30,6 +30,7 @@
 #include "gpu/config/gpu_finch_features.h"
 #include "gpu/ipc/command_buffer_task_executor.h"
 #include "gpu/ipc/common/surface_handle.h"
+#include "gpu/ipc/scheduler_sequence.h"
 #include "gpu/ipc/service/gpu_channel_manager_delegate.h"
 #include "gpu/ipc/service/image_transport_surface.h"
 #include "ui/base/ui_base_switches.h"
@@ -117,10 +118,13 @@ std::unique_ptr<OutputSurface> OutputSurfaceProviderImpl::CreateOutputSurface(
     NOTIMPLEMENTED();
     return nullptr;
 #else
-    output_surface = SkiaOutputSurfaceImpl::Create(
-        std::make_unique<SkiaOutputSurfaceDependencyImpl>(gpu_service_impl_,
-                                                          surface_handle),
-        renderer_settings);
+    {
+      gpu::ScopedAllowScheduleGpuTask allow_schedule_gpu_task;
+      output_surface = SkiaOutputSurfaceImpl::Create(
+          std::make_unique<SkiaOutputSurfaceDependencyImpl>(gpu_service_impl_,
+                                                            surface_handle),
+          renderer_settings);
+    }
     if (!output_surface) {
 #if defined(OS_CHROMEOS) || defined(IS_CHROMECAST)
       // GPU compositing is expected to always work on Chrome OS so we should

components/viz/service/display_embedder/skia_output_surface_impl.cc

@@ -151,7 +151,8 @@ void SkiaOutputSurfaceImpl::EnsureBackbuffer() {
   // SkiaOutputSurfaceImpl::dtor. So it is safe to use base::Unretained.
   auto callback = base::BindOnce(&SkiaOutputSurfaceImplOnGpu::EnsureBackbuffer,
                                  base::Unretained(impl_on_gpu_.get()));
-  ScheduleGpuTask(std::move(callback), std::vector<gpu::SyncToken>());
+  task_sequence_->ScheduleOrRetainTask(std::move(callback),
+                                       std::vector<gpu::SyncToken>());
 }
 
 void SkiaOutputSurfaceImpl::DiscardBackbuffer() {
@@ -347,7 +348,8 @@ void SkiaOutputSurfaceImpl::ReleaseCachedResources(
   auto callback = base::BindOnce(
       &SkiaOutputSurfaceImplOnGpu::ReleaseImageContexts,
       base::Unretained(impl_on_gpu_.get()), std::move(image_contexts));
-  ScheduleGpuTask(std::move(callback), std::vector<gpu::SyncToken>());
+  task_sequence_->ScheduleOrRetainTask(std::move(callback),
+                                       std::vector<gpu::SyncToken>());
 }
 
 void SkiaOutputSurfaceImpl::SkiaSwapBuffers(OutputSurfaceFrame frame) {

components/viz/service/main/DEPS

@@ -3,6 +3,7 @@
 include_rules = [
   "+components/discardable_memory/client",
   "+components/ui_devtools",
+  "+components/viz/common/features.h",
   "+components/viz/common/switches.h",
   "+components/viz/service",
   "+gpu/command_buffer",

components/viz/service/main/viz_compositor_thread_runner.cc

@@ -14,6 +14,7 @@
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
+#include "components/viz/common/features.h"
 #include "components/viz/common/switches.h"
 #include "components/viz/service/display_embedder/in_process_gpu_memory_buffer_manager.h"
 #include "components/viz/service/display_embedder/output_surface_provider_impl.h"
@@ -23,6 +24,7 @@
 #include "gpu/config/gpu_finch_features.h"
 #include "gpu/config/gpu_switches.h"
 #include "gpu/ipc/command_buffer_task_executor.h"
+#include "gpu/ipc/scheduler_sequence.h"
 #include "gpu/ipc/service/gpu_memory_buffer_factory.h"
 #include "ui/gfx/switches.h"
 
@@ -139,6 +141,8 @@ void VizCompositorThreadRunner::CreateFrameSinkManagerOnCompositorThread(
     GpuServiceImpl* gpu_service) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(!frame_sink_manager_);
+  if (features::IsUsingSkiaRenderer())
+    gpu::SchedulerSequence::DefaultDisallowScheduleTaskOnCurrentThread();
 
   server_shared_bitmap_manager_ = std::make_unique<ServerSharedBitmapManager>();
   base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(

gpu/ipc/scheduler_sequence.cc

@@ -3,10 +3,47 @@
 // found in the LICENSE file.
 
 #include "gpu/ipc/scheduler_sequence.h"
+
+#include "base/no_destructor.h"
+#include "base/threading/thread_local.h"
 #include "gpu/command_buffer/service/scheduler.h"
 
 namespace gpu {
 
+namespace {
+
+#if DCHECK_IS_ON()
+base::ThreadLocalBoolean* GetScheduleTaskDisallowed() {
+  static base::NoDestructor<base::ThreadLocalBoolean> disallowed;
+  return disallowed.get();
+}
+#endif  // DCHECK_IS_ON()
+
+}  // namespace
+
+ScopedAllowScheduleGpuTask::ScopedAllowScheduleGpuTask()
+#if DCHECK_IS_ON()
+    : original_value_(GetScheduleTaskDisallowed()->Get())
+#endif  // DCHECK_IS_ON()
+{
+#if DCHECK_IS_ON()
+  GetScheduleTaskDisallowed()->Set(false);
+#endif  // DCHECK_IS_ON()
+}
+
+ScopedAllowScheduleGpuTask::~ScopedAllowScheduleGpuTask() {
+#if DCHECK_IS_ON()
+  GetScheduleTaskDisallowed()->Set(original_value_);
+#endif  // DCHECK_IS_ON()
+}
+
+// static
+void SchedulerSequence::DefaultDisallowScheduleTaskOnCurrentThread() {
+#if DCHECK_IS_ON()
+  GetScheduleTaskDisallowed()->Set(true);
+#endif
+}
+
 SchedulerSequence::SchedulerSequence(Scheduler* scheduler)
     : SingleTaskSequence(),
       scheduler_(scheduler),
@@ -28,6 +65,24 @@ bool SchedulerSequence::ShouldYield() {
 
 void SchedulerSequence::ScheduleTask(base::OnceClosure task,
                                      std::vector<SyncToken> sync_token_fences) {
+  // If your CL is failing this DCHECK, then that means you are probably calling
+  // ScheduleGpuTask at a point that cannot be supported by Android Webview.
+  // Consider using ScheduleOrRetainGpuTask which will delay (not reorder) the
+  // task in Android Webview until the next DrawAndSwap.
+#if DCHECK_IS_ON()
+  DCHECK(!GetScheduleTaskDisallowed()->Get())
+      << "If your CL is failing this DCHECK, then that means you are probably "
+         "calling ScheduleGpuTask at a point that cannot be supported by "
+         "Android Webview. Consider using ScheduleOrRetainGpuTask which will "
+         "delay (not reorder) the task in Android Webview until the next "
+         "DrawAndSwap.";
+#endif
+  ScheduleOrRetainTask(std::move(task), std::move(sync_token_fences));
+}
+
+void SchedulerSequence::ScheduleOrRetainTask(
+    base::OnceClosure task,
+    std::vector<gpu::SyncToken> sync_token_fences) {
   scheduler_->ScheduleTask(Scheduler::Task(sequence_id_, std::move(task),
                                            std::move(sync_token_fences)));
 }

gpu/ipc/scheduler_sequence.h

@@ -9,32 +9,61 @@
 #include <vector>
 
 #include "base/callback.h"
+#include "base/logging.h"
 #include "base/macros.h"
 #include "gpu/command_buffer/common/sync_token.h"
 #include "gpu/command_buffer/service/sequence_id.h"
 #include "gpu/ipc/gl_in_process_context_export.h"
 #include "gpu/ipc/single_task_sequence.h"
 
+namespace viz {
+class Display;
+class OutputSurfaceProviderImpl;
+}  // namespace viz
+
 namespace gpu {
 class Scheduler;
 
+// Selectively allow ScheduleTask if DefaultDisallowScheduleTaskOnCurrentThread
+// is used for a thread.
+class GL_IN_PROCESS_CONTEXT_EXPORT ScopedAllowScheduleGpuTask {
+ public:
+  ~ScopedAllowScheduleGpuTask();
+
+ private:
+  // Only add more friend declarations for classes that Android WebView is
+  // guaranteed to be able to support. Talk to boliu@ if in doubt.
+  friend class viz::Display;
+  friend class viz::OutputSurfaceProviderImpl;
+  ScopedAllowScheduleGpuTask();
+
+#if DCHECK_IS_ON()
+  const bool original_value_;
+#endif
+  DISALLOW_COPY_AND_ASSIGN(ScopedAllowScheduleGpuTask);
+};
+
 // SingleTaskSequence implementation that uses gpu scheduler sequences.
 class GL_IN_PROCESS_CONTEXT_EXPORT SchedulerSequence
     : public SingleTaskSequence {
  public:
+  // Enable DCHECKs for Android WebView restrictions for ScheduleTask for
+  // current thread. Then use ScopedAllowScheduleGpuTask to selectively
+  // allow ScheduleTask.
+  static void DefaultDisallowScheduleTaskOnCurrentThread();
+
   explicit SchedulerSequence(Scheduler* scheduler);
 
   // Note: this drops tasks not executed yet.
   ~SchedulerSequence() override;
 
   // SingleTaskSequence implementation.
   SequenceId GetSequenceId() override;
-
   bool ShouldYield() override;
-
   void ScheduleTask(base::OnceClosure task,
                     std::vector<SyncToken> sync_token_fences) override;
-
+  void ScheduleOrRetainTask(base::OnceClosure task,
+                            std::vector<SyncToken> sync_token_fences) override;
   void ContinueTask(base::OnceClosure task) override;
 
  private:

gpu/ipc/single_task_sequence.h

@@ -33,9 +33,22 @@ class GL_IN_PROCESS_CONTEXT_EXPORT SingleTaskSequence {
   // Schedule a task with provided sync token dependencies. The dependencies
   // are hints for sync token waits within the task, and can be ignored by the
   // implementation.
+  // For scheduling from viz thread, due to limitations in Android WebView,
+  // ScheduleTask is only available to be called inside initialization,
+  // teardown, and DrawAndSwap.
   virtual void ScheduleTask(base::OnceClosure task,
                             std::vector<SyncToken> sync_token_fences) = 0;
 
+  // If |ScheduleGpuTask| is available, then this is equivalent to
+  // ScheduleGpuTask. Otherwise, the |task| and |sync_tokens| are retained
+  // and run when |ScheduleGpuTask| becomes available. Either case, tasks in
+  // |ScheduleTask| and |ScheduleOrRetainTask| are sequenced by the call order;
+  // calling this instead of |ScheduleTask| can only delay but not reorder
+  // tasks.
+  virtual void ScheduleOrRetainTask(
+      base::OnceClosure task,
+      std::vector<SyncToken> sync_token_fences) = 0;
+
   // Continue running the current task after yielding execution.
   virtual void ContinueTask(base::OnceClosure task) = 0;
 };