Add request_id to HttpRequestInfo and pass it to the NetworkDelegate for events.

net/http/http_network_transaction.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_network_transaction.h"
 
 #include <set>
 #include <vector>
 
 #include "base/compiler_specific.h"
@@ skipping 81 matching lines @@
 }
 
 }  // namespace
 
 //-----------------------------------------------------------------------------
 
 HttpNetworkTransaction::HttpNetworkTransaction(HttpNetworkSession* session)
     : pending_auth_target_(HttpAuth::AUTH_NONE),
       ALLOW_THIS_IN_INITIALIZER_LIST(
           io_callback_(this, &HttpNetworkTransaction::OnIOComplete)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(delegate_callback_(
+          new CancelableCompletionCallback<HttpNetworkTransaction>(
+              this, &HttpNetworkTransaction::OnIOComplete))),
       user_callback_(NULL),
       session_(session),
       request_(NULL),
       headers_valid_(false),
       logged_response_time_(false),
       request_headers_(),
       read_buf_len_(0),
       next_state_(STATE_NONE),
       establishing_tunnel_(false) {
   session->ssl_config_service()->GetSSLConfig(&ssl_config_);
@@ skipping 27 matching lines @@
         // HTTP, it can vary depending on whether or not we're pipelining.  It's
         // stream dependent, so the different subtypes should be implementing
         // their solutions.
         HttpResponseBodyDrainer* drainer =
           new HttpResponseBodyDrainer(stream_.release());
         drainer->Start(session_);
         // |drainer| will delete itself.
       }
     }
   }
+
+  delegate_callback_->Cancel();
 }
 
 int HttpNetworkTransaction::Start(const HttpRequestInfo* request_info,
                                   CompletionCallback* callback,
                                   const BoundNetLog& net_log) {
   SIMPLE_STATS_COUNTER("HttpNetworkTransaction.Count");
 
   net_log_ = net_log;
   request_ = request_info;
   start_time_ = base::Time::Now();
@@ skipping 348 matching lines @@
       case STATE_GENERATE_PROXY_AUTH_TOKEN_COMPLETE:
         rv = DoGenerateProxyAuthTokenComplete(rv);
         break;
       case STATE_GENERATE_SERVER_AUTH_TOKEN:
         DCHECK_EQ(OK, rv);
         rv = DoGenerateServerAuthToken();
         break;
       case STATE_GENERATE_SERVER_AUTH_TOKEN_COMPLETE:
         rv = DoGenerateServerAuthTokenComplete(rv);
         break;
-      case STATE_SEND_REQUEST:
+      case STATE_BUILD_REQUEST:
         DCHECK_EQ(OK, rv);
         net_log_.BeginEvent(NetLog::TYPE_HTTP_TRANSACTION_SEND_REQUEST, NULL);
-        rv = DoSendRequest();
+        rv = DoBuildRequest();
+        break;
+      case STATE_BUILD_REQUEST_COMPLETE:
+        rv = DoBuildRequestComplete(rv);
+        break;
+      case STATE_SEND_REQUEST:
+        rv = DoSendRequest(rv);
         break;
       case STATE_SEND_REQUEST_COMPLETE:
         rv = DoSendRequestComplete(rv);
         net_log_.EndEventWithNetErrorCode(
             NetLog::TYPE_HTTP_TRANSACTION_SEND_REQUEST, rv);
         break;
       case STATE_READ_HEADERS:
         DCHECK_EQ(OK, rv);
         net_log_.BeginEvent(NetLog::TYPE_HTTP_TRANSACTION_READ_HEADERS, NULL);
         rv = DoReadHeaders();
@@ skipping 123 matching lines @@
   if (!ShouldApplyServerAuth())
     return OK;
   return auth_controllers_[target]->MaybeGenerateAuthToken(request_,
                                                            &io_callback_,
                                                            net_log_);
 }
 
 int HttpNetworkTransaction::DoGenerateServerAuthTokenComplete(int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
   if (rv == OK)
-    next_state_ = STATE_SEND_REQUEST;
+    next_state_ = STATE_BUILD_REQUEST;
   return rv;
 }
 
-int HttpNetworkTransaction::DoSendRequest() {
-  next_state_ = STATE_SEND_REQUEST_COMPLETE;
+int HttpNetworkTransaction::DoBuildRequest() {
+  next_state_ = STATE_BUILD_REQUEST_COMPLETE;
 
-  UploadDataStream* request_body = NULL;
+  request_body_.reset(NULL);
   if (request_->upload_data) {
     int error_code;
-    request_body = UploadDataStream::Create(request_->upload_data, &error_code);
-    if (!request_body)
+    request_body_.reset(
+        UploadDataStream::Create(request_->upload_data, &error_code));
+    if (!request_body_.get())
       return error_code;
   }
 
+  headers_valid_ = false;
+
   // This is constructed lazily (instead of within our Start method), so that
   // we have proxy info available.
   if (request_headers_.IsEmpty()) {
-    bool using_proxy = (proxy_info_.is_http()|| proxy_info_.is_https()) &&
+    bool using_proxy = (proxy_info_.is_http() || proxy_info_.is_https()) &&
                         !is_https_request();
-    HttpUtil::BuildRequestHeaders(request_, request_body, auth_controllers_,
+    HttpUtil::BuildRequestHeaders(request_, request_body_.get(),
+                                  auth_controllers_,
                                   ShouldApplyServerAuth(),
                                   ShouldApplyProxyAuth(), using_proxy,
                                   &request_headers_);
-
-    if (session_->network_delegate())
-      session_->network_delegate()->NotifySendHttpRequest(&request_headers_);
   }
 
-  headers_valid_ = false;
-  return stream_->SendRequest(request_headers_, request_body, &response_,
-                              &io_callback_);
+  return OK;
+}
+
+int HttpNetworkTransaction::DoBuildRequestComplete(int result) {

[willchan no longer on Chromium, 2011/03/22 23:05:35]

Sorry for nitpicking like this, but to make it more like net code, the
parameters need to make sense too.

Please move this code into DoBuildRequest(). DoBuildRequestComplete() should be
simple. Something like:

DoBuildRequestComplete(int result) {
  if (result == OK)
    next_state_ = STATE_SEND_REQUEST
  return result;
}

DoSendRequest(void) shouldn't need a parameter, and thus shouldn't need the if
(result == OK)

[Matt Perry, 2011/03/24 00:11:25]

Done. Though now I notice that delegate_callback_ may never get to the Release()
in DoSendRequest() - I think. Should I  be concerned about that? I'm following
the pattern in HttpCache::Transaction, and it doesn't seem to handle that case,
so maybe I'm misinterpreting it.

[willchan no longer on Chromium, 2011/03/26 01:46:49]

I think maybe you want to only create the delegate_callback_ In BuildRequest().
AddRef() it at that point. Then in the destructor, if |delegate_callback_| is
non-NULL, Cancel() it. And in BuildRequestComplete(), NULL it out too, in
addition to the Release() call.

[Matt Perry, 2011/03/28 22:51:01]

Any reason to prefer doing it that way as opposed to always creating the
callback?

[willchan no longer on Chromium, 2011/03/29 20:46:27]

I don't know that this is possible today, but let's say we get to BuildRequest()
and get ERR_IO_PENDING. Now, let's say we restart the transaction (I think we
only restart the transaction in STATE_NONE right now, but I'm not sure how
strict of a convention that is).

At that point, we have pending callback that should be Cancel()'d. Now that
we've restarted, we need to use a new callback object, otherwise, we'll be
trying to use one that has already have Cancel() called on it.

The fact that we can restart transactions and Cancel() can't be reset (nor we
would want to), makes me feel like it's best not to keep the callback alive for
the whole transaction, just create and release it when we're done.

+  next_state_ = STATE_SEND_REQUEST;
+  delegate_callback_->AddRef();  // balanced in DoSendRequest
+
+  if (session_->network_delegate()) {
+    if (session_->network_delegate()->NotifyBeforeSendHeaders(
+            request_->request_id, &request_headers_, delegate_callback_))
+      return ERR_IO_PENDING;
+  }
+
+  return OK;
+}
+
+int HttpNetworkTransaction::DoSendRequest(int result) {
+  next_state_ = STATE_SEND_REQUEST_COMPLETE;
+  delegate_callback_->Release();  // balanced in DoBuildRequestComplete
+
+  if (result == net::OK) {
+    return stream_->SendRequest(
+        request_headers_, request_body_.release(), &response_, &io_callback_);
+  }
+  return result;
 }
 
 int HttpNetworkTransaction::DoSendRequestComplete(int result) {
   if (result < 0)
     return HandleIOError(result);
   next_state_ = STATE_READ_HEADERS;
   return OK;
 }
 
 int HttpNetworkTransaction::DoReadHeaders() {
@@ skipping 407 matching lines @@
     case ERR_CONNECTION_ABORTED:
       if (ShouldResendRequest(error)) {
         ResetConnectionAndRequestForResend();
         error = OK;
       }
       break;
   }
   return error;
 }
 
 void HttpNetworkTransaction::ResetStateForRestart() {

[willchan no longer on Chromium, 2011/03/22 23:05:35]

This is the core place where all state gets reset when we "restart" a
transaction (primarily done for authentication purposes, like HTTP auth and SSL
client auth). At this point, you should probably Cancel() your callback, and
mark the transaction as restarted. You should pass that into the
NetworkDelegate. I say pass that into the NetworkDelegate rather than not call
the NetworkDelegate, because eventually when we make NetLog hide behind
NetworkDelegate, then we will still need to make sure it's getting called.

[Matt Perry, 2011/03/24 00:11:25]

Mark it how?
You lost me here... What's being passed into the NetworkDelegate? And for what
purpose?

[willchan no longer on Chromium, 2011/03/26 01:46:49]

Note that we will "restart" a transaction for authentication purposes. I believe
you previously told me that we only want onBeforeSendHeaders() to be called for
the first request, not for the subsequent restarted transactions where we try to
send the correct authentication credentials. We should always call
NetworkDelegate though, for future NetLog purposes, but within the
ChromeNetworkDelegate or ExtensionWebRequestEventRouter, we may want to filter
out these extra authentication requests. Therefore, I think you need a bool
member variable to track whether this is the original transaction, or a
restarted authentication transaction.

[Matt Perry, 2011/03/28 22:51:01]

Oh I see now. FWIW, OnBeforeSendHeaders will be called for restarted requests as
well. In the ExtensionWebRequestEventRouter, the request will no longer be in
the map (since it was already processed), so it'll be ignored. This is already
the behavior you suggest, I believe.

[willchan no longer on Chromium, 2011/03/29 20:46:27]

OIC, ok!

   ResetStateForAuthRestart();
   stream_.reset();
 }
 
 void HttpNetworkTransaction::ResetStateForAuthRestart() {
   pending_auth_target_ = HttpAuth::AUTH_NONE;
   read_buf_ = NULL;
   read_buf_len_ = 0;
   headers_valid_ = false;
   request_headers_.Clear();
@@ skipping 97 matching lines @@
 #define STATE_CASE(s) \
   case s: \
     description = base::StringPrintf("%s (0x%08X)", #s, s); \
     break
 
 std::string HttpNetworkTransaction::DescribeState(State state) {
   std::string description;
   switch (state) {
     STATE_CASE(STATE_CREATE_STREAM);
     STATE_CASE(STATE_CREATE_STREAM_COMPLETE);
+    STATE_CASE(STATE_BUILD_REQUEST);
+    STATE_CASE(STATE_BUILD_REQUEST_COMPLETE);
     STATE_CASE(STATE_SEND_REQUEST);
     STATE_CASE(STATE_SEND_REQUEST_COMPLETE);
     STATE_CASE(STATE_READ_HEADERS);
     STATE_CASE(STATE_READ_HEADERS_COMPLETE);
     STATE_CASE(STATE_READ_BODY);
     STATE_CASE(STATE_READ_BODY_COMPLETE);
     STATE_CASE(STATE_DRAIN_BODY_FOR_AUTH_RESTART);
     STATE_CASE(STATE_DRAIN_BODY_FOR_AUTH_RESTART_COMPLETE);
     STATE_CASE(STATE_NONE);
     default:
       description = base::StringPrintf("Unknown state 0x%08X (%u)", state,
                                        state);
       break;
   }
   return description;
 }
 
 #undef STATE_CASE
 
 }  // namespace net