Refuse invalid SearchProvider and OSDD suggest URLs; etc.

chrome/browser/search_engines/template_url_parser.cc

Index: chrome/browser/search_engines/template_url_parser.cc
diff --git a/chrome/browser/search_engines/template_url_parser.cc b/chrome/browser/search_engines/template_url_parser.cc
index f48eb391dd569558e8ed09cf48574cb92e055109..4ab1e2d676e4a239d44ccf6543377c5be92421ad 100644
--- a/chrome/browser/search_engines/template_url_parser.cc
+++ b/chrome/browser/search_engines/template_url_parser.cc
@@ -144,7 +144,7 @@ class TemplateURLParsingContext {
   // Returns a heap-allocated TemplateURL representing the result of parsing.
   // This will be NULL if parsing failed or if the results were invalid for some
   // reason (e.g. the resulting URL was not HTTP[S], a name wasn't supplied,
-  // etc.).
+  // a resulting TempateURLRef was invalid, etc.).
   TemplateURL* GetTemplateURL(Profile* profile, bool show_in_default_list);
 
  private:
@@ -291,27 +291,31 @@ void TemplateURLParsingContext::CharactersImpl(void* ctx,
 TemplateURL* TemplateURLParsingContext::GetTemplateURL(
     Profile* profile,
     bool show_in_default_list) {
-  // Basic legality checks.
-  if (data_.short_name.empty() || !IsHTTPRef(data_.url()) ||
-      !IsHTTPRef(data_.suggestions_url))
+  // TODO(jcampan): Support engines that use POST; see http://crbug.com/18107
+  if (method_ == TemplateURLParsingContext::POST ||
+      data_.short_name.empty() || data_.url().empty() ||

[Peter Kasting, 2012/09/05 23:31:27]

There's no need to add the check for data_.url().empty(); TemplateURLData
guarantees this field is never empty.

[msw, 2012/09/06 20:02:29]

Done.

+      !IsHTTPRef(data_.url()) || !IsHTTPRef(data_.suggestions_url))
     return NULL;
+  if (suggestion_method_ == TemplateURLParsingContext::POST)
+    data_.suggestions_url.clear();
 
   // If the image was a data URL, use the favicon from the search URL instead.
-  // (see TODO inEndElementImpl()).
-  GURL url(data_.url());
+  // (see the TODO in EndElementImpl()).
+  GURL search_url(data_.url());

[Peter Kasting, 2012/09/05 23:31:27]

Urf.  Both GenerateFaviconURL() and GenerateKeyword() below DCHECK that
|search_url| is valid.  But I don't think that's actually guaranteed by the
parsing code.  (I think I screwed this up refactoring it months back.)

I think what we need to do here is construct a "temporary" TemplateURL from
|data_|, then call ReplaceSearchTerms() with "x" (or whatever) as the dummy
search string, and use _that_ URL as the search_url here.  We can then check
this for validity (and check that suggestions_url() is either empty() or the
result of ReplaceSearchTerms() on it is valid as well) below; this can replace
the IsValid() calls on the TemplateURLRefs because if those are not valid,
ReplaceSearchTerms() will return the empty string.

The annoying bit about this is that we basically have to construct a TemplateURL
twice, but I don't see what we can do about that.

[msw, 2012/09/06 20:02:29]

Invalid search/suggest URLs bail above in the IsHTTPRef checks anyway.
Adding full support for invalid URLs is out of scope for this *crash fix*.

Removed the DCHECKs anyway, as the functions fallback okay for invalid GURLs:
GenerateFaviconURL returns GURL() for optional TemplateURLData::favicon_url.
GenerateKeyword returns "www" or some best effort via GURL::host(), editable.

   if (derive_image_from_url_ && data_.favicon_url.is_empty())
-    data_.favicon_url = TemplateURL::GenerateFaviconURL(url);
+    data_.favicon_url = TemplateURL::GenerateFaviconURL(search_url);
+
+  data_.SetKeyword(TemplateURLService::GenerateKeyword(search_url));
+  data_.show_in_default_list = show_in_default_list;
 
-  // TODO(jcampan): http://b/issue?id=1196285 we do not support search engines
-  //                that use POST yet.
-  if (method_ == TemplateURLParsingContext::POST)
+  // Bail if the search URL is empty or if either TemplateURLRef is invalid.
+  scoped_ptr<TemplateURL> template_url(new TemplateURL(profile, data_));
+  if (template_url->url().empty() || !template_url->url_ref().IsValid() ||
+      (!template_url->suggestions_url().empty() &&
+       !template_url->suggestions_url_ref().IsValid()))
     return NULL;
-  if (suggestion_method_ == TemplateURLParsingContext::POST)
-    data_.suggestions_url.clear();
 
-  data_.SetKeyword(TemplateURLService::GenerateKeyword(url));
-  data_.show_in_default_list = show_in_default_list;
-  return new TemplateURL(profile, data_);
+  return template_url.release();
 }
 
 // static