Tracing: fix a race if streaming at shutdown

Bug: 832981
Change-Id: I3711f0f03306a74b9a062f388beb2f3915eecb91
Reviewed-on: https://chromium-review.googlesource.com/1024969
Commit-Queue: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553589}

chrome/browser/tracing/chrome_tracing_delegate_browsertest.cc

@@ -296,14 +296,8 @@ IN_PROC_BROWSER_TEST_F(ChromeTracingDelegateBrowserTestOnStartup,
                         base::Time::Now().ToInternalValue());
 }
 
-// Flaky mojo CHECK stop on ChromeOS. https://crbug.com/832981
-#if defined(OS_CHROMEOS)
-#define MAYBE_StartupTracingThrottle DISABLED_StartupTracingThrottle
-#else
-#define MAYBE_StartupTracingThrottle StartupTracingThrottle
-#endif
 IN_PROC_BROWSER_TEST_F(ChromeTracingDelegateBrowserTestOnStartup,
-                       MAYBE_StartupTracingThrottle) {
+                       StartupTracingThrottle) {
   // The startup scenario should *not* be started, since not enough
   // time has elapsed since the last upload (set in the PRE_ above).
   EXPECT_FALSE(

services/tracing/coordinator.cc

@@ -19,6 +19,7 @@
 #include "base/logging.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/synchronization/lock.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/task_scheduler/task_traits.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -97,6 +98,7 @@ class Coordinator::TraceStreamer : public base::SupportsWeakPtr<TraceStreamer> {
   // shutdown. We either will not write to the stream afterwards or do not care
   // what happens to what we try to write.
   void CloseStream() {
+    base::AutoLock mutex(stream_lock_);
     DCHECK(stream_.is_valid());
     stream_.reset();
   }
@@ -108,6 +110,14 @@ class Coordinator::TraceStreamer : public base::SupportsWeakPtr<TraceStreamer> {
   }
 
  private:
+  // Handles synchronize writes to |stream_|, if the stream is not already
+  // closed.
+  void WriteToStream(const std::string& data) {
+    base::AutoLock mutex(stream_lock_);
+    if (stream_->is_valid())
+      mojo::BlockingCopyFromString(data, stream_);
+  }
+
   // Called from |background_task_runner_|.
   void OnRecorderDataChange(const std::string& label) {
     // Bail out if we are in the middle of writing events for another label to
@@ -145,7 +155,7 @@ class Coordinator::TraceStreamer : public base::SupportsWeakPtr<TraceStreamer> {
         if (all_finished) {
           StreamMetadata();
           if (!stream_is_empty_ && agent_label_.empty()) {
-            mojo::BlockingCopyFromString("}", stream_);
+            WriteToStream("}");
             stream_is_empty_ = false;
           }
           // Recorder connections should be closed on their binding thread.
@@ -193,11 +203,11 @@ class Coordinator::TraceStreamer : public base::SupportsWeakPtr<TraceStreamer> {
         std::string escaped;
         base::EscapeJSONString(recorder->data(), false /* put_in_quotes */,
                                &escaped);
-        mojo::BlockingCopyFromString(prefix + escaped, stream_);
+        WriteToStream(prefix + escaped);
       } else {
         if (prefix.empty() && !stream_is_empty_)
           prefix = ",";
-        mojo::BlockingCopyFromString(prefix + recorder->data(), stream_);
+        WriteToStream(prefix + recorder->data());
       }
       stream_is_empty_ = false;
       recorder->clear_data();
@@ -206,13 +216,13 @@ class Coordinator::TraceStreamer : public base::SupportsWeakPtr<TraceStreamer> {
       if (json_field_name_written_) {
         switch (data_type) {
           case mojom::TraceDataType::ARRAY:
-            mojo::BlockingCopyFromString("]", stream_);
+            WriteToStream("]");
             break;
           case mojom::TraceDataType::OBJECT:
-            mojo::BlockingCopyFromString("}", stream_);
+            WriteToStream("}");
             break;
           case mojom::TraceDataType::STRING:
-            mojo::BlockingCopyFromString("\"", stream_);
+            WriteToStream("\"");
             break;
           default:
             NOTREACHED();
@@ -238,9 +248,8 @@ class Coordinator::TraceStreamer : public base::SupportsWeakPtr<TraceStreamer> {
     if (!metadata_->empty() &&
         base::JSONWriter::Write(*metadata_, &metadataJSON)) {
       std::string prefix = stream_is_empty_ ? "{\"" : ",\"";
-      mojo::BlockingCopyFromString(
-          prefix + std::string(kMetadataTraceLabel) + "\":" + metadataJSON,
-          stream_);
+      WriteToStream(prefix + std::string(kMetadataTraceLabel) +
+                    "\":" + metadataJSON);
       stream_is_empty_ = false;
     }
   }
@@ -248,6 +257,7 @@ class Coordinator::TraceStreamer : public base::SupportsWeakPtr<TraceStreamer> {
   // The stream to which trace events from different agents should be
   // serialized, eventually. This is set when tracing is stopped.
   mojo::ScopedDataPipeProducerHandle stream_;
+  base::Lock stream_lock_;
   std::string agent_label_;
   scoped_refptr<base::SequencedTaskRunner> main_task_runner_;
   base::WeakPtr<Coordinator> coordinator_;

testing/buildbot/filters/mash.browser_tests.filter

@@ -342,6 +342,5 @@
 -DeclarativeNetRequestResourceTypeBrowserTest.Test2/1
 -MediaEngagementSessionRestoreBrowserTest.RestoredSession_Playback_MEI
 -AutoplayExtensionBrowserTest.*
--ChromeTracingDelegateBrowserTestOnStartup.StartupTracingThrottle
 
 # See comment at top of file regarding adding test exclusions.