Makes migration on write error asynchronous to avoid reentrancy issues

net/quic/chromium/quic_chromium_client_session.h

Index: net/quic/chromium/quic_chromium_client_session.h
diff --git a/net/quic/chromium/quic_chromium_client_session.h b/net/quic/chromium/quic_chromium_client_session.h
index b51fc440d57c5cfafe5e76936bb6f87e0088f1a7..fc4a3bdae64a056290a0e32a1605380661aa1988 100644
--- a/net/quic/chromium/quic_chromium_client_session.h
+++ b/net/quic/chromium/quic_chromium_client_session.h
@@ -163,6 +163,7 @@ class NET_EXPORT_PRIVATE QuicChromiumClientSession
                        scoped_refptr<StringIOBuffer> last_packet) override;
   void OnWriteError(int error_code) override;
   void OnWriteUnblocked() override;
+  bool ShouldWriteBlock() override;
 
   // QuicSpdySession methods:
   void OnHeadersHeadOfLineBlocking(QuicTime::Delta delta) override;
@@ -253,19 +254,25 @@ class NET_EXPORT_PRIVATE QuicChromiumClientSession
 
   QuicDisabledReason disabled_reason() const { return disabled_reason_; }
 
+  // Attempts to migrate session when a write error is encountered.
+  void MigrateSessionOnWriteError();
+
+  // Helper method that writes a packet on the new socket after
+  // migration completes. If not null, the packet_ member is written,
+  // else a PING packet is written.
+  void WriteToNewSocket();
+
   // Migrates session onto new socket, i.e., starts reading from
   // |socket| in addition to any previous sockets, and sets |writer|
   // to be the new default writer. Returns true if socket was
   // successfully added to the session and the session was
-  // successfully migrated to using the new socket. If not null,
-  // |packet| is sent on the new network, else a PING frame is
-  // sent. Returns true on successful migration, or false if number of
-  // migrations exceeds kMaxReadersPerQuicSession.  Takes ownership of
-  // |socket|, |reader|, and |writer|.
+  // successfully migrated to using the new socket. Returns true on
+  // successful migration, or false if number of migrations exceeds
+  // kMaxReadersPerQuicSession. Takes ownership of |socket|, |reader|,
+  // and |writer|.
   bool MigrateToSocket(std::unique_ptr<DatagramClientSocket> socket,
                        std::unique_ptr<QuicChromiumPacketReader> reader,
-                       std::unique_ptr<QuicChromiumPacketWriter> writer,
-                       scoped_refptr<StringIOBuffer> packet);
+                       std::unique_ptr<QuicChromiumPacketWriter> writer);
 
   // Populates network error details for this session.
   void PopulateNetErrorDetails(NetErrorDetails* details);
@@ -365,10 +372,11 @@ class NET_EXPORT_PRIVATE QuicChromiumClientSession
   // UMA histogram counters for streams pushed to this session.
   int streams_pushed_count_;
   int streams_pushed_and_claimed_count_;
-  // Return value from packet rewrite packet on new socket. Used
-  // during connection migration on socket write error.
-  int error_code_from_rewrite_;
-  bool use_error_code_from_rewrite_;
+  // Stores packet that witnesses socket write error. This packet is
+  // written to a new socket after migration completes.
+  scoped_refptr<StringIOBuffer> packet_;
+  bool migration_pending_;   // True while migration is underway.
+  bool should_write_block_;  // True if the writer should write block.

[Ryan Hamilton, 2016/09/11 15:38:42]

Instead of doings this, it might just be easier to call set_write_blocked on the
writer?

[Jana, 2016/09/11 18:32:30]

Given that the delegate is part of the process anyways for the writer, the
current interaction doesn't seem unnatural... I'm inclined to leave it as is
because the writer is using the delegate to block and all delegate logic is in
the session. Though if you feel strongly, I'm happy to change it.

[Ryan Hamilton, 2016/09/11 20:44:24]

That's a fair point. I think set_write_blocked() might be better (after noodling
about it for a while) for a few reasons. 1) if we already have set_write_blocked
we dont' really need a delegate method. 2) it requires even more state in the
session (which is full of state already :>). 3) ShouldWriteBlock() might (via a
bug) be intended to apply to writer A but we swap it out and it ends up applying
with writer B.

This is minor in the grand scheme of things, but with all the exciting
complications of migration, I think it'd be slightly cleaner. 

[Jana, 2016/09/11 21:37:40]

Fair enough. Done.

   base::WeakPtrFactory<QuicChromiumClientSession> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(QuicChromiumClientSession);