(Reland) cc: Remove recycled tilings when active tree removes them.

(Reland) cc: Remove recycled tilings when active tree removes them.

This patch is the second part of ensuring that recycle tree does not
contain any unshared tiles.

This is done by removing recycle tree tilings when active tree removes
those tilings.

This includes a check in RemoveTilings for NULL tilings. When the first
pending tree activates, it swaps with active tree's NULL tilings, which
become the recycle tilings. When removing tilings, we have to ensure
that tilings is not NULL before accessing its members.

BUG=393802
R=danakj, enne

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285114

[vmpstr, 2014-07-20 01:32:35]

PTAL.

tonyg@, thanks for fixing the crashes. From my understanding of
crbug.com/395398, the crash was happening in RemoveTilings, can you confirm that
from the logs?

This reland includes a fix, but I will make sure to run through the cases that
were affected on my dev android devices when I'm in the office.

[enne (OOO), 2014-07-21 22:59:35]

lgtm

[tonyg, 2014-07-22 02:51:23]

On Sat, Jul 19, 2014 at 6:32 PM, <vmpstr@chromium.org> wrote:

> Reviewers: danakj, enne(OOO until 21st),
>
> Message:
> PTAL.
>
> tonyg@, thanks for fixing the crashes. From my understanding of
> crbug.com/395398, the crash was happening in RemoveTilings, can you
> confirm that
> from the logs?
>

Yep. RemoveTilings.


>
> This reland includes a fix, but I will make sure to run through the cases
> that
> were affected on my dev android devices when I'm in the office.
>
> Description:
> (Reland) cc: Remove recycled tilings when active tree removes them.
>
> This patch is the second part of ensuring that recycle tree does not
> contain any unshared tiles.
>
> This is done by removing recycle tree tilings when active tree removes
> those tilings.
>
> This includes a check in RemoveTilings for NULL tilings. When the first
> pending tree activates, it swaps with active tree's NULL tilings, which
> become the recycle tilings. When removing tilings, we have to ensure
> that tilings is not NULL before accessing its members.
>
> BUG=393802
> R=danakj, enne
>
> Please review this at https://codereview.chromium.org/399233004/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+27, -1 lines):
>   M cc/layers/picture_layer_impl.cc
>   M cc/trees/layer_tree_host_impl.h
>   M cc/trees/layer_tree_impl.h
>   M cc/trees/layer_tree_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 4a7e0c0b0b66ae5bdf20743b65c0f51c69824c0d..
> 7007e75f0cabc9921fc2276c2c79dc7d2d874f4c 100644
> --- a/cc/layers/picture_layer_impl.cc
> +++ b/cc/layers/picture_layer_impl.cc
> @@ -936,6 +936,9 @@ PictureLayerTiling* PictureLayerImpl::AddTiling(float
> contents_scale) {
>  }
>
>  void PictureLayerImpl::RemoveTiling(float contents_scale) {
> +  if (!tilings_ || tilings_->num_tilings() == 0)
> +    return;
> +
>    for (size_t i = 0; i < tilings_->num_tilings(); ++i) {
>      PictureLayerTiling* tiling = tilings_->tiling_at(i);
>      if (tiling->contents_scale() == contents_scale) {
> @@ -1195,18 +1198,28 @@ void PictureLayerImpl::
> CleanUpTilingsOnActiveLayer(
>      to_remove.push_back(tiling);
>    }
>
> +  if (to_remove.empty())
> +    return;
> +
> +  PictureLayerImpl* recycled_twin = static_cast<PictureLayerImpl*>(
> +      layer_tree_impl()->FindRecycleTreeLayerById(id()));
> +  // Remove tilings on this tree and the twin tree.
>    for (size_t i = 0; i < to_remove.size(); ++i) {
>      const PictureLayerTiling* twin_tiling = GetTwinTiling(to_remove[i]);
>      // Only remove tilings from the twin layer if they have
>      // NON_IDEAL_RESOLUTION.
>      if (twin_tiling && twin_tiling->resolution() == NON_IDEAL_RESOLUTION)
>        twin->RemoveTiling(to_remove[i]->contents_scale());
> +    // Remove the tiling from the recycle tree. Note that we ignore
> resolution,
> +    // since we don't need to maintain high/low res on the recycle tree.
> +    if (recycled_twin)
> +      recycled_twin->RemoveTiling(to_remove[i]->contents_scale());
>      // TODO(enne): temporary sanity CHECK for http://crbug.com/358350
>      CHECK_NE(HIGH_RESOLUTION, to_remove[i]->resolution());
>      tilings_->Remove(to_remove[i]);
>    }
> -  DCHECK_GT(tilings_->num_tilings(), 0u);
>
> +  DCHECK_GT(tilings_->num_tilings(), 0u);
>    SanityCheckTilingState();
>  }
>
> @@ -1255,6 +1268,10 @@ bool PictureLayerImpl::CanHaveTilingWithScale(float
> contents_scale) const {
>
>  void PictureLayerImpl::SanityCheckTilingState() const {
>  #if DCHECK_IS_ON
> +  // Recycle tree doesn't have any restrictions.
> +  if (layer_tree_impl()->IsRecycleTree())
> +    return;
> +
>    if (!CanHaveTilings()) {
>      DCHECK_EQ(0u, tilings_->num_tilings());
>      return;
> Index: cc/trees/layer_tree_host_impl.h
> diff --git a/cc/trees/layer_tree_host_impl.h b/cc/trees/layer_tree_host_
> impl.h
> index d6dd2c5e903974a5f38a2f65f751802239361eee..
> 496bd1e419a31a9b750603792c770a532095786a 100644
> --- a/cc/trees/layer_tree_host_impl.h
> +++ b/cc/trees/layer_tree_host_impl.h
> @@ -296,6 +296,7 @@ class CC_EXPORT LayerTreeHostImpl
>    const LayerTreeImpl* active_tree() const { return active_tree_.get(); }
>    LayerTreeImpl* pending_tree() { return pending_tree_.get(); }
>    const LayerTreeImpl* pending_tree() const { return pending_tree_.get();
> }
> +  LayerTreeImpl* recycle_tree() { return recycle_tree_.get(); }
>    const LayerTreeImpl* recycle_tree() const { return recycle_tree_.get();
> }
>    // Returns the tree LTH synchronizes with.
>    LayerTreeImpl* sync_tree() {
> Index: cc/trees/layer_tree_impl.cc
> diff --git a/cc/trees/layer_tree_impl.cc b/cc/trees/layer_tree_impl.cc
> index 392e80b658b041d71e09d53b63a164c90917945e..
> ee936022a04c17f507b8968a84c7ca39dbfcd65b 100644
> --- a/cc/trees/layer_tree_impl.cc
> +++ b/cc/trees/layer_tree_impl.cc
> @@ -718,6 +718,13 @@ LayerImpl* LayerTreeImpl::FindPendingTreeLayerById(int
> id) {
>    return tree->LayerById(id);
>  }
>
> +LayerImpl* LayerTreeImpl::FindRecycleTreeLayerById(int id) {
> +  LayerTreeImpl* tree = layer_tree_host_impl_->recycle_tree();
> +  if (!tree)
> +    return NULL;
> +  return tree->LayerById(id);
> +}
> +
>  int LayerTreeImpl::MaxTextureSize() const {
>    return layer_tree_host_impl_->GetRendererCapabilities().max_
> texture_size;
>  }
> Index: cc/trees/layer_tree_impl.h
> diff --git a/cc/trees/layer_tree_impl.h b/cc/trees/layer_tree_impl.h
> index 48c6f4732d6cbd83bb327a1107a31baf7146aa6d..
> 1f72a9395dd09fd21b3598ac42bdc1414b684060 100644
> --- a/cc/trees/layer_tree_impl.h
> +++ b/cc/trees/layer_tree_impl.h
> @@ -81,6 +81,7 @@ class CC_EXPORT LayerTreeImpl {
>    bool IsRecycleTree() const;
>    LayerImpl* FindActiveTreeLayerById(int id);
>    LayerImpl* FindPendingTreeLayerById(int id);
> +  LayerImpl* FindRecycleTreeLayerById(int id);
>    int MaxTextureSize() const;
>    bool PinchGestureActive() const;
>    base::TimeTicks CurrentFrameTimeTicks() const;
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmpstr, 2014-07-23 19:34:44]

Ok, I finally confirmed that I can repro a crash without my fix and it goes away
with the fix.

[vmpstr, 2014-07-23 19:34:48]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-07-23 19:36:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vmpstr@chromium.org/399233004/1

[commit-bot: I haz the power, 2014-07-23 22:19:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-24 02:23:49]

Change committed as 285114