Address 2 crashes in download attribution code

Address 2 crashes in download attribution code.

crbug.com/676675
Out of memory crash caused by memory alloc inside vector::push_back().
Since vector grows its internal buffer following some kind of geometric
progression, it makes more sense to keep pointers inside the vector
instead of actual objects.  Therefore, change the
vector<ReferrerChainEntry> into vector<unique_ptr<ReferrerChainEntry>>.
This will help with the memory allocation inside push_back() function.

crbug.com/679252
Add additional check on the return of FindNavigationEvent() function to
make suer it is valid.

BUG=676675, 679252
Review-Url: https://codereview.chromium.org/2624463003
Cr-Commit-Position: refs/heads/master@{#442780}
Committed: https://chromium.googlesource.com/chromium/src/+/d1bf4b965304fb16a99edbd7268dd9ab9a41c76b

[Jialiu Lin, 2017-01-09 22:02:02]

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

[commit-bot: I haz the power, 2017-01-09 22:03:47]

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

[Jialiu Lin, 2017-01-09 22:38:26]

Description was changed from

==========
Address 2 crashes in SafeBrowsingNavigationObserver.

crbug.com/676675
Out of memory crash caused by memory alloc inside vector::push_back().
Since vector grow its internal buffer following some kind of geometric
progression, it makes more sense to keep pointers inside the vector
instead of actual objects.  Therefore, change the
vector<ReferrerChainEntry> into vector<unique_ptr<ReferrerChainEntry>>.
This will help with the memory allocation inside push_back() function.

crbug.com/679252
Add additional check on the return of FindNavigationEvent() function to
make suer it is valid.

BUG=676675,679252
==========

to

==========
Address 2 crashes in SafeBrowsingNavigationObserver.

crbug.com/676675
Out of memory crash caused by memory alloc inside vector::push_back().
Since vector grows its internal buffer following some kind of geometric
progression, it makes more sense to keep pointers inside the vector
instead of actual objects.  Therefore, change the
vector<ReferrerChainEntry> into vector<unique_ptr<ReferrerChainEntry>>.
This will help with the memory allocation inside push_back() function.

crbug.com/679252
Add additional check on the return of FindNavigationEvent() function to
make suer it is valid.

BUG=676675,679252
==========

[Jialiu Lin, 2017-01-09 22:54:47]

Description was changed from

==========
Address 2 crashes in SafeBrowsingNavigationObserver.

crbug.com/676675
Out of memory crash caused by memory alloc inside vector::push_back().
Since vector grows its internal buffer following some kind of geometric
progression, it makes more sense to keep pointers inside the vector
instead of actual objects.  Therefore, change the
vector<ReferrerChainEntry> into vector<unique_ptr<ReferrerChainEntry>>.
This will help with the memory allocation inside push_back() function.

crbug.com/679252
Add additional check on the return of FindNavigationEvent() function to
make suer it is valid.

BUG=676675,679252
==========

to

==========
Address 2 crashes in download attribution code.

crbug.com/676675
Out of memory crash caused by memory alloc inside vector::push_back().
Since vector grows its internal buffer following some kind of geometric
progression, it makes more sense to keep pointers inside the vector
instead of actual objects.  Therefore, change the
vector<ReferrerChainEntry> into vector<unique_ptr<ReferrerChainEntry>>.
This will help with the memory allocation inside push_back() function.

crbug.com/679252
Add additional check on the return of FindNavigationEvent() function to
make suer it is valid.

BUG=676675,679252
==========

[commit-bot: I haz the power, 2017-01-09 23:14:42]

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

[commit-bot: I haz the power, 2017-01-09 23:14:42]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-01-09 23:23:03]

jialiul@chromium.org changed reviewers:
+ nparker@chromium.org

[Jialiu Lin, 2017-01-09 23:23:04]

Hi nparker@,
Could you take a look? Thanks!

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc
(left):

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:400:
if (rit->destination_url == search_url)
not sure why this line slipped through last CL... 
Looks like a rebase went bad. 
There should be at lease a clang warning.

[Nathan Parker, 2017-01-10 01:25:13]

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:1850: for (auto it =
attribution_chain.begin(); it != attribution_chain.end(); it++)
Could this be

for (auto& entry : attribution_chain) {
  .. Swap(it.release())
}
?

Actually, I think neither is valid... Swap() doesn't take ownership of the
pointer -- it swaps the innards of the proto (internal pointer, I think).

So maybe you'd want

for (auto& entry : attribution_chain) {
  ... Swap(entry.get())
}
and then let attribution_chain go out of scope, so all the unique_ptrs will get
deleted.

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
File
chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc
(right):

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:252:
void IdentifyReferrerChainForDownload(
Maybe add a comment that this populates referrer_chain, since it's a little hard
to tell.

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:437:
SafeBrowsingNavigationObserverManager::ReferrerChain referrer_chain;
(You could consider typedeffing these to "ReferrerChain" for this file, to make
it less verbose)

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc
(left):

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:400:
if (rit->destination_url == search_url)
On 2017/01/09 23:23:04, Jialiu Lin wrote:
> not sure why this line slipped through last CL... 
> Looks like a rebase went bad. 
> There should be at lease a clang warning. 

Weird! At least it's a NOOP. :-)

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc
(right):

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:415:
if (!retargeting_nav_event)
Does this imply has_server_redirect was wrong?

[Jialiu Lin, 2017-01-10 02:14:50]

Patchset #2 (id:20001) has been deleted

[Jialiu Lin, 2017-01-10 02:15:12]

Thanks!

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:1850: for (auto it =
attribution_chain.begin(); it != attribution_chain.end(); it++)
On 2017/01/10 01:25:13, Nathan Parker wrote:
> Could this be
> 
> for (auto& entry : attribution_chain) {
>   .. Swap(it.release())
> }
> ?
> 
> Actually, I think neither is valid... Swap() doesn't take ownership of the
> pointer -- it swaps the innards of the proto (internal pointer, I think).
> 
> So maybe you'd want
> 
> for (auto& entry : attribution_chain) {
>   ... Swap(entry.get())
> }
> and then let attribution_chain go out of scope, so all the unique_ptrs will
get
> deleted.

You're right. Done.

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
File
chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc
(right):

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:252:
void IdentifyReferrerChainForDownload(
On 2017/01/10 01:25:13, Nathan Parker wrote:
> Maybe add a comment that this populates referrer_chain, since it's a little
hard
> to tell.

Done.

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:437:
SafeBrowsingNavigationObserverManager::ReferrerChain referrer_chain;
On 2017/01/10 01:25:13, Nathan Parker wrote:
> (You could consider typedeffing these to "ReferrerChain" for this file, to
make
> it less verbose)

Done.

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc
(right):

https://codereview.chromium.org/2624463003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:415:
if (!retargeting_nav_event)
On 2017/01/10 01:25:13, Nathan Parker wrote:
> Does this imply has_server_redirect was wrong?

Not necessarily. For some cases, previous NavigationEvent might just been
cleared by the periodic cleanup

[Jialiu Lin, 2017-01-10 23:14:19]

any further comments, nparker@?

[Nathan Parker, 2017-01-11 01:32:32]

lgtm

https://codereview.chromium.org/2624463003/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h
(right):

https://codereview.chromium.org/2624463003/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:35:
typedef std::vector<std::unique_ptr<ReferrerChainEntry>> ReferrerChain;
One idea: Could this be the repeated field type for RefferrerChainEntry? Then
you don't have to deal with ptrs, etc.  I don't know if it'd be any better. And
I'm less confident I understand proto memory management, so never mind.

[Jialiu Lin, 2017-01-11 01:54:40]

The CQ bit was checked by jialiul@chromium.org

[commit-bot: I haz the power, 2017-01-11 01:55:24]

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, 2017-01-11 03:34:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484099680427930,
"parent_rev": "37240b4fdef1ec7017f59d4cb7ced8c655e5c595", "commit_rev":
"d1bf4b965304fb16a99edbd7268dd9ab9a41c76b"}

[commit-bot: I haz the power, 2017-01-11 03:35:29]

Description was changed from

==========
Address 2 crashes in download attribution code.

crbug.com/676675
Out of memory crash caused by memory alloc inside vector::push_back().
Since vector grows its internal buffer following some kind of geometric
progression, it makes more sense to keep pointers inside the vector
instead of actual objects.  Therefore, change the
vector<ReferrerChainEntry> into vector<unique_ptr<ReferrerChainEntry>>.
This will help with the memory allocation inside push_back() function.

crbug.com/679252
Add additional check on the return of FindNavigationEvent() function to
make suer it is valid.

BUG=676675,679252
==========

to

==========
Address 2 crashes in download attribution code.

crbug.com/676675
Out of memory crash caused by memory alloc inside vector::push_back().
Since vector grows its internal buffer following some kind of geometric
progression, it makes more sense to keep pointers inside the vector
instead of actual objects.  Therefore, change the
vector<ReferrerChainEntry> into vector<unique_ptr<ReferrerChainEntry>>.
This will help with the memory allocation inside push_back() function.

crbug.com/679252
Add additional check on the return of FindNavigationEvent() function to
make suer it is valid.

BUG=676675,679252

Review-Url: https://codereview.chromium.org/2624463003
Cr-Commit-Position: refs/heads/master@{#442780}
Committed:
https://chromium.googlesource.com/chromium/src/+/d1bf4b965304fb16a99edbd7268d...
==========

[commit-bot: I haz the power, 2017-01-11 03:35:30]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d1bf4b965304fb16a99edbd7268d...