Create Http2PushPromiseIndex::Delegate.

This CL is in preparation for https://crrev.com/c/734223.

Create Http2PushPromiseIndex::Delegate and thus remove circular dependency
between Http2PushPromiseIndex and SpdySession.

Also, change the internal container type of Http2PushPromiseIndex from
map<vector> to set<pair>.  Later, when
SpdySession::UnclaimedStreamContainer is torn out from SpdySession and
is merged into Http2PushPromiseIndex, there will be multile set<tuple>
containers, storing identical data but sorted by different keys, kept in
sync internally (because fast lookup will be required both by GURL and
by SpdySession*).  set<tuple> will be a lot simpler for this purpose
than map<vector>.

Also, unlike in draft https://crrev.com/c/734223, keep only Delegate raw
pointers and use those to generate the WeakPtr<SpdySession>, instead of
keeping a copy of WeakPtr in addition.  Again, this is to simplify
things.

Bug: 791055
Change-Id: I8c9fcd77c85f3801eb24ca171c66d18ea027965b
Reviewed-on: https://chromium-review.googlesource.com/797438
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521156}

net/spdy/chromium/http2_push_promise_index.cc

@@ -6,8 +6,6 @@
 
 #include <utility>
 
-#include "net/spdy/chromium/spdy_session.h"
-
 namespace net {
 
 Http2PushPromiseIndex::Http2PushPromiseIndex() = default;
@@ -21,73 +19,42 @@ base::WeakPtr<SpdySession> Http2PushPromiseIndex::Find(
     const GURL& url) const {
   DCHECK(!url.is_empty());
 
-  UnclaimedPushedStreamMap::const_iterator url_it =
-      unclaimed_pushed_streams_.find(url);
+  UnclaimedPushedStreamMap::const_iterator it =
+      unclaimed_pushed_streams_.lower_bound(std::make_pair(url, nullptr));
 
-  if (url_it == unclaimed_pushed_streams_.end()) {
+  if (it == unclaimed_pushed_streams_.end() || it->first != url) {
     return base::WeakPtr<SpdySession>();
   }
 
   DCHECK(url.SchemeIsCryptographic());
-  for (const auto& session : url_it->second) {
-    const SpdySessionKey& spdy_session_key = session->spdy_session_key();
-    if (spdy_session_key.proxy_server() != key.proxy_server() ||
-        spdy_session_key.privacy_mode() != key.privacy_mode()) {
-      continue;
-    }
-    if (!session->VerifyDomainAuthentication(key.host_port_pair().host())) {
-      continue;
+  while (it != unclaimed_pushed_streams_.end() && it->first == url) {
+    if (it->second->ValidatePushedStream(key)) {
+      it->second->OnPushedStreamClaimed(url);
+      return it->second->GetWeakPtrToSession();
     }
-    return session;
+    ++it;
   }
 
   return base::WeakPtr<SpdySession>();
 }
 
-void Http2PushPromiseIndex::RegisterUnclaimedPushedStream(
-    const GURL& url,
-    base::WeakPtr<SpdySession> spdy_session) {
+void Http2PushPromiseIndex::RegisterUnclaimedPushedStream(const GURL& url,
+                                                          Delegate* delegate) {
   DCHECK(!url.is_empty());
   DCHECK(url.SchemeIsCryptographic());
 
-  // Use lower_bound() so that if key does not exists, then insertion can use
-  // its return value as a hint.
-  UnclaimedPushedStreamMap::iterator url_it =
-      unclaimed_pushed_streams_.lower_bound(url);
-  if (url_it == unclaimed_pushed_streams_.end() || url_it->first != url) {
-    WeakSessionList list;
-    list.push_back(std::move(spdy_session));
-    UnclaimedPushedStreamMap::value_type value(url, std::move(list));
-    unclaimed_pushed_streams_.insert(url_it, std::move(value));
-    return;
-  }
-  url_it->second.push_back(spdy_session);
+  unclaimed_pushed_streams_.insert(std::make_pair(url, delegate));
 }
 
 void Http2PushPromiseIndex::UnregisterUnclaimedPushedStream(
     const GURL& url,
-    SpdySession* spdy_session) {
+    Delegate* delegate) {
   DCHECK(!url.is_empty());
   DCHECK(url.SchemeIsCryptographic());
 
-  UnclaimedPushedStreamMap::iterator url_it =
-      unclaimed_pushed_streams_.find(url);
-  if (url_it == unclaimed_pushed_streams_.end()) {
-    NOTREACHED();
-    return;
-  }
-  for (WeakSessionList::iterator it = url_it->second.begin();
-       it != url_it->second.end();) {
-    if (it->get() == spdy_session) {
-      url_it->second.erase(it);
-      if (url_it->second.empty()) {
-        unclaimed_pushed_streams_.erase(url_it);
-      }
-      return;
-    }
-    ++it;
-  }
-  NOTREACHED();
+  size_t result =
+      unclaimed_pushed_streams_.erase(std::make_pair(url, delegate));
+  DCHECK_EQ(1u, result);
 }
 
 }  // namespace net

net/spdy/chromium/http2_push_promise_index.h

@@ -5,8 +5,7 @@
 #ifndef NET_SPDY_CHROMIUM_HTTP2_PUSH_PROMISE_INDEX_H_
 #define NET_SPDY_CHROMIUM_HTTP2_PUSH_PROMISE_INDEX_H_
 
-#include <map>
-#include <vector>
+#include <set>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -26,31 +25,53 @@ class SpdySession;
 // Only pushed streams with cryptographic schemes (for example, https) are
 // allowed to be shared across connections.  Non-cryptographic scheme pushes
 // (for example, http) are fully managed within each SpdySession.
+// TODO(bnc): Move unclaimed pushed stream management out of SpdySession,
+// regardless of scheme, to avoid redundant bookkeeping and complicated
+// interactions between SpdySession::UnclaimedPushedStreamContainer and
+// Http2PushPromiseIndex.  https://crbug.com/791054.
 class NET_EXPORT Http2PushPromiseIndex {
  public:
+  // Interface for validating pushed streams, signaling when a pushed stream is
+  // claimed, and generating weak pointer.
+  class NET_EXPORT Delegate {
+   public:
+    Delegate() {}
+    virtual ~Delegate() {}
+
+    // Return true if the pushed stream can be used for a request with |key|.
+    virtual bool ValidatePushedStream(const SpdySessionKey& key) const = 0;
+
+    // Called when a pushed stream is claimed.
+    virtual void OnPushedStreamClaimed(const GURL& url) = 0;
+
+    // Generate weak pointer.
+    virtual base::WeakPtr<SpdySession> GetWeakPtrToSession() = 0;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Delegate);
+  };
+
   Http2PushPromiseIndex();
   ~Http2PushPromiseIndex();
 
   // Returns a session with |key| that has an unclaimed push stream for |url| if
-  // such exists.  Returns nullptr otherwise.
+  // such exists.  Makes no guarantee on which one it returns if there are
+  // multiple.  Returns nullptr if no such session exists.
   base::WeakPtr<SpdySession> Find(const SpdySessionKey& key,
                                   const GURL& url) const;
 
-  // (Un)registers a SpdySession with an unclaimed pushed stream for |url|.
-  void RegisterUnclaimedPushedStream(const GURL& url,
-                                     base::WeakPtr<SpdySession> spdy_session);
-  void UnregisterUnclaimedPushedStream(const GURL& url,
-                                       SpdySession* spdy_session);
+  // (Un)registers a Delegate with an unclaimed pushed stream for |url|.
+  // Caller must make sure |delegate| stays valid by unregistering the exact
+  // same entry before |delegate| is destroyed.
+  void RegisterUnclaimedPushedStream(const GURL& url, Delegate* delegate);
+  void UnregisterUnclaimedPushedStream(const GURL& url, Delegate* delegate);
 
  private:
-  typedef std::vector<base::WeakPtr<SpdySession>> WeakSessionList;
-  typedef std::map<GURL, WeakSessionList> UnclaimedPushedStreamMap;
-
-  // A multimap of all SpdySessions that have an unclaimed pushed stream for a
-  // GURL.  SpdySession must unregister its streams before destruction,
-  // therefore all weak pointers must be valid.  A single SpdySession can only
-  // have at most one pushed stream for each GURL, but it is possible that
-  // multiple SpdySessions have pushed streams for the same GURL.
+  using UnclaimedPushedStreamMap = std::set<std::pair<GURL, Delegate*>>;
+
+  // A collection of all unclaimed pushed streams.  Delegate must unregister its
+  // streams before destruction, so that all pointers remain valid.  It is
+  // possible that multiple Delegates have pushed streams for the same GURL.
   UnclaimedPushedStreamMap unclaimed_pushed_streams_;
 
   DISALLOW_COPY_AND_ASSIGN(Http2PushPromiseIndex);

net/spdy/chromium/http2_push_promise_index_test.cc

@@ -73,12 +73,12 @@ TEST_F(Http2PushPromiseIndexTest, FindMultipleSessionsWithDifferentUrl) {
   EXPECT_FALSE(index_.Find(key1_, url1_));
   EXPECT_FALSE(index_.Find(key2_, url2_));
 
-  index_.RegisterUnclaimedPushedStream(url1_, spdy_session1);
+  index_.RegisterUnclaimedPushedStream(url1_, spdy_session1.get());
 
   EXPECT_EQ(spdy_session1.get(), index_.Find(key1_, url1_).get());
   EXPECT_FALSE(index_.Find(key2_, url2_));
 
-  index_.RegisterUnclaimedPushedStream(url2_, spdy_session2);
+  index_.RegisterUnclaimedPushedStream(url2_, spdy_session2.get());
 
   EXPECT_EQ(spdy_session1.get(), index_.Find(key1_, url1_).get());
   EXPECT_EQ(spdy_session2.get(), index_.Find(key2_, url2_).get());
@@ -131,7 +131,7 @@ TEST_F(Http2PushPromiseIndexTest, MultipleSessionsForSingleUrl) {
   EXPECT_FALSE(index_.Find(key1_, url2_));
   EXPECT_FALSE(index_.Find(key2_, url2_));
 
-  index_.RegisterUnclaimedPushedStream(url1_, spdy_session1);
+  index_.RegisterUnclaimedPushedStream(url1_, spdy_session1.get());
 
   // Note that Find() only uses its SpdySessionKey argument to verify proxy and
   // privacy mode.  Cross-origin pooling is supported, therefore HostPortPair of
@@ -141,11 +141,14 @@ TEST_F(Http2PushPromiseIndexTest, MultipleSessionsForSingleUrl) {
   EXPECT_FALSE(index_.Find(key1_, url2_));
   EXPECT_FALSE(index_.Find(key2_, url2_));
 
-  index_.RegisterUnclaimedPushedStream(url1_, spdy_session2);
+  index_.RegisterUnclaimedPushedStream(url1_, spdy_session2.get());
 
-  // Find returns the first SpdySession if there are multiple for the same URL.
-  EXPECT_EQ(spdy_session1.get(), index_.Find(key1_, url1_).get());
-  EXPECT_EQ(spdy_session1.get(), index_.Find(key2_, url1_).get());
+  // Find() makes no guarantee about which SpdySession it returns if there are
+  // multiple for the same URL.
+  SpdySession* result = index_.Find(key1_, url1_).get();
+  EXPECT_TRUE(result == spdy_session1.get() || result == spdy_session2.get());
+  result = index_.Find(key2_, url1_).get();
+  EXPECT_TRUE(result == spdy_session1.get() || result == spdy_session2.get());
   EXPECT_FALSE(index_.Find(key1_, url2_));
   EXPECT_FALSE(index_.Find(key2_, url2_));
 

net/spdy/chromium/spdy_session.cc

@@ -912,7 +912,7 @@ void SpdySession::InitializeWithSocket(
                  READ_STATE_DO_READ, OK));
 }
 
-bool SpdySession::VerifyDomainAuthentication(const SpdyString& domain) {
+bool SpdySession::VerifyDomainAuthentication(const SpdyString& domain) const {
   if (availability_state_ == STATE_DRAINING)
     return false;
 
@@ -1352,6 +1352,28 @@ bool SpdySession::CloseOneIdleConnection() {
   return false;
 }
 
+bool SpdySession::ValidatePushedStream(const SpdySessionKey& key) const {
+  return key.proxy_server() == spdy_session_key_.proxy_server() &&
+         key.privacy_mode() == spdy_session_key_.privacy_mode() &&
+         VerifyDomainAuthentication(key.host_port_pair().host());
+}
+
+void SpdySession::OnPushedStreamClaimed(const GURL& url) {
+  UnclaimedPushedStreamContainer::const_iterator unclaimed_it =
+      unclaimed_pushed_streams_.find(url);
+  // This is only possible in tests.
+  // TODO(bnc): Change to DCHECK once Http2PushPromiseIndexTest stops using
+  // actual SpdySession instances.  https://crbug.com/791055.
+  if (unclaimed_it == unclaimed_pushed_streams_.end())
+    return;
+
+  LogPushStreamClaimed(url, unclaimed_it->second);
+}
+
+base::WeakPtr<SpdySession> SpdySession::GetWeakPtrToSession() {
+  return GetWeakPtr();
+}
+
 size_t SpdySession::DumpMemoryStats(StreamSocket::SocketMemoryStats* stats,
                                     bool* is_session_active) const {
   // TODO(xunjieli): Include |pending_create_stream_queues_| when WeakPtr is
@@ -1415,7 +1437,7 @@ bool SpdySession::UnclaimedPushedStreamContainer::insert(
   // Only allow cross-origin push for https resources.
   if (url.SchemeIsCryptographic()) {
     spdy_session_->pool_->push_promise_index()->RegisterUnclaimedPushedStream(
-        url, spdy_session_->GetWeakPtr());
+        url, spdy_session_);
   }
   return true;
 }
@@ -2432,11 +2454,9 @@ SpdyStream* SpdySession::GetActivePushStream(const GURL& url) {
     return nullptr;
   }
 
-  SpdyStream* stream = active_it->second;
-  net_log_.AddEvent(
-      NetLogEventType::HTTP2_STREAM_ADOPTED_PUSH_STREAM,
-      base::Bind(&NetLogSpdyAdoptedPushStreamCallback, stream_id, &url));
-  return stream;
+  LogPushStreamClaimed(url, stream_id);
+
+  return active_it->second;
 }
 
 void SpdySession::RecordPingRTTHistogram(base::TimeDelta duration) {
@@ -2553,6 +2573,13 @@ void SpdySession::LogAbandonedStream(SpdyStream* stream, Error status) {
   // LogAbandonedActiveStream() will increment the counters.
 }
 
+void SpdySession::LogPushStreamClaimed(const GURL& url,
+                                       SpdyStreamId stream_id) {
+  net_log_.AddEvent(
+      NetLogEventType::HTTP2_STREAM_ADOPTED_PUSH_STREAM,
+      base::Bind(&NetLogSpdyAdoptedPushStreamCallback, stream_id, &url));
+}
+
 void SpdySession::LogAbandonedActiveStream(ActiveStreamMap::const_iterator it,
                                            Error status) {
   DCHECK_GT(it->first, 0u);

net/spdy/chromium/spdy_session.h

@@ -234,7 +234,8 @@ class NET_EXPORT_PRIVATE SpdyStreamRequest {
 class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
                                public SpdyFramerDebugVisitorInterface,
                                public MultiplexedSession,
-                               public HigherLayeredPool {
+                               public HigherLayeredPool,
+                               public Http2PushPromiseIndex::Delegate {
  public:
   // TODO(akalin): Use base::TickClock when it becomes available.
   typedef base::TimeTicks (*TimeFunc)(void);
@@ -316,7 +317,7 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
   // TODO(wtc): rename this function and the Net.SpdyIPPoolDomainMatch
   // histogram because this function does more than verifying domain
   // authentication now.
-  bool VerifyDomainAuthentication(const SpdyString& domain);
+  bool VerifyDomainAuthentication(const SpdyString& domain) const;
 
   // Pushes the given producer into the write queue for
   // |stream|. |stream| is guaranteed to be activated before the
@@ -493,12 +494,17 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
   // standards for TLS.
   bool HasAcceptableTransportSecurity() const;
 
-  // Must be used only by |pool_|.
+  // Must be used only by |pool_| (including |pool_.push_promise_index_|).
   base::WeakPtr<SpdySession> GetWeakPtr();
 
   // HigherLayeredPool implementation:
   bool CloseOneIdleConnection() override;
 
+  // Http2PushPromiseIndex::Delegate implementation:
+  bool ValidatePushedStream(const SpdySessionKey& key) const override;
+  void OnPushedStreamClaimed(const GURL& url) override;
+  base::WeakPtr<SpdySession> GetWeakPtrToSession() override;
+
   // Dumps memory allocation stats to |stats|. Sets |*is_session_active| to
   // indicate whether session is active.
   // |stats| can be assumed as being default initialized upon entry.
@@ -822,6 +828,9 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
   // reason other than being requested to by the stream.
   void LogAbandonedStream(SpdyStream* stream, Error status);
 
+  // Called when a pushed stream is claimed by a request.
+  void LogPushStreamClaimed(const GURL& url, SpdyStreamId stream_id);
+
   // Called right before closing an active stream for a reason other
   // than being requested to by the stream.
   void LogAbandonedActiveStream(ActiveStreamMap::const_iterator it,