Android: Second attempt at freeing GLHelper ashmem

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index 2412e18cc69ae41c91a283b12e02f3f8c6cd4c17..6fa407e91b8d7c98b2a995dd69d54e2697bd2093 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -17,7 +17,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/memory/memory_pressure_listener.h"
+#include "base/memory/ref_counted.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
@@ -102,6 +102,23 @@ const int kUndefinedCompositorFrameSinkId = -1;
 
 static const char kAsyncReadBackString[] = "Compositing.CopyFromSurfaceTime";
 
+class PendingReadbackLock;
+
+PendingReadbackLock* g_pending_readback_lock = nullptr;
+
+class PendingReadbackLock : public base::RefCounted<PendingReadbackLock> {

[danakj, 2016/10/03 20:47:43]

This is fine, but another way to do this would be

int g_num_readbacks_pending = 0;

class PendingReadbackLock() {
  PendingReadbackLock() { ++g_num_readbacks_pending; }
  ~PendingReadbackLock() { --g_num_readbacks_pending; }
};

Then we don't need any refcounted objects, the global is simply the refcount. Up
to you.

+ public:
+  PendingReadbackLock() {
+    DCHECK_EQ(g_pending_readback_lock, nullptr);
+    g_pending_readback_lock = this;
+  }
+
+  ~PendingReadbackLock() {

[danakj, 2016/10/03 20:47:43]

Destructor should be private with friend of RefCounted.

[no sievers, 2016/10/05 00:38:40]

Done.

+    DCHECK_EQ(g_pending_readback_lock, this);
+    g_pending_readback_lock = nullptr;
+  }
+};
+
 using base::android::ApplicationState;
 using base::android::ApplicationStatusListener;
 using base::MemoryPressureListener;
@@ -117,25 +134,21 @@ class GLHelperHolder {
     return provider_->ContextGL()->GetGraphicsResetStatusKHR() != GL_NO_ERROR;
   }
 
-  void FreeUnusedSharedMemoryIfNeeded();
+  void ReleaseIfPossible();
 
  private:
   GLHelperHolder();
   void Initialize();
   void OnContextLost();
   void OnApplicationStatusChanged(ApplicationState new_state);
-  void OnMemoryPressure(MemoryPressureListener::MemoryPressureLevel level);
 
   scoped_refptr<ContextProviderCommandBuffer> provider_;
   std::unique_ptr<display_compositor::GLHelper> gl_helper_;
 
   // Set to |false| if there are only stopped activities (or none).
   bool has_running_or_paused_activities_;
-  // Whether we recently were signaled a low memory condition.
-  bool did_signal_memory_pressure_;
 
   std::unique_ptr<ApplicationStatusListener> app_status_listener_;
-  std::unique_ptr<MemoryPressureListener> mem_pressure_listener_;
 
   DISALLOW_COPY_AND_ASSIGN(GLHelperHolder);
 };
@@ -145,8 +158,7 @@ GLHelperHolder::GLHelperHolder()
           (ApplicationStatusListener::GetState() ==
            base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) ||
           (ApplicationStatusListener::GetState() ==
-           base::android::APPLICATION_STATE_HAS_PAUSED_ACTIVITIES)),
-      did_signal_memory_pressure_(false) {}
+           base::android::APPLICATION_STATE_HAS_PAUSED_ACTIVITIES)) {}
 
 GLHelperHolder* GLHelperHolder::Create() {
   GLHelperHolder* holder = new GLHelperHolder;
@@ -214,18 +226,17 @@ void GLHelperHolder::Initialize() {
   // Unretained() is safe because |this| owns the following two callbacks.
   app_status_listener_.reset(new ApplicationStatusListener(base::Bind(
       &GLHelperHolder::OnApplicationStatusChanged, base::Unretained(this))));
-  mem_pressure_listener_.reset(new MemoryPressureListener(base::Bind(
-      &GLHelperHolder::OnMemoryPressure, base::Unretained(this))));
 }
 
-void GLHelperHolder::FreeUnusedSharedMemoryIfNeeded() {
-  if (!has_running_or_paused_activities_ || did_signal_memory_pressure_)
-    provider_->FreeUnusedSharedMemory();
+void GLHelperHolder::ReleaseIfPossible() {
+  if (!has_running_or_paused_activities_ && !g_pending_readback_lock) {
+    gl_helper_.reset();
+    provider_ = nullptr;
+  }
 }
 
 void GLHelperHolder::OnContextLost() {
   app_status_listener_.reset();
-  mem_pressure_listener_.reset();
   gl_helper_.reset();
   // Need to post a task because the command buffer client cannot be deleted
   // from within this callback.
@@ -234,29 +245,10 @@ void GLHelperHolder::OnContextLost() {
 }
 
 void GLHelperHolder::OnApplicationStatusChanged(ApplicationState new_state) {
-  bool new_has_running_or_paused_activities =
+  has_running_or_paused_activities_ =
       new_state == base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES ||
       new_state == base::android::APPLICATION_STATE_HAS_PAUSED_ACTIVITIES;
-
-  if (!has_running_or_paused_activities_ &&
-      new_has_running_or_paused_activities) {
-    // Reset at this time since on Android there is no 'back to normal'
-    // notification (but only onTrim/onLowMemory).
-    did_signal_memory_pressure_ = false;
-  } else if (!new_has_running_or_paused_activities) {
-    provider_->FreeUnusedSharedMemory();
-  }
-
-  has_running_or_paused_activities_ = new_has_running_or_paused_activities;
-}
-
-void GLHelperHolder::OnMemoryPressure(
-    MemoryPressureListener::MemoryPressureLevel level) {
-  if (level == MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE ||
-      level == MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL) {
-    did_signal_memory_pressure_ = true;
-    provider_->FreeUnusedSharedMemory();
-  }
+  ReleaseIfPossible();
 }
 
 GLHelperHolder* GetPostReadbackGLHelperHolder(bool create_if_necessary) {
@@ -281,12 +273,12 @@ display_compositor::GLHelper* GetPostReadbackGLHelper() {
   return GetPostReadbackGLHelperHolder(create_if_necessary)->gl_helper();
 }
 
-void FreeUnusedGLHelperMemory() {
+void ReleaseGLHelper() {
   bool create_if_necessary = false;
   GLHelperHolder* holder = GetPostReadbackGLHelperHolder(create_if_necessary);
 
   if (holder) {
-    holder->FreeUnusedSharedMemoryIfNeeded();
+    holder->ReleaseIfPossible();
   }
 }
 
@@ -296,6 +288,7 @@ void CopyFromCompositingSurfaceFinished(
     std::unique_ptr<SkBitmap> bitmap,
     const base::TimeTicks& start_time,
     std::unique_ptr<SkAutoLockPixels> bitmap_pixels_lock,
+    scoped_refptr<PendingReadbackLock> readback_lock,
     bool result) {
   TRACE_EVENT0(
       "cc", "RenderWidgetHostViewAndroid::CopyFromCompositingSurfaceFinished");
@@ -307,7 +300,8 @@ void CopyFromCompositingSurfaceFinished(
       gl_helper->GenerateSyncToken(&sync_token);
   }
 
-  FreeUnusedGLHelperMemory();
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,

[danakj, 2016/10/03 20:47:44]

I'd like a comment explaining why this is posting the task instead of doing it
immediately, cuz that's not clear to me

[no sievers, 2016/10/05 00:38:40]

Done.

+                          base::Bind(&ReleaseGLHelper));
 
   const bool lost_resource = !sync_token.HasData();
   release_callback->Run(sync_token, lost_resource);
@@ -352,6 +346,14 @@ gfx::RectF GetSelectionRect(const ui::TouchSelectionController& controller) {
   return rect;
 }
 
+void PrepareTextureCopyOutputResult(
+    const gfx::Size& dst_size_in_pixel,
+    SkColorType color_type,
+    const base::TimeTicks& start_time,
+    const ReadbackRequestCallback& callback,
+    scoped_refptr<PendingReadbackLock> readback_lock,
+    std::unique_ptr<cc::CopyOutputResult> result);
+
 }  // anonymous namespace
 
 RenderWidgetHostViewAndroid::LastFrameInfo::LastFrameInfo(
@@ -948,10 +950,13 @@ void RenderWidgetHostViewAndroid::CopyFromCompositingSurface(
       content_view_core_->GetWindowAndroid()->GetCompositor();
   DCHECK(compositor);
   DCHECK(delegated_frame_host_);
+  scoped_refptr<PendingReadbackLock> readback_lock(
+      g_pending_readback_lock ? g_pending_readback_lock
+                              : new PendingReadbackLock);
   delegated_frame_host_->RequestCopyOfSurface(
       compositor, src_subrect_in_pixel,
       base::Bind(&PrepareTextureCopyOutputResult, dst_size_in_pixel,
-                 preferred_color_type, start_time, callback));
+                 preferred_color_type, start_time, callback, readback_lock));
 }
 
 void RenderWidgetHostViewAndroid::CopyFromCompositingSurfaceToVideoFrame(
@@ -1864,12 +1869,14 @@ void RenderWidgetHostViewAndroid::OnLostResources() {
 // this file, and the versions in surface_utils.cc. They should
 // be merged. See https://crbug.com/582955
 
-// static
-void RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResult(
+namespace {

[danakj, 2016/10/03 20:47:44]

bonus points if this method can move up to the anon namespace at the top of the
file.

[no sievers, 2016/10/05 00:38:40]

Done.

+
+void PrepareTextureCopyOutputResult(
     const gfx::Size& dst_size_in_pixel,
     SkColorType color_type,
     const base::TimeTicks& start_time,
     const ReadbackRequestCallback& callback,
+    scoped_refptr<PendingReadbackLock> readback_lock,
     std::unique_ptr<cc::CopyOutputResult> result) {
   base::ScopedClosureRunner scoped_callback_runner(
       base::Bind(callback, SkBitmap(), READBACK_FAILED));
@@ -1916,10 +1923,12 @@ void RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResult(
       gfx::Rect(result->size()), output_size_in_pixel, pixels, color_type,
       base::Bind(&CopyFromCompositingSurfaceFinished, callback,
                  base::Passed(&release_callback), base::Passed(&bitmap),
-                 start_time, base::Passed(&bitmap_pixels_lock)),
+                 start_time, base::Passed(&bitmap_pixels_lock), readback_lock),
       display_compositor::GLHelper::SCALER_QUALITY_GOOD);
 }
 
+}  // anonymous namespace

[danakj, 2016/10/03 20:47:44]

just "// namespace"

[no sievers, 2016/10/05 00:38:40]

Done.

+
 void RenderWidgetHostViewAndroid::OnStylusSelectBegin(float x0,
                                                       float y0,
                                                       float x1,