cc : Make LayerImpl destruction independent of tree hierarchy

cc : Make LayerImpl destruction independent of tree hierarchy

Instead of deleting all layers when the root is deleted, we delete
all layers when the layer tree impl is shutdown.

BUG=617791
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e536d2292fa548ee6231ff23ac65203c261e4a14
Cr-Commit-Position: refs/heads/master@{#398606}

[jaydasika, 2016-06-07 22:25:37]

Description was changed from

==========
cc : Make LayerImpl desctruction independent of tree hierarchy

Instead of deleting all layers when teh root si deleted, we delete
all layers when the layer tree impl is shutdown.

BUG=617791
==========

to

==========
cc : Make LayerImpl desctruction independent of tree hierarchy

Instead of deleting all layers when teh root si deleted, we delete
all layers when the layer tree impl is shutdown.

BUG=617791
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-06-07 22:28:22]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, weiliangc@chromium.org

[jaydasika, 2016-06-07 22:28:23]

PTAL

[jaydasika, 2016-06-07 22:28:48]

Description was changed from

==========
cc : Make LayerImpl desctruction independent of tree hierarchy

Instead of deleting all layers when teh root si deleted, we delete
all layers when the layer tree impl is shutdown.

BUG=617791
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Make LayerImpl destruction independent of tree hierarchy

Instead of deleting all layers when the root is deleted, we delete
all layers when the layer tree impl is shutdown.

BUG=617791
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ajuma, 2016-06-07 23:25:36]

Generally looks great.

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc#new...
cc/layers/layer_impl.cc:110: layer_tree_impl_->RemoveLayer(replica_layer_id_);
Since masks and replicas are part of layers_ in LayerTreeImpl, will they already
have been destroyed at the same time that children got destroyed? That is, if
you were to DCHECK that the return value of the calls to RemoveLayer above is
always null, would that succeed?

https://codereview.chromium.org/2043963005/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2043963005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl_unittest.cc:7360:
LayerImpl::Create(host_impl_->active_tree(), 100));
I think the line above can be removed now that we're detaching layers below.

[jaydasika, 2016-06-08 00:17:37]

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc#new...
cc/layers/layer_impl.cc:110: layer_tree_impl_->RemoveLayer(replica_layer_id_);
On 2016/06/07 23:25:36, ajuma wrote:
> Since masks and replicas are part of layers_ in LayerTreeImpl, will they
already
> have been destroyed at the same time that children got destroyed? That is, if
> you were to DCHECK that the return value of the calls to RemoveLayer above is
> always null, would that succeed?

I din't understand that fully. masks and replicas are in layers_ but not in
children_. But as you said, I think we need not explicitly delete them as they
are part of layers_. But I don't think they are sure to be deleted before the
layer itself. If might be true now because of the order in which we add layers
to layers_ but may not be true when we push layer list.

[jaydasika, 2016-06-08 00:49:32]

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc#new...
cc/layers/layer_impl.cc:110: layer_tree_impl_->RemoveLayer(replica_layer_id_);
On 2016/06/08 00:17:37, jaydasika wrote:
> On 2016/06/07 23:25:36, ajuma wrote:
> > Since masks and replicas are part of layers_ in LayerTreeImpl, will they
> already
> > have been destroyed at the same time that children got destroyed? That is,
if
> > you were to DCHECK that the return value of the calls to RemoveLayer above
is
> > always null, would that succeed?
> 
> I din't understand that fully. masks and replicas are in layers_ but not in
> children_. But as you said, I think we need not explicitly delete them as they
> are part of layers_. But I don't think they are sure to be deleted before the
> layer itself. If might be true now because of the order in which we add layers
> to layers_ but may not be true when we push layer list.

Scratch my previous comment, I understand what you mean.

https://codereview.chromium.org/2043963005/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2043963005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl_unittest.cc:7360:
LayerImpl::Create(host_impl_->active_tree(), 100));
On 2016/06/07 23:25:36, ajuma wrote:
> I think the line above can be removed now that we're detaching layers below.

Done.

[ajuma, 2016-06-08 13:48:03]

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc#new...
cc/layers/layer_impl.cc:110: layer_tree_impl_->RemoveLayer(replica_layer_id_);
On 2016/06/08 00:17:37, jaydasika wrote:
> On 2016/06/07 23:25:36, ajuma wrote:
> > Since masks and replicas are part of layers_ in LayerTreeImpl, will they
> already
> > have been destroyed at the same time that children got destroyed? That is,
if
> > you were to DCHECK that the return value of the calls to RemoveLayer above
is
> > always null, would that succeed?
> 
> I din't understand that fully. masks and replicas are in layers_ but not in
> children_. But as you said, I think we need not explicitly delete them as they
> are part of layers_. But I don't think they are sure to be deleted before the
> layer itself. If might be true now because of the order in which we add layers
> to layers_ but may not be true when we push layer list.

That's a good point about destruction order -- even if masks and replicas are in
layers_, there's no guarantee about the order in which things inside layers_
will be destroyed when layers_ gets cleared or destroyed. That is, we don't need
to delete the masks and replicas explicitly here, but we can't be sure they'll
be deleted before the layer itself. So the DCHECKs should be removed then.

[jaydasika, 2016-06-08 16:29:30]

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc#new...
cc/layers/layer_impl.cc:110: layer_tree_impl_->RemoveLayer(replica_layer_id_);
On 2016/06/08 13:48:02, ajuma wrote:
> On 2016/06/08 00:17:37, jaydasika wrote:
> > On 2016/06/07 23:25:36, ajuma wrote:
> > > Since masks and replicas are part of layers_ in LayerTreeImpl, will they
> > already
> > > have been destroyed at the same time that children got destroyed? That is,
> if
> > > you were to DCHECK that the return value of the calls to RemoveLayer above
> is
> > > always null, would that succeed?
> > 
> > I din't understand that fully. masks and replicas are in layers_ but not in
> > children_. But as you said, I think we need not explicitly delete them as
they
> > are part of layers_. But I don't think they are sure to be deleted before
the
> > layer itself. If might be true now because of the order in which we add
layers
> > to layers_ but may not be true when we push layer list.
> 
> That's a good point about destruction order -- even if masks and replicas are
in
> layers_, there's no guarantee about the order in which things inside layers_
> will be destroyed when layers_ gets cleared or destroyed. That is, we don't
need
> to delete the masks and replicas explicitly here, but we can't be sure they'll
> be deleted before the layer itself. So the DCHECKs should be removed then.

I agree that there is no guarantee about destruction order, but I think it is
fine to have these DCHECKs because the destruction of layers can start only
after all the layers that are to be destroyed are removed from layers_. This is
because of the way LTI::DetachLayers is written. The function itself doesn't
lead to destructor being called. It removes all layers from layers_ and
transfers their ownership to a new vector that it returns. Layer destruction can
start after this new vector goes out of scope (so we can be sure all layers are
removed from layers_ at that point).

[ajuma, 2016-06-08 16:40:46]

lgtm

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2043963005/diff/1/cc/layers/layer_impl.cc#new...
cc/layers/layer_impl.cc:110: layer_tree_impl_->RemoveLayer(replica_layer_id_);
On 2016/06/08 16:29:30, jaydasika wrote:
> On 2016/06/08 13:48:02, ajuma wrote:
> > On 2016/06/08 00:17:37, jaydasika wrote:
> > > On 2016/06/07 23:25:36, ajuma wrote:
> > > > Since masks and replicas are part of layers_ in LayerTreeImpl, will they
> > > already
> > > > have been destroyed at the same time that children got destroyed? That
is,
> > if
> > > > you were to DCHECK that the return value of the calls to RemoveLayer
above
> > is
> > > > always null, would that succeed?
> > > 
> > > I din't understand that fully. masks and replicas are in layers_ but not
in
> > > children_. But as you said, I think we need not explicitly delete them as
> they
> > > are part of layers_. But I don't think they are sure to be deleted before
> the
> > > layer itself. If might be true now because of the order in which we add
> layers
> > > to layers_ but may not be true when we push layer list.
> > 
> > That's a good point about destruction order -- even if masks and replicas
are
> in
> > layers_, there's no guarantee about the order in which things inside layers_
> > will be destroyed when layers_ gets cleared or destroyed. That is, we don't
> need
> > to delete the masks and replicas explicitly here, but we can't be sure
they'll
> > be deleted before the layer itself. So the DCHECKs should be removed then.
> 
> I agree that there is no guarantee about destruction order, but I think it is
> fine to have these DCHECKs because the destruction of layers can start only
> after all the layers that are to be destroyed are removed from layers_. This
is
> because of the way LTI::DetachLayers is written. The function itself doesn't
> lead to destructor being called. It removes all layers from layers_ and
> transfers their ownership to a new vector that it returns. Layer destruction
can
> start after this new vector goes out of scope (so we can be sure all layers
are
> removed from layers_ at that point).

Ah, got it, that makes sense.

[ajuma, 2016-06-08 16:40:52]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2016-06-08 16:41:19]

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

[commit-bot: I haz the power, 2016-06-08 17:22:33]

Description was changed from

==========
cc : Make LayerImpl destruction independent of tree hierarchy

Instead of deleting all layers when the root is deleted, we delete
all layers when the layer tree impl is shutdown.

BUG=617791
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Make LayerImpl destruction independent of tree hierarchy

Instead of deleting all layers when the root is deleted, we delete
all layers when the layer tree impl is shutdown.

BUG=617791
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-08 17:22:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-08 17:23:49]

Description was changed from

==========
cc : Make LayerImpl destruction independent of tree hierarchy

Instead of deleting all layers when the root is deleted, we delete
all layers when the layer tree impl is shutdown.

BUG=617791
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Make LayerImpl destruction independent of tree hierarchy

Instead of deleting all layers when the root is deleted, we delete
all layers when the layer tree impl is shutdown.

BUG=617791
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e536d2292fa548ee6231ff23ac65203c261e4a14
Cr-Commit-Position: refs/heads/master@{#398606}
==========

[commit-bot: I haz the power, 2016-06-08 17:23:50]

Patchset 2 (id:??) landed as
https://crrev.com/e536d2292fa548ee6231ff23ac65203c261e4a14
Cr-Commit-Position: refs/heads/master@{#398606}

[Ken Rockot(use gerrit already), 2016-06-08 18:22:52]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2054483002/ by rockot@chromium.org.

The reason for reverting is: Tentative revert for strange linker failures:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Builder%...

I already tried reverting the other CL in the blame list (mine) with no success.
Doesn't make sense that this CL would cause it either, but just ruling it out.
I'll revert the revert if the builder stays red..

[Ken Rockot(use gerrit already), 2016-06-08 19:06:36]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2044253004/ by rockot@chromium.org.

The reason for reverting is: Didn't fix anything..