Part 2.3: Is policy list subsumed under subsuming policy?

third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp

Index: third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp
diff --git a/third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp b/third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp
index 119921624da02cdff03ede62c515a9ff99f63e55..3e2312e95cc9fa2ce283b0a3f19af2b902b62e6d 100644
--- a/third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp
@@ -569,17 +569,67 @@ bool SourceListDirective::hasSourceMatchInList(
   return false;
 }
 
+HashMap<String, CSPSource*> SourceListDirective::getIntersectSchemesOnly(
+    HeapVector<Member<CSPSource>> other) {

[Mike West, 2016/11/23 09:19:18]

Why is this a vector of `Member<CSPSource>` and not `CSPSource*`?

[amalika, 2016/11/24 14:50:33]

Will be answered in overall comments.

+  HashMap<String, CSPSource *> schemesA, intersect;

[Mike West, 2016/11/23 09:19:18]

Style nits:

1. Please don't use `,` to declare more than one variable on a line.

2. Please declare |intersect| above the next `for` loop, as it's not used until
then.

[amalika, 2016/11/24 14:50:33]

Done.

+  for (const auto& sourceA : m_list) {
+    if (sourceA->isSchemeOnly())
+      sourceA->addToMap(schemesA);
+  }
+  // Add schemes only sources if they are present in both `this` and `other`,
+  // allowing upgrading `http` to `https` and `ws` to `wss`.
+  for (const auto& sourceB : other) {
+    if (sourceB->isSchemeOnly()) {
+      if (schemesA.contains(sourceB->getScheme()))
+        sourceB->addToMap(intersect);
+      else if (sourceB->getScheme() == "http" && schemesA.contains("https"))
+        intersect.add("https", schemesA.get("https"));
+      else if (sourceB->getScheme() == "ws" && schemesA.contains("wss"))
+        intersect.add("wss", schemesA.get("wss"));
+    }
+  }
+
+  return intersect;
+}
+
 HeapVector<Member<CSPSource>> SourceListDirective::getIntersectCSPSources(
-    HeapVector<Member<CSPSource>> otherVector) {
+    HeapVector<Member<CSPSource>> other) {

[Mike West, 2016/11/23 09:19:18]

Why is this a vector of `Member<CSPSource>` and not `CSPSource*`?

[amalika, 2016/11/24 14:50:33]

Similarly as above for `getIntersectCSPSourcesSchemes`.

+  HashMap<String, CSPSource*> schemesMap = getIntersectSchemesOnly(other);
   HeapVector<Member<CSPSource>> normalized;
-  for (const auto& aCspSource : m_list) {
-    Member<CSPSource> matchedCspSource(nullptr);
-    for (const auto& bCspSource : otherVector) {
-      if ((matchedCspSource = bCspSource->intersect(aCspSource)))
+  // Add all normalized scheme source expressions.
+  for (HashMap<String, CSPSource*>::iterator it = schemesMap.begin();
+       it != schemesMap.end(); ++it) {

[Mike West, 2016/11/23 09:19:18]

`const auto&`. It's clear from context that it's going to be an iterator.

+    // We do not "https:" if "http:" is present.

[Mike West, 2016/11/23 09:19:18]

1. I don't understand this comment.
2. If you're special-casing `http`, shouldn't you special-case `ws` as well?

+    if (it->key != "https" || !schemesMap.contains("http"))
+      normalized.append(it->value);
+  }
+
+  for (const auto& sourceA : m_list) {
+    if (schemesMap.contains(sourceA->getScheme()))

[Mike West, 2016/11/23 09:19:18]

Should you check |schemesMap| here, or |normalized|?

[amalika, 2016/11/24 13:25:30]

|schemesMap| because we want to skip scheme sources that were already added in
the iteration above through schemesMap.

In general, we want to add all the scheme source before iterating over here
because we might never end up adding scheme source from B.

+      continue;
+
+    Member<CSPSource> match(nullptr), localMatch(nullptr);

[Mike West, 2016/11/23 09:19:18]

Style: Two lines, no comma.

Also, `CSPSource*` is fine. This isn't a member variable.

[amalika, 2016/11/24 14:50:33]

Addressed!

+    for (const auto& sourceB : other) {
+      // No need to add a host source expression if it is subsumed by the
+      // matching scheme source epxression.

[Mike West, 2016/11/23 09:19:18]

Nit: s/epxression/expression/

+      if (schemesMap.contains(sourceB->getScheme()))

[Mike West, 2016/11/23 09:19:18]

Should you check |schemesMap| here, or |normalized|?

[amalika, 2016/11/24 13:25:30]

As above, I was intending on checking |schemesMap|.

Should we also check if |normalized| contains the source already as well?

In current implementation, we normalize the `height` (`height` is a number of
policies which we squash into one) but allow for unnormalized `width` (number of
CSPSources in a source list), i.e. there might be repetitions or things like
"https https://example.com" (where obviously the former alone would be
sufficient). 

If we try to normalize `width`, it would be less efficient since most CSPSource
will be added only once but having repetitions is not harmful. But let me know
if it is better to add checks?

+        continue;
+      // If sourceA is scheme only but there was no intersection for it in the
+      // `other` list, we add all the sourceB with that scheme.
+      if (sourceA->isSchemeOnly()) {
+        if ((localMatch = sourceB->intersect(sourceA)))
+          normalized.append(localMatch);
+        continue;
+      }
+      if (sourceB->subsumes(sourceA)) {
+        match = sourceA;
         break;
+      }
+      if ((localMatch = sourceB->intersect(sourceA)))
+        match = localMatch;
     }
-    if (matchedCspSource)
-      normalized.append(matchedCspSource);
+    if (match)
+      normalized.append(match);
   }
   return normalized;
 }