aw: Fix race between first frame draw and view detach causing deadlock.

android_webview/browser/render_thread_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "android_webview/browser/render_thread_manager.h"
 
 #include <utility>
 
 #include "android_webview/browser/compositor_frame_producer.h"
 #include "android_webview/browser/compositor_id.h"
@@ skipping 80 matching lines @@
 base::LazyInstance<internal::RequestInvokeGLTracker>
     g_request_invoke_gl_tracker = LAZY_INSTANCE_INITIALIZER;
 }
 
 RenderThreadManager::RenderThreadManager(
     RenderThreadManagerClient* client,
     const scoped_refptr<base::SingleThreadTaskRunner>& ui_loop)
     : ui_loop_(ui_loop),
       client_(client),
       compositor_frame_producer_(nullptr),
+      has_received_frame_(false),
       renderer_manager_key_(GLViewRendererManager::GetInstance()->NullKey()),
-      hardware_renderer_has_frame_(false),
       sync_on_draw_hardware_(base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSyncOnDrawHardware)),
       inside_hardware_release_(false),
       weak_factory_on_ui_thread_(this) {
   DCHECK(ui_loop_->BelongsToCurrentThread());
   DCHECK(client_);
   ui_thread_weak_ptr_ = weak_factory_on_ui_thread_.GetWeakPtr();
   ResetRequestInvokeGLCallback();
 }
 
@@ skipping 63 matching lines @@
 
 gfx::Vector2d RenderThreadManager::GetScrollOffsetOnRT() {
   base::AutoLock lock(lock_);
   return scroll_offset_;
 }
 
 std::unique_ptr<ChildFrame> RenderThreadManager::SetFrameOnUI(
     std::unique_ptr<ChildFrame> new_frame) {
   DCHECK(new_frame);
   base::AutoLock lock(lock_);
+
+  has_received_frame_ = true;

[boliu, 2016/12/06 16:15:35]

maybe add a TODO here, this doesn't work if we switch to full async draw, ie we
only get a future here and don't actually know if the frame is empty or not,
maybe update TODO in BVR.h as well to point to this issue

[Tobias Sargeant, 2016/12/06 18:11:26]

Done.

+
   if (child_frames_.empty()) {
     child_frames_.emplace_back(std::move(new_frame));
     return nullptr;
   }
   std::unique_ptr<ChildFrame> uncommitted_frame;
   if (new_frame->frame) {
     // Optimization for synchronous path.
     // TODO(boliu): Remove when synchronous path is fully removed.
     DCHECK_LE(child_frames_.size(), 1u);
     if (!child_frames_.empty()) {
       uncommitted_frame = std::move(child_frames_.front());
       child_frames_.pop_front();
     }
     child_frames_.emplace_back(std::move(new_frame));
     return uncommitted_frame;
   }
 
   DCHECK_LE(child_frames_.size(), 2u);
   ChildFrameQueue pruned_frames =
       HardwareRenderer::WaitAndPruneFrameQueue(&child_frames_);
   DCHECK_LE(pruned_frames.size(), 1u);
   if (pruned_frames.size())
     uncommitted_frame = std::move(pruned_frames.front());
   child_frames_.emplace_back(std::move(new_frame));
   return uncommitted_frame;
 }
 
 ChildFrameQueue RenderThreadManager::PassFramesOnRT() {
   base::AutoLock lock(lock_);
-  hardware_renderer_has_frame_ =
-      hardware_renderer_has_frame_ || !child_frames_.empty();
   ChildFrameQueue returned_frames;
   returned_frames.swap(child_frames_);
   return returned_frames;
 }
 
 ChildFrameQueue RenderThreadManager::PassUncommittedFrameOnUI() {
   base::AutoLock lock(lock_);
   for (auto& frame_ptr : child_frames_)
     frame_ptr->WaitOnFutureIfNeeded();
   ChildFrameQueue returned_frames;
@@ skipping 100 matching lines @@
       !IsInsideHardwareRelease() && HasFrameForHardwareRendererOnRT()) {
     hardware_renderer_.reset(new HardwareRenderer(this));
     hardware_renderer_->CommitFrame();
   }
 
   if (draw_info->mode == AwDrawGLInfo::kModeProcessNoContext) {
     LOG(ERROR) << "Received unexpected kModeProcessNoContext";
   }
 
   if (IsInsideHardwareRelease()) {
-    hardware_renderer_has_frame_ = false;
     hardware_renderer_.reset();
+    // If there are no frames ready to pass to a new
+    // hardware_renderer_ then consider that equivalent to never
+    // having received a frame.
+    has_received_frame_ = HasFrameForHardwareRendererOnRT();

[boliu, 2016/12/06 16:15:35]

I don't understand this

also this means has_received_frame_ is accessed on RT as well

[Tobias Sargeant, 2016/12/06 16:29:27]

Good point about has_received_frame_. The thing I wanted to avoid was this
sequence:

* has_received_frame_ is true
* detach causes hr_ destroy but has_received_frame_ is not reset.
* first frame after attach (in sync mode) is empty, causing BVR::OnDrawHardware
to return true, even though there's nothing to draw.

[boliu, 2016/12/06 16:34:10]

can you unset has_received_frame_ at end of DeleteHardwareRendererOnUI, on UI
thread?

[Tobias Sargeant, 2016/12/06 16:36:38]

Yes. That's a much better solution.

[Tobias Sargeant, 2016/12/06 18:11:26]

Done.

     // Flush the idle queue in tear down.
     DeferredGpuCommandService::GetInstance()->PerformAllIdleWork();
     return;
   }
 
   if (draw_info->mode != AwDrawGLInfo::kModeDraw) {
     if (draw_info->mode == AwDrawGLInfo::kModeProcess) {
       DeferredGpuCommandService::GetInstance()->PerformIdleWork(true);
     }
     return;
   }
 
   if (hardware_renderer_)
     hardware_renderer_->DrawGL(draw_info);
   DeferredGpuCommandService::GetInstance()->PerformIdleWork(false);
 }
 
 void RenderThreadManager::DeleteHardwareRendererOnUI() {
   DCHECK(ui_loop_->BelongsToCurrentThread());
 
   InsideHardwareReleaseReset auto_inside_hardware_release_reset(this);
 
+  client_->DetachFunctorFromView();
+
   // If the WebView gets onTrimMemory >= MODERATE twice in a row, the 2nd
   // onTrimMemory will result in an unnecessary Render Thread InvokeGL call.
-  bool hardware_initialized = HasFrameOnUI();
-  if (hardware_initialized) {
-    // The functor has only been attached to the view hierarchy if a compositor
-    // frame has been generated. Thus, it should only be detached in this case.
-    client_->DetachFunctorFromView();
-
+  if (has_received_frame_) {
+    // Receiving at least one frame is a precondition for
+    // initialization (such as looing up GL bindings and constructing
+    // hardware_renderer_).
     bool draw_functor_succeeded = client_->RequestInvokeGL(true);
     if (!draw_functor_succeeded) {
       LOG(ERROR) << "Unable to free GL resources. Has the Window leaked?";
       // Calling release on wrong thread intentionally.
       AwDrawGLInfo info;
       info.mode = AwDrawGLInfo::kModeProcess;
       DrawGL(&info);
     }
   }
 
   GLViewRendererManager* manager = GLViewRendererManager::GetInstance();
 
   {
     base::AutoLock lock(lock_);
     if (renderer_manager_key_ != manager->NullKey()) {
       manager->Remove(renderer_manager_key_);
       renderer_manager_key_ = manager->NullKey();
     }
   }
 
-  if (hardware_initialized) {
+  if (has_received_frame_) {
     // Flush any invoke functors that's caused by ReleaseHardware.
     client_->RequestInvokeGL(true);
   }
 }
 
 void RenderThreadManager::SetCompositorFrameProducer(
     CompositorFrameProducer* compositor_frame_producer) {
   DCHECK(compositor_frame_producer == compositor_frame_producer_ ||
          compositor_frame_producer_ == nullptr ||
          compositor_frame_producer == nullptr);
   compositor_frame_producer_ = compositor_frame_producer;
 }
 
 bool RenderThreadManager::HasFrameOnUI() const {
   base::AutoLock lock(lock_);
-  return hardware_renderer_has_frame_ || !child_frames_.empty();
+  return has_received_frame_;
 }
 
 bool RenderThreadManager::HasFrameForHardwareRendererOnRT() const {
   base::AutoLock lock(lock_);
   return !child_frames_.empty();
 }
 
 void RenderThreadManager::InitializeHardwareDrawIfNeededOnUI() {
   DCHECK(ui_loop_->BelongsToCurrentThread());
   GLViewRendererManager* manager = GLViewRendererManager::GetInstance();
 
   base::AutoLock lock(lock_);
   if (renderer_manager_key_ == manager->NullKey()) {
     renderer_manager_key_ = manager->PushBack(this);
   }
 }
 
 RenderThreadManager::InsideHardwareReleaseReset::InsideHardwareReleaseReset(
     RenderThreadManager* render_thread_manager)
     : render_thread_manager_(render_thread_manager) {
   DCHECK(!render_thread_manager_->IsInsideHardwareRelease());
   render_thread_manager_->SetInsideHardwareRelease(true);
 }
 
 RenderThreadManager::InsideHardwareReleaseReset::~InsideHardwareReleaseReset() {
   render_thread_manager_->SetInsideHardwareRelease(false);
 }
 
 }  // namespace android_webview