cc: Always keep the PictureLayerImpl::twin_layer_ pointer valid.

cc/layers/picture_layer_impl.cc

Index: cc/layers/picture_layer_impl.cc
diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_impl.cc
index 8cf0fdbf233dfea6c3308c53a3f7d83adf50bfc4..ec27b70ed45c7ef002cfb225dcd1dfb26f57ad80 100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -90,6 +90,8 @@ PictureLayerImpl::PictureLayerImpl(LayerTreeImpl* tree_impl, int id)
 }
 
 PictureLayerImpl::~PictureLayerImpl() {
+  if (twin_layer_)
+    twin_layer_->twin_layer_ = nullptr;
   layer_tree_impl()->UnregisterPictureLayerImpl(this);
 }
 
@@ -110,10 +112,11 @@ void PictureLayerImpl::PushPropertiesTo(LayerImpl* base_layer) {
 
   LayerImpl::PushPropertiesTo(base_layer);
 
-  // When the pending tree pushes to the active tree, the pending twin
-  // becomes recycled.
-  layer_impl->twin_layer_ = nullptr;
-  twin_layer_ = nullptr;
+  // Twin relationships should never change once established.
+  DCHECK_IMPLIES(twin_layer_, twin_layer_ == layer_impl);
+  DCHECK_IMPLIES(twin_layer_, layer_impl->twin_layer_ == this);
+  twin_layer_ = layer_impl;

[vmpstr, 2014/10/24 16:47:05]

Can you make a comment here explaining why we don't need the twin relationship
before this push properties? My understanding is that before twins are
established we're in a situation where the active layer has no tilings/tiles, so
it won't access the twin for anything until we push. Is that correct?

[danakj, 2014/10/24 16:53:30]

Ya, before this the only thing we do is create the layer with tree synchronizer
and it knows nothing of picture-ness.

+  layer_impl->twin_layer_ = this;
 
   layer_impl->pile_ = pile_;
 
@@ -561,10 +564,16 @@ gfx::Rect PictureLayerImpl::GetViewportForTilePriorityInContentSpace() const {
   return visible_rect_in_content_space;
 }
 
-PictureLayerImpl* PictureLayerImpl::GetRecycledTwinLayer() {
-  // TODO(vmpstr): Maintain recycled twin as a member. crbug.com/407418
-  return static_cast<PictureLayerImpl*>(
-      layer_tree_impl()->FindRecycleTreeLayerById(id()));
+PictureLayerImpl* PictureLayerImpl::GetPendingOrActiveTwinLayer() const {
+  if (!twin_layer_ || !twin_layer_->IsOnActiveOrPendingTree())
+    return nullptr;
+  return twin_layer_;
+}
+
+PictureLayerImpl* PictureLayerImpl::GetRecycledTwinLayer() const {
+  if (!twin_layer_ || twin_layer_->IsOnActiveOrPendingTree())
+    return nullptr;
+  return twin_layer_;
 }
 
 void PictureLayerImpl::NotifyTileStateChanged(const Tile* tile) {
@@ -638,9 +647,10 @@ const Region* PictureLayerImpl::GetInvalidation() {
 
 const PictureLayerTiling* PictureLayerImpl::GetTwinTiling(

[vmpstr, 2014/10/24 16:47:05]

nit: This potentially should be GetActiveOrPendingTwinTiling (or GetXTwinTiling
where X is a single word that captures the fact that it's not recycled). Feel
free to ignore this though, since it already is the way it is.

[danakj, 2014/10/24 16:53:30]

Hm, it's part of the PLTClient API. I can change it though.

     const PictureLayerTiling* tiling) const {
-  if (!twin_layer_)
+  PictureLayerImpl* twin_layer = GetPendingOrActiveTwinLayer();
+  if (!twin_layer)
     return nullptr;
-  return twin_layer_->tilings_->TilingAtScale(tiling->contents_scale());
+  return twin_layer->tilings_->TilingAtScale(tiling->contents_scale());
 }
 
 PictureLayerTiling* PictureLayerImpl::GetRecycledTwinTiling(
@@ -845,16 +855,12 @@ void PictureLayerImpl::DoPostCommitInitialization() {
   if (!tilings_)
     tilings_.reset(new PictureLayerTilingSet(this, bounds()));
 
-  DCHECK(!twin_layer_);
-  twin_layer_ = static_cast<PictureLayerImpl*>(
-      layer_tree_impl()->FindActiveTreeLayerById(id()));
-  if (twin_layer_) {
-    DCHECK(!twin_layer_->twin_layer_);
-    twin_layer_->twin_layer_ = this;
+  PictureLayerImpl* twin_layer = GetPendingOrActiveTwinLayer();
+  if (twin_layer) {
     // If the twin has never been pushed to, do not sync from it.
     // This can happen if this function is called during activation.
-    if (!twin_layer_->needs_post_commit_initialization_)
-      SyncFromActiveLayer(twin_layer_);
+    if (!twin_layer->needs_post_commit_initialization_)
+      SyncFromActiveLayer(twin_layer);
   }
 
   needs_post_commit_initialization_ = false;
@@ -868,8 +874,8 @@ PictureLayerTiling* PictureLayerImpl::AddTiling(float contents_scale) {
 
   DCHECK(pile_->HasRecordings());
 
-  if (twin_layer_)
-    twin_layer_->SyncTiling(tiling);
+  if (PictureLayerImpl* twin_layer = GetPendingOrActiveTwinLayer())
+    twin_layer->SyncTiling(tiling);
 
   return tiling;
 }
@@ -1113,7 +1119,7 @@ void PictureLayerImpl::CleanUpTilingsOnActiveLayer(
       raster_contents_scale_, ideal_contents_scale_);
   float twin_low_res_scale = 0.f;
 
-  PictureLayerImpl* twin = twin_layer_;
+  PictureLayerImpl* twin = GetPendingOrActiveTwinLayer();
   if (twin && twin->CanHaveTilings()) {
     min_acceptable_high_res_scale = std::min(
         min_acceptable_high_res_scale,