Temporarily remove DCHECK that triggers during hardware teardown.

android_webview/browser/shared_renderer_state.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/shared_renderer_state.h"
 
 #include "android_webview/browser/browser_view_renderer.h"
 #include "android_webview/browser/deferred_gpu_command_service.h"
 #include "android_webview/browser/hardware_renderer.h"
 #include "android_webview/browser/scoped_app_gl_state_restore.h"
@@ skipping 51 matching lines @@
 
 void RequestDrawGLTracker::ResetPending() {
   base::AutoLock lock(lock_);
   pending_non_ui_ = NULL;
   pending_ui_ = NULL;
 }
 
 void RequestDrawGLTracker::SetQueuedFunctorOnUi(SharedRendererState* state) {
   base::AutoLock lock(lock_);
   DCHECK(state);
-  DCHECK(pending_ui_ == state || pending_non_ui_ == state);
+  // DCHECK(pending_ui_ == state || pending_non_ui_ == state);

[boliu, 2015/02/05 17:10:22]

Just remove it.

We usually don't comment things out in hopes of uncommenting them in the future,
because it usually never happens and it's just cruft left in the source.

   pending_ui_ = state;
   pending_non_ui_ = NULL;
 }
 
 }  // namespace internal
 
 namespace {
 
 base::LazyInstance<internal::RequestDrawGLTracker> g_request_draw_gl_tracker =
     LAZY_INSTANCE_INITIALIZER;
@@ skipping 189 matching lines @@
 void SharedRendererState::DrawGL(AwDrawGLInfo* draw_info) {
   TRACE_EVENT0("android_webview", "DrawFunctor");
   if (draw_info->mode == AwDrawGLInfo::kModeSync) {
     TRACE_EVENT_INSTANT0("android_webview", "kModeSync",
                          TRACE_EVENT_SCOPE_THREAD);
     if (hardware_renderer_)
       hardware_renderer_->CommitFrame();
     return;
   }
 
-  // kModeProcessNoContext should never happen because we tear down hardware
-  // in onTrimMemory. However that guarantee is maintained outside of chromium
-  // code. Not notifying shared state in kModeProcessNoContext can lead to
-  // immediate deadlock, which is slightly more catastrophic than leaks or
-  // corruption.
-  if (draw_info->mode == AwDrawGLInfo::kModeProcess ||
-      draw_info->mode == AwDrawGLInfo::kModeProcessNoContext) {
-    DidDrawGLProcess();
-  }
-
   {
     GLViewRendererManager* manager = GLViewRendererManager::GetInstance();
     base::AutoLock lock(lock_);
     if (renderer_manager_key_ != manager->NullKey()) {
       manager->DidDrawGL(renderer_manager_key_);
     }
   }
 
   ScopedAppGLStateRestore state_restore(
       draw_info->mode == AwDrawGLInfo::kModeDraw
           ? ScopedAppGLStateRestore::MODE_DRAW
           : ScopedAppGLStateRestore::MODE_RESOURCE_MANAGEMENT);
   ScopedAllowGL allow_gl;
 
-  if (draw_info->mode == AwDrawGLInfo::kModeProcessNoContext) {
+  switch (draw_info->mode) {
+  case AwDrawGLInfo::kModeProcessNoContext: {

[boliu, 2015/02/05 17:10:22]

Indent here is wrong.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...

In general, don't really have to worry about style issues and just learn to use
clang-format: https://code.google.com/p/chromium/wiki/ClangFormat

[Tobias Sargeant, 2015/02/05 17:29:39]

Done.

     LOG(ERROR) << "Received unexpected kModeProcessNoContext";
+    DidDrawGLProcess();
+    break;
   }
 
-  if (IsInsideHardwareRelease()) {
-    hardware_renderer_.reset();
-    // Flush the idle queue in tear down.
-    DeferredGpuCommandService::GetInstance()->PerformAllIdleWork();
-    return;
+  case AwDrawGLInfo::kModeProcess: {
+    // kModeProcessNoContext should never happen because we tear down hardware

[boliu, 2015/02/05 17:10:22]

Move this comment up.

[Tobias Sargeant, 2015/02/05 17:29:39]

Done.

+    // in onTrimMemory. However that guarantee is maintained outside of chromium
+    // code. Not notifying shared state in kModeProcessNoContext can lead to
+    // immediate deadlock, which is slightly more catastrophic than leaks or
+    // corruption.
+    DidDrawGLProcess();
+
+    if (IsInsideHardwareRelease()) {
+      hardware_renderer_.reset();
+      // Flush the idle queue in tear down.
+      DeferredGpuCommandService::GetInstance()->PerformAllIdleWork();
+    } else {
+      DeferredGpuCommandService::GetInstance()->PerformIdleWork(true);
+    }
+    break;
   }
 
-  if (draw_info->mode != AwDrawGLInfo::kModeDraw) {
-    if (draw_info->mode == AwDrawGLInfo::kModeProcess) {
-      DeferredGpuCommandService::GetInstance()->PerformIdleWork(true);
+  case AwDrawGLInfo::kModeDraw: {
+    if (browser_view_renderer_->IsVisible()) {

[boliu, 2015/02/05 17:10:22]

Not thread safe. BVR is a UI thread only object, and DrawGL is called on render
thread.

What's wrong with using !IsInsideHardwareRelease() here?

[Tobias Sargeant, 2015/02/05 17:29:39]

The problem is that it seems possible to get draw calls that happen after the
teardown, when IsInsideHardwareRelease() is no longer true, but before the
shared_renderer_state destructor is called.

[boliu, 2015/02/05 17:36:17]

And that call is kModeDraw (not kModeProcess)? The answer matters a lot to where
the actual bug is.

The goal here is DrawGL should not be called after
ReleaseHardwareDrawIfNeededOnUI (at least until the next time this BVR is
hardware initialized again)

+      if (!hardware_renderer_) {
+        hardware_renderer_.reset(new HardwareRenderer(this));
+        hardware_renderer_->CommitFrame();
+      }
+
+      hardware_renderer_->DrawGL(state_restore.stencil_enabled(),
+                                 state_restore.framebuffer_binding_ext(),
+                                 draw_info);
+      DeferredGpuCommandService::GetInstance()->PerformIdleWork(false);
     }
-    return;
+    break;
   }
 
-  if (!hardware_renderer_) {
-    hardware_renderer_.reset(new HardwareRenderer(this));
-    hardware_renderer_->CommitFrame();
+  default: {
+    break;
   }
-
-  hardware_renderer_->DrawGL(state_restore.stencil_enabled(),
-                             state_restore.framebuffer_binding_ext(),
-                             draw_info);
-  DeferredGpuCommandService::GetInstance()->PerformIdleWork(false);
+  }
 }
 
 void SharedRendererState::ReleaseHardwareDrawIfNeededOnUI() {
   DCHECK(ui_loop_->BelongsToCurrentThread());
-  InsideHardwareReleaseReset auto_inside_hardware_release_reset(this);
-
   browser_view_renderer_->InvalidateOnFunctorDestroy();
   bool hardware_initialized = browser_view_renderer_->hardware_enabled();
   if (hardware_initialized) {
-    bool draw_functor_succeeded = browser_view_renderer_->RequestDrawGL(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);
+    {
+      InsideHardwareReleaseReset auto_inside_hardware_release_reset(this);

[boliu, 2015/02/05 17:10:22]

This used to wrap BVR::ReleaseHardware too.

+
+      bool draw_functor_succeeded = browser_view_renderer_->RequestDrawGL(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);
+      }
     }
-
     browser_view_renderer_->ReleaseHardware();
   }
 
   GLViewRendererManager* manager = GLViewRendererManager::GetInstance();
 
   {
     base::AutoLock lock(lock_);
     if (renderer_manager_key_ != manager->NullKey()) {
       manager->Remove(renderer_manager_key_);
       renderer_manager_key_ = manager->NullKey();
@@ skipping 22 matching lines @@
     : shared_renderer_state_(shared_renderer_state) {
   DCHECK(!shared_renderer_state_->IsInsideHardwareRelease());
   shared_renderer_state_->SetInsideHardwareRelease(true);
 }
 
 SharedRendererState::InsideHardwareReleaseReset::~InsideHardwareReleaseReset() {
   shared_renderer_state_->SetInsideHardwareRelease(false);
 }
 
 }  // namespace android_webview