Skip to content

GitLab

T
tangled
Commit d1bf4b96 authored by jialiul's avatar jialiul Committed by Commit bot
Browse files
Options

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}
parent 37240b4f
Expand all Show whitespace changes
Inline Side-by-side
Showing with 173 additions and 147 deletions
chrome/browser/safe_browsing/download_protection_service.cc
View file @ d1bf4b96
......@@ -1834,7 +1834,7 @@ void DownloadProtectionService::AddReferrerChainToClientDownloadRequest(
UMA_HISTOGRAM_BOOLEAN(
"SafeBrowsing.ReferrerHasInvalidTabID.DownloadAttribution",
download_tab_id == -1);
std::vector<ReferrerChainEntry> attribution_chain;
SafeBrowsingNavigationObserverManager::ReferrerChain attribution_chain;
SafeBrowsingNavigationObserverManager::AttributionResult result =
navigation_observer_manager_->IdentifyReferrerChainForDownload(
download_url,
......@@ -1847,8 +1847,8 @@ void DownloadProtectionService::AddReferrerChainToClientDownloadRequest(
UMA_HISTOGRAM_ENUMERATION(
"SafeBrowsing.ReferrerAttributionResult.DownloadAttribution", result,
SafeBrowsingNavigationObserverManager::ATTRIBUTION_FAILURE_TYPE_MAX);
for (auto entry : attribution_chain)
out_request->add_referrer_chain()->Swap(&entry);
for (auto& entry : attribution_chain)
out_request->add_referrer_chain()->Swap(entry.get());
}
void DownloadProtectionService::AddReferrerChainToPPAPIClientDownloadRequest(
......@@ -1865,7 +1865,7 @@ void DownloadProtectionService::AddReferrerChainToPPAPIClientDownloadRequest(
UMA_HISTOGRAM_BOOLEAN(
"SafeBrowsing.ReferrerHasInvalidTabID.DownloadAttribution",
tab_id == -1);
std::vector<ReferrerChainEntry> attribution_chain;
SafeBrowsingNavigationObserverManager::ReferrerChain attribution_chain;
SafeBrowsingNavigationObserverManager::AttributionResult result =
navigation_observer_manager_->IdentifyReferrerChainForPPAPIDownload(
initiating_frame_url,
......@@ -1879,8 +1879,8 @@ void DownloadProtectionService::AddReferrerChainToPPAPIClientDownloadRequest(
UMA_HISTOGRAM_ENUMERATION(
"SafeBrowsing.ReferrerAttributionResult.PPAPIDownloadAttribution", result,
SafeBrowsingNavigationObserverManager::ATTRIBUTION_FAILURE_TYPE_MAX);
for (auto entry : attribution_chain)
out_request->add_referrer_chain()->Swap(&entry);
for (auto& entry : attribution_chain)
out_request->add_referrer_chain()->Swap(entry.get());
}
} // namespace safe_browsing
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc
View file @ d1bf4b96
......@@ -194,7 +194,7 @@ SafeBrowsingNavigationObserverManager::IdentifyReferrerChainForDownload(
const GURL& target_url,
int target_tab_id,
int user_gesture_count_limit,
std::vector<ReferrerChainEntry>* out_referrer_chain) {
ReferrerChain* out_referrer_chain) {
if (!target_url.is_valid())
return INVALID_URL;
......@@ -225,7 +225,7 @@ SafeBrowsingNavigationObserverManager::IdentifyReferrerChainForPPAPIDownload(
int tab_id,
bool has_user_gesture,
int user_gesture_count_limit,
std::vector<ReferrerChainEntry>* out_referrer_chain) {
ReferrerChain* out_referrer_chain) {
if (!initiating_frame_url.is_valid())
return INVALID_URL;
......@@ -397,7 +397,6 @@ NavigationEvent* SafeBrowsingNavigationObserverManager::FindNavigationEvent(
// the vector in reverse order to get the latest match.
for (auto rit = it->second.rbegin(); rit != it->second.rend(); ++rit) {
// If tab id is not valid, we only compare url, otherwise we compare both.
if (rit->destination_url == search_url)
if (rit->destination_url == search_url &&
(target_tab_id == -1 || rit->target_tab_id == target_tab_id)) {
// If both source_url and source_main_frame_url are empty, and this
......@@ -413,6 +412,8 @@ NavigationEvent* SafeBrowsingNavigationObserverManager::FindNavigationEvent(
FindNavigationEvent(rit->original_request_url,
GURL(),
rit->target_tab_id);
if (!retargeting_nav_event)
return nullptr;
// Adjust retargeting navigation event's attributes.
retargeting_nav_event->has_server_redirect = true;
retargeting_nav_event->destination_url = search_url;
......@@ -429,28 +430,29 @@ NavigationEvent* SafeBrowsingNavigationObserverManager::FindNavigationEvent(
}
void SafeBrowsingNavigationObserverManager::AddToReferrerChain(
std::vector<ReferrerChainEntry>* referrer_chain,
ReferrerChain* referrer_chain,
NavigationEvent* nav_event,
ReferrerChainEntry::URLType type) {
ReferrerChainEntry referrer_chain_entry;
referrer_chain_entry.set_url(nav_event->destination_url.spec());
referrer_chain_entry.set_type(type);
std::unique_ptr<ReferrerChainEntry> referrer_chain_entry =
base::MakeUnique<ReferrerChainEntry>();
referrer_chain_entry->set_url(nav_event->destination_url.spec());
referrer_chain_entry->set_type(type);
auto ip_it = host_to_ip_map_.find(nav_event->destination_url.host());
if (ip_it != host_to_ip_map_.end()) {
for (ResolvedIPAddress entry : ip_it->second) {
referrer_chain_entry.add_ip_addresses(entry.ip);
referrer_chain_entry->add_ip_addresses(entry.ip);
}
}
// Since we only track navigation to landing referrer, we will not log the
// referrer of the landing referrer page.
if (type != ReferrerChainEntry::LANDING_REFERRER) {
referrer_chain_entry.set_referrer_url(nav_event->source_url.spec());
referrer_chain_entry.set_referrer_main_frame_url(
referrer_chain_entry->set_referrer_url(nav_event->source_url.spec());
referrer_chain_entry->set_referrer_main_frame_url(
nav_event->source_main_frame_url.spec());
}
referrer_chain_entry.set_is_retargeting(nav_event->source_tab_id !=
referrer_chain_entry->set_is_retargeting(nav_event->source_tab_id !=
nav_event->target_tab_id);
referrer_chain_entry.set_navigation_time_msec(
referrer_chain_entry->set_navigation_time_msec(
nav_event->last_updated.ToJavaTime());
referrer_chain->push_back(std::move(referrer_chain_entry));
}
......@@ -459,7 +461,7 @@ void SafeBrowsingNavigationObserverManager::GetRemainingReferrerChain(
NavigationEvent* last_nav_event_traced,
int current_user_gesture_count,
int user_gesture_count_limit,
std::vector<ReferrerChainEntry>* out_referrer_chain,
ReferrerChain* out_referrer_chain,
SafeBrowsingNavigationObserverManager::AttributionResult* out_result) {
while (current_user_gesture_count < user_gesture_count_limit) {
......
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h
View file @ d1bf4b96
......@@ -32,6 +32,7 @@ class SafeBrowsingNavigationObserverManager
public base::RefCountedThreadSafe<SafeBrowsingNavigationObserverManager> {
public:
static const base::Feature kDownloadAttribution;
typedef std::vector<std::unique_ptr<ReferrerChainEntry>> ReferrerChain;
// For UMA histogram counting. Do NOT change order.
enum AttributionResult {
......@@ -90,7 +91,7 @@ class SafeBrowsingNavigationObserverManager
const GURL& target_url,
int target_tab_id, // -1 if tab id is not valid
int user_gesture_count_limit,
std::vector<ReferrerChainEntry>* out_referrer_chain);
ReferrerChain* out_referrer_chain);
// Based on the |initiating_frame_url| and its associated |tab_id|, trace back
// the observed NavigationEvents in navigation_map_ to identify the sequence
......@@ -105,7 +106,7 @@ class SafeBrowsingNavigationObserverManager
int tab_id,
bool has_user_gesture,
int user_gesture_count_limit,
std::vector<ReferrerChainEntry>* out_referrer_chain);
ReferrerChain* out_referrer_chain);
private:
friend class base::RefCountedThreadSafe<
......@@ -180,18 +181,17 @@ class SafeBrowsingNavigationObserverManager
const GURL& target_main_frame_url,
int target_tab_id);
void AddToReferrerChain(std::vector<ReferrerChainEntry>* referrer_chain,
void AddToReferrerChain(ReferrerChain* referrer_chain,
NavigationEvent* nav_event,
ReferrerChainEntry::URLType type);
// Helper function to get the remaining referrer chain when we've already
// traced back |current_user_gesture_count| number of user gestures.
// This function modifies the |out_referrer_chain| and |out_result|.
void GetRemainingReferrerChain(
NavigationEvent* last_nav_event_traced,
void GetRemainingReferrerChain(NavigationEvent* last_nav_event_traced,
int current_user_gesture_count,
int user_gesture_count_limit,
std::vector<ReferrerChainEntry>* out_referrer_chain,
ReferrerChain* out_referrer_chain,
AttributionResult* out_result);
// navigation_map_ keeps track of all the observed navigations. This map is
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment