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

Failure to parse full hash metadata shouldn't discard the response. TBR since kcarattini is OOO.

BUG=660133
TBR=kcarattini

Committed: https://crrev.com/4588549b86714ddb6b770a7ef420043c49b6af4e
Cr-Commit-Position: refs/heads/master@{#428218}

[vakh (use Gerrit instead), 2016-10-27 20:53:11]

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

[vakh (use Gerrit instead), 2016-10-27 20:53:45]

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

[commit-bot: I haz the power, 2016-10-27 20:54:46]

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-10-27 20:55:47]

Nit: Remove extra return

[vakh (use Gerrit instead), 2016-10-27 20:55:49]

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

[commit-bot: I haz the power, 2016-10-27 20:56:46]

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-10-27 22:06:01]

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

[commit-bot: I haz the power, 2016-10-27 22:06:02]

Dry run: This issue passed the CQ dry run.

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

lgtm

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right):

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:545:
full_hash_infos->push_back(full_hash_info);
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.

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:578: // TODO: Need
to confirm the below key/value pairs with CSD backend.
Is this TODO still valid?

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right):

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.h:275: // errors if the
metadata information was not parsed correctly or was
nit: "Logs errors to UMA if the metadata..."

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc
(right):

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:476:
ASSERT_EQ(1ul, full_hash_infos.size());
nit: Add a comment: The PHA threat still remains since it's valid.
(same below)

[vakh (use Gerrit instead), 2016-10-27 22:35:33]

Nits: Updated comments from nparker@'s review

[vakh (use Gerrit instead), 2016-10-27 22:35:35]

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

[commit-bot: I haz the power, 2016-10-27 22:36:15]

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-10-27 22:40:05]

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right):

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:545:
full_hash_infos->push_back(full_hash_info);
On 2016/10/27 at 22:06:49, Nathan Parker wrote:
> 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.

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

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:578: // TODO: Need
to confirm the below key/value pairs with CSD backend.
On 2016/10/27 22:06:49, Nathan Parker wrote:
> Is this TODO still valid?

Yes, I'll check with the CSD folks.

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right):

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.h:275: // errors if the
metadata information was not parsed correctly or was
On 2016/10/27 22:06:49, Nathan Parker wrote:
> nit: "Logs errors to UMA if the metadata..."

Done.

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc
(right):

https://codereview.chromium.org/2458743003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:476:
ASSERT_EQ(1ul, full_hash_infos.size());
On 2016/10/27 22:06:49, Nathan Parker wrote:
> nit: Add a comment: The PHA threat still remains since it's valid.
> (same below)

Done.

[commit-bot: I haz the power, 2016-10-27 23:33:25]

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

[commit-bot: I haz the power, 2016-10-27 23:33:26]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2016-10-27 23:34:16]

Description was changed from

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

BUG=660133
==========

to

==========
Failure to parse full hash metadata shouldn't discard the response. TBR since
kcarattini is OOO.

BUG=660133

TBR=kcarattini
==========

[vakh (use Gerrit instead), 2016-10-27 23:34:22]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-10-27 23:34:23]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2458743003/#ps40001
(title: "Nits: Updated comments from nparker@'s review")

[commit-bot: I haz the power, 2016-10-27 23:35:06]

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-10-27 23:40:28]

Description was changed from

==========
Failure to parse full hash metadata shouldn't discard the response. TBR since
kcarattini is OOO.

BUG=660133

TBR=kcarattini
==========

to

==========
Failure to parse full hash metadata shouldn't discard the response. TBR since
kcarattini is OOO.

BUG=660133

TBR=kcarattini
==========

[commit-bot: I haz the power, 2016-10-27 23:40:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-27 23:42:25]

Description was changed from

==========
Failure to parse full hash metadata shouldn't discard the response. TBR since
kcarattini is OOO.

BUG=660133

TBR=kcarattini
==========

to

==========
Failure to parse full hash metadata shouldn't discard the response. TBR since
kcarattini is OOO.

BUG=660133

TBR=kcarattini

Committed: https://crrev.com/4588549b86714ddb6b770a7ef420043c49b6af4e
Cr-Commit-Position: refs/heads/master@{#428218}
==========

[commit-bot: I haz the power, 2016-10-27 23:42:25]

Patchset 3 (id:??) landed as
https://crrev.com/4588549b86714ddb6b770a7ef420043c49b6af4e
Cr-Commit-Position: refs/heads/master@{#428218}

[kcarattini, 2016-11-02 00:46:04]

lgtm