cc: Post the missed BeginFrame to a new stack

cc: Post the missed BeginFrame to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). To avoid this, and similar
problems, we prevent DisplayScheduler from running a new BeginFrame
inside an external call to DisplayScheduler::SurfaceDamaged() which
prevents re-entrancy for its clients.

We do this by posting the missed BeginFrame to respond to it on a new
stack (and cancelling it if another BeginFrame comes in the meantime
since it was already posted).

For TestDelegatedOutputSurface synchronous composites, we simply
post the SwapBuffersComplete message to get it on a new call stack.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/e15b5e856818e2113a4b23cd8b6ad5cee521a126
Cr-Commit-Position: refs/heads/master@{#408289}

[danakj, 2016-07-27 00:48:18]

Description was changed from

==========
cc: Post the DidSwapBuffersComplete notice to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). So we make the draw callback
post the DidSwapBuffersComplete() call to a new stack frame using
OutputSurface::PostSwapBuffersComplete().

This is also what other OutputSurface implementations do, and is
standard behaviour. The only other that doesn't are the GPU backed
OutputSurfaces, for whom the swap ack should already be asynchronous.

R=jbauman
BUG=495650
==========

to

==========
cc: Post the DidSwapBuffersComplete notice to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). So we make the draw callback
post the DidSwapBuffersComplete() call to a new stack frame using
OutputSurface::PostSwapBuffersComplete().

This is also what other OutputSurface implementations do, and is
standard behaviour. The only other that doesn't are the GPU backed
OutputSurfaces, for whom the swap ack should already be asynchronous.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[danakj, 2016-07-27 00:48:20]

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

[commit-bot: I haz the power, 2016-07-27 00:48:37]

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

[jbauman, 2016-07-27 00:51:51]

lgtm

[sunnyps, 2016-07-27 00:53:12]

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

[sunnyps, 2016-07-27 00:53:13]

This is ok but I think fundamentally the problem is with DisplayScheduler which
calls AddObserver and receives OnBeginFrame in the same call stack. It might be
bad for other cases too.

[danakj, 2016-07-27 00:55:27]

On 2016/07/27 00:53:13, sunnyps wrote:
> This is ok but I think fundamentally the problem is with DisplayScheduler
which
> calls AddObserver and receives OnBeginFrame in the same call stack. It might
be
> bad for other cases too.

Hm ok. I need the one in TestDOS also for the case of synchronous compositing
(layout tests) as it calls DrawAndSwap from directly TestDOS::SwapBuffers. But
SurfaceDOS doesn't support that.

[chromium-reviews, 2016-07-27 00:59:06]

cc scheduler CL that adds the PostTask for begin frame:
https://codereview.chromium.org/2158023005/

Ignore the retro frame stuff. The scheduler does a PostTask when it gets a
MISSED begin frame.

On Tue, Jul 26, 2016 at 5:55 PM <danakj@chromium.org> wrote:

> On 2016/07/27 00:53:13, sunnyps wrote:
> > This is ok but I think fundamentally the problem is with DisplayScheduler
> which
> > calls AddObserver and receives OnBeginFrame in the same call stack. It
> might
> be
> > bad for other cases too.
>
> Hm ok. I need the one in TestDOS also for the case of synchronous
> compositing
> (layout tests) as it calls DrawAndSwap from directly TestDOS::SwapBuffers.
> But
> SurfaceDOS doesn't support that.
>
> https://codereview.chromium.org/2183333003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-07-27 01:43:11]

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

[danakj, 2016-07-27 01:43:13]

PTAL

[commit-bot: I haz the power, 2016-07-27 01:43:35]

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

[danakj, 2016-07-27 01:45:26]

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

[commit-bot: I haz the power, 2016-07-27 01:45:42]

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

[danakj, 2016-07-27 01:50:25]

And now with IgnoreResult tricks.

[danakj, 2016-07-27 01:50:41]

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

[commit-bot: I haz the power, 2016-07-27 01:51:03]

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

[sunnyps, 2016-07-27 02:00:14]

The missed frame callback should also be cancelled when we call RemoveObserver.

https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.cc:103: inside_add_observer_ = true;
nit: base::AutoReset<bool> mark_inside(&inside_add_observer_, true)

https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.h (right):

https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.h:72: bool inside_add_observer_ = false;
nit: TBH it's not AddObserver that we really care about, we care about being in
the middle of some state transaction and getting a BeginFrame during that.

In the scheduler we keep track of this as inside_process_scheduled_actions_.

[danakj, 2016-07-27 02:19:05]

https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.cc:103: inside_add_observer_ = true;
On 2016/07/27 02:00:14, sunnyps wrote:
> nit: base::AutoReset<bool> mark_inside(&inside_add_observer_, true)

Done.

https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.h (right):

https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.h:72: bool inside_add_observer_ = false;
On 2016/07/27 02:00:14, sunnyps wrote:
> nit: TBH it's not AddObserver that we really care about, we care about being
in
> the middle of some state transaction and getting a BeginFrame during that.
> 
> In the scheduler we keep track of this as inside_process_scheduled_actions_.

I can call this inside_external_action_, which I've done. But IDK if that is
what you're really hoping for.

[danakj, 2016-07-27 02:19:58]

PTAL

[danakj, 2016-07-27 02:21:42]

On 2016/07/27 02:00:14, sunnyps wrote:
> The missed frame callback should also be cancelled when we call
RemoveObserver.

Oh, I missed this. Done now.

[danakj, 2016-07-27 02:21:46]

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

[commit-bot: I haz the power, 2016-07-27 02:22:11]

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

[danakj, 2016-07-27 02:28:50]

Description was changed from

==========
cc: Post the DidSwapBuffersComplete notice to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). So we make the draw callback
post the DidSwapBuffersComplete() call to a new stack frame using
OutputSurface::PostSwapBuffersComplete().

This is also what other OutputSurface implementations do, and is
standard behaviour. The only other that doesn't are the GPU backed
OutputSurfaces, for whom the swap ack should already be asynchronous.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
cc: Post the missed BeginFrame to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). To avoid this, and similar
problems, we prevent DisplayScheduler from running a new BeginFrame
inside an external call to DisplayScheduler::SurfaceDamaged() which
prevents re-entrancy for its clients.

We do this by posting the missed BeginFrame to respond to it on a new
stack (and cancelling it if another BeginFrame comes in the meantime
since it was already posted).

For TestDelegatedOutputSurface synchronous composites, we simply
post the SwapBuffersComplete message to get it on a new call stack.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-27 03:27:56]

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

[commit-bot: I haz the power, 2016-07-27 03:27:57]

Dry run: This issue passed the CQ dry run.

[sunnyps, 2016-07-27 05:09:57]

On 2016/07/27 02:19:05, danakj wrote:
>
https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
> File cc/surfaces/display_scheduler.cc (right):
> 
>
https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
> cc/surfaces/display_scheduler.cc:103: inside_add_observer_ = true;
> On 2016/07/27 02:00:14, sunnyps wrote:
> > nit: base::AutoReset<bool> mark_inside(&inside_add_observer_, true)
> 
> Done.
> 
>
https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
> File cc/surfaces/display_scheduler.h (right):
> 
>
https://codereview.chromium.org/2183333003/diff/60001/cc/surfaces/display_sch...
> cc/surfaces/display_scheduler.h:72: bool inside_add_observer_ = false;
> On 2016/07/27 02:00:14, sunnyps wrote:
> > nit: TBH it's not AddObserver that we really care about, we care about being
> in
> > the middle of some state transaction and getting a BeginFrame during that.
> > 
> > In the scheduler we keep track of this as inside_process_scheduled_actions_.
> 
> I can call this inside_external_action_, which I've done. But IDK if that is
> what you're really hoping for.

inside_surface_damaged_ is better IMO and moving the base::AutoReset to the
scope of SurfaceDamaged (instead of just around AddObserver. It conveys what we
really care about here.

[sunnyps, 2016-07-27 05:21:02]

LGTM % nits + comment about inside_surface_damaged_

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:175: // If we get another BeginFrame before a
posted missed frame, just drop the
nit: We also need to cancel the missed frame in case of a missed frame otherwise
our DCHECKs above will fail (IsCancelled won't return true unless we explicitly
Cancel or Reset).

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:176: // missed frame. (Do this last because
this might be the missed frame and
nit: dangling parens

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.h (right):

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.h:72: bool inside_external_action_ = false;
nit: can you follow existing style here i.e. initializing this in the ctor?

[danakj, 2016-07-27 22:34:56]

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:175: // If we get another BeginFrame before a
posted missed frame, just drop the
On 2016/07/27 05:21:02, sunnyps wrote:
> nit: We also need to cancel the missed frame in case of a missed frame
otherwise
> our DCHECKs above will fail (IsCancelled won't return true unless we
explicitly
> Cancel or Reset).

I'm not sure what you mean here sorry, can you explain?

  bool IsCancelled() const {
    return callback_.is_null();
  }
 
IsCancelled() returns true for a new CancelableCallback if not created with a
callback (which is the case here).

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:176: // missed frame. (Do this last because
this might be the missed frame and
On 2016/07/27 05:21:02, sunnyps wrote:
> nit: dangling parens

Done.

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.h (right):

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.h:72: bool inside_external_action_ = false;
On 2016/07/27 05:21:02, sunnyps wrote:
> nit: can you follow existing style here i.e. initializing this in the ctor?

Done.

[danakj, 2016-07-27 22:35:18]

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

[commit-bot: I haz the power, 2016-07-27 22:35:44]

Dry run: 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-07-27 22:35:51]

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

[commit-bot: I haz the power, 2016-07-27 22:35:52]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[danakj, 2016-07-27 22:37:21]

Description was changed from

==========
cc: Post the missed BeginFrame to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). To avoid this, and similar
problems, we prevent DisplayScheduler from running a new BeginFrame
inside an external call to DisplayScheduler::SurfaceDamaged() which
prevents re-entrancy for its clients.

We do this by posting the missed BeginFrame to respond to it on a new
stack (and cancelling it if another BeginFrame comes in the meantime
since it was already posted).

For TestDelegatedOutputSurface synchronous composites, we simply
post the SwapBuffersComplete message to get it on a new call stack.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
cc: Post the missed BeginFrame to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). To avoid this, and similar
problems, we prevent DisplayScheduler from running a new BeginFrame
inside an external call to DisplayScheduler::SurfaceDamaged() which
prevents re-entrancy for its clients.

We do this by posting the missed BeginFrame to respond to it on a new
stack (and cancelling it if another BeginFrame comes in the meantime
since it was already posted).

For TestDelegatedOutputSurface synchronous composites, we simply
post the SwapBuffersComplete message to get it on a new call stack.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-07-27 22:37:25]

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

[commit-bot: I haz the power, 2016-07-27 22:37:58]

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

[sunnyps, 2016-07-27 22:44:38]

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:175: // If we get another BeginFrame before a
posted missed frame, just drop the
On 2016/07/27 22:34:55, danakj wrote:
> On 2016/07/27 05:21:02, sunnyps wrote:
> > nit: We also need to cancel the missed frame in case of a missed frame
> otherwise
> > our DCHECKs above will fail (IsCancelled won't return true unless we
> explicitly
> > Cancel or Reset).
> 
> I'm not sure what you mean here sorry, can you explain?
> 
>   bool IsCancelled() const {
>     return callback_.is_null();
>   }
>  
> IsCancelled() returns true for a new CancelableCallback if not created with a
> callback (which is the case here).

Just a nit about the comment.

The comment seems to imply that we need to cancel the missed frame only if we
get another BeginFrame before the posted missed frame. But we also need to
cancel it when missed frame callback is called.

CancelableCallback doesn't cancel the callback on its own even after running the
callback.

[danakj, 2016-07-27 22:48:42]

On Wed, Jul 27, 2016 at 3:44 PM, <sunnyps@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
> File cc/surfaces/display_scheduler.cc (right):
>
>
>
https://codereview.chromium.org/2183333003/diff/100001/cc/surfaces/display_sc...
> cc/surfaces/display_scheduler.cc:175: // If we get another BeginFrame
> before a posted missed frame, just drop the
> On 2016/07/27 22:34:55, danakj wrote:
> > On 2016/07/27 05:21:02, sunnyps wrote:
> > > nit: We also need to cancel the missed frame in case of a missed
> frame
> > otherwise
> > > our DCHECKs above will fail (IsCancelled won't return true unless we
> > explicitly
> > > Cancel or Reset).
> >
> > I'm not sure what you mean here sorry, can you explain?
> >
> > bool IsCancelled() const {
> > return callback_.is_null();
> > }
> >
> > IsCancelled() returns true for a new CancelableCallback if not created
> with a
> > callback (which is the case here).
>
> Just a nit about the comment.
>
> The comment seems to imply that we need to cancel the missed frame only
> if we get another BeginFrame before the posted missed frame. But we also
> need to cancel it when missed frame callback is called.
>
> CancelableCallback doesn't cancel the callback on its own even after
> running the callback.
>

Ooh okay thanks.


>
> https://codereview.chromium.org/2183333003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-07-27 22:50:01]

Done. jbauman you're cool with this too?

[danakj, 2016-07-27 22:50:05]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-07-27 22:50:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/2183333003/#ps140001
(title: "post-swapcomplete: comment")

[danakj, 2016-07-27 22:50:16]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-07-27 22:50:20]

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

[commit-bot: I haz the power, 2016-07-27 22:50:21]

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

[danakj, 2016-07-27 22:54:57]

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

[commit-bot: I haz the power, 2016-07-27 22:55:29]

Dry run: 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-07-27 23:37:52]

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

[commit-bot: I haz the power, 2016-07-27 23:37:52]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-07-28 00:06:22]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-07-28 00:06:32]

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-07-28 00:11:40]

Description was changed from

==========
cc: Post the missed BeginFrame to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). To avoid this, and similar
problems, we prevent DisplayScheduler from running a new BeginFrame
inside an external call to DisplayScheduler::SurfaceDamaged() which
prevents re-entrancy for its clients.

We do this by posting the missed BeginFrame to respond to it on a new
stack (and cancelling it if another BeginFrame comes in the meantime
since it was already posted).

For TestDelegatedOutputSurface synchronous composites, we simply
post the SwapBuffersComplete message to get it on a new call stack.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Post the missed BeginFrame to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). To avoid this, and similar
problems, we prevent DisplayScheduler from running a new BeginFrame
inside an external call to DisplayScheduler::SurfaceDamaged() which
prevents re-entrancy for its clients.

We do this by posting the missed BeginFrame to respond to it on a new
stack (and cancelling it if another BeginFrame comes in the meantime
since it was already posted).

For TestDelegatedOutputSurface synchronous composites, we simply
post the SwapBuffersComplete message to get it on a new call stack.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-07-28 00:11:41]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-28 00:14:11]

Description was changed from

==========
cc: Post the missed BeginFrame to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). To avoid this, and similar
problems, we prevent DisplayScheduler from running a new BeginFrame
inside an external call to DisplayScheduler::SurfaceDamaged() which
prevents re-entrancy for its clients.

We do this by posting the missed BeginFrame to respond to it on a new
stack (and cancelling it if another BeginFrame comes in the meantime
since it was already posted).

For TestDelegatedOutputSurface synchronous composites, we simply
post the SwapBuffersComplete message to get it on a new call stack.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Post the missed BeginFrame to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). To avoid this, and similar
problems, we prevent DisplayScheduler from running a new BeginFrame
inside an external call to DisplayScheduler::SurfaceDamaged() which
prevents re-entrancy for its clients.

We do this by posting the missed BeginFrame to respond to it on a new
stack (and cancelling it if another BeginFrame comes in the meantime
since it was already posted).

For TestDelegatedOutputSurface synchronous composites, we simply
post the SwapBuffersComplete message to get it on a new call stack.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/e15b5e856818e2113a4b23cd8b6ad5cee521a126
Cr-Commit-Position: refs/heads/master@{#408289}
==========

[commit-bot: I haz the power, 2016-07-28 00:14:13]

Patchset 8 (id:??) landed as
https://crrev.com/e15b5e856818e2113a4b23cd8b6ad5cee521a126
Cr-Commit-Position: refs/heads/master@{#408289}