[Cronet]Make delaying sending request headers explicit in bidirection…

…al stream

Always delaying sending request headers when
disableAutoFlush is not safe (in the case of bidirectional
streaming). Because server might be expecting request
headers before sending a response, while client might only
call SendData/SendvData after server responds.

This CL adds an explicit flag to tell
net::BidirectionalStream when to delay sending request
headers and coalesce them with data frames in
SendData/SendvData.

BUG=599902

Review-Url: https://codereview.chromium.org/1992953004
Cr-Commit-Position: refs/heads/master@{#397567}

components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java

@@ -48,7 +48,9 @@ public static class Builder {
         // Priority of the stream. Default is medium.
         @StreamPriority private int mPriority = STREAM_PRIORITY_MEDIUM;
 
+        // TODO(xunjieli): Remove mDisableAutoFlush and make flush() required as part of th API.
         private boolean mDisableAutoFlush;
+        private boolean mDelayRequestHeadersUntilFirstFlush;
 
         /**
          * Creates a builder for {@link BidirectionalStream} objects. All callbacks for
@@ -174,6 +176,22 @@ public Builder disableAutoFlush(boolean disableAutoFlush) {
             return this;
         }
 
+        /**
+         * Delays sending request headers until {@link BidirectionalStream#flush()}
+         * is called. This flag is currently only respected when QUIC is negotiated.
+         * When true, QUIC will send request header frame along with data frame(s)
+         * as a single packet when possible.
+         *
+         * @param delayRequestHeadersUntilFirstFlush if true, sending request headers will
+         *         be delayed until flush() is called.
+         * @return the builder to facilitate chaining.
+         */
+        public Builder delayRequestHeadersUntilFirstFlush(
+                boolean delayRequestHeadersUntilFirstFlush) {
+            mDelayRequestHeadersUntilFirstFlush = delayRequestHeadersUntilFirstFlush;
+            return this;
+        }
+
         /**
          * Creates a {@link BidirectionalStream} using configuration from this
          * {@link Builder}. The returned {@code BidirectionalStream} can then be started
@@ -185,7 +203,8 @@ public Builder disableAutoFlush(boolean disableAutoFlush) {
         @SuppressLint("WrongConstant") // TODO(jbudorick): Remove this after rolling to the N SDK.
         public BidirectionalStream build() {
             return mCronetEngine.createBidirectionalStream(mUrl, mCallback, mExecutor, mHttpMethod,
-                    mRequestHeaders, mPriority, mDisableAutoFlush);
+                    mRequestHeaders, mPriority, mDisableAutoFlush,
+                    mDelayRequestHeadersUntilFirstFlush);
         }
     }
 
@@ -380,8 +399,10 @@ public abstract static class PingCallback {
     public abstract void write(ByteBuffer buffer, boolean endOfStream);
 
     /**
-     * Flushes pending writes. This method should only be invoked when auto
-     * flush is disabled through {@link Builder#disableAutoFlush}.
+     * Flushes pending writes. This method should not be invoked before {@link
+     * Callback#onStreamReady onStreamReady()}. For previously delayed {@link
+     * #write write()}s, a corresponding {@link Callback#onWriteCompleted onWriteCompleted()}
+     * will be invoked when the buffer is sent.
      */
     public abstract void flush();
 

components/cronet/android/api/src/org/chromium/net/CronetEngine.java

@@ -713,12 +713,16 @@ protected abstract UrlRequest createRequest(String url, UrlRequest.Callback call
      *         {@link BidirectionalStream.Builder#STREAM_PRIORITY_IDLE STREAM_PRIORITY_*}
      *         values.
      * @param disableAutoFlush whether auto flush should be disabled
+     * @param delayRequestHeadersUntilFirstFlush whether to delay sending request
+     *         headers until flush() is called, and try to combine them
+     *         with the next data frame.
      * @return a new stream.
      */
     abstract BidirectionalStream createBidirectionalStream(String url,
             BidirectionalStream.Callback callback, Executor executor, String httpMethod,
             List<Map.Entry<String, String>> requestHeaders,
-            @BidirectionalStream.Builder.StreamPriority int priority, boolean disableAutoFlush);
+            @BidirectionalStream.Builder.StreamPriority int priority, boolean disableAutoFlush,
+            boolean delayRequestHeadersUntilFirstFlush);
 
     /**
      * @return {@code true} if the engine is enabled.

components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java

@@ -65,7 +65,8 @@ public UrlRequest createRequest(String url, UrlRequest.Callback callback, Execut
     @Override
     BidirectionalStream createBidirectionalStream(String url, BidirectionalStream.Callback callback,
             Executor executor, String httpMethod, List<Map.Entry<String, String>> requestHeaders,
-            @BidirectionalStream.Builder.StreamPriority int priority, boolean disableAutoFlush) {
+            @BidirectionalStream.Builder.StreamPriority int priority, boolean disableAutoFlush,
+            boolean delayRequestHeadersUntilFirstFlush) {
         throw new UnsupportedOperationException(
                 "Can't create a bidi stream - httpurlconnection doesn't have those APIs");
     }

components/cronet/android/cronet_bidirectional_stream_adapter.cc

@@ -71,15 +71,15 @@ static jlong CreateBidirectionalStream(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& jbidi_stream,
     jlong jurl_request_context_adapter,
-    jboolean jdisable_auto_flush) {
+    jboolean jsend_request_headers_automatically) {
   CronetURLRequestContextAdapter* context_adapter =
       reinterpret_cast<CronetURLRequestContextAdapter*>(
           jurl_request_context_adapter);
   DCHECK(context_adapter);
 
   CronetBidirectionalStreamAdapter* adapter =
       new CronetBidirectionalStreamAdapter(context_adapter, env, jbidi_stream,
-                                           jdisable_auto_flush);
+                                           jsend_request_headers_automatically);
 
   return reinterpret_cast<jlong>(adapter);
 }
@@ -93,16 +93,26 @@ CronetBidirectionalStreamAdapter::CronetBidirectionalStreamAdapter(
     CronetURLRequestContextAdapter* context,
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& jbidi_stream,
-    bool disable_auto_flush)
+    bool send_request_headers_automatically)
     : context_(context),
       owner_(env, jbidi_stream),
-      disable_auto_flush_(disable_auto_flush),
+      send_request_headers_automatically_(send_request_headers_automatically),
       stream_failed_(false) {}
 
 CronetBidirectionalStreamAdapter::~CronetBidirectionalStreamAdapter() {
   DCHECK(context_->IsOnNetworkThread());
 }
 
+void CronetBidirectionalStreamAdapter::SendRequestHeaders(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& jcaller) {
+  context_->PostTaskToNetworkThread(
+      FROM_HERE,
+      base::Bind(
+          &CronetBidirectionalStreamAdapter::SendRequestHeadersOnNetworkThread,
+          base::Unretained(this)));
+}
+
 jint CronetBidirectionalStreamAdapter::Start(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& jcaller,
@@ -228,10 +238,12 @@ void CronetBidirectionalStreamAdapter::Destroy(
                  base::Unretained(this), jsend_on_canceled));
 }
 
-void CronetBidirectionalStreamAdapter::OnStreamReady() {
+void CronetBidirectionalStreamAdapter::OnStreamReady(
+    bool request_headers_sent) {
   DCHECK(context_->IsOnNetworkThread());
   JNIEnv* env = base::android::AttachCurrentThread();
-  cronet::Java_CronetBidirectionalStream_onStreamReady(env, owner_.obj());
+  cronet::Java_CronetBidirectionalStream_onStreamReady(
+      env, owner_.obj(), request_headers_sent ? JNI_TRUE : JNI_FALSE);
 }
 
 void CronetBidirectionalStreamAdapter::OnHeadersReceived(
@@ -321,7 +333,22 @@ void CronetBidirectionalStreamAdapter::StartOnNetworkThread(
       std::move(request_info), context_->GetURLRequestContext()
                                    ->http_transaction_factory()
                                    ->GetSession(),
-      disable_auto_flush_, this));
+      send_request_headers_automatically_, this));
+}
+
+void CronetBidirectionalStreamAdapter::SendRequestHeadersOnNetworkThread() {
+  DCHECK(context_->IsOnNetworkThread());
+  DCHECK(!send_request_headers_automatically_);
+
+  if (stream_failed_) {
+    // If stream failed between the time when SendRequestHeaders is invoked and
+    // SendRequestHeadersOnNetworkThread is executed, do not call into
+    // |bidi_stream_| since the underlying stream might have been destroyed.
+    // Do not invoke Java callback either, since onError is posted when
+    // |stream_failed_| is set to true.
+    return;
+  }
+  bidi_stream_->SendRequestHeaders();
 }
 
 void CronetBidirectionalStreamAdapter::ReadDataOnNetworkThread(

components/cronet/android/cronet_bidirectional_stream_adapter.h

@@ -71,7 +71,7 @@ class CronetBidirectionalStreamAdapter
       CronetURLRequestContextAdapter* context,
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& jbidi_stream,
-      bool jdisable_auto_flush);
+      bool jsend_request_headers_automatically);
   ~CronetBidirectionalStreamAdapter() override;
 
   // Validates method and headers, initializes and starts the request. If
@@ -89,6 +89,19 @@ class CronetBidirectionalStreamAdapter
              const base::android::JavaParamRef<jobjectArray>& jheaders,
              jboolean jend_of_stream);
 
+  // Sends request headers to server.
+  // When |send_request_headers_automatically_| is
+  // false and OnStreamReady() is invoked with request_headers_sent = false,
+  // headers will be combined with next WriteData/WritevData unless this
+  // method is called first, in which case headers will be sent separately
+  // without delay.
+  // (This method cannot be called when |send_request_headers_automatically_| is
+  // true nor when OnStreamReady() is invoked with request_headers_sent = true,
+  // since headers have been sent by the stream when stream is negotiated
+  // successfully.)
+  void SendRequestHeaders(JNIEnv* env,
+                          const base::android::JavaParamRef<jobject>& jcaller);
+
   // Reads more data into |jbyte_buffer| starting at |jposition| and not
   // exceeding |jlimit|. Arguments are preserved to ensure that |jbyte_buffer|
   // is not modified by the application during read.
@@ -121,7 +134,7 @@ class CronetBidirectionalStreamAdapter
 
  private:
   // net::BidirectionalStream::Delegate implementations:
-  void OnStreamReady() override;
+  void OnStreamReady(bool request_headers_sent) override;
   void OnHeadersReceived(const net::SpdyHeaderBlock& response_headers) override;
   void OnDataRead(int bytes_read) override;
   void OnDataSent() override;
@@ -130,6 +143,7 @@ class CronetBidirectionalStreamAdapter
 
   void StartOnNetworkThread(
       std::unique_ptr<net::BidirectionalStreamRequestInfo> request_info);
+  void SendRequestHeadersOnNetworkThread();
   void ReadDataOnNetworkThread(
       scoped_refptr<IOBufferWithByteBuffer> read_buffer,
       int buffer_size);
@@ -145,7 +159,7 @@ class CronetBidirectionalStreamAdapter
 
   // Java object that owns this CronetBidirectionalStreamAdapter.
   base::android::ScopedJavaGlobalRef<jobject> owner_;
-  const bool disable_auto_flush_;
+  const bool send_request_headers_automatically_;
 
   scoped_refptr<IOBufferWithByteBuffer> read_buffer_;
   std::unique_ptr<PendingWriteData> pending_write_data_;