aw: Check compositor pointer before use

android_webview/browser/browser_view_renderer.cc

Index: android_webview/browser/browser_view_renderer.cc
diff --git a/android_webview/browser/browser_view_renderer.cc b/android_webview/browser/browser_view_renderer.cc
index 33fad4f66416fd69484580d57e6aa7c97a4af619..6b984d2ac0a666fa48a8454904cc4af2e0133ea2 100644
--- a/android_webview/browser/browser_view_renderer.cc
+++ b/android_webview/browser/browser_view_renderer.cc
@@ -112,6 +112,9 @@ void BrowserViewRenderer::TrimMemory(const int level, const bool visible) {
     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.
@@ -159,6 +162,7 @@ BrowserViewRenderer::CalculateDesiredMemoryPolicy() {
 // 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().
@@ -295,14 +299,14 @@ void BrowserViewRenderer::ReturnUnusedResource(
   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);
   }
 }
@@ -432,13 +436,15 @@ void BrowserViewRenderer::ReleaseHardware() {
   ReturnUnusedResource(shared_renderer_state_->PassCompositorFrame());
   ReturnResourceFromParent();
   DCHECK(shared_renderer_state_->ReturnedResourcesEmpty());
-
-  compositor_->ReleaseHwDraw();
   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();
   SynchronousCompositorMemoryPolicy zero_policy;
   RequestMemoryPolicy(zero_policy);
-  GlobalTileManager::GetInstance()->Remove(tile_manager_key_);
 }
 
 bool BrowserViewRenderer::IsVisible() const {
@@ -463,9 +469,9 @@ void BrowserViewRenderer::DidDestroyCompositor(
     content::SynchronousCompositor* compositor) {
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::DidDestroyCompositor");
   DCHECK(compositor_);
-  compositor_ = NULL;
   SynchronousCompositorMemoryPolicy zero_policy;
-  DCHECK(memory_policy_ == 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.

 }
 
 void BrowserViewRenderer::SetContinuousInvalidate(bool invalidate) {