Cleanup components/grpc_support.

- Use base::BindOnce instead of base::Bind.
- Add ShutdownQuicTestServerDispatcher to trigger network errors without shutting down the server.
- Don't spam test log with bidirectional_stream state.
- Fix lint warnings.


Bug: 714018
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I4baf1f1e8718d4b5cc21f5664ee2dd1cc0bfa83d
Reviewed-on: https://chromium-review.googlesource.com/890058
Commit-Queue: Misha Efimov <mef@chromium.org>
Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533065}

components/grpc_support/bidirectional_stream.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
@@ -100,8 +101,8 @@ int BidirectionalStream::Start(const char* url,
   request_info->end_stream_on_headers = end_of_stream;
   write_end_of_stream_ = end_of_stream;
   PostToNetworkThread(FROM_HERE,
-                      base::Bind(&BidirectionalStream::StartOnNetworkThread,
-                                 weak_this_, base::Passed(&request_info)));
+                      base::BindOnce(&BidirectionalStream::StartOnNetworkThread,
+                                     weak_this_, base::Passed(&request_info)));
   return 0;
 }
 
@@ -111,9 +112,9 @@ bool BidirectionalStream::ReadData(char* buffer, int capacity) {
   scoped_refptr<net::WrappedIOBuffer> read_buffer(
       new net::WrappedIOBuffer(buffer));
 
-  PostToNetworkThread(FROM_HERE,
-                      base::Bind(&BidirectionalStream::ReadDataOnNetworkThread,
-                                 weak_this_, read_buffer, capacity));
+  PostToNetworkThread(
+      FROM_HERE, base::BindOnce(&BidirectionalStream::ReadDataOnNetworkThread,
+                                weak_this_, read_buffer, capacity));
   return true;
 }
 
@@ -127,30 +128,31 @@ bool BidirectionalStream::WriteData(const char* buffer,
       new net::WrappedIOBuffer(buffer));
 
   PostToNetworkThread(
-      FROM_HERE, base::Bind(&BidirectionalStream::WriteDataOnNetworkThread,
-                            weak_this_, write_buffer, count, end_of_stream));
+      FROM_HERE,
+      base::BindOnce(&BidirectionalStream::WriteDataOnNetworkThread, weak_this_,
+                     write_buffer, count, end_of_stream));
   return true;
 }
 
 void BidirectionalStream::Flush() {
   PostToNetworkThread(
       FROM_HERE,
-      base::Bind(&BidirectionalStream::FlushOnNetworkThread, weak_this_));
+      base::BindOnce(&BidirectionalStream::FlushOnNetworkThread, weak_this_));
 }
 
 void BidirectionalStream::Cancel() {
   PostToNetworkThread(
       FROM_HERE,
-      base::Bind(&BidirectionalStream::CancelOnNetworkThread, weak_this_));
+      base::BindOnce(&BidirectionalStream::CancelOnNetworkThread, weak_this_));
 }
 
 void BidirectionalStream::Destroy() {
   // Destroy could be called from any thread, including network thread (if
   // posting task to executor throws an exception), but is posted, so |this|
   // is valid until calling task is complete.
-  PostToNetworkThread(FROM_HERE,
-                      base::Bind(&BidirectionalStream::DestroyOnNetworkThread,
-                                 base::Unretained(this)));
+  PostToNetworkThread(
+      FROM_HERE, base::BindOnce(&BidirectionalStream::DestroyOnNetworkThread,
+                                base::Unretained(this)));
 }
 
 void BidirectionalStream::OnStreamReady(bool request_headers_sent) {
@@ -249,9 +251,9 @@ void BidirectionalStream::OnFailed(int error) {
   read_state_ = write_state_ = ERR;
   weak_factory_.InvalidateWeakPtrs();
   // Delete underlying |bidi_stream_| asynchronously as it may still be used.
-  PostToNetworkThread(FROM_HERE,
-                      base::Bind(&base::DeletePointer<net::BidirectionalStream>,
-                                 bidi_stream_.release()));
+  PostToNetworkThread(
+      FROM_HERE, base::BindOnce(&base::DeletePointer<net::BidirectionalStream>,
+                                bidi_stream_.release()));
   delegate_->OnFailed(error);
 }
 
@@ -381,8 +383,9 @@ void BidirectionalStream::MaybeOnSucceded() {
     weak_factory_.InvalidateWeakPtrs();
     // Delete underlying |bidi_stream_| asynchronously as it may still be used.
     PostToNetworkThread(
-        FROM_HERE, base::Bind(&base::DeletePointer<net::BidirectionalStream>,
-                              bidi_stream_.release()));
+        FROM_HERE,
+        base::BindOnce(&base::DeletePointer<net::BidirectionalStream>,
+                       bidi_stream_.release()));
     delegate_->OnSucceeded();
   }
 }
@@ -393,8 +396,9 @@ bool BidirectionalStream::IsOnNetworkThread() {
 }
 
 void BidirectionalStream::PostToNetworkThread(const base::Location& from_here,
-                                              const base::Closure& task) {
-  request_context_getter_->GetNetworkTaskRunner()->PostTask(from_here, task);
+                                              base::OnceClosure task) {
+  request_context_getter_->GetNetworkTaskRunner()->PostTask(from_here,
+                                                            std::move(task));
 }
 
-}  // namespace cronet
+}  // namespace grpc_support

components/grpc_support/bidirectional_stream.h

@@ -17,7 +17,7 @@
 
 namespace base {
 class Location;
-}  // namespace tracked_objects
+}  // namespace base
 
 namespace net {
 class HttpRequestHeaders;
@@ -193,7 +193,7 @@ class BidirectionalStream : public net::BidirectionalStream::Delegate {
 
   bool IsOnNetworkThread();
   void PostToNetworkThread(const base::Location& from_here,
-                           const base::Closure& task);
+                           base::OnceClosure task);
 
   // Read state is tracking reading flow. Only accessed on network thread.
   //                         | <--- READING <--- |

components/grpc_support/bidirectional_stream_c.cc

@@ -38,12 +38,12 @@ namespace {
 
 class HeadersArray : public bidirectional_stream_header_array {
  public:
-  HeadersArray(const net::SpdyHeaderBlock& header_block);
+  explicit HeadersArray(const net::SpdyHeaderBlock& header_block);
   ~HeadersArray();
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(HeadersArray);
   base::StringPairs headers_strings_;
+  DISALLOW_COPY_AND_ASSIGN(HeadersArray);
 };
 
 HeadersArray::HeadersArray(const net::SpdyHeaderBlock& header_block)
@@ -195,8 +195,9 @@ void BidirectionalStreamAdapter::DestroyAdapterForStream(
   // is valid until calling task is complete.
   adapter->bidirectional_stream_->Destroy();
   adapter->request_context_getter_->GetNetworkTaskRunner()->PostTask(
-      FROM_HERE, base::Bind(&BidirectionalStreamAdapter::DestroyOnNetworkThread,
-                            base::Unretained(adapter)));
+      FROM_HERE,
+      base::BindOnce(&BidirectionalStreamAdapter::DestroyOnNetworkThread,
+                     base::Unretained(adapter)));
 }
 
 void BidirectionalStreamAdapter::DestroyOnNetworkThread() {

components/grpc_support/bidirectional_stream_unittest.cc

@@ -119,13 +119,14 @@ class TestBidirectionalStreamCallback {
   static TestBidirectionalStreamCallback* FromStream(
       bidirectional_stream* stream) {
     DCHECK(stream);
-    return (TestBidirectionalStreamCallback*)stream->annotation;
+    return reinterpret_cast<TestBidirectionalStreamCallback*>(
+        stream->annotation);
   }
 
   virtual bool MaybeCancel(bidirectional_stream* stream, ResponseStep step) {
     DCHECK_EQ(stream, this->stream);
     response_step = step;
-    DLOG(WARNING) << "Step: " << step;
+    DVLOG(3) << "Step: " << step;
 
     if (step != cancel_from_step)
       return false;
@@ -538,9 +539,8 @@ TEST_P(BidirectionalStreamTest, StreamFailBeforeReadIsExecutedOnNetworkThread) {
       : public TestBidirectionalStreamCallback {
     bool MaybeCancel(bidirectional_stream* stream, ResponseStep step) override {
       if (step == ResponseStep::ON_READ_COMPLETED) {
-        // Shut down the server, and the stream should error out.
-        // The second call to ShutdownQuicTestServer is no-op.
-        ShutdownQuicTestServer();
+        // Shut down the server dispatcher, and the stream should error out.
+        ShutdownQuicTestServerDispatcher();
       }
       return TestBidirectionalStreamCallback::MaybeCancel(stream, step);
     }
@@ -582,9 +582,8 @@ TEST_P(BidirectionalStreamTest, StreamFailAfterStreamReadyCallback) {
       : public TestBidirectionalStreamCallback {
     bool MaybeCancel(bidirectional_stream* stream, ResponseStep step) override {
       if (step == ResponseStep::ON_STREAM_READY) {
-        // Shut down the server, and the stream should error out.
-        // The second call to ShutdownQuicTestServer is no-op.
-        ShutdownQuicTestServer();
+        // Shut down the server dispatcher, and the stream should error out.
+        ShutdownQuicTestServerDispatcher();
       }
       return TestBidirectionalStreamCallback::MaybeCancel(stream, step);
     }
@@ -612,9 +611,8 @@ TEST_P(BidirectionalStreamTest,
       : public TestBidirectionalStreamCallback {
     bool MaybeCancel(bidirectional_stream* stream, ResponseStep step) override {
       if (step == ResponseStep::ON_WRITE_COMPLETED) {
-        // Shut down the server, and the stream should error out.
-        // The second call to ShutdownQuicTestServer is no-op.
-        ShutdownQuicTestServer();
+        // Shut down the server dispatcher, and the stream should error out.
+        ShutdownQuicTestServerDispatcher();
       }
       return TestBidirectionalStreamCallback::MaybeCancel(stream, step);
     }

components/grpc_support/test/quic_test_server.cc

@@ -4,6 +4,7 @@
 
 #include "components/grpc_support/test/quic_test_server.h"
 
+#include <memory>
 #include <utility>
 
 #include "base/bind.h"
@@ -19,6 +20,7 @@
 #include "net/quic/chromium/crypto/proof_source_chromium.h"
 #include "net/spdy/core/spdy_header_block.h"
 #include "net/test/test_data_directory.h"
+#include "net/tools/quic/quic_dispatcher.h"
 #include "net/tools/quic/quic_http_response_cache.h"
 #include "net/tools/quic/quic_simple_server.h"
 
@@ -110,8 +112,15 @@ void ShutdownOnServerThread(base::WaitableEvent* server_stopped_event) {
   server_stopped_event->Signal();
 }
 
+void ShutdownDispatcherOnServerThread(
+    base::WaitableEvent* dispatcher_stopped_event) {
+  DCHECK(g_quic_server_thread->task_runner()->BelongsToCurrentThread());
+  g_quic_server->dispatcher()->Shutdown();
+  dispatcher_stopped_event->Signal();
+}
+
 bool StartQuicTestServer() {
-  LOG(INFO) << g_quic_server_thread;
+  DVLOG(3) << g_quic_server_thread;
   DCHECK(!g_quic_server_thread);
   g_quic_server_thread = new base::Thread("quic server thread");
   base::Thread::Options thread_options;
@@ -124,21 +133,36 @@ bool StartQuicTestServer() {
       base::WaitableEvent::ResetPolicy::MANUAL,
       base::WaitableEvent::InitialState::NOT_SIGNALED);
   g_quic_server_thread->task_runner()->PostTask(
-      FROM_HERE, base::Bind(&StartQuicServerOnServerThread, test_files_root,
-                            &server_started_event));
+      FROM_HERE, base::BindOnce(&StartQuicServerOnServerThread, test_files_root,
+                                &server_started_event));
   server_started_event.Wait();
   return true;
 }
 
+// Shut down the server dispatcher, and the stream should error out.
+void ShutdownQuicTestServerDispatcher() {
+  if (!g_quic_server)
+    return;
+  DCHECK(!g_quic_server_thread->task_runner()->BelongsToCurrentThread());
+  base::WaitableEvent dispatcher_stopped_event(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  g_quic_server_thread->task_runner()->PostTask(
+      FROM_HERE, base::BindOnce(&ShutdownDispatcherOnServerThread,
+                                &dispatcher_stopped_event));
+  dispatcher_stopped_event.Wait();
+}
+
 void ShutdownQuicTestServer() {
-  if (!g_quic_server_thread)
+  if (!g_quic_server)
     return;
   DCHECK(!g_quic_server_thread->task_runner()->BelongsToCurrentThread());
   base::WaitableEvent server_stopped_event(
       base::WaitableEvent::ResetPolicy::MANUAL,
       base::WaitableEvent::InitialState::NOT_SIGNALED);
   g_quic_server_thread->task_runner()->PostTask(
-      FROM_HERE, base::Bind(&ShutdownOnServerThread, &server_stopped_event));
+      FROM_HERE,
+      base::BindOnce(&ShutdownOnServerThread, &server_stopped_event));
   server_stopped_event.Wait();
   delete g_quic_server_thread;
   g_quic_server_thread = nullptr;

components/grpc_support/test/quic_test_server.h

@@ -13,6 +13,10 @@ bool StartQuicTestServer();
 
 void ShutdownQuicTestServer();
 
+// Shuts down the server dispatcher, which results in sending ConnectionClose
+// frames to all connected clients.
+void ShutdownQuicTestServerDispatcher();
+
 int GetQuicTestServerPort();
 
 extern const char kTestServerDomain[];
@@ -40,4 +44,4 @@ extern const char kSimpleHeaderValue[];
 
 }  // namespace grpc_support
 
-#endif  // COMPONENTS_CRONET_IOS_TEST_QUIC_TEST_SERVER_H_
+#endif  // COMPONENTS_GRPC_SUPPORT_TEST_QUIC_TEST_SERVER_H_