CPP bindings: DCHECK when a Callback is destructed without being invoked

DCHECK when the Runnable associated with a Callback is destructed without being invoked, unless a connection error occurred or the pipe was manually closed first.

BUG=455298
R=jamesr@chromium.org, sky@chromium.org, jamesr, sky

Committed: https://chromium.googlesource.com/external/mojo/+/5f59634fbe6d12a5e31b385281525d6a9aa4e624

[rudominer, 2015-03-12 16:04:18]

rudominer@chromium.org changed reviewers:
+ jamesr@chromium.org

[rudominer, 2015-03-12 16:38:39]

This is for discussion only--not ready to be submitted.

This is a follow-up to cl issue 987113003.

As discussed there, it would be nice to DCHECK only in the case that the
application mistakenly abandons the Callback, not in the case that the Callback
is destructed in the course of handling a connection error.

I attempted to do that in this CL based on a conversation between jamesr and me.
I claimed that it was not possible to distinguish a "legitimate" abandonment
(i.e. one that is happening due to a connection error on the pipe) from an
illegitimate one. James convinced me that it was possible. But the test results
have made doubt this again.

Consider the last stack trace here
https://build.chromium.org/p/tryserver.client.mojo/builders/Mojo%20Linux%20Na...

What I think is happening is that there was a connection error on the pipe
associated with NatoveViewport and that causes
NativeViewportImpl::OnConnectionError(). But NativeViewportImpl contains a
subordinate object, OnscreenContextProvider that implements the ContextProvider
interface. This is a "subordinate" interface to NativeViewport interface in the
sense that it was requested from NativeViewport via a ConnectionRequest.

The pipe associated with ContextProvider does not seem to have experienced a
connection error. But the parent object wants to destroy the subordinate object
and thereby tear down the "subordinate" pipe. That seems reasonable to me. I
don't think we should DCHECK in this case--we should just silently close the
pipe. What do you think James?

p.s. I also have some questions about the correctness of the C++ in my changes
based on the fact that I don't yet fully understand the Mojo threading model. I
asked one question in the code review comments. This is orthogonal to whether or
not the main idea of this CL will work.

https://codereview.chromium.org/1003773002/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1003773002/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:119:
if (owning_stub_ && !owning_stub_->connection_error_occurred()) {
Question: I don't yet understand the Mojo threading model well enough to
understand whether or not this is thread safe.

[jamesr, 2015-03-12 18:18:37]

On 2015/03/12 16:38:39, rudominer wrote:
> Consider the last stack trace here
>
https://build.chromium.org/p/tryserver.client.mojo/builders/Mojo%20Linux%20Na...
> 
> The pipe associated with ContextProvider does not seem to have experienced a
> connection error. But the parent object wants to destroy the subordinate
object
> and thereby tear down the "subordinate" pipe. That seems reasonable to me. I
> don't think we should DCHECK in this case--we should just silently close the
> pipe. What do you think James?

Here OnscreenContextProvider wants go go away and it's holding a callback as
member.  The class declaration of OnscreenContextProvider is (in part):

..
  CreateCallback pending_create_callback_;
  mojo::Binding<mojo::ContextProvider> binding_;
};

here the |binding_| member holds the pipe handle and will be destroyed before
|pending_create_callback_| since it's later in the member list.  Destroying the
mojo::Binding should close the pipe associated with the callback, so by the time
the callback is destroyed we should be able to observe that the associated pipe
is closed and avoid generating an error.

Two quesitons:
1.) Does destroying the mojo::Binding actually close the pipe?  Looking at the
code it seems like it destroys the router but doesn't actually call
CloseMessagePipe (yikes!).  I may have written a nasty leak here :(
2.) Assuming that closing the mojo::Binding does close the pipe, can we detect
that the pipe is cleanly closed when destroying the responder?  I think we can
in theory but aren't tracking clean closure currently, only error cases.  To
resolve this I think we'd want to keep a bit around next to the connector's
error_ bit

[jamesr, 2015-03-12 18:20:04]

https://codereview.chromium.org/1003773002/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1003773002/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:119:
if (owning_stub_ && !owning_stub_->connection_error_occurred()) {
On 2015/03/12 16:38:39, rudominer wrote:
> Question: I don't yet understand the Mojo threading model well enough to
> understand whether or not this is thread safe.

The C++ bindings are only usable from a single thread (for the most part), so
this is fine.  In particular, once a mojo::Binding or mojo::InterfacePtr is used
on a particular thread the set of objects created (like the router, connector,
responder etc) all live on that thread for the rest of their lives.  The handle
can only be used on another thread by unbinding it, which tears down all these
objects, and then rebinding elsewhere.

[rudominer, 2015-03-12 19:08:23]

On 2015/03/12 18:18:37, jamesr wrote:
> On 2015/03/12 16:38:39, rudominer wrote:
> > Consider the last stack trace here
> >
>
https://build.chromium.org/p/tryserver.client.mojo/builders/Mojo%20Linux%20Na...
> > 
> > The pipe associated with ContextProvider does not seem to have experienced a
> > connection error. But the parent object wants to destroy the subordinate
> object
> > and thereby tear down the "subordinate" pipe. That seems reasonable to me. I
> > don't think we should DCHECK in this case--we should just silently close the
> > pipe. What do you think James?
> 
> Here OnscreenContextProvider wants go go away and it's holding a callback as
> member.  The class declaration of OnscreenContextProvider is (in part):
> 
> ..
>   CreateCallback pending_create_callback_;
>   mojo::Binding<mojo::ContextProvider> binding_;
> };
> 
> here the |binding_| member holds the pipe handle and will be destroyed before
> |pending_create_callback_| since it's later in the member list.  

So just to be clear, you are comfortable with making it the application
developer's responsibility to know:
(i) Destroying a Binding closes the associated pipe
(ii) It is mandatory to close a pipe before destroying any unfinished Callbacks
associated with the pipe.
If so then I agree with your analysis of this particular situation. 

It is not clear to me who Mojo application
developers will be and how much of this plumbing we want to shield them from. In
particular if you work directly
with the objects in mojo/public/cpp/binding I think you don't ever work directly
with an object called "Pipe" so
why would we expect the developer to know about the necessity of closing a pipe.


Destroying the
> mojo::Binding should close the pipe associated with the callback, so by the
time
> the callback is destroyed we should be able to observe that the associated
pipe
> is closed and avoid generating an error.
> 
> Two quesitons:
> 1.) Does destroying the mojo::Binding actually close the pipe?  Looking at the
> code it seems like it destroys the router but doesn't actually call
> CloseMessagePipe (yikes!).  I may have written a nasty leak here :(
> 2.) Assuming that closing the mojo::Binding does close the pipe, can we detect
> that the pipe is cleanly closed when destroying the responder?  I think we can
> in theory but aren't tracking clean closure currently, only error cases.  To
> resolve this I think we'd want to keep a bit around next to the connector's
> error_ bit

[jamesr, 2015-03-12 19:13:15]

Yeah, they won't see the pipe but they will see the mojo::Binding<> and from
that they will receive callbacks.  I think it's reasonable to say that any code
that receives a callback object from a mojo::Binding<> must either invoke that
callback, destroy the mojo::Binding<> or explicitly Close() the mojo::Binding<>
before destroying that callback.

[rudominer, 2015-03-12 19:18:02]

On 2015/03/12 19:13:15, jamesr wrote:
> Yeah, they won't see the pipe but they will see the mojo::Binding<> and from
> that they will receive callbacks.  I think it's reasonable to say that any
code
> that receives a callback object from a mojo::Binding<> must either invoke that
> callback, destroy the mojo::Binding<> or explicitly Close() the
mojo::Binding<>
> before destroying that callback.

OK, that sgtm.

[rudominer, 2015-03-12 22:00:26]

https://codereview.chromium.org/1003773002/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1003773002/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:119:
if (owning_stub_ && !owning_stub_->connection_error_occurred()) {
On 2015/03/12 18:20:04, jamesr wrote:
> On 2015/03/12 16:38:39, rudominer wrote:
> > Question: I don't yet understand the Mojo threading model well enough to
> > understand whether or not this is thread safe.
> 
> The C++ bindings are only usable from a single thread (for the most part), so
> this is fine.  In particular, once a mojo::Binding or mojo::InterfacePtr is
used
> on a particular thread the set of objects created (like the router, connector,
> responder etc) all live on that thread for the rest of their lives.  The
handle
> can only be used on another thread by unbinding it, which tears down all these
> objects, and then rebinding elsewhere.

Great, that comment was very helpful to me, thanks. (The thing I was concerned
about was that owning_stub_ could be deleted by another thread concurrently.) Do
you know if that contract you describe is explicitly written anywhere?

[jamesr, 2015-03-12 22:05:43]

On 2015/03/12 22:00:26, rudominer wrote:
>
https://codereview.chromium.org/1003773002/diff/1/mojo/public/tools/bindings/...
> File
> mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
> (right):
> 
>
https://codereview.chromium.org/1003773002/diff/1/mojo/public/tools/bindings/...
>
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:119:
> if (owning_stub_ && !owning_stub_->connection_error_occurred()) {
> On 2015/03/12 18:20:04, jamesr wrote:
> > On 2015/03/12 16:38:39, rudominer wrote:
> > > Question: I don't yet understand the Mojo threading model well enough to
> > > understand whether or not this is thread safe.
> > 
> > The C++ bindings are only usable from a single thread (for the most part),
so
> > this is fine.  In particular, once a mojo::Binding or mojo::InterfacePtr is
> used
> > on a particular thread the set of objects created (like the router,
connector,
> > responder etc) all live on that thread for the rest of their lives.  The
> handle
> > can only be used on another thread by unbinding it, which tears down all
these
> > objects, and then rebinding elsewhere.
> 
> Great, that comment was very helpful to me, thanks. (The thing I was concerned
> about was that owning_stub_ could be deleted by another thread concurrently.)
Do
> you know if that contract you describe is explicitly written anywhere?

Soooooooort of?  On the InterfacePtr side there's this comment:

https://github.com/domokit/mojo/blob/master/mojo/public/cpp/bindings/interfac...

// This class is thread hostile, as is the local proxy it manages. All calls to
// this class or the proxy should be from the same thread that created it. If
// you need to move the proxy to a different thread, extract the message pipe
// using PassMessagePipe(), pass it to a different thread, and create a new
// InterfacePtr from that thread.

and on the interface_impl.h side (which predates mojo::Binding, but is now
deprecated) there's this:

https://github.com/domokit/mojo/blob/master/mojo/public/cpp/bindings/interfac...

// The instance is also bound to the current thread. Its methods will only be
// called on the current thread, and if the current thread exits, then the end
// point of the pipe will be closed and the error handler's OnConnectionError
// method will be called.

I think we should probably have a comment to this effect in binding.h

[rudominer, 2015-03-12 22:52:14]

https://codereview.chromium.org/1003773002/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1003773002/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:119:
if (owning_stub_ && !owning_stub_->connection_error_occurred()) {
Actually this line is not safe, but not for threading reasons. It is not safe
because the Stub can be deleted before this object in which case owning_stub_ is
a non-Null pointer to a deleted object.

In order to make this safe I need to use a SharedData<Stub*> in the same way
that Router::ResponderThunk keeps a SharedData<Router*> because the Router can
be deleted before the ResponderThunk.

[rudominer, 2015-03-13 01:01:20]

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

[rudominer, 2015-03-13 01:06:58]

James: This is still not ready to submit because of the need for me to use
SharedData<Stub*> rather than Stub* for owning_stub_ in
interface_definition.tmpl. I will do that in the next patchest.

But I wanted you to take a look at the current strategy. You were right: I
managed to get all of the app tests (but one) to pass by capturing when a
Binding is closed or destroyed. After that change the only failing app test
turned out to be a bug in the app test.

Scott: I added you just to look at the fix I made to
view_manager_service_apptest.cc

[sky, 2015-03-13 15:20:50]

services/view_manager/view_manager_service_apptest.cc LGTM

[rudominer, 2015-03-13 23:52:36]

James, I don't know if you have looked at this since my last comment, but if you
haven't then please wait until the next patchset to look again. I really need to
fix the problem with needing to use SharedData<Stub*> rather than Stub* for
owning_stub_ in interface_definition.tmpl or else my change doesn't really make
much sense.

https://codereview.chromium.org/1003773002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/binding.h (right):

https://codereview.chromium.org/1003773002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding.h:103: stub_.Close();
I was confused here. Closing the stub here makes little sense because we are
about to destroy the stub.

[rudominer, 2015-03-15 19:50:10]

James, this is now fully ready for your review. ptal.

[jamesr, 2015-03-16 19:21:39]

Thanks - I haven't had a chance to look in detail yet (was out Friday) but will
today.

[jamesr, 2015-03-16 21:05:57]

Keeping an extra pointer around from the generated responder object seems a bit
unfortunate.  Is this just so the type of the method is printed in the error
message? Is this in the callstack in the case of a DCHECK failure?

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:12: "binding_callback_test.cc",
_unittest.cc, please (we might have integration or performance tests in the
future, for instance)

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/binding_callback_test.cc (right):

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_test.cc:4: #include
"mojo/public/cpp/bindings/interface_ptr.h"
nit: blank line after copyright header

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_test.cc:17: // application
developer. For example it is his responsibility to ensure that
nit: avoid gendered pronouns like "his" in documentation. either go with
something neutral like "they" or rephrase to avoid needing the pronoun

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_test.cc:35: mutable int32_t*
last_value_seen_;
this can be const as the pointer value isn't changed and it is a pointer to a
non-const int32_t

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_test.cc:74: int
last_server_value_seen;
s/int/int32_t/

if you want these to be private since this is a class add a private: declaration
and give these trailing underscores (and getters if needed)

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_test.cc:84:
ScopedMessagePipeHandle handle_client_;
can these have more specific types (i.e. are these handles always used for the
same interface type)?

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl
(right):

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl:4:
virtual ~{{interface.name}}Stub() override;
"virtual" and "override" are redundant, you only want "override" here

[rudominer, 2015-03-16 22:57:20]

PTAL.

Re your comment about it being unfortunate to keep around an extra pointer: If
you are thinking about putting the DCHECK into the ResponderThunk in the Router,
then I think the answer to your question is no, the name of the interface and
method would not be in the call stack.

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:12: "binding_callback_test.cc",
On 2015/03/16 21:05:56, jamesr wrote:
> _unittest.cc, please (we might have integration or performance tests in the
> future, for instance)

Done.

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/binding_callback_test.cc (right):

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_test.cc:4: #include
"mojo/public/cpp/bindings/interface_ptr.h"
On 2015/03/16 21:05:56, jamesr wrote:
> nit: blank line after copyright header

Done.

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_test.cc:17: // application
developer. For example it is his responsibility to ensure that
On 2015/03/16 21:05:57, jamesr wrote:
> nit: avoid gendered pronouns like "his" in documentation. either go with
> something neutral like "they" or rephrase to avoid needing the pronoun

Done.

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_test.cc:35: mutable int32_t*
last_value_seen_;
On 2015/03/16 21:05:57, jamesr wrote:
> this can be const as the pointer value isn't changed and it is a pointer to a
> non-const int32_t

Done.

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_test.cc:74: int
last_server_value_seen;
On 2015/03/16 21:05:56, jamesr wrote:
> s/int/int32_t/
> 
> if you want these to be private since this is a class add a private:
declaration
> and give these trailing underscores (and getters if needed)

Done.

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_test.cc:84:
ScopedMessagePipeHandle handle_client_;
On 2015/03/16 21:05:56, jamesr wrote:
> can these have more specific types (i.e. are these handles always used for the
> same interface type)?

Help me out here. I'm not familiar with any more specific types. What are you
referring to?

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl
(right):

https://codereview.chromium.org/1003773002/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl:4:
virtual ~{{interface.name}}Stub() override;
On 2015/03/16 21:05:57, jamesr wrote:
> "virtual" and "override" are redundant, you only want "override" here

Done.

[jamesr, 2015-03-18 22:48:31]

https://codereview.chromium.org/1003773002/diff/80001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1003773002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:142:
::mojo::internal::SharedData<{{class_name}}Stub*> associated_stub_;
sorry to get caught up on this but something here is a bit strange. on this type
we have a request_id_ and a pointer to a mojo::MessageReceiver* which is a
ResponderThunk which has a weak pointer back to the Router.  It seems like this
should be sufficient information to get back to the stub or every the binding
itself (modulo types) - i.e. by walking through the responder_ chain this code
should be able to determine if the pipe is errored or closed. is that right?

[rudominer, 2015-03-18 23:02:19]

https://codereview.chromium.org/1003773002/diff/80001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1003773002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:142:
::mojo::internal::SharedData<{{class_name}}Stub*> associated_stub_;
On 2015/03/18 22:48:31, jamesr wrote:
> sorry to get caught up on this but something here is a bit strange. on this
type
> we have a request_id_ and a pointer to a mojo::MessageReceiver* which is a
> ResponderThunk which has a weak pointer back to the Router.  It seems like
this
> should be sufficient information to get back to the stub or every the binding
> itself (modulo types) - i.e. by walking through the responder_ chain this code
> should be able to determine if the pipe is errored or closed. is that right?

I agree that we could down cast responder_ to a ResponderThunk* (or change the
type of responder_ to ResponderThunk*) and we could add a method to
ResponderThunk that could consult the Router and determine if the pipe has been
closed or an error occurred. This might be good enough without tracing back to
the stub or binding. I take it you would prefer that I do this?

[jamesr, 2015-03-18 23:14:06]

Yeah - if you can use a more specific type here I think it'd be easier to follow
what is happening.

[rudominer, 2015-03-21 01:57:46]

James, PTAL. I completely changed strategies based on your most recent
suggestion. I got rid of the pointer from the ProxyToResponder object to its
associated Stub, and instead I make use of the existing pointer from the
ProxyToResponder object to the ResponderThunk.

I introduced a new interface MessageReceiverWithStatus and changed
ResponderThunk to implement MessageReceiverWithStatus so that it is possible to
ask it whether or not the MessagePipe has been closed.

It was also necessary to create an alternative MessageReceiverWithResponder that
accepts a responder that is a MessageReceiverWithStatus instead of just a
MessageReceiver. I call this new interface MessageReceiverWithResponderStauts.
(Open to other name suggestions for this.) I then changed the Stub to implement
MessageReceiverWithResponderStatus instead of MessageReceiverWithResponder so
that it could take a MessageRecieverWithStatus in its Accept().

[jamesr, 2015-03-23 18:16:33]

Thanks for all the iterations, I think this looks really nice.  Naming is hard
but I think the 'with status' names are pretty good.

lgtm

https://codereview.chromium.org/1003773002/diff/100001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl
(right):

https://codereview.chromium.org/1003773002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl:10:
mojo::MessageReceiverWithStatus* responder) override;
nit: since you're here (hah!) generating both 'virtual' and 'override' is
unnecessary, this should generate only the 'override' keyword

[rudominer, 2015-03-30 21:00:50]

thanks for the review, submitting.

https://codereview.chromium.org/1003773002/diff/100001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl
(right):

https://codereview.chromium.org/1003773002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl:10:
mojo::MessageReceiverWithStatus* responder) override;
On 2015/03/23 18:16:33, jamesr wrote:
> nit: since you're here (hah!) generating both 'virtual' and 'override' is
> unnecessary, this should generate only the 'override' keyword

Done.

[rudominer, 2015-03-30 21:02:44]

Committed patchset #9 (id:160001) manually as
5f59634fbe6d12a5e31b385281525d6a9aa4e624 (presubmit successful).