The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a71fbc30b977cf95aaaef70e68d794510648198b

commit a71fbc30b977cf95aaaef70e68d794510648198b
Author: sadrul <sadrul@chromium.org>
Date: Fri May 20 00:44:33 2016

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

BUG= 602038 , 613371

Review-Url: https://codereview.chromium.org/1995613003
Cr-Commit-Position: refs/heads/master@{#394935}

[modify] https://crrev.com/a71fbc30b977cf95aaaef70e68d794510648198b/ui/views/mus/surface_binding.cc
[modify] https://crrev.com/a71fbc30b977cf95aaaef70e68d794510648198b/ui/views/mus/surface_context_factory.cc
[modify] https://crrev.com/a71fbc30b977cf95aaaef70e68d794510648198b/ui/views/test/views_test_base.cc
[modify] https://crrev.com/a71fbc30b977cf95aaaef70e68d794510648198b/ui/views/test/views_test_base.h
[modify] https://crrev.com/a71fbc30b977cf95aaaef70e68d794510648198b/ui/views/widget/widget_unittest.cc

(Not say that it is unreasonable...) in what case you need to call encountered_error() but don't de-reference or set error handler?

We don't start listening unless the pointer is de-referenced or error handler is set. The reason why we don't do that as soon as Bind is called: imagine you have the following interface:
interface Bar {};
interface Foo {
  SendBar(Bar bar);
};

When you make a SendBar() call, typically you create a new BarPtr, bind it, and then pass it to SendBar(). If we start listening as soon as it is bound, that means we do quite some setup work and throw it away immediately. :/

One solution is to map mojom Bar parameter to BarPtrInfo instead of BarPtr. That is more consistent with associated interfaces (in which case we map to BarAssociatedPtrInfo).

I know the standalone Mojo repo has made this change.

Thanks Yuzhu. I'm in favor of changing the bindings so that SendBar would
take a BarPtrInfo rather than a BarPtr. The change should not cause much
churn since it just means affixing a .PassInterface() wherever we pass
InterfacePtrs now, and that in itself is a rare occurrence.

I would also be in favor of changing InterfacePtr Bind to configure the
proxy ASAP, even before we update the bindings. I think it's unlikely that
this redundant setup+teardown will have any meaningful impact on
performance today.

Chatted with Ken about this some more:

Another approach is to get rid of the "encountered_error()" getter. Users need to setup error handler explicitly to learn whether there is an error.

Correspondingly, we need to change set_connection_error_handler() to guarantee that if a handler is set after pipe disconnection is detected, we need to run it exactly once.

There are about 10 non-testing callsites of encountered_errro(). Quite some of them are unnecessary. For example, the code bind an interface pointer/binding using either ConnectToInterface() or an InterfacePtrInfo/InterfaceRequest instance, and then test encountered_error() immediately (which should be always false at that point because we need to watch the pipe to learn about error asynchrously).

From the usage, it seems encountered_error() is misleading. So it may be a good idea to remove it.

I could try out the idea and send a mail to chromium-mojo when I am sure it is a good idea.

Thanks!

Updating bug statuses so that "Available" really means available. I would like to be able to consistently query the available workload for Mojo related stuff, so if you would rather add this to the pile of nice-to-haves, please consider marking this as Available and removing Owner. Thanks!

Sorry. I think having it in my backburner for one year means it is actually available. :/ I removed myself from the owner field.

Heh, that's what I've been realizing too with my own bugs. Just doing
cleanup of all Mojo bugs to make tracking priorities and opportunities
(hopefully) easier. Thanks!

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Triaging. This still comes up occasionally. I think two reasonable options are

1. remove encountered_error(), or
2. make encountered_error() lazily bring up all the same binding state that dereferencing or set_connection_error_handler does, and in the case where it has to do that, have it also query the bound handle to see if it's already unreadable.