[Cleanup] Migrate ExternalMetrics to use the Task Scheduler.

chromecast/browser/metrics/external_metrics.cc

Index: chromecast/browser/metrics/external_metrics.cc
diff --git a/chromecast/browser/metrics/external_metrics.cc b/chromecast/browser/metrics/external_metrics.cc
index 40de5ba0ad33da89e63b01eeb7b0ed8feb4df51c..0374a1a5e02c1027b1f0207652c1432e39816f8a 100644
--- a/chromecast/browser/metrics/external_metrics.cc
+++ b/chromecast/browser/metrics/external_metrics.cc
@@ -16,6 +16,9 @@
 #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"
@@ -62,30 +65,24 @@ ExternalMetrics::ExternalMetrics(
 }
 
 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.

 
@@ -107,6 +104,8 @@ void ExternalMetrics::RecordSparseHistogram(
 }
 
 int ExternalMetrics::CollectEvents() {
+  base::ThreadRestrictions::AssertIOAllowed();
+
   std::vector<std::unique_ptr<::metrics::MetricSample>> samples;
   ::metrics::SerializationUtils::ReadAndTruncateMetricsFromFile(
       uma_events_file_, &samples);
@@ -152,15 +151,20 @@ int ExternalMetrics::CollectEvents() {
 }
 
 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