Restore KeyedServiceFactory diagnostics for context use-after-destroy.

chrome/browser/engagement/site_engagement_service_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/engagement/site_engagement_service.h"
 
 #include <utility>
 
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
@@ skipping 138 matching lines @@
  public:
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     g_temp_history_dir = temp_dir_.GetPath();
     HistoryServiceFactory::GetInstance()->SetTestingFactory(
         profile(), &BuildTestHistoryService);
     SiteEngagementScore::SetParamValuesForTesting();
   }
 
+  void TearDown() override {
+    if (service_with_test_clock_) {
+      service_with_test_clock_->Shutdown();
+      service_with_test_clock_.reset();
+    }
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
+
   void NavigateWithTransitionAndExpectHigherScore(
       SiteEngagementService* service,
       const GURL& url,
       ui::PageTransition transition) {
     double prev_score = service->GetScore(url);
     controller().LoadURL(url, content::Referrer(), transition, std::string());
     int pending_id = controller().GetPendingEntry()->GetUniqueID();
     content::WebContentsTester::For(web_contents())
         ->TestDidNavigate(web_contents()->GetMainFrame(), pending_id, true,
                           url, transition);
@@ skipping 29 matching lines @@
     base::RunLoop run_loop;
     content::BrowserThread::PostTaskAndReply(
         thread_id, FROM_HERE,
         base::Bind(&SiteEngagementServiceTest::CheckScoreFromSettings,
                    base::Unretained(this), settings_map, url, &score),
                    run_loop.QuitClosure());
     run_loop.Run();
     return score;
   }
 
+  void CreateServiceWithTestClock(SiteEngagementService** service,

[benwells, 2017/03/21 23:44:51]

Thanks for factoring out this too. We can delete all the setup from the tests
themselves by putting this function into the SetUp method, and having clock_ and
service_ fields on the test.

[sense (YandexTeam), 2017/03/22 07:38:21]

Okay, I think we can do this.

+                                  base::SimpleTestClock** clock) {
+    DCHECK(service && clock);
+    DCHECK(!service_with_test_clock_);
+    (*clock) = new base::SimpleTestClock();
+    (*service) = new SiteEngagementService(profile(), base::WrapUnique(*clock));
+    service_with_test_clock_ = base::WrapUnique(*service);
+  }
+
  private:
   void CheckScoreFromSettings(HostContentSettingsMap* settings_map,
                               const GURL& url,
                               double *score) {
     *score = SiteEngagementService::GetScoreFromSettings(settings_map, url);
   }
 
   base::ScopedTempDir temp_dir_;
+  std::unique_ptr<SiteEngagementService> service_with_test_clock_;
 };
 
 TEST_F(SiteEngagementServiceTest, GetMedianEngagement) {
   SiteEngagementService* service = SiteEngagementService::Get(profile());
   ASSERT_TRUE(service);
 
   GURL url1("http://www.google.com/");
   GURL url2("https://www.google.com/");
   GURL url3("https://drive.google.com/");
   GURL url4("https://maps.google.com/");
@@ skipping 188 matching lines @@
   service->HandleMediaPlaying(web_contents(), true);
   EXPECT_EQ(0, service->GetScore(url3));
 
   NavigateAndCommit(url4);
   service->HandleUserInput(web_contents(),
                            SiteEngagementMetrics::ENGAGEMENT_KEYPRESS);
   EXPECT_EQ(0, service->GetScore(url4));
 }
 
 TEST_F(SiteEngagementServiceTest, LastShortcutLaunch) {
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
 
   base::HistogramTester histograms;
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day - base::TimeDelta::FromDays(5));
 
   // The https and http versions of www.google.com should be separate. But
   // different paths on the same origin should be treated the same.
   GURL url1("https://www.google.com/");
   GURL url2("http://www.google.com/");
@@ skipping 41 matching lines @@
   clock->SetNow(GetReferenceTime() + base::TimeDelta::FromDays(10));
   EXPECT_DOUBLE_EQ(1.0, service->GetScore(url1));
   EXPECT_DOUBLE_EQ(5.0, service->GetScore(url2));
 
   clock->SetNow(GetReferenceTime() + base::TimeDelta::FromDays(11));
   EXPECT_DOUBLE_EQ(1.0, service->GetScore(url1));
   EXPECT_DOUBLE_EQ(0.0, service->GetScore(url2));
 }
 
 TEST_F(SiteEngagementServiceTest, NotificationPermission) {
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
 
   GURL url1("https://www.google.com/");
   GURL url2("http://www.google.com/");
   GURL url3("https://drive.google.com/");
   clock->SetNow(GetReferenceTime());
 
   EXPECT_EQ(0, service->GetScore(url1));
   EXPECT_EQ(0, service->GetScore(url2));
   EXPECT_EQ(0, service->GetScore(url3));
 
@@ skipping 24 matching lines @@
   settings_map->SetContentSettingDefaultScope(
       url1, url1, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string(),
       CONTENT_SETTING_BLOCK);
 
   EXPECT_EQ(1, service->GetScore(url1));
 }
 
 TEST_F(SiteEngagementServiceTest, CheckHistograms) {
   base::HistogramTester histograms;
 
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day);
 
   // Histograms should start empty as the testing SiteEngagementService
   // constructor does not record metrics.
   histograms.ExpectTotalCount(SiteEngagementMetrics::kTotalEngagementHistogram,
                               0);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kTotalOriginsHistogram, 0);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kMeanEngagementHistogram,
@@ skipping 267 matching lines @@
   histograms.ExpectBucketCount(engagement_bucket_histogram_names[0], 33, 1);
   histograms.ExpectBucketCount(engagement_bucket_histogram_names[0], 66, 1);
   histograms.ExpectBucketCount(engagement_bucket_histogram_names[1], 33, 1);
   histograms.ExpectBucketCount(engagement_bucket_histogram_names[1], 66, 1);
 }
 
 // Expect that sites that have reached zero engagement are cleaned up. Expect
 // engagement times to be reset if too much time has passed since the last
 // engagement.
 TEST_F(SiteEngagementServiceTest, CleanupEngagementScores) {
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
 
   // Set the base time to be 3 weeks past the stale period in the past.
   // Use a 1 second offset to make sure scores don't yet decay.
   base::TimeDelta one_second = base::TimeDelta::FromSeconds(1);
   base::TimeDelta one_day = base::TimeDelta::FromDays(1);
   base::TimeDelta decay_period =
       base::TimeDelta::FromHours(SiteEngagementScore::GetDecayPeriodInHours());
   base::TimeDelta shorter_than_decay_period = decay_period - one_second;
 
   base::Time max_decay_time = GetReferenceTime() - service->GetMaxDecayPeriod();
@@ skipping 197 matching lines @@
     EXPECT_EQ(0, service->GetScore(url1));
     EXPECT_EQ(clock->Now() - decay_period, service->GetLastEngagementTime());
   }
 }
 
 TEST_F(SiteEngagementServiceTest, CleanupEngagementScoresProportional) {
   SetParamValue(SiteEngagementScore::DECAY_PROPORTION, 0.5);
   SetParamValue(SiteEngagementScore::DECAY_POINTS, 0);
   SetParamValue(SiteEngagementScore::SCORE_CLEANUP_THRESHOLD, 0.5);
 
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day);
 
   GURL url1("https://www.google.com/");
   GURL url2("https://www.somewhereelse.com/");
 
   service->AddPoints(url1, 1.0);
   service->AddPoints(url2, 1.2);
 
@@ skipping 35 matching lines @@
                                             ui::PAGE_TRANSITION_AUTO_TOPLEVEL);
   NavigateWithTransitionAndExpectEqualScore(service, url,
                                             ui::PAGE_TRANSITION_LINK);
   NavigateWithTransitionAndExpectEqualScore(service, url,
                                             ui::PAGE_TRANSITION_RELOAD);
   NavigateWithTransitionAndExpectEqualScore(service, url,
                                             ui::PAGE_TRANSITION_FORM_SUBMIT);
 }
 
 TEST_F(SiteEngagementServiceTest, IsBootstrapped) {
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day);
 
   GURL url1("https://www.google.com/");
   GURL url2("https://www.somewhereelse.com/");
 
   EXPECT_FALSE(service->IsBootstrapped());
 
   service->AddPoints(url1, 5.0);
   EXPECT_FALSE(service->IsBootstrapped());
 
   service->AddPoints(url2, 5.0);
   EXPECT_TRUE(service->IsBootstrapped());
 
   clock->SetNow(current_day + base::TimeDelta::FromDays(8));
   EXPECT_FALSE(service->IsBootstrapped());
 }
 
 TEST_F(SiteEngagementServiceTest, CleanupOriginsOnHistoryDeletion) {
   // Enable proportional decay to ensure that the undecay that happens to
   // balance out history deletion also accounts for the proportional decay.
   SetParamValue(SiteEngagementScore::DECAY_PROPORTION, 0.5);
 
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> engagement(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
-  ASSERT_TRUE(engagement.get());
+  SiteEngagementService* engagement;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&engagement, &clock);
 
   GURL origin1("http://www.google.com/");
   GURL origin1a("http://www.google.com/search?q=asdf");
   GURL origin1b("http://www.google.com/maps/search?q=asdf");
   GURL origin2("https://drive.google.com/");
   GURL origin2a("https://drive.google.com/somedoc");
   GURL origin3("http://notdeleted.com/");
   GURL origin4("http://decayed.com/");
   GURL origin4a("http://decayed.com/index.html");
 
@@ skipping 105 matching lines @@
   static_assert(blink::mojom::EngagementLevel::LOW !=
                     blink::mojom::EngagementLevel::MEDIUM,
                 "enum values should not be equal");
   static_assert(blink::mojom::EngagementLevel::MEDIUM !=
                     blink::mojom::EngagementLevel::HIGH,
                 "enum values should not be equal");
   static_assert(blink::mojom::EngagementLevel::HIGH !=
                     blink::mojom::EngagementLevel::MAX,
                 "enum values should not be equal");
 
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day);
 
   GURL url1("https://www.google.com/");
   GURL url2("http://www.google.com/");
 
   EXPECT_EQ(blink::mojom::EngagementLevel::NONE,
             service->GetEngagementLevel(url1));
   EXPECT_EQ(blink::mojom::EngagementLevel::NONE,
@@ skipping 169 matching lines @@
     service->HandleMediaPlaying(web_contents(), true);
     tester.Wait();
 
     EXPECT_TRUE(tester.callback_called());
     EXPECT_FALSE(tester_not_called.callback_called());
     tester.Observe(nullptr);
   }
 }
 
 TEST_F(SiteEngagementServiceTest, ScoreDecayHistograms) {
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day);
   base::HistogramTester histograms;
   GURL origin1("http://www.google.com/");
   GURL origin2("http://drive.google.com/");
 
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedFromHistogram,
                               0);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedToHistogram,
@@ skipping 82 matching lines @@
                               kMorePeriodsThanNeededToDecayMaxScore + 1);
 }
 
 TEST_F(SiteEngagementServiceTest, LastEngagementTime) {
   // The last engagement time should start off null in prefs and in the service.
   base::Time last_engagement_time = base::Time::FromInternalValue(
       profile()->GetPrefs()->GetInt64(prefs::kSiteEngagementLastUpdateTime));
 
   ASSERT_TRUE(last_engagement_time.is_null());
 
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
 
   ASSERT_TRUE(service->GetLastEngagementTime().is_null());
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day);
 
   // Add points should set the last engagement time in the service, and persist
   // it to disk.
   GURL origin("http://www.google.com/");
   service->AddPoints(origin, 1);
@@ skipping 31 matching lines @@
   service->AddPoints(origin, 3);
 
   last_engagement_time = base::Time::FromInternalValue(
       profile()->GetPrefs()->GetInt64(prefs::kSiteEngagementLastUpdateTime));
 
   EXPECT_EQ(later_in_day, last_engagement_time);
   EXPECT_EQ(later_in_day, service->GetLastEngagementTime());
 }
 
 TEST_F(SiteEngagementServiceTest, CleanupMovesScoreBackToNow) {
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
   base::Time last_engagement_time;
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day);
 
   GURL origin("http://www.google.com/");
   service->AddPoints(origin, 1);
   EXPECT_EQ(1, service->GetScore(origin));
   EXPECT_EQ(current_day, service->GetLastEngagementTime());
 
@@ skipping 47 matching lines @@
   service->AddPoints(origin1, 1);
 
   EXPECT_EQ(clock->Now(),
             service->CreateEngagementScore(origin).last_engagement_time());
   EXPECT_EQ(clock->Now(), service->GetLastEngagementTime());
   EXPECT_EQ(12, service->GetScore(origin));
   EXPECT_EQ(1, service->GetScore(origin1));
 }
 
 TEST_F(SiteEngagementServiceTest, CleanupMovesScoreBackToRebase) {
-  base::SimpleTestClock* clock = new base::SimpleTestClock();
-  std::unique_ptr<SiteEngagementService> service(
-      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+  SiteEngagementService* service;
+  base::SimpleTestClock* clock;
+  CreateServiceWithTestClock(&service, &clock);
   base::Time last_engagement_time;
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day);
 
   GURL origin("http://www.google.com/");
   service->ResetBaseScoreForURL(origin, 5);
   service->AddPoints(origin, 5);
   EXPECT_EQ(10, service->GetScore(origin));
   EXPECT_EQ(current_day, service->GetLastEngagementTime());
@@ skipping 116 matching lines @@
 
   EXPECT_EQ(3, CheckScoreFromSettingsOnThread(content::BrowserThread::IO,
                                               settings_map, url1));
   EXPECT_EQ(3, CheckScoreFromSettingsOnThread(content::BrowserThread::FILE,
                                               settings_map, url2));
   EXPECT_EQ(4, CheckScoreFromSettingsOnThread(content::BrowserThread::FILE,
                                               incognito_settings_map, url1));
   EXPECT_EQ(3, CheckScoreFromSettingsOnThread(content::BrowserThread::IO,
                                               incognito_settings_map, url2));
 }