Preserving Content-Type header from http request in OpenURL path.

content/child/web_url_request_util.cc

Index: content/child/web_url_request_util.cc
diff --git a/content/child/web_url_request_util.cc b/content/child/web_url_request_util.cc
index 51830c388fc82894eaf085e36dc777c19276fb24..6b1f3ab15480248d9d9a77b2efda6ed66f3de2db 100644
--- a/content/child/web_url_request_util.cc
+++ b/content/child/web_url_request_util.cc
@@ -7,7 +7,10 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <algorithm>
 #include <limits>
+#include <string>
+#include <vector>
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
@@ -42,15 +45,26 @@ class HeaderFlattener : public blink::WebHTTPHeaderVisitor {
   HeaderFlattener() {}
   ~HeaderFlattener() override {}
 
+  void FilterOutHeader(std::string header_name) {
+    // |header_name| needs to be lower-case - otherwise
+    // base::LowerCaseEqualsASCII won't work properly.
+    DCHECK(base::LowerCaseEqualsASCII(header_name, header_name));
+
+    header_names_to_filter_out_.push_back(std::move(header_name));
+  }
+
   void visitHeader(const WebString& name, const WebString& value) override {
     // Headers are latin1.
     const std::string& name_latin1 = name.latin1();
     const std::string& value_latin1 = value.latin1();
 
-    // Skip over referrer headers found in the header map because we already
-    // pulled it out as a separate parameter.
-    if (base::LowerCaseEqualsASCII(name_latin1, "referer"))
+    if (std::any_of(header_names_to_filter_out_.begin(),
+                    header_names_to_filter_out_.end(),
+                    [&name_latin1](const std::string& filter) {
+                      return base::LowerCaseEqualsASCII(name_latin1, filter);
+                    })) {
       return;
+    }
 
     if (!buffer_.empty())
       buffer_.append("\r\n");
@@ -63,6 +77,7 @@ class HeaderFlattener : public blink::WebHTTPHeaderVisitor {
 
  private:
   std::string buffer_;
+  std::vector<std::string> header_names_to_filter_out_;
 };
 
 }  // namespace
@@ -182,8 +197,28 @@ ResourceType WebURLRequestToResourceType(const WebURLRequest& request) {
   }
 }
 
-std::string GetWebURLRequestHeaders(const blink::WebURLRequest& request) {
+std::string GetWebURLRequestHeadersForResourceRequest(
+    const blink::WebURLRequest& request) {
   HeaderFlattener flattener;
+
+  // Skip over referrer headers found in the header map because we already
+  // pulled it out as a separate parameter.
+  flattener.FilterOutHeader("referer");
+
+  request.visitHTTPHeaderFields(&flattener);
+  return flattener.GetBuffer();
+}
+
+std::string GetWebURLRequestHeadersForOpenURLParams(
+    const blink::WebURLRequest& request) {
+  HeaderFlattener flattener;
+
+  // Skip over headers added by Blink on top of |extra_headers| passed from the
+  // browser, so that OpenURLParams IPC contains only the headers that the
+  // browser would have passed.
+  flattener.FilterOutHeader("referer");
+  flattener.FilterOutHeader("upgrade-insecure-requests");

[Łukasz Anforowicz, 2016/09/22 21:23:16]

The filtering is needed, so that
NavigationController::LoadURLParams::extra_headers is round-tripped into
OpenURLParams::extra_headers - we need to filter out headers that were added by
Blink.

Singling out the "referer" and "update-insecure-requests" headers seems rather
ad-hoc and fragile, but I don't see any way to deal with all possible headers in
a comprehensive way.  If there is indeed no comprehensive way / if we indeed
have to find and fix one header at a time, then I think the filtering approach
is as good as any other specific-headers-focused-approach.  In particular, I
don't think filtering is any worse than moving where the header is added (i.e.
whether the header is added by Blink before or after the construction of
FrameHostMsg_OpenURL_Params), especially since the timing of when the header is
added is sensitive to other requirements (e.g. coming from PlzNavigate, as
pointed out by carlosk@).

[Charlie Reis, 2016/09/30 21:31:55]

Still, it seems hard to maintain this if Blink adds more headers in the future. 
No one will think to add them to this filter.

I'm not entirely clear on which headers you need.  If it's just the ones sent by
LoadURLParams (presumably stored in StartNavigationParams?), can we get to them
via NavigationState?  (And does none of this apply for PlzNavigate, which
doesn't use StartNavigationParams or make requests from the renderer?)

[Łukasz Anforowicz, 2016/09/30 23:17:00]

Ack.
There are 2 conflicting requirements:

Req#1: We don't want OpenURL IPC to loose any headers that have been added by
Blink (e.g. Content-Type header).  From that perspective, we want to send *all*
the headers via OpenURL IPC.

Req#2: We don't want headers which Blink *always* adds, because

1) this is wasteful (after OpenURL roundtrips via //chrome back to //content and
Blink the headers will be readded anyway)

2) it confuses prerender - it A) passes extra_headers=X (empty today) to
NavigationController::LoadURLWithParams and B) wants to compare them with
OpenURLParams.extra_headers from another navigation (to see if it can reuse web
contents created in (A)).

I don't know how to reconcile those 2 requirements without hardcoding affected
headers *somewhere* (if not above, then in prerender?  or in overall Blink code
- by tweaking *when* Blink adds headers?  bleh...).
I don't understand what you mean by "NavigationState" :-( 
content::NavigationState doesn't look very interesting (i.e. doesn't seem to
contain http request headers).  There is content::NavigationStateImpl, but it
seems to only be used for RenderFrameImpl (and not for RemoteFrameProxy which is
the object that before this CL would send an incomplete OpenURL IPC to the
browser).

RE: presumably stored in StartNavigationParams

Right - when a renderer submits a form that targets a remote frame, we want to
send to the browser (in OpenURL IPC) the extra headers that this renderer has
put into WebURLRequest.  The browser will send those extra headers to the other
renderer (where the remote frame lives) via
StartNavigationParams::extra_headers.
Hmmm... it does apply to PlzNavigate - it has BeginNavigationParams::headers.  I
see that it gets populated in RenderFrameImpl::BeginNavigation right before it
sends a FrameHostMsg_BeginNavigation message to the browser.  FWIW, it uses
GetWebURLRequestHeadersForResourceRequest - it probably should use
GetWebURLRequestHeadersForOpenURLParams instead (so my CR comment is wrong in
https://codereview.chromium.org/2355023002/diff/60001/content/renderer/render...).

[Charlie Reis, 2016/09/30 23:44:44]

Yeah, I see how it's a bit awkward regardless of the approach we choose.
Yes, I meant NavigationStateImpl on RenderFrameImpl, but it's only present for
navigations that come down from the browser process.  I was thinking about your
LoadURLParams comment the wrong way-- that's after the OpenURL IPC is sent up to
the browser, and we care about grabbing the headers before.  Nevermind.

+
   request.visitHTTPHeaderFields(&flattener);
   return flattener.GetBuffer();
 }