Add support for writing system traces at startup

content/browser/tracing/trace_subscriber_stdio.cc

Index: content/browser/tracing/trace_subscriber_stdio.cc
diff --git a/content/browser/tracing/trace_subscriber_stdio.cc b/content/browser/tracing/trace_subscriber_stdio.cc
index b5f6e828c60b581f83a9ab4bb35209717e259d8b..f278eaa274547b0e396899de56235e8c6153072a 100644
--- a/content/browser/tracing/trace_subscriber_stdio.cc
+++ b/content/browser/tracing/trace_subscriber_stdio.cc
@@ -14,39 +14,78 @@
 namespace content {
 
 // All method calls on this class are done on a SequencedWorkerPool thread.
-class TraceSubscriberStdioImpl
-    : public base::RefCountedThreadSafe<TraceSubscriberStdioImpl> {
+class TraceSubscriberStdio::TraceSubscriberStdioWorker
+    : public base::RefCountedThreadSafe<TraceSubscriberStdioWorker> {
  public:
-  explicit TraceSubscriberStdioImpl(const base::FilePath& path)
+  explicit TraceSubscriberStdioWorker(const base::FilePath& path,

[sky, 2013/08/20 14:28:44]

no explicit

[DaveMoore, 2013/08/20 14:58:00]

Done.

+                                      FileType file_type,
+                                      bool has_system_trace)
       : path_(path),
-        file_(0) {}
-
-  void OnStart() {
+        file_type_(file_type),
+        has_system_trace_(has_system_trace),
+        file_(0),
+        needs_comma_(false),
+        wrote_trace_(false),
+        has_pending_system_trace_(false),
+        wrote_system_trace_(false) {}
+
+  void OnTraceStart() {
     DCHECK(!file_);
-    trace_buffer_.SetOutputCallback(
-        base::Bind(&TraceSubscriberStdioImpl::Write, this));
     file_ = file_util::OpenFile(path_, "w+");
-    if (IsValid()) {
-      LOG(INFO) << "Logging performance trace to file: " << path_.value();
-      trace_buffer_.Start();
-    } else {
+    if (!IsValid()) {
       LOG(ERROR) << "Failed to open performance trace file: " << path_.value();
+      return;
     }
+
+    LOG(INFO) << "Logging performance trace to file: " << path_.value();
+    if (file_type_ == FILE_TYPE_PROPERTY_LIST)
+      WriteString("{\"traceEvents\":");
+    WriteString("[");
   }
 
-  void OnData(const scoped_refptr<base::RefCountedString>& data_ptr) {
-    trace_buffer_.AddFragment(data_ptr->data());
+  void OnTraceData(const scoped_refptr<base::RefCountedString>& data_ptr) {
+    if (!IsValid())
+      return;
+    if (needs_comma_)
+      WriteString(",");
+    WriteString(data_ptr->data());

[sky, 2013/08/20 14:28:44]

How come you don't need to escape this?

[DaveMoore, 2013/08/20 14:58:00]

It wasn't escaped in the previous implementation. There's no change here from
before, other than it can optionally be put in a property list. I'll research if
this was incorrect and if so do another cl to fix it.

On 2013/08/20 14:28:44, sky wrote:

+    needs_comma_ = true;
   }
 
-  void OnEnd() {
-    trace_buffer_.Finish();
-    CloseFile();
+  void OnSystemTraceData(
+      const scoped_refptr<base::RefCountedString>& data_ptr) {
+    if (wrote_trace_) {
+      WriteSystemTrace(data_ptr);
+      End();
+    } else {
+      pending_system_trace_ = data_ptr;
+      has_pending_system_trace_ = true;
+    }
+  }
+
+  void OnTraceEnd() {
+    if (!IsValid())
+      return;
+    WriteString("]");
+
+    wrote_trace_ = true;
+
+    if (!has_system_trace_ || wrote_system_trace_) {
+      End();
+      return;
+    }
+
+    WriteString(",");
+    if (has_pending_system_trace_) {
+      WriteSystemTrace(pending_system_trace_);
+      End();
+    }
   }
 
  private:
-  friend class base::RefCountedThreadSafe<TraceSubscriberStdioImpl>;
+  friend class base::RefCountedThreadSafe<TraceSubscriberStdioWorker>;
 
-  ~TraceSubscriberStdioImpl() {
+  ~TraceSubscriberStdioWorker() {
     CloseFile();
   }
 
@@ -58,32 +97,75 @@ class TraceSubscriberStdioImpl
     if (file_) {
       fclose(file_);
       file_ = 0;
+
+    }
+  }
+
+  void End() {
+    if (file_type_ == FILE_TYPE_PROPERTY_LIST)
+      WriteString("}");
+    CloseFile();
+  }
+
+  void WriteSystemTrace(const scoped_refptr<base::RefCountedString>& data_ptr) {
+    // Newlines need to be replaced with the string "\n" to be parsed correctly.
+    // Double quotes need to be replaced with the string "\"".
+    // System logs are ASCII.
+    std::string data = data_ptr->data();

[sky, 2013/08/20 14:28:44]

const std::string&

[DaveMoore, 2013/08/20 14:58:00]

Done.

+    const char* chars = data.c_str();
+    WriteString("\"systemTraceEvents\":\"");
+    int old_index = 0;

[sky, 2013/08/20 14:28:44]

Shouldn't this be a size_t too?

[DaveMoore, 2013/08/20 14:58:00]

Done.

+    for (size_t new_index = data.find_first_of("\n\"");
+         std::string::npos != new_index;
+         old_index = new_index + 1,
+         new_index = data.find_first_of("\n\"", old_index)) {
+      WriteChars(chars + old_index, new_index - old_index);
+      if (chars[new_index] == '\n')
+        WriteChars("\\n", 2);
+      else
+        WriteChars("\\\"", 2);
     }
-    // This is important, as it breaks a reference cycle.
-    trace_buffer_.SetOutputCallback(
-        base::debug::TraceResultBuffer::OutputCallback());
+    WriteChars(chars + old_index, data.size() - old_index);
+    WriteString("\"");
+    wrote_system_trace_ = true;
   }
 
-  void Write(const std::string& output_str) {
+  void WriteChars(const char* output_chars, size_t size) {
     if (IsValid()) {

[sky, 2013/08/20 14:28:44]

Early out if size == 0? Can happen on 128 new.

[DaveMoore, 2013/08/20 14:58:00]

Done.

-      size_t written = fwrite(output_str.data(), 1, output_str.size(), file_);
-      if (written != output_str.size()) {
+      size_t written = fwrite(output_chars, 1, size, file_);
+      if (written != size) {
         LOG(ERROR) << "Error " << ferror(file_) << " in fwrite() to trace file";
         CloseFile();
       }
     }
   }
 
+  void WriteString(const std::string& output_str) {
+    WriteChars(output_str.data(), output_str.size());
+  }
+
   base::FilePath path_;
+  FileType file_type_;

[sky, 2013/08/20 14:28:44]

const on these two (I think).

+  bool has_system_trace_;
   FILE* file_;
-  base::debug::TraceResultBuffer trace_buffer_;
+  bool needs_comma_;
+  bool wrote_trace_;
+  bool has_pending_system_trace_;
+  bool wrote_system_trace_;
+  scoped_refptr<base::RefCountedString> pending_system_trace_;
 };

[sky, 2013/08/20 14:28:44]

DISALLOW_COPY...

[DaveMoore, 2013/08/20 14:58:00]

Done.

 
-TraceSubscriberStdio::TraceSubscriberStdio(const base::FilePath& path)
-    : impl_(new TraceSubscriberStdioImpl(path)) {
+TraceSubscriberStdio::TraceSubscriberStdio(const base::FilePath& path,
+                                           FileType file_type,
+                                           bool has_system_trace)
+    : worker_(new TraceSubscriberStdioWorker(path,
+                                             file_type,
+                                             has_system_trace)) {
+  if (has_system_trace)
+    CHECK_EQ(FILE_TYPE_PROPERTY_LIST, file_type);
   BrowserThread::PostBlockingPoolSequencedTask(
       __FILE__, FROM_HERE,
-      base::Bind(&TraceSubscriberStdioImpl::OnStart, impl_));
+      base::Bind(&TraceSubscriberStdioWorker::OnTraceStart, worker_));
 }
 
 TraceSubscriberStdio::~TraceSubscriberStdio() {
@@ -92,14 +174,23 @@ TraceSubscriberStdio::~TraceSubscriberStdio() {
 void TraceSubscriberStdio::OnEndTracingComplete() {
   BrowserThread::PostBlockingPoolSequencedTask(
       __FILE__, FROM_HERE,
-      base::Bind(&TraceSubscriberStdioImpl::OnEnd, impl_));
+      base::Bind(&TraceSubscriberStdioWorker::OnTraceEnd, worker_));
 }
 
 void TraceSubscriberStdio::OnTraceDataCollected(
     const scoped_refptr<base::RefCountedString>& data_ptr) {
   BrowserThread::PostBlockingPoolSequencedTask(
       __FILE__, FROM_HERE,
-      base::Bind(&TraceSubscriberStdioImpl::OnData, impl_, data_ptr));
+      base::Bind(&TraceSubscriberStdioWorker::OnTraceData, worker_, data_ptr));
+}
+
+void TraceSubscriberStdio::OnEndSystemTracing(
+    const scoped_refptr<base::RefCountedString>& events_str_ptr) {
+  BrowserThread::PostBlockingPoolSequencedTask(
+      __FILE__, FROM_HERE,
+      base::Bind(&TraceSubscriberStdioWorker::OnSystemTraceData,
+                 worker_,
+                 events_str_ptr));
 }
 
 }  // namespace content