Fix crash in sync compositor registration

Fix crash in sync compositor registration

RenderWidgetHostViewAndroid can send begin frames even when renderer did
not request it. In case where renderer side is actually missing, it
leads to DCHECK failure and probably crash.

This is a follow up fix to
https://codereview.chromium.org/1150423003

BUG=497291
Committed: https://crrev.com/67debd402e689382d88231b43a7e2bf8bae760a1
Cr-Commit-Position: refs/heads/master@{#333199}

[boliu, 2015-06-05 22:45:57]

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

[boliu, 2015-06-05 22:45:58]

ptal

[sgurun-gerrit only, 2015-06-05 22:50:09]

On 2015/06/05 22:45:58, boliu wrote:
> ptal

rubberstamp, but can you please create a bug to write a test for this so we
don't forgeet.

[sgurun-gerrit only, 2015-06-05 22:50:18]

On 2015/06/05 22:50:09, sgurun wrote:
> On 2015/06/05 22:45:58, boliu wrote:
> > ptal
> 
> rubberstamp, but can you please create a bug to write a test for this so we
> don't forgeet.

lgtm

[boliu, 2015-06-05 22:51:45]

The CQ bit was checked by boliu@chromium.org

[sunnyps, 2015-06-05 22:52:13]

sunnyps@chromium.org changed reviewers:
+ sunnyps@chromium.org

[sunnyps, 2015-06-05 22:52:14]

Don't we try to prevent this in RWHVA? using the outstanding_vsync_requests_
bitmask?

[sunnyps, 2015-06-05 22:52:14]

Don't we try to prevent this in RWHVA? using the outstanding_vsync_requests_
bitmask?

[commit-bot: I haz the power, 2015-06-05 22:52:48]

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

[boliu, 2015-06-05 22:55:31]

On 2015/06/05 22:52:14, sunnyps wrote:
> Don't we try to prevent this in RWHVA? using the outstanding_vsync_requests_
> bitmask?

The PERSISTENT_BEGIN_FRAME bit is only set by renderer compositor. But the
BEGIN_FRAME bit is set by random things like input in RWHVA. BEGIN_FRAME bit is
the problem here, ie RWHVA can decide to request and send random vsyncs when it
wants to

[sunnyps, 2015-06-05 22:59:21]

On 2015/06/05 22:55:31, boliu wrote:
> On 2015/06/05 22:52:14, sunnyps wrote:
> > Don't we try to prevent this in RWHVA? using the outstanding_vsync_requests_
> > bitmask?
> 
> The PERSISTENT_BEGIN_FRAME bit is only set by renderer compositor. But the
> BEGIN_FRAME bit is set by random things like input in RWHVA. BEGIN_FRAME bit
is
> the problem here, ie RWHVA can decide to request and send random vsyncs when
it
> wants to

Ok, I think BEGIN_FRAME is an optimization for the case when we have input and
can guess that the renderer will ask for a begin frame but want to avoid the IPC
latency.

Can we instead ignore BEGIN_FRAME when using synchronous compositor? That
optimization is not needed for synchronous compositor because it can start
asking for begin frame immediately i.e. inside the choreographer vsync callback.

[boliu, 2015-06-05 23:16:13]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2015-06-05 23:29:21]

On 2015/06/05 22:59:21, sunnyps wrote:
> On 2015/06/05 22:55:31, boliu wrote:
> > On 2015/06/05 22:52:14, sunnyps wrote:
> > > Don't we try to prevent this in RWHVA? using the
outstanding_vsync_requests_
> > > bitmask?
> > 
> > The PERSISTENT_BEGIN_FRAME bit is only set by renderer compositor. But the
> > BEGIN_FRAME bit is set by random things like input in RWHVA. BEGIN_FRAME bit
> is
> > the problem here, ie RWHVA can decide to request and send random vsyncs when
> it
> > wants to
> 
> Ok, I think BEGIN_FRAME is an optimization for the case when we have input and
> can guess that the renderer will ask for a begin frame but want to avoid the
IPC
> latency.
> 
> Can we instead ignore BEGIN_FRAME when using synchronous compositor? That
> optimization is not needed for synchronous compositor because it can start
> asking for begin frame immediately i.e. inside the choreographer vsync
callback.

That's true, but on the other hand, I don't want to add more "if webview" code
to general areas if possible. Switching to vsyncs was in part unifying that part
of the code.

I think building browser-renderer to have less assumptions about what browser or
renderer objects can do is better way to go..

Sounds good?

[sunnyps, 2015-06-05 23:56:55]

On 2015/06/05 23:29:21, boliu wrote:
> On 2015/06/05 22:59:21, sunnyps wrote:
> > On 2015/06/05 22:55:31, boliu wrote:
> > > On 2015/06/05 22:52:14, sunnyps wrote:
> > > > Don't we try to prevent this in RWHVA? using the
> outstanding_vsync_requests_
> > > > bitmask?
> > > 
> > > The PERSISTENT_BEGIN_FRAME bit is only set by renderer compositor. But the
> > > BEGIN_FRAME bit is set by random things like input in RWHVA. BEGIN_FRAME
bit
> > is
> > > the problem here, ie RWHVA can decide to request and send random vsyncs
when
> > it
> > > wants to
> > 
> > Ok, I think BEGIN_FRAME is an optimization for the case when we have input
and
> > can guess that the renderer will ask for a begin frame but want to avoid the
> IPC
> > latency.
> > 
> > Can we instead ignore BEGIN_FRAME when using synchronous compositor? That
> > optimization is not needed for synchronous compositor because it can start
> > asking for begin frame immediately i.e. inside the choreographer vsync
> callback.
> 
> That's true, but on the other hand, I don't want to add more "if webview" code
> to general areas if possible. Switching to vsyncs was in part unifying that
part
> of the code.
> 
> I think building browser-renderer to have less assumptions about what browser
or
> renderer objects can do is better way to go..
> 
> Sounds good?

Yeah. Another approach is to set is_active = true when renderer objects are
initialized and only check is_active in the condition -
renderer_needs_begin_frames doesn't really matter - cc can handle BeginFrames
even if they weren't asked for. But I'm okay with the CL the way it is.

[boliu, 2015-06-06 00:08:00]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-06-06 00:08:42]

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

[commit-bot: I haz the power, 2015-06-06 01:30:39]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-06-06 01:31:36]

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