[Cleanup] Migrate ExternalMetrics to use the Task Scheduler.

chromecast/browser/metrics/external_metrics.cc

 // Copyright 2014 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 "chromecast/browser/metrics/external_metrics.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/metrics/statistics_recorder.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/task_scheduler/task_traits.h"
+#include "base/threading/thread_restrictions.h"
 #include "chromecast/base/metrics/cast_histograms.h"
 #include "chromecast/base/metrics/cast_metrics_helper.h"
 #include "chromecast/browser/metrics/cast_stability_metrics_provider.h"
 #include "components/metrics/metrics_service.h"
 #include "components/metrics/serialization/metric_sample.h"
 #include "components/metrics/serialization/serialization_utils.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace chromecast {
 namespace metrics {
@@ skipping 26 matching lines @@
 ExternalMetrics::ExternalMetrics(
     CastStabilityMetricsProvider* stability_provider,
     const std::string& uma_events_file)
     : stability_provider_(stability_provider),
       uma_events_file_(uma_events_file),
       weak_factory_(this) {
   DCHECK(stability_provider);
 }
 
 ExternalMetrics::~ExternalMetrics() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
 }

[gfhuang, 2017/06/27 00:34:54]

should we add a sequence_checker?

[Ilya Sherman, 2017/06/27 19:17:16]

Done.

 
 void ExternalMetrics::StopAndDestroy() {
-  content::BrowserThread::DeleteSoon(
-      content::BrowserThread::FILE, FROM_HERE, this);
 }
 

[gfhuang, 2017/06/27 00:34:54]

Are above 2 changes safe?

Now the object could be destroyed outside of the sequence (in main thread I
believe).

[Ilya Sherman, 2017/06/27 19:17:16]

Sorry, I missed that |this| was actually used within CollectEvents, so I
oversimplified.  Thanks for calling this out; I've restored the previous
behavior now.

 void ExternalMetrics::Start() {
-  bool result = content::BrowserThread::PostDelayedTask(
-      content::BrowserThread::FILE,
-      FROM_HERE,
-      base::Bind(&ExternalMetrics::CollectEventsAndReschedule,
-                 weak_factory_.GetWeakPtr()),
-      base::TimeDelta::FromSeconds(kExternalMetricsCollectionIntervalSeconds));
-  DCHECK(result);
+  ScheduleCollection();
 }
 
 void ExternalMetrics::ProcessExternalEvents(const base::Closure& cb) {
-  content::BrowserThread::PostTaskAndReply(
-      content::BrowserThread::FILE, FROM_HERE,
-      base::Bind(
-          base::IgnoreResult(&ExternalMetrics::CollectEvents),
-          weak_factory_.GetWeakPtr()),
+  // Note that CollectEvents accesses a global singleton, and thus scheduling
+  // with CONTINUE_ON_SHUTDOWN might not be safe.
+  base::PostTaskWithTraitsAndReply(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskPriority::BACKGROUND,
+       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
+      base::Bind(base::IgnoreResult(&ExternalMetrics::CollectEvents),
+                 weak_factory_.GetWeakPtr()),

[gfhuang, 2017/06/27 00:34:54]

nit. BindOnce

[Ilya Sherman, 2017/06/27 19:17:16]

Done.

       cb);
 }

[gfhuang, 2017/06/27 00:55:01]

I think another issue is here.

because ScheduleCollection() starts a loop of CollectEvents().
This (manual) triggered ProcessExternalEvents() needs to be run in the same
sequence.

otherwise, 2 threads might enter CollectEvents() at the same time.

[Ilya Sherman, 2017/06/27 19:17:16]

Yes, that's a good point, thanks!  I got much too carried away trying to
simplify yesterday.  Done.

 
 void ExternalMetrics::RecordCrash(const std::string& crash_kind) {
   content::BrowserThread::PostTask(
       content::BrowserThread::UI,
       FROM_HERE,
       base::Bind(&CastStabilityMetricsProvider::LogExternalCrash,
                  base::Unretained(stability_provider_),
                  crash_kind));
 }
 
 void ExternalMetrics::RecordSparseHistogram(
     const ::metrics::MetricSample& sample) {
   CHECK_EQ(::metrics::MetricSample::SPARSE_HISTOGRAM, sample.type());
   base::HistogramBase* counter = base::SparseHistogram::FactoryGet(
       sample.name(), base::HistogramBase::kUmaTargetedHistogramFlag);
   counter->Add(sample.sample());
 }
 
 int ExternalMetrics::CollectEvents() {
+  base::ThreadRestrictions::AssertIOAllowed();
+
   std::vector<std::unique_ptr<::metrics::MetricSample>> samples;
   ::metrics::SerializationUtils::ReadAndTruncateMetricsFromFile(
       uma_events_file_, &samples);
 
   for (auto it = samples.begin(); it != samples.end(); ++it) {
     const ::metrics::MetricSample& sample = **it;
 
     switch (sample.type()) {
       case ::metrics::MetricSample::CRASH:
         RecordCrash(sample.name());
@@ skipping 25 matching lines @@
       case ::metrics::MetricSample::SPARSE_HISTOGRAM:
         RecordSparseHistogram(sample);
         break;
     }
   }
 
   return samples.size();
 }
 
 void ExternalMetrics::CollectEventsAndReschedule() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
   CollectEvents();
-  bool result = content::BrowserThread::PostDelayedTask(
-      content::BrowserThread::FILE,
+  ScheduleCollection();
+}
+
+void ExternalMetrics::ScheduleCollection() {
+  // Note that CollectEvents accesses a global singleton, and thus scheduling
+  // with CONTINUE_ON_SHUTDOWN might not be safe.
+  base::PostDelayedTaskWithTraits(
       FROM_HERE,
+      {base::MayBlock(), base::TaskPriority::BACKGROUND,
+       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
       base::Bind(&ExternalMetrics::CollectEventsAndReschedule,
                  weak_factory_.GetWeakPtr()),
       base::TimeDelta::FromSeconds(kExternalMetricsCollectionIntervalSeconds));
-  DCHECK(result);
 }
 
 }  // namespace metrics
 }  // namespace chromecast