Android: Don't hold frame lock during readback

Android: Don't hold frame lock during readback

The problem with this is that it might keep the browser-side
tiles from ever getting freed if for some reason the copy output
request does not finish. And currently there are no such
guarantees for example if the app goes to the background and we don't
have a context and cannot composite.

Also, it would cause jank since the lock keeps from submitting new
frames from the renderer.

As far as readback requests are concerned, this would not cause
the readback to be dropped even if we destroyed the Surface since
it's garbage-collected and there should still be a GC root through
the parent Surface and the hidden layer we attach until the readback
is completed.

Remove more of this logic and public API in a follow-up.

BUG=636628
Committed: https://crrev.com/9df27b364447148d91e8afe8589ab0914fbb3688
Cr-Commit-Position: refs/heads/master@{#411523}

[no sievers, 2016-08-11 21:56:51]

Description was changed from

==========
Android: Don't hold frame lock during readback

The problem with this is that it might keep the browser-side
tiles from ever getting freed if for some reason the copy output
request does not finish. And currently there are no such
guarantees for example if the app goes to the background and we don't
have a context and cannot composite.

Also, it would cause jank since the lock keeps from submitting new
frames from the renderer.

As far as readback requests are concerned, this would not cause
the readback to be dropped even if we destroyed the Surface since
it's garbage-collected and there should still be a GC root through
the parent Surface and the hidden layer we attach until the readback
is completed.

Remove more of this logic and public API in a follow-up.

BUG=636628
==========

to

==========
Android: Don't hold frame lock during readback

The problem with this is that it might keep the browser-side
tiles from ever getting freed if for some reason the copy output
request does not finish. And currently there are no such
guarantees for example if the app goes to the background and we don't
have a context and cannot composite.

Also, it would cause jank since the lock keeps from submitting new
frames from the renderer.

As far as readback requests are concerned, this would not cause
the readback to be dropped even if we destroyed the Surface since
it's garbage-collected and there should still be a GC root through
the parent Surface and the hidden layer we attach until the readback
is completed.

Remove more of this logic and public API in a follow-up.

BUG=636628
==========

[no sievers, 2016-08-11 21:56:51]

sievers@chromium.org changed reviewers:
+ jbauman@chromium.org

[no sievers, 2016-08-11 21:57:39]

jbauman@, ptal. Is it correct what I say in the description about
Surface/copy_request lifetime (also see patch dependency)?

[no sievers, 2016-08-11 22:25:13]

The CQ bit was checked by sievers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 22:26:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2016-08-11 22:48:36]

On 2016/08/11 21:57:39, sievers wrote:
> jbauman@, ptal. Is it correct what I say in the description about
> Surface/copy_request lifetime (also see patch dependency)?

lgtm, I think you're right that having a layer referencing it should keep it
alive. I guess this is the last use of the frame lock?

[no sievers, 2016-08-11 22:56:15]

On 2016/08/11 22:48:36, jbauman wrote:
> On 2016/08/11 21:57:39, sievers wrote:
> > jbauman@, ptal. Is it correct what I say in the description about
> > Surface/copy_request lifetime (also see patch dependency)?
> 
> lgtm, I think you're right that having a layer referencing it should keep it
> alive. I guess this is the last use of the frame lock?

Yes, I'm going to remove it as a follow-up. In particular from the public
content api.

We added it as a mechanism to allow the app to do a readback and compress the UI
resource down and make sure the tiles are still there until we have a static
replacement layer. But we never acquired the lock correctly, and now it just
scares me with the multiple call sites that exist now for layer eviction and
don't even respect the lock (and imagine refcounting bugs).

And then I rather come up with a better way to keep the layer in place until we
have a placeholder where the logic is more internal to the content layer /
RWHVA.

[commit-bot: I haz the power, 2016-08-11 23:34:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 23:34:28]

Dry run: This issue passed the CQ dry run.

[no sievers, 2016-08-12 00:31:16]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2016-08-12 00:32:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 00:32:42]

This CL has an open dependency (Issue 2243663002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[no sievers, 2016-08-12 02:17:31]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2016-08-12 02:18:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 02:22:03]

Description was changed from

==========
Android: Don't hold frame lock during readback

The problem with this is that it might keep the browser-side
tiles from ever getting freed if for some reason the copy output
request does not finish. And currently there are no such
guarantees for example if the app goes to the background and we don't
have a context and cannot composite.

Also, it would cause jank since the lock keeps from submitting new
frames from the renderer.

As far as readback requests are concerned, this would not cause
the readback to be dropped even if we destroyed the Surface since
it's garbage-collected and there should still be a GC root through
the parent Surface and the hidden layer we attach until the readback
is completed.

Remove more of this logic and public API in a follow-up.

BUG=636628
==========

to

==========
Android: Don't hold frame lock during readback

The problem with this is that it might keep the browser-side
tiles from ever getting freed if for some reason the copy output
request does not finish. And currently there are no such
guarantees for example if the app goes to the background and we don't
have a context and cannot composite.

Also, it would cause jank since the lock keeps from submitting new
frames from the renderer.

As far as readback requests are concerned, this would not cause
the readback to be dropped even if we destroyed the Surface since
it's garbage-collected and there should still be a GC root through
the parent Surface and the hidden layer we attach until the readback
is completed.

Remove more of this logic and public API in a follow-up.

BUG=636628
==========

[commit-bot: I haz the power, 2016-08-12 02:22:04]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-12 02:23:17]

Description was changed from

==========
Android: Don't hold frame lock during readback

The problem with this is that it might keep the browser-side
tiles from ever getting freed if for some reason the copy output
request does not finish. And currently there are no such
guarantees for example if the app goes to the background and we don't
have a context and cannot composite.

Also, it would cause jank since the lock keeps from submitting new
frames from the renderer.

As far as readback requests are concerned, this would not cause
the readback to be dropped even if we destroyed the Surface since
it's garbage-collected and there should still be a GC root through
the parent Surface and the hidden layer we attach until the readback
is completed.

Remove more of this logic and public API in a follow-up.

BUG=636628
==========

to

==========
Android: Don't hold frame lock during readback

The problem with this is that it might keep the browser-side
tiles from ever getting freed if for some reason the copy output
request does not finish. And currently there are no such
guarantees for example if the app goes to the background and we don't
have a context and cannot composite.

Also, it would cause jank since the lock keeps from submitting new
frames from the renderer.

As far as readback requests are concerned, this would not cause
the readback to be dropped even if we destroyed the Surface since
it's garbage-collected and there should still be a GC root through
the parent Surface and the hidden layer we attach until the readback
is completed.

Remove more of this logic and public API in a follow-up.

BUG=636628

Committed: https://crrev.com/9df27b364447148d91e8afe8589ab0914fbb3688
Cr-Commit-Position: refs/heads/master@{#411523}
==========

[commit-bot: I haz the power, 2016-08-12 02:23:18]

Patchset 1 (id:??) landed as
https://crrev.com/9df27b364447148d91e8afe8589ab0914fbb3688
Cr-Commit-Position: refs/heads/master@{#411523}