Add a new QuicChromiumClientSession::Handle class

net/quic/chromium/quic_chromium_client_session.cc

Index: net/quic/chromium/quic_chromium_client_session.cc
diff --git a/net/quic/chromium/quic_chromium_client_session.cc b/net/quic/chromium/quic_chromium_client_session.cc
index cd123b19de48d537ea0477a194e63ba669259869..6b77af15792655e5180f8aba19586bcfbb5c9032 100644
--- a/net/quic/chromium/quic_chromium_client_session.cc
+++ b/net/quic/chromium/quic_chromium_client_session.cc
@@ -186,26 +186,182 @@ class QuicServerPushHelper : public ServerPushDelegate::ServerPushHelper {
 
 }  // namespace
 
+QuicChromiumClientSession::Handle::Handle(
+    const base::WeakPtr<QuicChromiumClientSession>& session)
+    : MultiplexedSessionHandle(session),
+      session_(session),
+      net_log_(session_->net_log()),
+      was_handshake_confirmed_(session->IsCryptoHandshakeConfirmed()),
+      error_(OK),
+      port_migration_detected_(false),
+      server_id_(session_->server_id()),
+      quic_version_(session->connection()->version()) {
+  DCHECK(session_);
+  session_->AddHandle(this);
+}
+
+QuicChromiumClientSession::Handle::Handle(const Handle& other)
+    : MultiplexedSessionHandle(other),
+      session_(other.session_),
+      net_log_(other.net_log_),
+      was_handshake_confirmed_(other.was_handshake_confirmed_),
+      error_(other.error_),
+      port_migration_detected_(other.port_migration_detected_),
+      server_id_(other.server_id_),
+      quic_version_(other.quic_version_) {
+  if (session_)
+    session_->AddHandle(this);
+}
+
+QuicChromiumClientSession::Handle::~Handle() {
+  if (session_)
+    session_->RemoveHandle(this);
+}
+
+void QuicChromiumClientSession::Handle::OnCryptoHandshakeConfirmed() {
+  was_handshake_confirmed_ = true;
+}
+
+void QuicChromiumClientSession::Handle::OnSessionClosed(
+    int error,
+    bool port_migration_detected) {
+  port_migration_detected_ = port_migration_detected;
+  error_ = error;
+  quic_version_ = session_->connection()->version();
+  connect_timing_ = session_->GetConnectTiming();
+  session_ = nullptr;
+}
+
+bool QuicChromiumClientSession::Handle::IsConnected() const {
+  return session_ != nullptr;
+}
+
+bool QuicChromiumClientSession::Handle::IsCryptoHandshakeConfirmed() const {
+  return was_handshake_confirmed_;
+}
+
+const LoadTimingInfo::ConnectTiming&
+QuicChromiumClientSession::Handle::GetConnectTiming() {
+  if (!session_)
+    return connect_timing_;
+
+  return session_->GetConnectTiming();
+}
+
+Error QuicChromiumClientSession::Handle::GetTokenBindingSignature(
+    crypto::ECPrivateKey* key,
+    TokenBindingType tb_type,
+    std::vector<uint8_t>* out) {
+  if (!session_)
+    return ERR_CONNECTION_CLOSED;
+
+  return session_->GetTokenBindingSignature(key, tb_type, out);
+}
+
+void QuicChromiumClientSession::Handle::PopulateNetErrorDetails(
+    NetErrorDetails* details) const {
+  if (session_) {
+    session_->PopulateNetErrorDetails(details);
+  } else {
+    details->quic_port_migration_detected = port_migration_detected_;
+  }
+}
+
+QuicVersion QuicChromiumClientSession::Handle::GetQuicVersion() const {
+  if (!session_)
+    return quic_version_;
+
+  return session_->connection()->version();
+}
+
+void QuicChromiumClientSession::Handle::ResetPromised(
+    QuicStreamId id,
+    QuicRstStreamErrorCode error_code) {
+  if (session_)
+    session_->ResetPromised(id, error_code);
+}
+
+std::unique_ptr<QuicConnection::ScopedPacketBundler>
+QuicChromiumClientSession::Handle::CreatePacketBundler(
+    QuicConnection::AckBundling bundling_mode) {
+  if (!session_)
+    return nullptr;
+
+  return base::MakeUnique<QuicConnection::ScopedPacketBundler>(
+      session_->connection(), bundling_mode);
+}
+
+bool QuicChromiumClientSession::Handle::SharesSameSession(
+    const Handle& other) const {
+  return session_.get() == other.session_.get();
+}
+
+std::unique_ptr<QuicChromiumClientSession::StreamRequest>
+QuicChromiumClientSession::Handle::CreateStreamRequest(
+    bool requires_confirmation) const {
+  // base::MakeUnique does not work because the StreamRequest constructor
+  // is private.
+  return std::unique_ptr<StreamRequest>(
+      new StreamRequest(*this, requires_confirmation));
+}
+
+int QuicChromiumClientSession::Handle::WaitForHandshakeConfirmation(
+    const CompletionCallback& callback) {
+  if (!session_)
+    return ERR_CONNECTION_CLOSED;
+
+  return session_->WaitForHandshakeConfirmation(callback);
+}
+
+void QuicChromiumClientSession::Handle::CancelRequest(StreamRequest* request) {
+  if (session_)
+    session_->CancelRequest(request);
+}
+
+int QuicChromiumClientSession::Handle::TryCreateStream(StreamRequest* request) {
+  if (!session_)
+    return ERR_CONNECTION_CLOSED;
+
+  return session_->TryCreateStream(request);
+}
+
+QuicClientPushPromiseIndex*
+QuicChromiumClientSession::Handle::GetPushPromiseIndex() {
+  if (!session_)
+    return push_promise_index_;
+
+  return session_->push_promise_index();
+}
+
+int QuicChromiumClientSession::Handle::GetPeerAddress(
+    IPEndPoint* address) const {
+  if (!session_)
+    return ERR_CONNECTION_CLOSED;
+
+  *address = session_->peer_address().impl().socket_address();
+  return OK;
+}
+
 QuicChromiumClientSession::StreamRequest::StreamRequest(
-    const base::WeakPtr<QuicChromiumClientSession>& session,
+    QuicChromiumClientSession::Handle session,
     bool requires_confirmation)
     : session_(session),
       requires_confirmation_(requires_confirmation),
       stream_(nullptr),
-      next_state_(STATE_NONE),
       weak_factory_(this) {}
 
 QuicChromiumClientSession::StreamRequest::~StreamRequest() {
   if (stream_)
     stream_->Reset(QUIC_STREAM_CANCELLED);
 
-  if (session_)
-    session_->CancelRequest(this);
+  if (session_.IsConnected())
+    session_.CancelRequest(this);
 }
 
 int QuicChromiumClientSession::StreamRequest::StartRequest(
     const CompletionCallback& callback) {
-  DCHECK(session_);
+  if (!session_.IsConnected())
+    return ERR_CONNECTION_CLOSED;
 
   next_state_ = STATE_WAIT_FOR_CONFIRMATION;
   int rv = DoLoop(OK);
@@ -226,7 +382,7 @@ QuicChromiumClientSession::StreamRequest::ReleaseStream() {
 void QuicChromiumClientSession::StreamRequest::OnRequestCompleteSuccess(
     QuicChromiumClientStream* stream) {
   DCHECK_EQ(STATE_REQUEST_STREAM_COMPLETE, next_state_);
-  session_.reset();
+
   stream_ = stream;
   // This method is called even when the request completes synchronously.
   if (callback_)
@@ -236,7 +392,6 @@ void QuicChromiumClientSession::StreamRequest::OnRequestCompleteSuccess(
 void QuicChromiumClientSession::StreamRequest::OnRequestCompleteFailure(
     int rv) {
   DCHECK_EQ(STATE_REQUEST_STREAM_COMPLETE, next_state_);
-  session_.reset();
   // This method is called even when the request completes synchronously.
   if (callback_)
     DoCallback(rv);
@@ -290,7 +445,7 @@ int QuicChromiumClientSession::StreamRequest::DoLoop(int rv) {
 int QuicChromiumClientSession::StreamRequest::DoWaitForConfirmation() {
   next_state_ = STATE_WAIT_FOR_CONFIRMATION_COMPLETE;
   if (requires_confirmation_) {
-    return session_->WaitForHandshakeConfirmation(
+    return session_.WaitForHandshakeConfirmation(
         base::Bind(&QuicChromiumClientSession::StreamRequest::OnIOComplete,
                    weak_factory_.GetWeakPtr()));
   }
@@ -311,12 +466,11 @@ int QuicChromiumClientSession::StreamRequest::DoWaitForConfirmationComplete(
 int QuicChromiumClientSession::StreamRequest::DoRequestStream() {
   next_state_ = STATE_REQUEST_STREAM_COMPLETE;
 
-  return session_->TryCreateStream(this);
+  return session_.TryCreateStream(this);
 }
 
 int QuicChromiumClientSession::StreamRequest::DoRequestStreamComplete(int rv) {
   DCHECK(rv == OK || !stream_);
-  session_.reset();
 
   return rv;
 }
@@ -377,6 +531,7 @@ QuicChromiumClientSession::QuicChromiumClientSession(
           server_id, this, base::MakeUnique<ProofVerifyContextChromium>(
                                cert_verify_flags, net_log_),
           crypto_config));
+  self_handle_ = base::MakeUnique<Handle>(weak_factory_.GetWeakPtr());
   connection->set_debug_visitor(logger_.get());
   connection->set_creator_debug_delegate(logger_.get());
   net_log_.BeginEvent(NetLogEventType::QUIC_SESSION,
@@ -399,18 +554,18 @@ QuicChromiumClientSession::~QuicChromiumClientSession() {
   DCHECK(waiting_for_confirmation_callbacks_.empty());
   if (!dynamic_streams().empty())
     RecordUnexpectedOpenStreams(DESTRUCTOR);
-  if (!observers_.empty())
+  if (!handles_.empty())
     RecordUnexpectedObservers(DESTRUCTOR);
   if (!going_away_)
     RecordUnexpectedNotGoingAway(DESTRUCTOR);
 
-  while (!dynamic_streams().empty() || !observers_.empty() ||
+  while (!dynamic_streams().empty() || !handles_.empty() ||
          !stream_requests_.empty()) {
     // The session must be closed before it is destroyed.
     DCHECK(dynamic_streams().empty());
     CloseAllStreams(ERR_UNEXPECTED);
-    DCHECK(observers_.empty());
-    CloseAllObservers(ERR_UNEXPECTED);
+    DCHECK(handles_.empty());
+    CloseAllHandles(ERR_UNEXPECTED);
     CancelAllRequests(ERR_UNEXPECTED);
 
     connection()->set_debug_visitor(nullptr);
@@ -528,28 +683,20 @@ void QuicChromiumClientSession::OnStreamFrame(const QuicStreamFrame& frame) {
   return QuicSpdySession::OnStreamFrame(frame);
 }
 
-void QuicChromiumClientSession::AddObserver(Observer* observer) {
+void QuicChromiumClientSession::AddHandle(Handle* handle) {
   if (going_away_) {
     RecordUnexpectedObservers(ADD_OBSERVER);
-    observer->OnSessionClosed(ERR_UNEXPECTED, port_migration_detected_);
+    handle->OnSessionClosed(ERR_UNEXPECTED, port_migration_detected_);
     return;
   }
 
-  DCHECK(!base::ContainsKey(observers_, observer));
-  observers_.insert(observer);
+  DCHECK(!base::ContainsKey(handles_, handle));
+  handles_.insert(handle);
 }
 
-void QuicChromiumClientSession::RemoveObserver(Observer* observer) {
-  DCHECK(base::ContainsKey(observers_, observer));
-  observers_.erase(observer);
-}
-
-std::unique_ptr<QuicChromiumClientSession::StreamRequest>
-QuicChromiumClientSession::CreateStreamRequest(bool requires_confirmation) {
-  // base::MakeUnique does not work because the StreamRequest constructor
-  // is private.
-  return std::unique_ptr<StreamRequest>(
-      new StreamRequest(weak_factory_.GetWeakPtr(), requires_confirmation));
+void QuicChromiumClientSession::RemoveHandle(Handle* handle) {
+  DCHECK(base::ContainsKey(handles_, handle));
+  handles_.erase(handle);
 }
 
 int QuicChromiumClientSession::WaitForHandshakeConfirmation(
@@ -960,11 +1107,11 @@ void QuicChromiumClientSession::OnCryptoHandshakeEvent(
           base::TimeTicks::Now() - connect_timing_.dns_end);
     }
 
-    ObserverSet::iterator it = observers_.begin();
-    while (it != observers_.end()) {
-      Observer* observer = *it;
+    HandleSet::iterator it = handles_.begin();
+    while (it != handles_.end()) {
+      Handle* handle = *it;
       ++it;
-      observer->OnCryptoHandshakeConfirmed();
+      handle->OnCryptoHandshakeConfirmed();
     }
 
     NotifyRequestsOfConfirmation(OK);
@@ -1108,7 +1255,7 @@ void QuicChromiumClientSession::OnConnectionClosed(
   }
   DCHECK(dynamic_streams().empty());
   CloseAllStreams(ERR_UNEXPECTED);
-  CloseAllObservers(ERR_UNEXPECTED);
+  CloseAllHandles(ERR_UNEXPECTED);
   CancelAllRequests(ERR_CONNECTION_CLOSED);
   NotifyRequestsOfConfirmation(ERR_CONNECTION_CLOSED);
   NotifyFactoryOfSessionClosedLater();
@@ -1118,13 +1265,6 @@ void QuicChromiumClientSession::OnSuccessfulVersionNegotiation(
     const QuicVersion& version) {
   logger_->OnSuccessfulVersionNegotiation(version);
   QuicSpdySession::OnSuccessfulVersionNegotiation(version);
-
-  ObserverSet::iterator it = observers_.begin();
-  while (it != observers_.end()) {
-    Observer* observer = *it;
-    ++it;
-    observer->OnSuccessfulVersionNegotiation(version);
-  }
 }
 
 int QuicChromiumClientSession::HandleWriteError(
@@ -1330,7 +1470,7 @@ void QuicChromiumClientSession::CloseSessionOnError(int net_error,
     base::ResetAndReturn(&callback_).Run(net_error);
   }
   CloseAllStreams(net_error);
-  CloseAllObservers(net_error);
+  CloseAllHandles(net_error);
   net_log_.AddEvent(NetLogEventType::QUIC_SESSION_CLOSE_ON_ERROR,
                     NetLog::IntCallback("net_error", net_error));
 
@@ -1351,11 +1491,11 @@ void QuicChromiumClientSession::CloseAllStreams(int net_error) {
   }
 }
 
-void QuicChromiumClientSession::CloseAllObservers(int net_error) {
-  while (!observers_.empty()) {
-    Observer* observer = *observers_.begin();
-    observers_.erase(observer);
-    observer->OnSessionClosed(net_error, port_migration_detected_);
+void QuicChromiumClientSession::CloseAllHandles(int net_error) {
+  while (!handles_.empty()) {
+    Handle* handle = *handles_.begin();
+    handles_.erase(handle);
+    handle->OnSessionClosed(net_error, port_migration_detected_);
   }
 }
 
@@ -1410,9 +1550,8 @@ std::unique_ptr<base::Value> QuicChromiumClientSession::GetInfoAsValue(
   return std::move(dict);
 }
 
-base::WeakPtr<QuicChromiumClientSession>
-QuicChromiumClientSession::GetWeakPtr() {
-  return weak_factory_.GetWeakPtr();
+QuicChromiumClientSession::Handle QuicChromiumClientSession::GetHandle() {
+  return *self_handle_.get();

[xunjieli, 2017/05/04 16:54:42]

So this makes a new Handle?  This is quite subtle.
I suggest we make the creation of a new Handle more explicit.
We could get rid of |self_handle_|, and explicitly create a new one each time.

[Ryan Hamilton, 2017/05/05 03:50:24]

Done.

 }
 
 void QuicChromiumClientSession::OnReadError(
@@ -1508,7 +1647,7 @@ bool QuicChromiumClientSession::MigrateToSocket(
 }
 
 void QuicChromiumClientSession::PopulateNetErrorDetails(
-    NetErrorDetails* details) {
+    NetErrorDetails* details) const {
   details->quic_port_migration_detected = port_migration_detected_;
 }