Connection error callback doesn't get called if Connector::BindInterface fails |
|||||
Issue description
If you try to bind to a service interface which has never been registered on the Connector (e.g. because you registered your service's interface on the wrong Connector instance) binding fails but the connection error callback never gets called.
Given this (unsubmitted) code:
prefs::mojom::PrefStoreRegistryPtr registry_ptr;
connector->BindInterface(prefs::mojom::kPrefStoreServiceName, ®istry_ptr);
registry_ptr.set_connection_error_handler(base::Bind([]() {
LOG(INFO) << "registry_ptr connection error!";
}));
// Call methods on |registry_ptr|.
I'd expect the binding to either work, allowing me to call methods on |registry_ptr|, or the connection error handler to be called. Instead the service manger seems to hold on to the pipe forever and thus we're left in limbo.
Adding debug logging it seems like the SM does know that the connection failed, because the control flow, starting in BindInterface, eventually falls off the end of this function:
void ServiceManagerConnectionImpl::CreateService(
service_manager::mojom::ServiceRequest request,
const std::string& name) {
auto it = request_handlers_.find(name);
if (it != request_handlers_.end())
it->second.Run(std::move(request));
}
(i.e. there was no handler so nothing will ever happen.)
,
Feb 21 2017
The issue is that adding a connection error handler has no effect if peer closure has already been detected. I'm not sure what the best solution is here, but I don't think it should have anything to do with Connector specifically.
,
Feb 22 2017
Should adding CHECK(!registry_ptr.encountered_error()); just before the call to |set_connection_error_handler| handle that case? If so it doesn't when I tested it just now.
,
Feb 22 2017
Yes, that should be sufficient. Otherwise that means either the peer is being closed and we're never detecting it, or the peer is never closed. Either one would be quite surprising.
,
Feb 22 2017
Note that ServiceManagerConnectionImpl::CreateService dropping the request means that the ServiceRequest's handle is closed. Any pending message on that pipe (e.g. the OnBindInterfaceRequest resulting from your connector call) will be dropped and any contained message pipe handles (e.g. your PrefStoreRegistryRequest's handle) should in turn be closed. If any of these things isn't happening, something major has broken.
,
Feb 22 2017
I dumped a repro case (part of a larger CL) in https://codereview.chromium.org/2706383002. If you check the `chrome --enable-logging=stderr` you can see [5848:5848:0222/111400.808217:INFO:service_manager_connection_impl.cc(526)] no request handler found and all the things that the service manager did up to that point.
,
Feb 22 2017
After some more discussion/testing we think that the error connection not being called is working as intended, in the sense that Mojo was designed to work this way if you delete |registry_ptr| after sending the message. A bit debugging unfriendly, but working as intended. That being said, we agreed to add a DCHECK at the end of ServiceManagerConnectionImpl::CreateService because if we can't find a handler to create the service, something probably went wrong. I will create a CL.
,
Feb 22 2017
,
Feb 22 2017
RE #7: > After some more discussion/testing we think that the error connection not being called is working as intended, in the sense that Mojo was designed to work this way if > you delete |registry_ptr| after sending the message. A bit debugging unfriendly, but working as intended. Would you please explain that it is working as intended? To me, it seems more consistent to change the behavior so that (1) connection error handler will be called if the pipe has been disconnected; (2) setting a new connection error handler will also be called, even if a previous error handler has been set and has been called. Btw, this bug reminds me that I have some cleanup work of removing encountered_error() to do. (crbug.com/613371)
,
Feb 22 2017
A little clarification about #9: The side that initiates the disconnection (for example, InterfacePtr::reset()) won't get the connection error handler called.
,
Feb 22 2017
Johan and I had an offline chat. He explained to me the issue was just that we let the interface ptr got out of scope and therefore the error callback registered on it had no effect. So my commment #9 is an irrelevant issue. Thanks for the clarification, Johan!
,
Feb 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/afa4a1208478bdad5d47151e1248eb14571aa942 commit afa4a1208478bdad5d47151e1248eb14571aa942 Author: tibell <tibell@chromium.org> Date: Thu Feb 23 01:59:25 2017 Add DCHECK when service handler is missing At this point we've already verified that the service exists and that the client is allowed to connect, so failure to create the service indicates a bug in its setup (i.e. it wasn't registered properly). BUG= 694807 Review-Url: https://codereview.chromium.org/2716513002 Cr-Commit-Position: refs/heads/master@{#452344} [modify] https://crrev.com/afa4a1208478bdad5d47151e1248eb14571aa942/content/common/service_manager/service_manager_connection_impl.cc
,
Feb 23 2017
,
Nov 1 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tibell@chromium.org
, Feb 21 2017