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

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

We currently null the twin_layer_ pointer when pushing to the active
tree and then using hashmap lookups to find the recycled twin, and
reconnecting the pointers in DoPostCommitInitializationIfNeeded().

Instead, we can leave the pointers always valid and use Getter methods
that check what tree the twin is on to decide if we should consider it
a pending twin or a recycle twin.

This means that during commit the pending layer will be able to find
its active twin when it updates its picture pile, so that it can share
tiles that are not invalidated. (Previously it wouldn't know about its
twin until after commit, when we set it up and then did SyncTilings.)
This allows us to have a more straightforward flow of data, setting up
the pending tilings when we give it a pile from the main thread
instead of doing it at some hazy future time (usually inside
UpdateDrawProperties).

R=enne, vmpstr
BUG=407418, 387116, 427213
Committed: https://crrev.com/11f13546fd3ee3d53921c23861bf970bd2bb6428
Cr-Commit-Position: refs/heads/master@{#301155}

Committed: https://crrev.com/7b08e2869cf7f6a93503ffe66998244ea66b55e7
Cr-Commit-Position: refs/heads/master@{#301432}

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

https://codereview.chromium.org/676953003/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/676953003/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:118: twin_layer_ = layer_impl;
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?

https://codereview.chromium.org/676953003/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:648: const PictureLayerTiling*
PictureLayerImpl::GetTwinTiling(
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.

https://codereview.chromium.org/676953003/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/676953003/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:4382:
host_impl_.pending_tree()->DetachLayerTree();
I'm not sure this tests the same thing since we now have a pending tree.
However, the previous test was also impossible since the active tree structure
should always match the recycle tree structure... That's just a hard thing to
enforce, I think.

https://codereview.chromium.org/676953003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/676953003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:7447: EXPECT_EQ(2u,
layer_pairs.size());
Can you add another test here that tests active layer and pending layer that are
not related, so that we get 2 layer pairs one has active one has pending?

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

https://codereview.chromium.org/676953003/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/676953003/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:118: twin_layer_ = layer_impl;
On 2014/10/24 16:47:05, vmpstr wrote:
> 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?

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

https://codereview.chromium.org/676953003/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:648: const PictureLayerTiling*
PictureLayerImpl::GetTwinTiling(
On 2014/10/24 16:47:05, vmpstr wrote:
> 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.

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

https://codereview.chromium.org/676953003/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/676953003/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:4382:
host_impl_.pending_tree()->DetachLayerTree();
On 2014/10/24 16:47:05, vmpstr wrote:
> I'm not sure this tests the same thing since we now have a pending tree.
> However, the previous test was also impossible since the active tree structure
> should always match the recycle tree structure... That's just a hard thing to
> enforce, I think.

I think the only way the layer can be deleted from the recycle tree is by making
a pending tree without it. I'm not sure what other scenario would be good to
test here, do you have any in mind?

https://codereview.chromium.org/676953003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/676953003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:7447: EXPECT_EQ(2u,
layer_pairs.size());
On 2014/10/24 16:47:05, vmpstr wrote:
> Can you add another test here that tests active layer and pending layer that
are
> not related, so that we get 2 layer pairs one has active one has pending?

Yep!

[danakj, 2014-10-24 17:06:54]

Test added PTAL

[vmpstr, 2014-10-24 17:20:10]

lgtm, thanks.

https://codereview.chromium.org/676953003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/676953003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:7498: PictureLayerImpl::Pair p =
layer_pairs[1];
std::swap!

[danakj, 2014-10-24 18:00:08]

The CQ bit was checked by danakj@chromium.org

[danakj, 2014-10-24 18:00:25]

https://codereview.chromium.org/676953003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/676953003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:7498: PictureLayerImpl::Pair p =
layer_pairs[1];
On 2014/10/24 17:20:10, vmpstr wrote:
> std::swap!

ooh

[commit-bot: I haz the power, 2014-10-24 18:01:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676953003/80001

[enne (OOO), 2014-10-24 18:39:06]

lgtm! This is a really nice cleanup.

[commit-bot: I haz the power, 2014-10-24 18:50:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-24 18:51:40]

Patchset 5 (id:??) landed as
https://crrev.com/11f13546fd3ee3d53921c23861bf970bd2bb6428
Cr-Commit-Position: refs/heads/master@{#301155}

[pdr., 2014-10-25 22:02:47]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/680793002/ by pdr@chromium.org.

The reason for reverting is: I suspect this patch is causing canary crashers
(see http://crbug.com/427215).

[danakj, 2014-10-27 14:51:42]

I think the crash is that now you can have a twin immediately after commit, but
the twin has no tilings, since we swap the active null tiling set over to the
recycle tree on the first activate. And we don't set the tilings_ to non-null
until later on the pending tree.

I've added a nullcheck for the twin's tilings_ that can go away once we stop
swapping tiling sets.

PTAL

[danakj, 2014-10-27 14:54:42]

Oh, studying the crash stack a bit more I think what I said is wrong but the fix
is still right.

I think we are rastering a layer with an active twin which was not in the RSLL
last frame. So it never did UpdateTiles, and DoPostCommit stuff and made a
tilings_. Then when rastering we deref the active twin's null tilings_.

The same check will be right, until we stop swapping tilings, at which point I
am also making the tilings_ object in the PLI constructor.

[danakj, 2014-10-27 15:47:43]

On 2014/10/27 14:54:42, danakj wrote:
> Oh, studying the crash stack a bit more I think what I said is wrong but the
fix
> is still right.
> 
> I think we are rastering a layer with an active twin which was not in the RSLL
> last frame. So it never did UpdateTiles, and DoPostCommit stuff and made a
> tilings_. Then when rastering we deref the active twin's null tilings_.
> 
> The same check will be right, until we stop swapping tilings, at which point I
> am also making the tilings_ object in the PLI constructor.

Hm, trying to make a test to repro this and failing. When we push to active we
always DoPostCommit. So I'm failing to see how the active tree could have a null
tilings_, but it must be somehow.

[danakj, 2014-10-27 16:08:16]

On 2014/10/27 15:47:43, danakj wrote:
> On 2014/10/27 14:54:42, danakj wrote:
> > Oh, studying the crash stack a bit more I think what I said is wrong but the
> fix
> > is still right.
> > 
> > I think we are rastering a layer with an active twin which was not in the
RSLL
> > last frame. So it never did UpdateTiles, and DoPostCommit stuff and made a
> > tilings_. Then when rastering we deref the active twin's null tilings_.
> > 
> > The same check will be right, until we stop swapping tilings, at which point
I
> > am also making the tilings_ object in the PLI constructor.
> 
> Hm, trying to make a test to repro this and failing. When we push to active we
> always DoPostCommit. So I'm failing to see how the active tree could have a
null
> tilings_, but it must be somehow.

OK I think I get it now.

1. Make a pending layer with content.
2. Activate.
3. Commit a pending layer that does not go in RSLL.
4. Finish commit
5. We UpdateDrawProps on the pending tree, but this will skip UpdateTiles for
the layer.
6. We ManageTiles.
7. If you require raster on the active layer for any reason, then the raster
iterator may try to get the twin tiling, which is what the crash is, since the
twin layer has a null tilings on the 2nd frame. (On the 3rd+ frame it won't be
null anymore.)

[danakj, 2014-10-27 18:13:56]

The CQ bit was checked by danakj@chromium.org

[danakj, 2014-10-27 18:13:59]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-10-27 18:14:13]

Patchset #8 (id:140001) has been deleted

[danakj, 2014-10-27 18:18:47]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-10-27 18:19:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676953003/160001

[danakj, 2014-10-27 18:23:22]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-10-27 18:23:24]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-10-27 18:23:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676953003/160001

[danakj, 2014-10-27 19:15:03]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-10-27 19:23:50]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-10-27 19:24:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676953003/180001

[danakj, 2014-10-27 19:26:32]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-10-27 19:31:30]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-10-27 19:33:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676953003/200001

[commit-bot: I haz the power, 2014-10-27 20:30:09]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2014-10-27 20:30:46]

Patchset 10 (id:??) landed as
https://crrev.com/7b08e2869cf7f6a93503ffe66998244ea66b55e7
Cr-Commit-Position: refs/heads/master@{#301432}