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 464 matching lines @@
   EXPECT_DOUBLE_EQ(5.0, service->GetScore(url2));
 
   service->AddPoints(url1, 1.0);
   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));
+
+  service->Shutdown();

[benwells, 2017/03/20 03:15:27]

This is unfortunate. Why do all the tests need to do this now? Is there a way to
avoid this?

[sense (YandexTeam), 2017/03/20 04:01:56]

Well, because of how KeyedService works, you should do shutdown work at
Shutdown() if your service depends on other service (for example, subscribe on
construction/unsubscribe on Shutdown()). Accessing services in destructor is
incorrect.
In this exactly case, SiteEngagementService subscribes to HistoryService
notifications at construction, and if you don't unsubscribe (say, destroy a
manually created test instance without calling Shutdown()), HistoryService will
try to notify the dead object at its own Shutdown().

To avoid this explicit call in tests, I can make a test class like this:
class TestSiteEngagementService : public SiteEngagementService {
 public:
  ~TestSiteEngagementService() override {
    Shutdown();
  }
};
In this case, we don't need to explicitly call Shutdown() in every test.

[benwells, 2017/03/20 04:14:50]

OK, that makes sense.

I think moving the Shutdown call to the TearDown method sounds good.

 }
 
 TEST_F(SiteEngagementServiceTest, NotificationPermission) {
   base::SimpleTestClock* clock = new base::SimpleTestClock();
   std::unique_ptr<SiteEngagementService> service(
       new SiteEngagementService(profile(), base::WrapUnique(clock)));
 
   GURL url1("https://www.google.com/");
   GURL url2("http://www.google.com/");
   GURL url3("https://drive.google.com/");
@@ skipping 25 matching lines @@
   service->AddPoints(url1, 1.0);
   service->AddPoints(url2, 3.0);
   EXPECT_EQ(6, service->GetScore(url1));
   EXPECT_EQ(3, service->GetScore(url2));
 
   settings_map->SetContentSettingDefaultScope(
       url1, url1, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string(),
       CONTENT_SETTING_BLOCK);
 
   EXPECT_EQ(1, service->GetScore(url1));
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, CheckHistograms) {
   base::HistogramTester histograms;
 
   base::SimpleTestClock* clock = new base::SimpleTestClock();
   std::unique_ptr<SiteEngagementService> service(
       new SiteEngagementService(profile(), base::WrapUnique(clock)));
 
   base::Time current_day = GetReferenceTime();
@@ skipping 269 matching lines @@
       SiteEngagementMetrics::ENGAGEMENT_FIRST_DAILY_ENGAGEMENT, 3);
 
   for (const std::string& histogram_name : engagement_bucket_histogram_names)
     histograms.ExpectTotalCount(histogram_name, 3);
 
   histograms.ExpectBucketCount(engagement_bucket_histogram_names[0], 100, 1);
   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);
+
+  service->Shutdown();
 }
 
 // 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)));
 
@@ skipping 199 matching lines @@
     EXPECT_EQ(clock->Now() - decay_period, service->GetLastEngagementTime());
 
     service->CleanupEngagementScores(false);
     ASSERT_FALSE(service->IsLastEngagementStale());
 
     score_map = service->GetScoreMap();
     EXPECT_EQ(0u, score_map.size());
     EXPECT_EQ(0, service->GetScore(url1));
     EXPECT_EQ(clock->Now() - decay_period, service->GetLastEngagementTime());
   }
+
+  service->Shutdown();
 }
 
 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)));
@@ skipping 12 matching lines @@
   std::map<GURL, double> score_map = service->GetScoreMap();
   EXPECT_EQ(2u, score_map.size());
   AssertInRange(0.5, service->GetScore(url1));
   AssertInRange(0.6, service->GetScore(url2));
 
   service->CleanupEngagementScores(false);
   score_map = service->GetScoreMap();
   EXPECT_EQ(1u, score_map.size());
   EXPECT_EQ(0, service->GetScore(url1));
   AssertInRange(0.6, service->GetScore(url2));
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, NavigationAccumulation) {
   GURL url("https://www.google.com/");
 
   SiteEngagementService* service = SiteEngagementService::Get(profile());
   ASSERT_TRUE(service);
 
   // Create the helper manually since it isn't present when a tab isn't created.
   SiteEngagementService::Helper::CreateForWebContents(web_contents());
@@ skipping 33 matching lines @@
   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());
+
+  service->Shutdown();
 }
 
 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)));
@@ skipping 100 matching lines @@
                            &task_tracker);
     waiter.Wait();
 
     // origin1 should be removed. origin3 and origin4 remain untouched.
     EXPECT_EQ(0, engagement->GetScore(origin1));
     EXPECT_EQ(0, engagement->GetScore(origin2));
     AssertInRange(5.0, engagement->GetScore(origin3));
     AssertInRange(2.5, engagement->GetScore(origin4));
     AssertInRange(7.5, engagement->GetTotalEngagementPoints());
   }
+
+  engagement->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, EngagementLevel) {
   static_assert(blink::mojom::EngagementLevel::NONE !=
                     blink::mojom::EngagementLevel::LOW,
                 "enum values should not be equal");
   static_assert(blink::mojom::EngagementLevel::LOW !=
                     blink::mojom::EngagementLevel::MEDIUM,
                 "enum values should not be equal");
   static_assert(blink::mojom::EngagementLevel::MEDIUM !=
@@ skipping 122 matching lines @@
   EXPECT_TRUE(service->IsEngagementAtLeast(
       url2, blink::mojom::EngagementLevel::MINIMAL));
   EXPECT_TRUE(
       service->IsEngagementAtLeast(url2, blink::mojom::EngagementLevel::LOW));
   EXPECT_TRUE(service->IsEngagementAtLeast(
       url2, blink::mojom::EngagementLevel::MEDIUM));
   EXPECT_TRUE(
       service->IsEngagementAtLeast(url2, blink::mojom::EngagementLevel::HIGH));
   EXPECT_TRUE(
       service->IsEngagementAtLeast(url2, blink::mojom::EngagementLevel::MAX));
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, Observers) {
   SiteEngagementService* service = SiteEngagementService::Get(profile());
 
   GURL url_score_1("http://www.google.com/maps");
   GURL url_score_2("http://www.google.com/drive");
   GURL url_score_3("http://www.google.com/");
   GURL url_not_called("https://www.google.com/");
 
@@ skipping 142 matching lines @@
   histograms.ExpectBucketCount(SiteEngagementMetrics::kScoreDecayedToHistogram,
                                0, 22);
 
   // Add more points and ensure no more samples are present.
   service->AddPoints(origin1, 0.01);
   service->AddPoints(origin2, 0.01);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedFromHistogram,
                               kMorePeriodsThanNeededToDecayMaxScore + 1);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedToHistogram,
                               kMorePeriodsThanNeededToDecayMaxScore + 1);
+
+  service->Shutdown();
 }
 
 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();
@@ skipping 40 matching lines @@
   EXPECT_EQ(rebased_time, service->GetLastEngagementTime());
 
   // Add some more points and ensure the value is persisted.
   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());
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, CleanupMovesScoreBackToNow) {
   base::SimpleTestClock* clock = new base::SimpleTestClock();
   std::unique_ptr<SiteEngagementService> service(
       new SiteEngagementService(profile(), base::WrapUnique(clock)));
   base::Time last_engagement_time;
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day);
@@ skipping 50 matching lines @@
   // appropriately, while origin1 is decayed correctly (once).
   clock->SetNow(origin1_last_updated + less_than_decay_period +
                 base::TimeDelta::FromSeconds(30));
   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));
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, CleanupMovesScoreBackToRebase) {
   base::SimpleTestClock* clock = new base::SimpleTestClock();
   std::unique_ptr<SiteEngagementService> service(
       new SiteEngagementService(profile(), base::WrapUnique(clock)));
   base::Time last_engagement_time;
 
   base::Time current_day = GetReferenceTime();
   clock->SetNow(current_day);
@@ skipping 23 matching lines @@
   base::Time rebased_time = clock->Now() - service->GetMaxDecayPeriod();
   service->CleanupEngagementScores(true);
 
   // Ensure that the original origin has its last engagement time updated to
   // rebase_time, and it has decayed when we access the score.
   EXPECT_EQ(rebased_time,
             service->CreateEngagementScore(origin).last_engagement_time());
   EXPECT_EQ(rebased_time, service->GetLastEngagementTime());
   EXPECT_EQ(5, service->GetScore(origin));
   EXPECT_EQ(0, service->GetScore(origin1));
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, IncognitoEngagementService) {
   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 79 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));
 }