Move handling of invalid referrer to the network delegate

net/url_request/url_request.cc

Index: net/url_request/url_request.cc
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index 15b9ab7d73e9aabb22f8149cc4ee9453acfa5d48..7f76e3be19c12a3345fabe09456f65b5984948d0 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -13,7 +13,6 @@
 #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"
@@ -691,12 +690,21 @@ void URLRequest::StartJob(URLRequestJob* job) {
   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().