Move handling of invalid referrer to the network delegate

net/url_request/url_request.cc

 // Copyright (c) 2012 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/url_request/url_request.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/debug/stack_trace.h"
 #include "base/lazy_instance.h"
 #include "base/memory/singleton.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/stats_counters.h"
-#include "base/metrics/user_metrics.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "base/values.h"
 #include "net/base/auth.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
 #include "net/base/load_timing_info.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
@@ skipping 657 matching lines @@
   is_redirecting_ = false;
 
   response_info_.was_cached = false;
 
   // If the referrer is secure, but the requested URL is not, the referrer
   // policy should be something non-default. If you hit this, please file a
   // bug.
   if (referrer_policy_ ==
           CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE &&
       GURL(referrer_).SchemeIsSecure() && !url().SchemeIsSecure()) {
-#if !defined(OFFICIAL_BUILD)
-    LOG(FATAL) << "Trying to send secure referrer for insecure load";
-#endif
-    referrer_.clear();
-    base::RecordAction(
-        base::UserMetricsAction("Net.URLRequest_StartJob_InvalidReferrer"));
+    if (!network_delegate_ ||
+        network_delegate_->CanCorrectInvalidReferrerHeader(

[mmenke, 2014/09/16 15:05:42]

I think this function name isn't clear.  We only call this on secure to insecure
transitions, and when referrer policy is
"CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE"...And in that case, we
fail requests rather than clearing the referrer, which seems a bit odd, given
the referrer policy.

I think we should rename "CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE"
to something like "DISALLOW_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE",
with a comment that the referrer will be cleared in that case, though the
delegate can optionally opt to block the request instead.

I also think "CanCorrectInvalidReferrerHeader" would be muck clearer as
"ShouldBlockRequestWithSecureReferrerToInsecure" or somesuch.

Open to other ideas, just think both of these names could better reflect the new
behavior.

[jochen (gone - plz use gerrit), 2014/09/16 16:27:57]

That's not correct. The transition happens here:
URLRequestJob::ComputeRedirectInfo

This code path is taken if somebody created an URLRequest with incorrect initial
data.
We might want to add different policies in the future, like origin-on-transition
or something. You could then create an URLRequest with a non-origin referrer
that would then also get handled here.

[mmenke, 2014/09/16 16:37:15]

Ahh...  I think that's not at all clear from the source here.  Suppose it could
also be caused by someone changing the referrer policy after start.
I still think that "CanCorrectInvalidReferrerHeader" is a very misleading name. 
Just looking at it, I'd assume that false would mean keep the invalid header, or
clear it, but that's not the case.  It's also not clear that "invalid" doesn't
mean !GURL(referrer_).is_valid().

+            *this, url(), GURL(referrer_))) {
+      referrer_.clear();
+    } else {
+      // We need to clear the referrer anyway to avoid an infinite recursion
+      // when starting the error job.

[mmenke, 2014/09/16 15:05:42]

nit:  Don't use "we" in comments.

+      referrer_.clear();
+      std::string source("delegate");
+      net_log_.AddEvent(NetLog::TYPE_CANCELLED,
+                        NetLog::StringCallback("source", &source));
+      RestartWithJob(new URLRequestErrorJob(
+          this, network_delegate_, ERR_BLOCKED_BY_CLIENT));
+      return;
+    }
   }
 
   // Don't allow errors to be sent from within Start().
   // TODO(brettw) this may cause NotifyDone to be sent synchronously,
   // we probably don't want this: they should be sent asynchronously so
   // the caller does not get reentered.
   job_->Start();
 }
 
 void URLRequest::Restart() {
@@ skipping 510 matching lines @@
       new base::debug::StackTrace(NULL, 0);
   *stack_trace_copy = stack_trace;
   stack_trace_.reset(stack_trace_copy);
 }
 
 const base::debug::StackTrace* URLRequest::stack_trace() const {
   return stack_trace_.get();
 }
 
 }  // namespace net