Use ConnectionErrorObserver, not callbacks, for error handling.

blimp/net/blimp_connection.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "blimp/net/blimp_connection.h"
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ skipping 73 matching lines @@
   writer_->WritePacket(
       scoped_refptr<net::DrainableIOBuffer>(
           new net::DrainableIOBuffer(buffer_.get(), message->ByteSize())),
       base::Bind(&BlimpMessageSender::OnWritePacketComplete,
                  weak_factory_.GetWeakPtr()));
 }
 
 void BlimpMessageSender::OnWritePacketComplete(int result) {
   DVLOG(2) << "OnWritePacketComplete, result=" << result;
   DCHECK_NE(net::ERR_IO_PENDING, result);
-  base::ResetAndReturn(&pending_process_msg_callback_).Run(result);
+
   if (result != net::OK) {
     error_observer_->OnConnectionError(result);
+    return;

[Wez, 2016/04/21 22:44:29]

This changes the MessageSender to have different semantics to every other
MessageProcessor wrt failures, which seems bogus.

Isn't the problem here that the caller is deleting the connection from within
the context of an observer callback?  i.e. it's doing something other than just
"observing"?

[Kevin M, 2016/04/22 22:10:41]

Isn't it normal for an observer of an object to have side effects, including
destruction of said object?

[Wez, 2016/05/07 22:13:46]

In most cases I'd expect that an observer might tear down other auxiliary
objects, but not the object it's observing; it's common to restrict what
observers are permitted to do.

The alternate, if you want to continue to allow the observer to delete |this|,
would be to move the pending-message callback into a stack variable, call the
observer, then call the pending-message callback from the stack - caller can
Bind() the pending-message callback to a WeakPtr to allow it to become a no-op
in case of the caller having deleted stuff in the observer.

   }
+
+  base::ResetAndReturn(&pending_process_msg_callback_).Run(result);
 }
 
 }  // namespace
 
 BlimpConnection::BlimpConnection(std::unique_ptr<PacketReader> reader,
                                  std::unique_ptr<PacketWriter> writer)
     : reader_(std::move(reader)),
       message_pump_(new BlimpMessagePump(reader_.get())),
-      writer_(std::move(writer)),
-      outgoing_msg_processor_(new BlimpMessageSender(writer_.get())) {
+      writer_(std::move(writer)) {
   DCHECK(writer_);

[Wez, 2016/04/21 22:44:29]

Why do we only DCHECK(writer_), but not reader_?

[Kevin M, 2016/04/22 22:10:41]

Done.

 
-  // Observe the connection errors received by any of this connection's network
-  // objects.
   message_pump_->set_error_observer(this);

[Wez, 2016/04/21 22:44:29]

Seems strange that this observer is set on the pump rather than on the reader,
since we set an observer on the writer below.

[Kevin M, 2016/04/22 22:10:41]

The use of observers is consistent here. The sender is distinct from the writer;
BlimpMessageSender converts BlimpMessage to bytes before pushing them to a
writer.

Likewise, the pump takes bytes from a reader and converts them to BlimpMessages.

[Wez, 2016/05/07 22:13:47]

Acknowledged.

-  BlimpMessageSender* sender =
-      static_cast<BlimpMessageSender*>(outgoing_msg_processor_.get());
+
+  std::unique_ptr<BlimpMessageSender> sender(
+      new BlimpMessageSender(writer_.get()));
   sender->set_error_observer(this);
+  outgoing_msg_processor_ = std::move(sender);
 }
 
 BlimpConnection::BlimpConnection() {}
 
 BlimpConnection::~BlimpConnection() {
   DVLOG(1) << "BlimpConnection destroyed.";
 }
 
 void BlimpConnection::AddConnectionErrorObserver(
     ConnectionErrorObserver* observer) {
@@ skipping 16 matching lines @@
 
 void BlimpConnection::OnConnectionError(int error) {
   VLOG(1) << "OnConnectionError, error=" << error;
 
   // Propagate the error to all observers.
   FOR_EACH_OBSERVER(ConnectionErrorObserver, error_observers_,
                     OnConnectionError(error));
 }
 
 }  // namespace blimp