Restore KeyedServiceFactory diagnostics for context use-after-destroy.

chrome/browser/engagement/site_engagement_service_unittest.cc

Index: chrome/browser/engagement/site_engagement_service_unittest.cc
diff --git a/chrome/browser/engagement/site_engagement_service_unittest.cc b/chrome/browser/engagement/site_engagement_service_unittest.cc
index 95c77a3c4986313a91977e8b00e3d7005526ae4e..74f89a73b6fe3805ec1b269e19b73c77b25a4654 100644
--- a/chrome/browser/engagement/site_engagement_service_unittest.cc
+++ b/chrome/browser/engagement/site_engagement_service_unittest.cc
@@ -482,6 +482,8 @@ TEST_F(SiteEngagementServiceTest, LastShortcutLaunch) {
   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) {
@@ -527,6 +529,8 @@ TEST_F(SiteEngagementServiceTest, NotificationPermission) {
       CONTENT_SETTING_BLOCK);
 
   EXPECT_EQ(1, service->GetScore(url1));
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, CheckHistograms) {
@@ -816,6 +820,8 @@ TEST_F(SiteEngagementServiceTest, CheckHistograms) {
   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
@@ -1035,6 +1041,8 @@ TEST_F(SiteEngagementServiceTest, CleanupEngagementScores) {
     EXPECT_EQ(0, service->GetScore(url1));
     EXPECT_EQ(clock->Now() - decay_period, service->GetLastEngagementTime());
   }
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, CleanupEngagementScoresProportional) {
@@ -1067,6 +1075,8 @@ TEST_F(SiteEngagementServiceTest, CleanupEngagementScoresProportional) {
   EXPECT_EQ(1u, score_map.size());
   EXPECT_EQ(0, service->GetScore(url1));
   AssertInRange(0.6, service->GetScore(url2));
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, NavigationAccumulation) {
@@ -1120,6 +1130,8 @@ TEST_F(SiteEngagementServiceTest, IsBootstrapped) {
 
   clock->SetNow(current_day + base::TimeDelta::FromDays(8));
   EXPECT_FALSE(service->IsBootstrapped());
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, CleanupOriginsOnHistoryDeletion) {
@@ -1240,6 +1252,8 @@ TEST_F(SiteEngagementServiceTest, CleanupOriginsOnHistoryDeletion) {
     AssertInRange(2.5, engagement->GetScore(origin4));
     AssertInRange(7.5, engagement->GetTotalEngagementPoints());
   }
+
+  engagement->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, EngagementLevel) {
@@ -1382,6 +1396,8 @@ TEST_F(SiteEngagementServiceTest, EngagementLevel) {
       service->IsEngagementAtLeast(url2, blink::mojom::EngagementLevel::HIGH));
   EXPECT_TRUE(
       service->IsEngagementAtLeast(url2, blink::mojom::EngagementLevel::MAX));
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, Observers) {
@@ -1544,6 +1560,8 @@ TEST_F(SiteEngagementServiceTest, ScoreDecayHistograms) {
                               kMorePeriodsThanNeededToDecayMaxScore + 1);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedToHistogram,
                               kMorePeriodsThanNeededToDecayMaxScore + 1);
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, LastEngagementTime) {
@@ -1604,6 +1622,8 @@ TEST_F(SiteEngagementServiceTest, LastEngagementTime) {
 
   EXPECT_EQ(later_in_day, last_engagement_time);
   EXPECT_EQ(later_in_day, service->GetLastEngagementTime());
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, CleanupMovesScoreBackToNow) {
@@ -1674,6 +1694,8 @@ TEST_F(SiteEngagementServiceTest, CleanupMovesScoreBackToNow) {
   EXPECT_EQ(clock->Now(), service->GetLastEngagementTime());
   EXPECT_EQ(12, service->GetScore(origin));
   EXPECT_EQ(1, service->GetScore(origin1));
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, CleanupMovesScoreBackToRebase) {
@@ -1717,6 +1739,8 @@ TEST_F(SiteEngagementServiceTest, CleanupMovesScoreBackToRebase) {
   EXPECT_EQ(rebased_time, service->GetLastEngagementTime());
   EXPECT_EQ(5, service->GetScore(origin));
   EXPECT_EQ(0, service->GetScore(origin1));
+
+  service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, IncognitoEngagementService) {