Fix crashes when NinePatchLayer doesn't have a resource.

Fix crashes when NinePatchLayer doesn't have a resource.

First issue is that if NinePatchLayer doesn't get a ResourceId because
of memory pressure, it will DCHECK. This is a valid situation -- just
draw nothing (checkerboard would be okay as well).

Second issue is that NinePatchLayer passes the ResourceId in for the
texture in PushPropertiesTo, but does not call SetNeedsPushProperties
when painting a new resource. The documentation of Layer::Update
suggests that a side-effect of returning true from Layer::Update is
that SetNeedsCommit will be called ont this layer, but this is not
the case.

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

[ccameron, 2013-08-20 02:02:01]

LMK if this makes sense to you -- if it does, I can add tests (and update the
comment about SetNeedsCommit) in layer.h.

[danakj, 2013-08-20 02:09:40]

On Mon, Aug 19, 2013 at 10:02 PM, <ccameron@chromium.org> wrote:

> Reviewers: danakj,
>
> Message:
> LMK if this makes sense to you -- if it does, I can add tests (and update
> the
> comment about SetNeedsCommit) in layer.h.
>
> Description:
> Fix crashes when NinePatchLayer doesn't have a resource.
>
> First issue is that if NinePatchLayer doesn't get a ResourceId because
> of memory pressure, it will DCHECK. This is a valid situation -- just
> draw nothing (checkerboard would be okay as well).
>
> Second issue is that NinePatchLayer passes the ResourceId in for the
> texture in PushPropertiesTo, but does not call SetNeedsPushProperties
> when painting a new resource. The documentation of Layer::Update
> suggests that a side-effect of returning true from Layer::Update is
> that SetNeedsCommit will be called ont this layer, but this is not
> the case.
>

Rather, if Update() returns true, then the commit is not aborted, and the
push properties phase will happen at all. (It will also happen if any layer
had previously SetNeedsPushProperties before the commit/BeginFrame
started.) However, properties are only pushed for layers that have
previously had SetNeedsCommit() or SetNeedsPushProperties() called for the
layer.

This layer's logic is kinda complicated! The SetNeedsUpdate() will cause a
commit to start and request the layer to push properties, but it will only
cause properties to be pushed if the needs-update had a non-empty rect!
Meaning if the layer has zero bounds then it does nothing.

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

[danakj, 2013-08-20 02:22:32]

https://codereview.chromium.org/22911024/diff/1/cc/layers/nine_patch_layer.cc
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22911024/diff/1/cc/layers/nine_patch_layer.cc...
cc/layers/nine_patch_layer.cc:74: SetNeedsPushProperties();
Oh, this uses prioritized texture manager so it can't tell if it needs to push
properties until we're inside the BeginFrame :(

For tiled layer, we just push properties every frame:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/tiled_la...

We could do that here too, and file a bug to fix this with the UIResourceManager
(similar for scrollbar layers).

In fact, this value of updated seems wrong in the case that
NinePatchLayer::SetTexturePriorities() decides it can't support the texture
anymore and drops the resource_. The commit would be aborted in that case, even
though the resource id will no longer be valid. +enne

We should fix updated to be true in the right cases (ie also when the resource
was present last frame but dropped this frame). Then I think we could SNPP()
here if updated was true, rather than just setting it true all the time like
scrollbar/tiled layer, if you like.

https://codereview.chromium.org/22911024/diff/1/cc/layers/nine_patch_layer_im...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/22911024/diff/1/cc/layers/nine_patch_layer_im...
cc/layers/nine_patch_layer_impl.cc:67: if (!resource_id_)
This looks right to me, but can you move this below the debug border quad?

[aelias_OOO_until_Jul13, 2013-08-20 02:42:42]

Note that powei@ is working on moving this layer type over to UIResource at the
moment anyway so this kind of bug will be moot soon.

[danakj, 2013-08-20 05:18:48]

Then maybe always setting needs push props with an attached crbug (like
scrollbars) is the way to go here. The updated vale being wrong is
something else and I worry applies to the scrollbar and tile layers also.
On Aug 19, 2013 10:42 PM, <aelias@chromium.org> wrote:

> Note that powei@ is working on moving this layer type over to UIResource
> at the
> moment anyway so this kind of bug will be moot soon.
>
>
https://codereview.chromium.**org/22911024/<https://codereview.chromium.org/2...
>

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

[ccameron, 2013-08-20 09:43:47]

Thanks!! I've uploaded a version with an unconditional check.

WRT UIResources, last I checked the infrastructure in place, they were not freed
when a tab is backgrounded -- we'll be wanting them to be freed when
backgrounded so that video memory consumption doesn't grow with the number of
tabs present.

Also, is there a bug on the planned UIResource+nine-patch work?

https://codereview.chromium.org/22911024/diff/1/cc/layers/nine_patch_layer.cc
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22911024/diff/1/cc/layers/nine_patch_layer.cc...
cc/layers/nine_patch_layer.cc:74: SetNeedsPushProperties();
On 2013/08/20 02:22:32, danakj wrote:
> Oh, this uses prioritized texture manager so it can't tell if it needs to push
> properties until we're inside the BeginFrame :(
> 
> For tiled layer, we just push properties every frame:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/tiled_la...
> 
> We could do that here too, and file a bug to fix this with the
UIResourceManager
> (similar for scrollbar layers).
> 
> In fact, this value of updated seems wrong in the case that
> NinePatchLayer::SetTexturePriorities() decides it can't support the texture
> anymore and drops the resource_. The commit would be aborted in that case,
even
> though the resource id will no longer be valid. +enne
> 
> We should fix updated to be true in the right cases (ie also when the resource
> was present last frame but dropped this frame). Then I think we could SNPP()
> here if updated was true, rather than just setting it true all the time like
> scrollbar/tiled layer, if you like.

I've set it to be SNPP unconditionally for now.

https://codereview.chromium.org/22911024/diff/1/cc/layers/nine_patch_layer_im...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/22911024/diff/1/cc/layers/nine_patch_layer_im...
cc/layers/nine_patch_layer_impl.cc:67: if (!resource_id_)
On 2013/08/20 02:22:32, danakj wrote:
> This looks right to me, but can you move this below the debug border quad?

Ah, good catch -- done.

[danakj, 2013-08-20 16:48:57]

https://codereview.chromium.org/22911024/diff/7001/cc/layers/nine_patch_layer.cc
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22911024/diff/7001/cc/layers/nine_patch_layer...
cc/layers/nine_patch_layer.cc:76: SetNeedsPushProperties();
Thanks, but I'd prefer if this matched the way similar layers do it.
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/scrollba...

Inside PushPropertiesTo with a comment and a crbug, if you don't mind?

[ccameron, 2013-08-20 18:14:01]

Thanks!!

https://codereview.chromium.org/22911024/diff/7001/cc/layers/nine_patch_layer.cc
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22911024/diff/7001/cc/layers/nine_patch_layer...
cc/layers/nine_patch_layer.cc:76: SetNeedsPushProperties();
On 2013/08/20 16:48:57, danakj wrote:
> Thanks, but I'd prefer if this matched the way similar layers do it.
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/scrollba...

Sure -- done! I find that a bit terrifying, but it's better to be consistent
than have a zoo of options.

> Inside PushPropertiesTo with a comment and a crbug, if you don't mind?

Filed & added (from aelias' comment it had sounded like one may exist).

[aelias_OOO_until_Jul13, 2013-08-20 18:17:12]

On 2013/08/20 18:14:01, ccameron1 wrote:
> Thanks!!
> 
>
https://codereview.chromium.org/22911024/diff/7001/cc/layers/nine_patch_layer.cc
> File cc/layers/nine_patch_layer.cc (right):
> 
>
https://codereview.chromium.org/22911024/diff/7001/cc/layers/nine_patch_layer...
> cc/layers/nine_patch_layer.cc:76: SetNeedsPushProperties();
> On 2013/08/20 16:48:57, danakj wrote:
> > Thanks, but I'd prefer if this matched the way similar layers do it.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/scrollba...
> 
> Sure -- done! I find that a bit terrifying, but it's better to be consistent
> than have a zoo of options.
> 
> > Inside PushPropertiesTo with a comment and a crbug, if you don't mind?
> 
> Filed & added (from aelias' comment it had sounded like one may exist).

[aelias_OOO_until_Jul13, 2013-08-20 18:17:41]

OK, I filed http://crbug.com/276487 about the UIResource work.

[danakj, 2013-08-20 18:18:04]

https://codereview.chromium.org/22911024/diff/12001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22911024/diff/12001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:79: needs_push_properties_ = true;
Cool, just move this to the end of PushPropertiesTo() instead of doing it in
Update(), and this LGTM.

[ccameron, 2013-08-20 18:27:48]

Ah, good catch.

Thanks!!

[ccameron, 2013-08-20 18:27:49]

Ah, good catch.

Thanks!!

[commit-bot: I haz the power, 2013-08-20 18:28:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/22911024/19001

[commit-bot: I haz the power, 2013-08-20 23:42:42]

Change committed as 218574