Commit edcb399f authored by Dominick Ng's avatar Dominick Ng Committed by Commit Bot

Fix use-after-free due to SiteEngagementService background thread activity.

This CL:
 - stops recording site engagement metrics at startup
 - passes a scoped_refptr to HostContentSettingsMap in a background
   thread call to query the content settings.

This addresses a use-after-free due to the HostContentSettingsMap
being freed before the background thread execution completes.

BUG=901287

Change-Id: I7a1cd46a4628da490387aa98a7fc383ae5a5adb3
Reviewed-on: https://chromium-review.googlesource.com/c/1322339Reviewed-by: default avatarcalamity <calamity@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606332}
parent 442fbe43
...@@ -10,6 +10,7 @@ ...@@ -10,6 +10,7 @@
#include <utility> #include <utility>
#include "base/bind.h" #include "base/bind.h"
#include "base/memory/scoped_refptr.h"
#include "base/metrics/field_trial.h" #include "base/metrics/field_trial.h"
#include "base/strings/string_util.h" #include "base/strings/string_util.h"
#include "base/task/post_task.h" #include "base/task/post_task.h"
...@@ -109,6 +110,14 @@ std::vector<mojom::SiteEngagementDetails> GetAllDetailsImpl( ...@@ -109,6 +110,14 @@ std::vector<mojom::SiteEngagementDetails> GetAllDetailsImpl(
return details; return details;
} }
// Takes a scoped_refptr to keep HostContentSettingsMap alive. See
// crbug.com/901287.
std::vector<mojom::SiteEngagementDetails> GetAllDetailsImplInBackground(
base::Clock* clock,
scoped_refptr<HostContentSettingsMap> map) {
return GetAllDetailsImpl(clock, map.get());
}
// Only accept a navigation event for engagement if it is one of: // Only accept a navigation event for engagement if it is one of:
// a. direct typed navigation // a. direct typed navigation
// b. clicking on an omnibox suggestion brought up by typing a keyword // b. clicking on an omnibox suggestion brought up by typing a keyword
...@@ -350,7 +359,6 @@ void SiteEngagementService::AfterStartupTask() { ...@@ -350,7 +359,6 @@ void SiteEngagementService::AfterStartupTask() {
// in AddPoints for people who never restart Chrome, but leave it open and // in AddPoints for people who never restart Chrome, but leave it open and
// their computer on standby. // their computer on standby.
CleanupEngagementScores(IsLastEngagementStale()); CleanupEngagementScores(IsLastEngagementStale());
MaybeRecordMetrics();
} }
void SiteEngagementService::CleanupEngagementScores( void SiteEngagementService::CleanupEngagementScores(
...@@ -453,8 +461,8 @@ void SiteEngagementService::MaybeRecordMetrics() { ...@@ -453,8 +461,8 @@ void SiteEngagementService::MaybeRecordMetrics() {
{base::TaskPriority::BEST_EFFORT, {base::TaskPriority::BEST_EFFORT,
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}, base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
base::BindOnce( base::BindOnce(
&GetAllDetailsImpl, base::Unretained(clock_), &GetAllDetailsImplInBackground, base::Unretained(clock_),
base::Unretained( scoped_refptr<HostContentSettingsMap>(
HostContentSettingsMapFactory::GetForProfile(profile_))), HostContentSettingsMapFactory::GetForProfile(profile_))),
base::BindOnce(&SiteEngagementService::RecordMetrics, base::BindOnce(&SiteEngagementService::RecordMetrics,
weak_factory_.GetWeakPtr())); weak_factory_.GetWeakPtr()));
......
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