Failure to parse full hash metadata shouldn't discard the response

components/safe_browsing_db/v4_get_hash_protocol_manager.cc

 // Copyright 2016 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 "components/safe_browsing_db/v4_get_hash_protocol_manager.h"
 
 #include <utility>
 
 #include "base/base64url.h"
 #include "base/macros.h"
@@ skipping 523 matching lines @@
     base::Time positive_expiry;
     if (match.has_cache_duration()) {
       // Seconds resolution is good enough so we ignore the nanos field.
       positive_expiry = clock_->Now() + TimeDelta::FromSeconds(
                                             match.cache_duration().seconds());
     } else {
       positive_expiry = clock_->Now() - base::TimeDelta::FromSeconds(1);
     }
     FullHashInfo full_hash_info(match.threat().hash(), list_id,
                                 positive_expiry);
-    if (!ParseMetadata(match, &full_hash_info.metadata)) {
-      return false;
-    }
-
+    ParseMetadata(match, &full_hash_info.metadata);
     full_hash_infos->push_back(full_hash_info);

[Nathan Parker, 2016/10/27 22:06:49]

Do we want to add this full_hash_info if the metadata for it failed to parse? 
If not, then this CL could be just one line of skipping the push_back if
!ParseMetadata.

It depends on how the metadata will fail and what the down-stream expectations
are of it... I think either way has pros/cons.

[vakh (use Gerrit instead), 2016/10/27 22:40:04]

Yes, we still want to. See the scenario in: http://crbug.com/660133#c2

   }
   return true;
 }
 
-bool V4GetHashProtocolManager::ParseMetadata(const ThreatMatch& match,
+// static
+void V4GetHashProtocolManager::ParseMetadata(const ThreatMatch& match,
                                              ThreatMetadata* metadata) {
   // Different threat types will handle the metadata differently.
   if (match.threat_type() == API_ABUSE) {
     if (!match.has_platform_type() ||
         match.platform_type() != CHROME_PLATFORM) {
       RecordParseGetHashResult(UNEXPECTED_PLATFORM_TYPE_ERROR);
-      return false;
+      return;
     }
 
     if (!match.has_threat_entry_metadata()) {
       RecordParseGetHashResult(NO_METADATA_ERROR);
-      return false;
+      return;
     }
     // For API Abuse, store a list of the returned permissions.
     for (const ThreatEntryMetadata::MetadataEntry& m :
          match.threat_entry_metadata().entries()) {
       if (m.key() != kPermission) {
         RecordParseGetHashResult(UNEXPECTED_METADATA_VALUE_ERROR);
-        return false;
+        return;
       }
       metadata->api_permissions.insert(m.value());
     }
   } else if (match.threat_type() == MALWARE_THREAT ||
              match.threat_type() == POTENTIALLY_HARMFUL_APPLICATION) {
     for (const ThreatEntryMetadata::MetadataEntry& m :
          match.threat_entry_metadata().entries()) {
       // TODO: Need to confirm the below key/value pairs with CSD backend.

[Nathan Parker, 2016/10/27 22:06:49]

Is this TODO still valid?

[vakh (use Gerrit instead), 2016/10/27 22:40:05]

Yes, I'll check with the CSD folks.

       if (m.key() == kPhaPatternType || m.key() == kMalwarePatternType) {
         if (m.value() == kLanding) {
           metadata->threat_pattern_type = ThreatPatternType::MALWARE_LANDING;
           break;
         } else if (m.value() == kDistribution) {
           metadata->threat_pattern_type =
               ThreatPatternType::MALWARE_DISTRIBUTION;
           break;
         } else {
           RecordParseGetHashResult(UNEXPECTED_METADATA_VALUE_ERROR);
-          return false;
+          return;
         }
       }
     }
   } else if (match.threat_type() == SOCIAL_ENGINEERING_PUBLIC) {
     for (const ThreatEntryMetadata::MetadataEntry& m :
          match.threat_entry_metadata().entries()) {
       if (m.key() == kSePatternType) {
         if (m.value() == kSocialEngineeringAds) {
           metadata->threat_pattern_type =
               ThreatPatternType::SOCIAL_ENGINEERING_ADS;
           break;
         } else if (m.value() == kSocialEngineeringLanding) {
           metadata->threat_pattern_type =
               ThreatPatternType::SOCIAL_ENGINEERING_LANDING;
           break;
         } else if (m.value() == kPhishing) {
           metadata->threat_pattern_type = ThreatPatternType::PHISHING;
           break;
         } else {
           RecordParseGetHashResult(UNEXPECTED_METADATA_VALUE_ERROR);
-          return false;
+          return;
         }
       }
     }
-  } else {
+  } else if (match.has_threat_entry_metadata() &&
+             match.threat_entry_metadata().entries_size() > 1) {
     RecordParseGetHashResult(UNEXPECTED_THREAT_TYPE_ERROR);
-    return false;
   }
-
-  return true;
 }
 
 void V4GetHashProtocolManager::ResetGetHashErrors() {
   gethash_error_count_ = 0;
   gethash_back_off_mult_ = 1;
 }
 
 void V4GetHashProtocolManager::SetClockForTests(
     std::unique_ptr<base::Clock> clock) {
   clock_ = std::move(clock);
@@ skipping 111 matching lines @@
 std::ostream& operator<<(std::ostream& os, const FullHashInfo& fhi) {
   os << "{full_hash: " << fhi.full_hash << "; list_id: " << fhi.list_id
      << "; positive_expiry: " << fhi.positive_expiry
      << "; metadata.api_permissions.size(): "
      << fhi.metadata.api_permissions.size() << "}";
   return os;
 }
 #endif
 
 }  // namespace safe_browsing