views/mus: Fix some flaky crashes during test teardown.

views/mus: Fix some flaky crashes during test teardown.

BUG=602038, 613371
Committed: https://crrev.com/a71fbc30b977cf95aaaef70e68d794510648198b
Cr-Commit-Position: refs/heads/master@{#394935}

[sadrul, 2016-05-18 20:00:19]

sadrul@chromium.org changed reviewers:
+ rockot@chromium.org, sky@chromium.org

[sadrul, 2016-05-18 20:00:20]

rockot@ Please see the comment below in surface_binding.cc

sky@ Please review.

https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
File ui/views/mus/surface_binding.cc (right):

https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
ui/views/mus/surface_binding.cc:132: gpu_.set_connection_error_handler([]{});
rockot@: This seems to be necessary for the change above to work (i.e. without
this, |gpu_.encountered_error()| doesn't seem to see any errors). I assume this
is unexpected? Any ideas/suggestions as to why this may be making a difference?

[sadrul, 2016-05-18 20:01:24]

For an example of the failure, see the various crashes in
Compositor::DidFailToInitializeOutputSurface() in

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

etc. But with this patch:

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

[sky, 2016-05-18 20:10:32]

sky@chromium.org changed reviewers:
+ yzshen@chromium.org

[sky, 2016-05-18 20:10:33]

+yzshen for the encountered_error issue.

https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
File ui/views/mus/surface_binding.cc (right):

https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
ui/views/mus/surface_binding.cc:132: gpu_.set_connection_error_handler([]{});
On 2016/05/18 20:00:19, sadrul wrote:
> rockot@: This seems to be necessary for the change above to work (i.e. without
> this, |gpu_.encountered_error()| doesn't seem to see any errors). I assume
this
> is unexpected? Any ideas/suggestions as to why this may be making a
difference?

This seems like a bug in the bindings code.

[Ken Rockot(use gerrit already), 2016-05-18 20:11:08]

rockot@chromium.org changed reviewers:
- yzshen@chromium.org

[Ken Rockot(use gerrit already), 2016-05-18 20:11:09]

https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
File ui/views/mus/surface_binding.cc (right):

https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
ui/views/mus/surface_binding.cc:132: gpu_.set_connection_error_handler([]{});
On 2016/05/18 at 20:00:19, sadrul wrote:
> rockot@: This seems to be necessary for the change above to work (i.e. without
this, |gpu_.encountered_error()| doesn't seem to see any errors). I assume this
is unexpected? Any ideas/suggestions as to why this may be making a difference?

Because we lazily set up the internal proxy in InterfacePtr, and
set_connection_error_handler is one of the calls which forces proxy setup.
+yzshen@ +amistry@ I remember that we made this change for a reason, but I don't
remember why. Why doesn't Bind also configure the proxy?

[sadrul, 2016-05-19 20:14:17]

Because the flaky crashes are somewhat annoying when running tests locally, I
would like to land this with the one weird trick. WDYT?

https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
File ui/views/mus/surface_binding.cc (right):

https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
ui/views/mus/surface_binding.cc:132: gpu_.set_connection_error_handler([]{});
On 2016/05/18 20:11:08, Ken Rockot wrote:
> On 2016/05/18 at 20:00:19, sadrul wrote:
> > rockot@: This seems to be necessary for the change above to work (i.e.
without
> this, |gpu_.encountered_error()| doesn't seem to see any errors). I assume
this
> is unexpected? Any ideas/suggestions as to why this may be making a
difference?
> 
> Because we lazily set up the internal proxy in InterfacePtr, and
> set_connection_error_handler is one of the calls which forces proxy setup.
> +yzshen@ +amistry@ I remember that we made this change for a reason, but I
don't
> remember why. Why doesn't Bind also configure the proxy?

OK. Sounds like this is not entirely unexpected, then. Can I add a TODO here and
refer to some bug (does one already exist?) so this doesn't get lost?

[Ken Rockot(use gerrit already), 2016-05-19 20:27:07]

On 2016/05/19 at 20:14:17, sadrul wrote:
> Because the flaky crashes are somewhat annoying when running tests locally, I
would like to land this with the one weird trick. WDYT?
> 
>
https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
> File ui/views/mus/surface_binding.cc (right):
> 
>
https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
> ui/views/mus/surface_binding.cc:132: gpu_.set_connection_error_handler([]{});
> On 2016/05/18 20:11:08, Ken Rockot wrote:
> > On 2016/05/18 at 20:00:19, sadrul wrote:
> > > rockot@: This seems to be necessary for the change above to work (i.e.
without
> > this, |gpu_.encountered_error()| doesn't seem to see any errors). I assume
this
> > is unexpected? Any ideas/suggestions as to why this may be making a
difference?
> > 
> > Because we lazily set up the internal proxy in InterfacePtr, and
> > set_connection_error_handler is one of the calls which forces proxy setup.
> > +yzshen@ +amistry@ I remember that we made this change for a reason, but I
don't
> > remember why. Why doesn't Bind also configure the proxy?
> 
> OK. Sounds like this is not entirely unexpected, then. Can I add a TODO here
and refer to some bug (does one already exist?) so this doesn't get lost?

Yeah, please file a bug and reference it here if you're going to land this
as-is.

Though honestly I'd like to understand why/how this avoids crashes, because it
seems to be covering up symptoms of a bigger problem rather than solving the
problem. Why would this crash if the gpu pipe has encountered an error?

[sadrul, 2016-05-19 20:36:13]

On 2016/05/19 20:27:07, Ken Rockot wrote:
> On 2016/05/19 at 20:14:17, sadrul wrote:
> > Because the flaky crashes are somewhat annoying when running tests locally,
I
> would like to land this with the one weird trick. WDYT?
> > 
> >
>
https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
> > File ui/views/mus/surface_binding.cc (right):
> > 
> >
>
https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
> > ui/views/mus/surface_binding.cc:132:
gpu_.set_connection_error_handler([]{});
> > On 2016/05/18 20:11:08, Ken Rockot wrote:
> > > On 2016/05/18 at 20:00:19, sadrul wrote:
> > > > rockot@: This seems to be necessary for the change above to work (i.e.
> without
> > > this, |gpu_.encountered_error()| doesn't seem to see any errors). I assume
> this
> > > is unexpected? Any ideas/suggestions as to why this may be making a
> difference?
> > > 
> > > Because we lazily set up the internal proxy in InterfacePtr, and
> > > set_connection_error_handler is one of the calls which forces proxy setup.
> > > +yzshen@ +amistry@ I remember that we made this change for a reason, but I
> don't
> > > remember why. Why doesn't Bind also configure the proxy?
> > 
> > OK. Sounds like this is not entirely unexpected, then. Can I add a TODO here
> and refer to some bug (does one already exist?) so this doesn't get lost?
> 
> Yeah, please file a bug and reference it here if you're going to land this
> as-is.
> 
> Though honestly I'd like to understand why/how this avoids crashes, because it
> seems to be covering up symptoms of a bigger problem rather than solving the
> problem. Why would this crash if the gpu pipe has encountered an error?

The crash happens in non-mojo place, where the compositor expects a valid
output-surface, but it can't be given one because the connection to gpu has gone
away.

[Ken Rockot(use gerrit already), 2016-05-19 20:43:42]

On 2016/05/19 at 20:36:13, sadrul wrote:
> On 2016/05/19 20:27:07, Ken Rockot wrote:
> > On 2016/05/19 at 20:14:17, sadrul wrote:
> > > Because the flaky crashes are somewhat annoying when running tests
locally, I
> > would like to land this with the one weird trick. WDYT?
> > > 
> > >
> >
https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
> > > File ui/views/mus/surface_binding.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
> > > ui/views/mus/surface_binding.cc:132:
gpu_.set_connection_error_handler([]{});
> > > On 2016/05/18 20:11:08, Ken Rockot wrote:
> > > > On 2016/05/18 at 20:00:19, sadrul wrote:
> > > > > rockot@: This seems to be necessary for the change above to work (i.e.
> > without
> > > > this, |gpu_.encountered_error()| doesn't seem to see any errors). I
assume
> > this
> > > > is unexpected? Any ideas/suggestions as to why this may be making a
> > difference?
> > > > 
> > > > Because we lazily set up the internal proxy in InterfacePtr, and
> > > > set_connection_error_handler is one of the calls which forces proxy
setup.
> > > > +yzshen@ +amistry@ I remember that we made this change for a reason, but
I
> > don't
> > > > remember why. Why doesn't Bind also configure the proxy?
> > > 
> > > OK. Sounds like this is not entirely unexpected, then. Can I add a TODO
here
> > and refer to some bug (does one already exist?) so this doesn't get lost?
> > 
> > Yeah, please file a bug and reference it here if you're going to land this
> > as-is.
> > 
> > Though honestly I'd like to understand why/how this avoids crashes, because
it
> > seems to be covering up symptoms of a bigger problem rather than solving the
> > problem. Why would this crash if the gpu pipe has encountered an error?
> 
> The crash happens in non-mojo place, where the compositor expects a valid
output-surface, but it can't be given one because the connection to gpu has gone
away.

OK, but it seems like it's still possible for the gpu interface to encounter an
error any time after you're checking it here. Won't this still have a racy crash
if the code in question isn't tolerant to missing a valid output surface?

[sky, 2016-05-19 20:59:42]

LGTM

https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
File ui/views/mus/surface_binding.cc (right):

https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
ui/views/mus/surface_binding.cc:132: gpu_.set_connection_error_handler([]{});
On 2016/05/19 20:14:17, sadrul wrote:
> On 2016/05/18 20:11:08, Ken Rockot wrote:
> > On 2016/05/18 at 20:00:19, sadrul wrote:
> > > rockot@: This seems to be necessary for the change above to work (i.e.
> without
> > this, |gpu_.encountered_error()| doesn't seem to see any errors). I assume
> this
> > is unexpected? Any ideas/suggestions as to why this may be making a
> difference?
> > 
> > Because we lazily set up the internal proxy in InterfacePtr, and
> > set_connection_error_handler is one of the calls which forces proxy setup.
> > +yzshen@ +amistry@ I remember that we made this change for a reason, but I
> don't
> > remember why. Why doesn't Bind also configure the proxy?
> 
> OK. Sounds like this is not entirely unexpected, then. Can I add a TODO here
and
> refer to some bug (does one already exist?) so this doesn't get lost?

Yes, go for it.

[sadrul, 2016-05-19 23:18:49]

Description was changed from

==========
views/mus: Fix some flaky crashes during test teardown.

BUG=602038
==========

to

==========
views/mus: Fix some flaky crashes during test teardown.

BUG=602038, 613371
==========

[sadrul, 2016-05-19 23:21:36]

On 2016/05/19 20:43:42, Ken Rockot wrote:
> On 2016/05/19 at 20:36:13, sadrul wrote:
> > On 2016/05/19 20:27:07, Ken Rockot wrote:
> > > On 2016/05/19 at 20:14:17, sadrul wrote:
> > > > Because the flaky crashes are somewhat annoying when running tests
> locally, I
> > > would like to land this with the one weird trick. WDYT?
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
> > > > File ui/views/mus/surface_binding.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1995613003/diff/1/ui/views/mus/surface_bindin...
> > > > ui/views/mus/surface_binding.cc:132:
> gpu_.set_connection_error_handler([]{});
> > > > On 2016/05/18 20:11:08, Ken Rockot wrote:
> > > > > On 2016/05/18 at 20:00:19, sadrul wrote:
> > > > > > rockot@: This seems to be necessary for the change above to work
(i.e.
> > > without
> > > > > this, |gpu_.encountered_error()| doesn't seem to see any errors). I
> assume
> > > this
> > > > > is unexpected? Any ideas/suggestions as to why this may be making a
> > > difference?
> > > > > 
> > > > > Because we lazily set up the internal proxy in InterfacePtr, and
> > > > > set_connection_error_handler is one of the calls which forces proxy
> setup.
> > > > > +yzshen@ +amistry@ I remember that we made this change for a reason,
but
> I
> > > don't
> > > > > remember why. Why doesn't Bind also configure the proxy?
> > > > 
> > > > OK. Sounds like this is not entirely unexpected, then. Can I add a TODO
> here
> > > and refer to some bug (does one already exist?) so this doesn't get lost?
> > > 
> > > Yeah, please file a bug and reference it here if you're going to land this
> > > as-is.
> > > 
> > > Though honestly I'd like to understand why/how this avoids crashes,
because
> it
> > > seems to be covering up symptoms of a bigger problem rather than solving
the
> > > problem. Why would this crash if the gpu pipe has encountered an error?
> > 
> > The crash happens in non-mojo place, where the compositor expects a valid
> output-surface, but it can't be given one because the connection to gpu has
gone
> away.
> 
> OK, but it seems like it's still possible for the gpu interface to encounter
an
> error any time after you're checking it here. Won't this still have a racy
crash
> if the code in question isn't tolerant to missing a valid output surface?

Yeah. The mus-app should be able to handle lost connections to gpu more
gracefully. Filed https://crbug.com/613366 to track that and referenced it in a
TODO here.

[sadrul, 2016-05-20 00:19:00]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-05-20 00:19:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1995613003/#ps20001
(title: ".")

[commit-bot: I haz the power, 2016-05-20 00:19:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1995613003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1995613003/20001

[commit-bot: I haz the power, 2016-05-20 00:44:43]

Description was changed from

==========
views/mus: Fix some flaky crashes during test teardown.

BUG=602038, 613371
==========

to

==========
views/mus: Fix some flaky crashes during test teardown.

BUG=602038, 613371
==========

[commit-bot: I haz the power, 2016-05-20 00:44:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-20 00:46:30]

Description was changed from

==========
views/mus: Fix some flaky crashes during test teardown.

BUG=602038, 613371
==========

to

==========
views/mus: Fix some flaky crashes during test teardown.

BUG=602038, 613371

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

[commit-bot: I haz the power, 2016-05-20 00:46:31]

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