Improve merging of header modifications in webRequest.OnHeadersReceived

chrome/browser/extensions/extension_webrequest_api_helpers.cc

Index: chrome/browser/extensions/extension_webrequest_api_helpers.cc
diff --git a/chrome/browser/extensions/extension_webrequest_api_helpers.cc b/chrome/browser/extensions/extension_webrequest_api_helpers.cc
index d9357784c53d30957b4ea059ca09b68d2c6a8b14..22251126f9313f6bf9090774952dbd047e3efee1 100644
--- a/chrome/browser/extensions/extension_webrequest_api_helpers.cc
+++ b/chrome/browser/extensions/extension_webrequest_api_helpers.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/extensions/extension_webrequest_api_helpers.h"
 
+#include "base/string_util.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/extension_webrequest_api.h"
 #include "net/http/http_util.h"
@@ -165,21 +166,54 @@ EventResponseDelta* CalculateOnHeadersReceivedDelta(
     const std::string& extension_id,
     const base::Time& extension_install_time,
     bool cancel,
-    const std::string& status_line,
-    const std::string& response_headers_string) {
+    net::HttpResponseHeaders* old_response_headers,
+    ResponseHeaders* new_response_headers) {
   EventResponseDelta* result =
       new EventResponseDelta(extension_id, extension_install_time);
   result->cancel = cancel;
 
-  if (!response_headers_string.empty()) {
-    std::string new_headers_string =
-        status_line + "\n" + response_headers_string;
+  if (!new_response_headers)
+    return result;
 
-    result->new_response_headers =
-        new net::HttpResponseHeaders(
-            net::HttpUtil::AssembleRawHeaders(new_headers_string.c_str(),
-                                              new_headers_string.length()));
+  // Find deleted headers (header keys are treated case insensitively).
+  {
+    void* iter = NULL;
+    std::string name;
+    std::string value;
+    while (old_response_headers->EnumerateHeaderLines(&iter, &name, &value)) {
+      std::string name_lowercase(name);
+      StringToLowerASCII(&name_lowercase);
+
+      bool header_found = false;
+      for (ResponseHeaders::const_iterator i = new_response_headers->begin();
+           i != new_response_headers->end(); ++i) {
+        if (LowerCaseEqualsASCII(i->first, name_lowercase.c_str()) &&
+            value == i->second) {
+          header_found = true;
+          break;
+        }
+      }
+      if (!header_found)
+        result->deleted_response_headers.push_back(ResponseHeader(name, value));
+    }
   }
+
+  // Find added headers (header keys are treated case insensitively).
+  {
+    for (ResponseHeaders::const_iterator i = new_response_headers->begin();
+         i != new_response_headers->end(); ++i) {
+      void* iter = NULL;
+      std::string value;
+      bool header_found = false;
+      while (old_response_headers->EnumerateHeader(&iter, i->first, &value) &&
+             !header_found) {
+        header_found = (value == i->second);
+      }
+      if (!header_found)
+        result->added_response_headers.push_back(*i);
+    }
+  }
+
   return result;
 }
 
@@ -346,33 +380,82 @@ void MergeOnBeforeSendHeadersResponses(
   }
 }
 
+// Converts the key of the (key, value) pair to lower case.
+ResponseHeader toLowerCase(const ResponseHeader& header) {

[Matt Perry, 2011/11/11 22:05:08]

ToLowerCase

also make this static, or put it in an anonymous namespace

[battre, 2011/11/17 12:01:04]

Done.

+  std::string lower_key(header.first);
+  StringToLowerASCII(&lower_key);
+  return ResponseHeader(lower_key, header.second);
+}
+
 void MergeOnHeadersReceivedResponses(
     const EventResponseDeltas& deltas,
+    const net::HttpResponseHeaders* original_response_headers,
     scoped_refptr<net::HttpResponseHeaders>* override_response_headers,
     std::set<std::string>* conflicting_extensions,
     EventLogEntries* event_log_entries) {
   EventResponseDeltas::const_iterator delta;
 
-  // Whether any extension has overridden the response headers, yet.
-  bool headers_overridden = false;
+  // Here we collect which headers we have removed or added so far due to
+  // extensions of higher precedence. Header keys are always stored as
+  // lower case.
+  std::set<ResponseHeader> removed_headers;
+  std::set<ResponseHeader> added_headers;
 
   // We assume here that the deltas are sorted in decreasing extension
   // precedence (i.e. decreasing extension installation time).
   for (delta = deltas.begin(); delta != deltas.end(); ++delta) {
-    if (!(*delta)->new_response_headers.get())
+    if ((*delta)->added_response_headers.empty() &&
+        (*delta)->deleted_response_headers.empty()) {
       continue;
+    }
 
-    scoped_refptr<NetLogModificationParameter> log(
-        new NetLogModificationParameter((*delta)->extension_id));
+    // Only create a copy if we really want to modify the response headers.
+    if (override_response_headers->get() == NULL) {
+      *override_response_headers = new net::HttpResponseHeaders(
+          original_response_headers->raw_headers());
+    }
 
-    // Conflict if a second extension returned new response headers;
-    bool extension_conflicts = headers_overridden;
+    // We consider modifications as pairs of (delete, add) operations.
+    // If a header is deleted twice by different extensions we assume that the
+    // intention was to modify it to different values and consider this a

[Matt Perry, 2011/11/11 22:05:08]

this seems like a strange assumption to me

[battre, 2011/11/17 12:01:04]

Do you have a better proposal?

Say you have a response header "Foo: 1". Extension A changes it to "Foo: 2" and
extension B to "Foo: 3". If you allow deleting twice and use "Add" and "Delete"
as the only operations considered, you would end up with "Foo: 2" and "Foo: 3"
in the conflict resolved output.

In the case of outgoing requests we look at only the header key (if extension A
and B fiddled with "Foo", there is a conflict). This does not work for incoming
responses. E.g. the "Set-Cookie" key occurs several times in a request.

I hope I made my point clear. Otherwise, I can construct a set of examples where
1) We cannot sensibly define a "Edit" operation as long as multiple response
headers can share the same key.
2) We cannot sensibly conclude what the extension tried to do (delete a header
or modify a header)

[Matt Perry, 2011/11/17 20:04:09]

It still seems possible to me to distinguish between "2 extensions modify header
Foo", and "2 extensions delete header Foo". In your example, if both extensions
delete header "Foo" and do not add another one, there is no conflict. You'd have
to examine both the deleted and added headers to decide.

That said, I don't think it's that important an edge case to worry about right
now, so I'm OK with the code as is.

+    // conflict. As deltas is sorted by decreasing extension installation order,
+    // this takes care of precedence.
+    bool extension_conflicts = false;
+    ResponseHeaders::const_iterator i;
+    for (i = (*delta)->deleted_response_headers.begin();
+         i != (*delta)->deleted_response_headers.end(); ++i) {
+      if (removed_headers.find(toLowerCase(*i)) != removed_headers.end()) {
+        extension_conflicts = true;
+        break;
+      }
+    }
 
+    // Now execute the modifications if there were no conflicts.
     if (!extension_conflicts) {
-      headers_overridden = true;
-      *override_response_headers = (*delta)->new_response_headers;
+      // Delete headers
+      {
+        for (i = (*delta)->deleted_response_headers.begin();
+             i != (*delta)->deleted_response_headers.end(); ++i) {
+          (*override_response_headers)->RemoveHeader(i->first, i->second);
+          removed_headers.insert(toLowerCase(*i));
+        }
+      }
+
+      // Add headers.
+      {
+        for (i = (*delta)->added_response_headers.begin();
+             i != (*delta)->added_response_headers.end(); ++i) {
+          ResponseHeader lowercase_header(toLowerCase(*i));
+          if (added_headers.find(lowercase_header) != added_headers.end())
+            continue;
+          added_headers.insert(lowercase_header);
+          (*override_response_headers)->AddHeader(i->first + ": " + i->second);
+        }
+      }
       EventLogEntry log_entry(
-          net::NetLog::TYPE_CHROME_EXTENSION_MODIFIED_HEADERS, log);
+          net::NetLog::TYPE_CHROME_EXTENSION_MODIFIED_HEADERS,
+          make_scoped_refptr(
+              new NetLogModificationParameter((*delta)->extension_id)));
       event_log_entries->push_back(log_entry);
     } else {
       conflicting_extensions->insert((*delta)->extension_id);