Remove DCHECK in QueryContainsComponentHelper

page_load_metrics::IsGoogleSearchResultUrl() can, if given a URL where
the query identifier is after the fragment identifier (e.g.,
https://www.google.com/webmasters/#?modal_active=none), cause a DCHECK
to fail in QueryContainsComponentHelper.

This adds a unit test which triggers the DCHECK failure. It also removes
the DCHECK in favor of trimming initial [?#] characters.

Change-Id: Ibdc5d6d0aee2089deea2c777d94c0dd70ef1ffd0
Bug: 805155
Reviewed-on: https://chromium-review.googlesource.com/882211Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Commit-Queue: Christopher Thompson <cthomp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532595}

chrome/browser/page_load_metrics/page_load_metrics_util.cc

@@ -50,14 +50,18 @@ bool QueryContainsComponentHelper(const base::StringPiece query,
     return false;
   }
 
-  // Verify that the provided query string does not include the query or
-  // fragment start character, as the logic below depends on this character not
-  // being included.
-  DCHECK(query[0] != '?' && query[0] != '#');
+  // Ensures that the first character of |query| is not a query or fragment
+  // delimiter character (? or #). Including it can break the later test for
+  // |component| being at the start of the query string.
+  // Note: This heuristic can cause a component string that starts with one of
+  // these characters to not match a query string which contains it at the
+  // beginning.
+  const base::StringPiece trimmed_query =
+      base::TrimString(query, "?#", base::TrimPositions::TRIM_LEADING);
 
   // We shouldn't try to find matches beyond the point where there aren't enough
   // characters left in query to fully match the component.
-  const size_t last_search_start = query.length() - component.length();
+  const size_t last_search_start = trimmed_query.length() - component.length();
 
   // We need to search for matches in a loop, rather than stopping at the first
   // match, because we may initially match a substring that isn't a full query
@@ -70,14 +74,14 @@ bool QueryContainsComponentHelper(const base::StringPiece query,
   // successful component match.
   for (size_t start_offset = 0; start_offset <= last_search_start;
        start_offset += component.length()) {
-    start_offset = query.find(component, start_offset);
+    start_offset = trimmed_query.find(component, start_offset);
     if (start_offset == std::string::npos) {
       // We searched to end of string and did not find a match.
       return false;
     }
     // Verify that the character prior to the component is valid (either we're
     // at the beginning of the query string, or are preceded by an ampersand).
-    if (start_offset != 0 && query[start_offset - 1] != '&') {
+    if (start_offset != 0 && trimmed_query[start_offset - 1] != '&') {
       continue;
     }
     if (!component_is_prefix) {
@@ -85,7 +89,8 @@ bool QueryContainsComponentHelper(const base::StringPiece query,
       // (either we're at the end of the query string, or are followed by an
       // ampersand).
       const size_t after_offset = start_offset + component.length();
-      if (after_offset < query.length() && query[after_offset] != '&') {
+      if (after_offset < trimmed_query.length() &&
+          trimmed_query[after_offset] != '&') {
         continue;
       }
     }

chrome/browser/page_load_metrics/page_load_metrics_util.h

@@ -165,6 +165,10 @@ bool IsGoogleSearchRedirectorUrl(const GURL& url);
 // 'zzzfoo'. For QueryContainsComponent, the component should of the form
 // 'key=value'. For QueryContainsComponentPrefix, the component should be of
 // the form 'key=' (where the value is not specified).
+// Note: The heuristic used by these functions will not find a component at the
+// beginning of the query string if the component starts with a delimiter
+// character ('?' or '#'). For example, '?foo=bar' will match the query string
+// 'a=b&?foo=bar' but not the query string '?foo=bar&a=b'.
 bool QueryContainsComponent(const base::StringPiece query,
                             const base::StringPiece component);
 bool QueryContainsComponentPrefix(const base::StringPiece query,

chrome/browser/page_load_metrics/page_load_metrics_util_unittest.cc

@@ -124,6 +124,8 @@ TEST_F(PageLoadMetricsUtilTest, IsGoogleSearchResultUrl) {
       {false, "http://www.example.com/"},
       {false, "https://www.example.com/webhp?q=test"},
       {false, "https://google.com/#q=test"},
+      // Regression test for crbug.com/805155
+      {false, "https://www.google.com/webmasters/#?modal_active=none"},
   };
   for (const auto& test : test_cases) {
     EXPECT_EQ(test.expected_result,
@@ -190,6 +192,9 @@ TEST_F(PageLoadMetricsUtilTest, QueryContainsComponent) {
       {false, "a=b&foosource=web&c=d", "source=web"},
       {false, "a=b&source=webbar&c=d", "source=web"},
       {false, "a=b&foosource=webbar&c=d", "source=web"},
+      // Correctly handle cases where there is a leading "?" or "#" character.
+      {true, "?a=b&source=web", "a=b"},
+      {false, "a=b&?source=web", "source=web"},
   };
   for (const auto& test : test_cases) {
     EXPECT_EQ(test.expected_result, page_load_metrics::QueryContainsComponent(