Android WebView: Always send memory policy from Browser to Renderer.

Android WebView: Always send memory policy from Browser to Renderer.

Early out on the Browser side does not work because the IPC message send could
succeed even if the output surface in question is not yet bound to client and
registered with the SynchronousCompositorRegistry.

BUG=613781
Committed: https://crrev.com/90144532ec5fc69209a72fdb7561704dec1c3ced
Cr-Commit-Position: refs/heads/master@{#398460}

[hush (inactive), 2016-06-02 23:46:55]

hush@chromium.org changed reviewers:
+ boliu@chromium.org

[hush (inactive), 2016-06-02 23:46:56]

PTAL.
We can't remove the member yet because we need it in OutputSurfaceCreated().

I'm not sure about the performance implications. Maybe negligible.

[boliu, 2016-06-03 01:24:48]

On 2016/06/02 23:46:56, hush wrote:
> PTAL.
> We can't remove the member yet because we need it in OutputSurfaceCreated().

Hey... I forgot about that. Why did that not work in this case?

> 
> I'm not sure about the performance implications. Maybe negligible.

[hush (inactive), 2016-06-04 00:42:24]

On 2016/06/03 01:24:48, boliu wrote:
> On 2016/06/02 23:46:56, hush wrote:
> > PTAL.
> > We can't remove the member yet because we need it in OutputSurfaceCreated().
> 
> Hey... I forgot about that. Why did that not work in this case?
> 
> > 
> > I'm not sure about the performance implications. Maybe negligible.

Like we've just debugged, the life times of outputsurface and its sync client
(that is SynchronousCompositorProxy) are independent.
We sent SyncCompositorHostMsg_OutputSurfaceCreated at BindToClient time, which
is wrong, because the sync client may not be constructed at this time. So
Browser side's set memory policy after it receives
SyncCompositorHostMsg_OutputSurfaceCreated will be dropped.

In short, we have to wait for sync client to be bound to the output surface,
before we send SyncCompositorHostMsg_OutputSurfaceCreated.


I still removed the early out in *host.cc in PS2 out of paranoia.

[boliu, 2016-06-04 00:47:01]

keep the early out on the browser side unless you can tell me how it's a problem

https://codereview.chromium.org/2035993003/diff/20001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2035993003/diff/20001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:109: Send(new
SyncCompositorHostMsg_OutputSurfaceCreated(routing_id_));
only do this if sync_client_ is not null

[hush (inactive), 2016-06-06 18:41:22]

My problem with the early out is that there are so many ways this IPC could
fail, that I am not sure how to reason about.
Possible failures in the order of events:
1. fail to post the task on io thread
2. no sender in filter
3. no proxy at all when the IPC message is sent.
4. the sender in filter fails to send the message on IO thread.
5. when message is received, it fails to post on compositor thread.
6. no outputsurface in SynchronousCompositorProxy. (that is the output surface
is not bound to the "sync client" yet)
7. no OutputSurfaceClient bound to the OutputSurface when the IPC actually
reaches SyncCompostorOutputSurface.

Some are within our control, and could be managed by us, like #6, #7. But the
cause of the rest is not very clear. It's difficult for me to simply fire and
forget and trust the memory policy is delivered to LTHI (the
OutputSurfaceClient, which is what really matters).

[boliu, 2016-06-06 18:47:15]

On 2016/06/06 18:41:22, hush wrote:
> My problem with the early out is that there are so many ways this IPC could
> fail, that I am not sure how to reason about.
> Possible failures in the order of events:
> 1. fail to post the task on io thread
> 2. no sender in filter
> 3. no proxy at all when the IPC message is sent.
> 4. the sender in filter fails to send the message on IO thread.
> 5. when message is received, it fails to post on compositor thread.
> 6. no outputsurface in SynchronousCompositorProxy. (that is the output surface
> is not bound to the "sync client" yet)
> 7. no OutputSurfaceClient bound to the OutputSurface when the IPC actually
> reaches SyncCompostorOutputSurface.

All of these are (or should be) addressed by OutputSurfaceCreated IPC.

Sending an IPC should have the same reliability as posting to another thread.
The only failure case is if either end point is not "set up" yet, so on
initialization and teardown. It won't randomly break in the "middle" (unless
renderer process gets killed I guess, but that's a separate problem)

> 
> Some are within our control, and could be managed by us, like #6, #7. But the
> cause of the rest is not very clear. It's difficult for me to simply fire and
> forget and trust the memory policy is delivered to LTHI (the
> OutputSurfaceClient, which is what really matters).

[hush (inactive), 2016-06-08 01:27:44]

Patchset #3 (id:40001) has been deleted

[hush (inactive), 2016-06-08 01:27:57]

PTAL PS3

[boliu, 2016-06-08 01:28:39]

lgtm

[hush (inactive), 2016-06-08 01:33:16]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2016-06-08 01:33:55]

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

[commit-bot: I haz the power, 2016-06-08 02:10:16]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-06-08 02:14:30]

Description was changed from

==========
Android WebView: Always send memory policy from Browser to Renderer.

Early out on the Browser side does not work because the IPC message send could
succeed even if the output surface in question is not yet bound to client and
registered with the SynchronousCompositorRegistry.

BUG=613781
==========

to

==========
Android WebView: Always send memory policy from Browser to Renderer.

Early out on the Browser side does not work because the IPC message send could
succeed even if the output surface in question is not yet bound to client and
registered with the SynchronousCompositorRegistry.

BUG=613781

Committed: https://crrev.com/90144532ec5fc69209a72fdb7561704dec1c3ced
Cr-Commit-Position: refs/heads/master@{#398460}
==========

[commit-bot: I haz the power, 2016-06-08 02:14:31]

Patchset 3 (id:??) landed as
https://crrev.com/90144532ec5fc69209a72fdb7561704dec1c3ced
Cr-Commit-Position: refs/heads/master@{#398460}