aw: Check compositor pointer before use

android_webview/browser/browser_view_renderer.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/browser_view_renderer.h"
 
 #include "android_webview/browser/browser_view_renderer_client.h"
 #include "android_webview/browser/shared_renderer_state.h"
 #include "android_webview/common/aw_switches.h"
 #include "base/auto_reset.h"
@@ skipping 94 matching lines @@
 // This function updates the resource allocation in GlobalTileManager.
 void BrowserViewRenderer::TrimMemory(const int level, const bool visible) {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   // Constants from Android ComponentCallbacks2.
   enum {
     TRIM_MEMORY_RUNNING_LOW = 10,
     TRIM_MEMORY_UI_HIDDEN = 20,
     TRIM_MEMORY_BACKGROUND = 40,
   };
 
+  if (!compositor_ || !hardware_enabled_)
+    return;

[hush (inactive), 2014/10/18 01:34:01]

ideally, (memory_policy_ != zero_policy) means compositor != NULL &&
hardware_enabled_ != NULL.
That means this early out is the same as the early out in line 131.

So can you add a DCHECK after line 132 instead?

[boliu, 2014/10/18 17:08:15]

Done.

+
   // Not urgent enough. TRIM_MEMORY_UI_HIDDEN is treated specially because
   // it does not indicate memory pressure, but merely that the app is
   // backgrounded.
   if (level < TRIM_MEMORY_RUNNING_LOW || level == TRIM_MEMORY_UI_HIDDEN)
     return;
 
   // Do not release resources on view we expect to get DrawGL soon.
   if (level < TRIM_MEMORY_BACKGROUND && visible)
     return;
 
@@ skipping 27 matching lines @@
   // is also kTileAllocationStep.
   tiles = (tiles / kTileAllocationStep + 1) * kTileAllocationStep;
   policy.num_resources_limit = tiles;
   return policy;
 }
 
 // This function updates the cached memory policy in shared renderer state, as
 // well as the tile resource allocation in GlobalTileManager.
 void BrowserViewRenderer::RequestMemoryPolicy(
     SynchronousCompositorMemoryPolicy& new_policy) {
+  DCHECK(compositor_);
   GlobalTileManager* manager = GlobalTileManager::GetInstance();
 
   // The following line will call BrowserViewRenderer::SetMemoryPolicy().
   manager->RequestTiles(new_policy, tile_manager_key_);
 }
 
 void BrowserViewRenderer::SetMemoryPolicy(
     SynchronousCompositorMemoryPolicy new_policy,
     bool effective_immediately) {
   memory_policy_ = new_policy;
@@ skipping 65 matching lines @@
   scoped_ptr<cc::CompositorFrame> frame = CompositeHw();
   if (!frame.get())
     return false;
 
   shared_renderer_state_->SetCompositorFrame(frame.Pass(), false);
   GlobalTileManager::GetInstance()->DidUse(tile_manager_key_);
   return client_->RequestDrawGL(java_canvas, false);
 }
 
 scoped_ptr<cc::CompositorFrame> BrowserViewRenderer::CompositeHw() {
   SynchronousCompositorMemoryPolicy new_policy = CalculateDesiredMemoryPolicy();

[hush (inactive), 2014/10/18 01:34:01]

what is the policy to add DCHECK(compositor_)?
if we need it every time we use it, then add one here...

[boliu, 2014/10/18 17:08:15]

Generally only DCHECK public methods that rely on compositor being present.
Private methods we are supposed to check in this class ourselves.

   RequestMemoryPolicy(new_policy);
   compositor_->SetMemoryPolicy(memory_policy_);
 
   parent_draw_constraints_ = shared_renderer_state_->ParentDrawConstraints();
   gfx::Size surface_size(width_, height_);
   gfx::Rect viewport(surface_size);
   gfx::Rect clip = viewport;
   gfx::Transform transform_for_tile_priority =
       parent_draw_constraints_.transform;
 
@@ skipping 30 matching lines @@
 }
 
 void BrowserViewRenderer::ReturnUnusedResource(
     scoped_ptr<cc::CompositorFrame> frame) {
   if (!frame.get())
     return;
 
   cc::CompositorFrameAck frame_ack;
   cc::TransferableResource::ReturnResources(
       frame->delegated_frame_data->resource_list, &frame_ack.resources);
-  if (!frame_ack.resources.empty())
+  if (compositor_ && !frame_ack.resources.empty())
     compositor_->ReturnResources(frame_ack);
 }
 
 void BrowserViewRenderer::ReturnResourceFromParent() {
   cc::CompositorFrameAck frame_ack;
   shared_renderer_state_->SwapReturnedResources(&frame_ack.resources);
-  if (!frame_ack.resources.empty()) {
+  if (compositor_ && !frame_ack.resources.empty()) {
     compositor_->ReturnResources(frame_ack);
   }
 }
 
 bool BrowserViewRenderer::OnDrawSoftware(jobject java_canvas) {
   if (!compositor_) {
     TRACE_EVENT_INSTANT0(
         "android_webview", "EarlyOut_NoCompositor", TRACE_EVENT_SCOPE_THREAD);
     return false;
   }
@@ skipping 109 matching lines @@
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::OnDetachedFromWindow");
   attached_to_window_ = false;
   DCHECK(!hardware_enabled_);
 }
 
 void BrowserViewRenderer::ReleaseHardware() {
   DCHECK(hardware_enabled_);
   ReturnUnusedResource(shared_renderer_state_->PassCompositorFrame());
   ReturnResourceFromParent();
   DCHECK(shared_renderer_state_->ReturnedResourcesEmpty());
+  hardware_enabled_ = false;
+  GlobalTileManager::GetInstance()->Remove(tile_manager_key_);

[boliu, 2014/10/18 17:08:15]

Woo this is wrong. Ned to request 0 policy *before* we remove the key. In the
same vein, DidDestroy should only request memory if hardware is enabled.

+
+  if (!compositor_)
+    return;
 
   compositor_->ReleaseHwDraw();
-  hardware_enabled_ = false;
-
   SynchronousCompositorMemoryPolicy zero_policy;
   RequestMemoryPolicy(zero_policy);
-  GlobalTileManager::GetInstance()->Remove(tile_manager_key_);
 }
 
 bool BrowserViewRenderer::IsVisible() const {
   // Ignore |window_visible_| if |attached_to_window_| is false.
   return view_visible_ && (!attached_to_window_ || window_visible_);
 }
 
 gfx::Rect BrowserViewRenderer::GetScreenRect() const {
   return gfx::Rect(client_->GetLocationOnScreen(), gfx::Size(width_, height_));
 }
 
 void BrowserViewRenderer::DidInitializeCompositor(
     content::SynchronousCompositor* compositor) {
   TRACE_EVENT0("android_webview",
                "BrowserViewRenderer::DidInitializeCompositor");
   DCHECK(compositor);
   DCHECK(!compositor_);
   compositor_ = compositor;
 }
 
 void BrowserViewRenderer::DidDestroyCompositor(
     content::SynchronousCompositor* compositor) {
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::DidDestroyCompositor");
   DCHECK(compositor_);
+  SynchronousCompositorMemoryPolicy zero_policy;
+  RequestMemoryPolicy(zero_policy);
   compositor_ = NULL;

[hush (inactive), 2014/10/18 01:34:01]

since didDestroyCompositor could be called before ReleaseHardware, there is a
period of time when compositor_ == NULL, but hardware_enabled_ is true.
What does that mean? 
I think we should skip onDraw in this period at least.

[boliu, 2014/10/18 17:08:15]

We do already, both OnDrawHardware and will skip immediately if compositor_ is
null.

-  SynchronousCompositorMemoryPolicy zero_policy;
-  DCHECK(memory_policy_ == zero_policy);
 }
 
 void BrowserViewRenderer::SetContinuousInvalidate(bool invalidate) {
   if (compositor_needs_continuous_invalidate_ == invalidate)
     return;
 
   TRACE_EVENT_INSTANT1("android_webview",
                        "BrowserViewRenderer::SetContinuousInvalidate",
                        TRACE_EVENT_SCOPE_THREAD,
                        "invalidate",
@@ skipping 303 matching lines @@
   base::StringAppendF(&str,
                       "overscroll_rounding_error_: %s ",
                       overscroll_rounding_error_.ToString().c_str());
   base::StringAppendF(
       &str, "on_new_picture_enable: %d ", on_new_picture_enable_);
   base::StringAppendF(&str, "clear_view: %d ", clear_view_);
   return str;
 }
 
 }  // namespace android_webview