Parse out XSSI guards in Suggestion JSON response

chrome/browser/autocomplete/search_provider.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/autocomplete/search_provider.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/callback.h"
@@ skipping 582 matching lines @@
 
   if (clear_cached_results)
     ClearAllResults();
 }
 
 void SearchProvider::OnURLFetchComplete(const net::URLFetcher* source) {
   DCHECK(!done_);
   suggest_results_pending_--;
   LogOmniboxSuggestRequest(REPLY_RECEIVED);
   DCHECK_GE(suggest_results_pending_, 0);  // Should never go negative.
-  const net::HttpResponseHeaders* const response_headers =
-      source->GetResponseHeaders();
-  std::string json_data;
-  source->GetResponseAsString(&json_data);
-  // JSON is supposed to be UTF-8, but some suggest service providers send JSON
-  // files in non-UTF-8 encodings.  The actual encoding is usually specified in
-  // the Content-Type header field.
-  if (response_headers) {
-    std::string charset;
-    if (response_headers->GetCharset(&charset)) {
-      string16 data_16;
-      // TODO(jungshik): Switch to CodePageToUTF8 after it's added.
-      if (base::CodepageToUTF16(json_data, charset.c_str(),
-                                base::OnStringConversionError::FAIL,
-                                &data_16))
-        json_data = UTF16ToUTF8(data_16);
-    }
-  }
 
   const bool is_keyword = (source == keyword_fetcher_.get());
   // Ensure the request succeeded and that the provider used is still available.
   // A verbatim match cannot be generated without this provider, causing errors.
   const bool request_succeeded =
       source->GetStatus().is_success() && (source->GetResponseCode() == 200) &&
       (is_keyword ?
           providers_.GetKeywordProviderURL() :
           providers_.GetDefaultProviderURL());
 
@@ skipping 10 matching lines @@
       UMA_HISTOGRAM_TIMES("Omnibox.SuggestRequest.Success.GoogleResponseTime",
                           elapsed_time);
     } else {
       UMA_HISTOGRAM_TIMES("Omnibox.SuggestRequest.Failure.GoogleResponseTime",
                           elapsed_time);
     }
   }
 
   bool results_updated = false;
   if (request_succeeded) {
-    JSONStringValueSerializer deserializer(json_data);
-    deserializer.set_allow_trailing_comma(true);
-    scoped_ptr<Value> data(deserializer.Deserialize(NULL, NULL));
-    results_updated = data.get() && ParseSuggestResults(data.get(), is_keyword);
+    const net::HttpResponseHeaders* const response_headers =
+        source->GetResponseHeaders();
+    std::string json_data;
+    source->GetResponseAsString(&json_data);
+    // JSON is supposed to be UTF-8, but some suggest service providers send
+    // JSON files in non-UTF-8 encodings.  The actual encoding is usually
+    // specified in the Content-Type header field.
+    if (response_headers) {
+      std::string charset;
+      if (response_headers->GetCharset(&charset)) {
+        string16 data_16;
+        // TODO(jungshik): Switch to CodePageToUTF8 after it's added.
+        if (base::CodepageToUTF16(json_data, charset.c_str(),
+                                  base::OnStringConversionError::FAIL,
+                                  &data_16))
+          json_data = UTF16ToUTF8(data_16);
+      }
+    }
+
+    int error_code = 0;
+    while (true) {

[Peter Kasting, 2013/10/29 18:45:49]

This can infinite loop.

If we find the first '[', trim everything before that, then fail to
Deserialize(), we will loop, and the find() call will return 0 since the same
'[' we found the previous time is still present.

Why does there need to be a loop around this at all?  Why can't we just do all
this once?

[Maria, 2013/10/29 18:55:45]

I don't think it's likely that XSSI guard would change either. You could
actually look specifically for the guard characters at the start of the string
instead of just any opening square bracket to ensure that we are handling the
case we are expecting instead of possibly an error message with brackets in it.

[Anuj, 2013/10/29 18:58:05]

Please see the test cases - Hypothetically XSSI guard can itself contain "[".
I think I ran the test after uploading, and didn't upload the fix.

[Anuj, 2013/10/29 19:03:35]

XSSI guard may not change. But looking for response seems a better strategy to
me than stripping out specific XSSI guard.

And in any case, stripping XSSI guard doesn't help against the error message
scenario either.

+      // The JSON response should be an array.
+      size_t response_start_index = json_data.find("[");
+
+      if (response_start_index == std::string::npos)
+        break;
+
+      // Remove any XSSI guards to allow for JSON parsing.
+      if (response_start_index > 0)
+        json_data.erase(0, response_start_index - 1);
+
+      JSONStringValueSerializer deserializer(json_data);
+      deserializer.set_allow_trailing_comma(true);
+      scoped_ptr<Value> data(deserializer.Deserialize(&error_code, NULL));
+      if (error_code == 0) {
+        results_updated = data.get() &&
+            ParseSuggestResults(data.get(), is_keyword);
+        break;
+      }
+    }
   }
 
   UpdateMatches();
   if (done_ || results_updated)
     listener_->OnProviderUpdate(results_updated);
 }
 
 void SearchProvider::Run() {
   // Start a new request with the current input.
   suggest_results_pending_ = 0;
@@ skipping 1098 matching lines @@
     it->set_relevance(max_query_relevance);
     it->set_relevance_from_server(relevance_from_server);
   }
 }
 
 void SearchProvider::UpdateDone() {
   // We're done when the timer isn't running, there are no suggest queries
   // pending, and we're not waiting on Instant.
   done_ = !timer_.IsRunning() && (suggest_results_pending_ == 0);
 }