Use ConnectionErrorObserver, not callbacks, for error handling.

blimp/net/blimp_connection.cc

Index: blimp/net/blimp_connection.cc
diff --git a/blimp/net/blimp_connection.cc b/blimp/net/blimp_connection.cc
index 4b48f773d488d97ecd85b3a0673b198ffc5260bf..fd5eb7a924ef7cb4a11c23bfd2299d26e06c26ca 100644
--- a/blimp/net/blimp_connection.cc
+++ b/blimp/net/blimp_connection.cc
@@ -91,10 +91,13 @@ void BlimpMessageSender::ProcessMessage(
 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
@@ -103,16 +106,15 @@ 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() {}