cc: Remove tilings from recycle tree when active tree removes them.

cc: Remove tilings from recycle tree 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.

BUG=393802
R=danakj

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

[vmpstr, 2014-07-17 21:41:00]

PTAL

[danakj, 2014-07-17 21:54:48]

https://codereview.chromium.org/400633004/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/400633004/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1224:
recycled_twin->RemoveTiling(to_remove[i]->contents_scale());
We might remove the high res tiling on the recycle tree and then that's going to
DCHECK in SanityCheck.

[vmpstr, 2014-07-17 21:59:25]

On 2014/07/17 21:54:48, danakj wrote:
>
https://codereview.chromium.org/400633004/diff/1/cc/layers/picture_layer_impl.cc
> File cc/layers/picture_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/400633004/diff/1/cc/layers/picture_layer_impl...
> cc/layers/picture_layer_impl.cc:1224:
> recycled_twin->RemoveTiling(to_remove[i]->contents_scale());
> We might remove the high res tiling on the recycle tree and then that's going
to
> DCHECK in SanityCheck.

Hmm... but the removal is correct, right? I can add something along the lines of

if (!IsOnActiveOrPendingTree())
 return;

in SanityCheckTilingState. or would that be bad?

[danakj, 2014-07-17 22:00:15]

On 2014/07/17 21:59:25, vmpstr wrote:
> On 2014/07/17 21:54:48, danakj wrote:
> >
>
https://codereview.chromium.org/400633004/diff/1/cc/layers/picture_layer_impl.cc
> > File cc/layers/picture_layer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/400633004/diff/1/cc/layers/picture_layer_impl...
> > cc/layers/picture_layer_impl.cc:1224:
> > recycled_twin->RemoveTiling(to_remove[i]->contents_scale());
> > We might remove the high res tiling on the recycle tree and then that's
going
> to
> > DCHECK in SanityCheck.
> 
> Hmm... but the removal is correct, right? I can add something along the lines
of
> 
> if (!IsOnActiveOrPendingTree())
>  return;
> 
> in SanityCheckTilingState. or would that be bad?

That's fine, it doesn't need to be sane.

[vmpstr, 2014-07-17 22:16:36]

PTAL

[danakj, 2014-07-17 22:22:49]

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

https://codereview.chromium.org/400633004/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1230:
recycled_twin->RemoveTiling(to_remove[i]->contents_scale());
er, wait, to_remove[i] is a use-after-free here, cuz we Remove()'d it above

[vmpstr, 2014-07-17 22:30:59]

PTAL

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

https://codereview.chromium.org/400633004/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1230:
recycled_twin->RemoveTiling(to_remove[i]->contents_scale());
On 2014/07/17 22:22:49, danakj wrote:
> er, wait, to_remove[i] is a use-after-free here, cuz we Remove()'d it above

Hmm.. that's not good. Good thing my testing worked just fine? I've changed it
around a bit. Good catch.

Should we also change for (i from 0 to to_remove.size()) to be more like

while (!to_remove.empty()) { tiling = to_remove.back() .. to_remove.pop_back() }

to avoid people like me messing this up in the future?

[danakj, 2014-07-18 18:34:52]

On 2014/07/17 22:30:59, vmpstr wrote:
> PTAL
> 
>
https://codereview.chromium.org/400633004/diff/20001/cc/layers/picture_layer_...
> File cc/layers/picture_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/400633004/diff/20001/cc/layers/picture_layer_...
> cc/layers/picture_layer_impl.cc:1230:
> recycled_twin->RemoveTiling(to_remove[i]->contents_scale());
> On 2014/07/17 22:22:49, danakj wrote:
> > er, wait, to_remove[i] is a use-after-free here, cuz we Remove()'d it above
> 
> Hmm.. that's not good. Good thing my testing worked just fine? I've changed it
> around a bit. Good catch.
> 
> Should we also change for (i from 0 to to_remove.size()) to be more like
> 
> while (!to_remove.empty()) { tiling = to_remove.back() .. to_remove.pop_back()
}
> 
> to avoid people like me messing this up in the future?

Um, idk, you can if you want, its fine how it is too I guess? I don't feel
strongly either way.

LGTM

[danakj, 2014-07-18 18:34:58]

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

https://codereview.chromium.org/400633004/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1216: // Remove the tiling from the recycle
tree.
nit: mention we're ignoring it resolution because we don't need to maintain
high/low res on the recycle tree

[vmpstr, 2014-07-18 23:21:12]

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

https://codereview.chromium.org/400633004/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1216: // Remove the tiling from the recycle
tree.
On 2014/07/18 18:34:58, danakj wrote:
> nit: mention we're ignoring it resolution because we don't need to maintain
> high/low res on the recycle tree

Done.

[vmpstr, 2014-07-18 23:21:17]

The CQ bit was checked by vmpstr@chromium.org

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

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

[commit-bot: I haz the power, 2014-07-19 02:37:01]

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-19 04:51:35]

Change committed as 284311

[tonyg, 2014-07-19 20:28:44]

A revert of this CL has been created in
https://codereview.chromium.org/407763002/ by tonyg@chromium.org.

The reason for reverting is: Related crashes are showing up on Android bots.
Details in bug.

BUG=395398.