Ignore ThreatMatch responses for lists that we don't care about.

Ignore ThreatMatch responses for lists that we don't care about.

Due to aliasing used on the server side, the server may send ThreatMatch
response for lists that we don't track. For instance, ThreatMatch with
platform_type as CHROME on Linux. Ignore such ThreatMatch responses.

BUG=663247
Committed: https://crrev.com/bfcde5d0640845a210555226cd7de95cb5bb63b4
Cr-Commit-Position: refs/heads/master@{#431131}

[vakh (use Gerrit instead), 2016-11-08 22:35:08]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2016-11-08 22:35:32]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org, shess@chromium.org

[commit-bot: I haz the power, 2016-11-08 22:36:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 23:13:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 23:13:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nathan Parker, 2016-11-08 23:50:11]

lgtm

https://codereview.chromium.org/2490753002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right):

https://codereview.chromium.org/2490753002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:530: const
PlatformType& platform_type = match.platform_type();
Nit: these temporaries add a lot more chars then they save and don't improve to
readability.  How about:

ListIdentifier list_id(platform_type, threat_entry_type, threat_type);

if (!ContainsValue(platform_types, list_id.platform_type()) |\
    .. threat_entry_types_.. ||
    .. threat_type_..)

https://codereview.chromium.org/2490753002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc
(right):

https://codereview.chromium.org/2490753002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:298: // The
order of these is important.
How does the order matter?

[Scott Hess - ex-Googler, 2016-11-09 00:55:36]

lgtm.

Agree with Nathan's point about just using the list_id.

[vakh (use Gerrit instead), 2016-11-10 00:02:15]

nparker+shess review

[vakh (use Gerrit instead), 2016-11-10 00:02:17]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2016-11-10 00:02:31]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-10 00:03:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 00:42:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 00:42:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vakh (use Gerrit instead), 2016-11-10 01:02:17]

Use a set to order platform_types etc to ensure order across platforms

[vakh (use Gerrit instead), 2016-11-10 01:02:19]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 01:02:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2016-11-10 01:03:22]

Patchset #2 (id:40001) has been deleted

[vakh (use Gerrit instead), 2016-11-10 01:05:56]

https://codereview.chromium.org/2490753002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right):

https://codereview.chromium.org/2490753002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:530: const
PlatformType& platform_type = match.platform_type();
On 2016/11/08 23:50:11, Nathan Parker wrote:
> Nit: these temporaries add a lot more chars then they save and don't improve
to
> readability.  How about:
> 
> ListIdentifier list_id(platform_type, threat_entry_type, threat_type);
> 
> if (!ContainsValue(platform_types, list_id.platform_type()) |\
>     .. threat_entry_types_.. ||
>     .. threat_type_..) 
> 

Done.

https://codereview.chromium.org/2490753002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc
(right):

https://codereview.chromium.org/2490753002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:298: // The
order of these is important.
On 2016/11/08 23:50:11, Nathan Parker wrote:
> How does the order matter?

The order mattered because if they are not in the same order as they are in the
request generated by product code, then the base64 encoded strings won't match.

Turns out, the order in which the unordered_set returns values varies by
platform (which isn't wrong) so I changed the code to sort incoming
platform_types, threat_entry_types, and threat_types using a set and then store
them in a vector. That makes the order predictable.

[vakh (use Gerrit instead), 2016-11-10 01:06:36]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-11-10 01:06:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2490753002/#ps60001
(title: "Use a set to order platform_types etc to ensure order across
platforms")

[commit-bot: I haz the power, 2016-11-10 01:06:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 01:45:44]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-11-10 01:51:40]

Description was changed from

==========
Ignore ThreatMatch responses for lists that we don't care about.

Due to aliasing used on the server side, the server may send ThreatMatch
response for lists that we don't track. For instance, ThreatMatch with
platform_type as CHROME on Linux. Ignore such ThreatMatch responses.

BUG=663247
==========

to

==========
Ignore ThreatMatch responses for lists that we don't care about.

Due to aliasing used on the server side, the server may send ThreatMatch
response for lists that we don't track. For instance, ThreatMatch with
platform_type as CHROME on Linux. Ignore such ThreatMatch responses.

BUG=663247

Committed: https://crrev.com/bfcde5d0640845a210555226cd7de95cb5bb63b4
Cr-Commit-Position: refs/heads/master@{#431131}
==========

[commit-bot: I haz the power, 2016-11-10 01:51:42]

Patchset 2 (id:??) landed as
https://crrev.com/bfcde5d0640845a210555226cd7de95cb5bb63b4
Cr-Commit-Position: refs/heads/master@{#431131}