tracing: get rid of files in TracingController interface

content/browser/tracing/tracing_controller_impl.cc

 // Copyright (c) 2013 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 "content/browser/tracing/tracing_controller_impl.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/file_util.h"
 #include "base/json/string_escape.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 16 matching lines @@
 using base::debug::TraceOptions;
 using base::debug::CategoryFilter;
 
 namespace content {
 
 namespace {
 
 base::LazyInstance<TracingControllerImpl>::Leaky g_controller =
     LAZY_INSTANCE_INITIALIZER;
 
+class FileTraceDataSink : public TracingController::TraceDataSink {
+ public:
+  explicit FileTraceDataSink(
+      const base::FilePath& trace_file_path, const base::Closure& callback)
+      : file_path_(trace_file_path),
+        completion_callback_(callback),
+        file_(NULL) {
+  }
+
+  virtual void AddTraceChunk(const std::string& chunk) OVERRIDE {
+    std::string tmp = chunk;
+    scoped_refptr<base::RefCountedString> chunk_ptr =
+        base::RefCountedString::TakeString(&tmp);
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+        base::Bind(&FileTraceDataSink::AddTraceChunkOnFileThread,
+                   base::Unretained(this), chunk_ptr));
+  }
+  virtual void SetSystemTrace(const std::string& data) OVERRIDE {
+    system_trace_ = data;
+  }
+  virtual void Close() OVERRIDE {
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+        base::Bind(&FileTraceDataSink::CloseOnFileThread,
+                   base::Unretained(this)));
+  }
+
+ private:
+  virtual ~FileTraceDataSink() {}
+
+  void AddTraceChunkOnFileThread(
+      const scoped_refptr<base::RefCountedString> chunk) {
+    if (!OpenFileIfNeededOnFileThread())
+      return;
+    fwrite(chunk->data().c_str(), strlen(chunk->data().c_str()), 1, file_);
+  }
+
+  bool OpenFileIfNeededOnFileThread() {
+    if (file_)
+      return true;
+    file_ = base::OpenFile(file_path_, "w");
+    if (!file_) {
+      LOG(ERROR) << "Failed to open " << file_path_.value();
+      return false;
+    }
+    const char preamble[] = "{\"traceEvents\": [";
+    fwrite(preamble, strlen(preamble), 1, file_);
+    return true;
+  }
+
+  void CloseOnFileThread() {
+    if (OpenFileIfNeededOnFileThread()) {
+      fputc(']', file_);
+      if (!system_trace_.empty()) {
+        const char systemTraceEvents[] = ",\"systemTraceEvents\":[";
+        fwrite(systemTraceEvents, strlen(systemTraceEvents), 1, file_);
+        fwrite(system_trace_.c_str(), strlen(system_trace_.c_str()), 1, file_);
+        fputc(']', file_);
+      }
+      fputc('}', file_);
+      base::CloseFile(file_);
+    }
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&FileTraceDataSink::FinalizeOnUIThread,
+                   base::Unretained(this)));
+  }
+
+  void FinalizeOnUIThread() {
+    completion_callback_.Run();
+    delete this;
+  }
+
+  base::FilePath file_path_;
+  base::Closure completion_callback_;
+  FILE* file_;
+  std::string system_trace_;
+};
+
+class StringTraceDataSink : public TracingController::TraceDataSink {
+ public:
+  typedef base::Callback<void(base::RefCountedMemory*)>
+      CompletionCallback;
+
+  explicit StringTraceDataSink(CompletionCallback callback)
+      : completion_callback_(callback) {
+  }
+
+  // TracingController::TraceDataSink implementation
+  virtual void AddTraceChunk(const std::string& chunk) OVERRIDE {
+    if (!trace_.empty())
+      trace_ += ",";
+    trace_ += chunk;
+  }
+  virtual void SetSystemTrace(const std::string& data) OVERRIDE {
+    system_trace_ = data;
+  }
+  virtual void Close() OVERRIDE {
+    std::string result = "{\"traceEvents\":[" + trace_ + "]";
+    if (!system_trace_.empty())
+      result += ",\"systemTraceEvents\":[" + system_trace_ + "]";
+    result += "}";
+
+    completion_callback_.Run(base::RefCountedString::TakeString(&result));
+    delete this;

[dsinclair, 2014/09/04 18:03:12]

I'm not a fan of the delete this on the Close() methods. It's going to lead to
leaks (the File sync above does not delete this).

I think the places where a raw TraceDataSink are pass should be changed to
scoped pointers. The Close() methods can go away and we move the contents into
the destructor.

Then, the tracing controller can free the object at the right time and we can
maintain clear ownership of the sink's as they're passed around.

+  }
+
+ private:
+  virtual ~StringTraceDataSink() {}
+
+  std::string trace_;
+  std::string system_trace_;
+  CompletionCallback completion_callback_;
+};
+
 }  // namespace
 
 TracingController* TracingController::GetInstance() {
   return TracingControllerImpl::GetInstance();
 }
 
-class TracingControllerImpl::ResultFile {
- public:
-  explicit ResultFile(const base::FilePath& path);
-  void Write(const scoped_refptr<base::RefCountedString>& events_str_ptr) {
-    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-        base::Bind(&TracingControllerImpl::ResultFile::WriteTask,
-                   base::Unretained(this), events_str_ptr));
-  }
-  void Close(const base::Closure& callback) {
-    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-        base::Bind(&TracingControllerImpl::ResultFile::CloseTask,
-                   base::Unretained(this), callback));
-  }
-  void WriteSystemTrace(
-      const scoped_refptr<base::RefCountedString>& events_str_ptr) {
-    BrowserThread::PostTask(
-        BrowserThread::FILE,
-        FROM_HERE,
-        base::Bind(&TracingControllerImpl::ResultFile::WriteSystemTraceTask,
-                   base::Unretained(this), events_str_ptr));
-  }
-
-  const base::FilePath& path() const { return path_; }
-
- private:
-  void OpenTask();
-  void WriteTask(const scoped_refptr<base::RefCountedString>& events_str_ptr);
-  void WriteSystemTraceTask(
-      const scoped_refptr<base::RefCountedString>& events_str_ptr);
-  void CloseTask(const base::Closure& callback);
-
-  FILE* file_;
-  base::FilePath path_;
-  bool has_at_least_one_result_;
-  scoped_refptr<base::RefCountedString> system_trace_;
-
-  DISALLOW_COPY_AND_ASSIGN(ResultFile);
-};
-
-TracingControllerImpl::ResultFile::ResultFile(const base::FilePath& path)
-    : file_(NULL),
-      path_(path),
-      has_at_least_one_result_(false) {
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      base::Bind(&TracingControllerImpl::ResultFile::OpenTask,
-                 base::Unretained(this)));
+TracingController::TraceDataSink* TracingControllerImpl::CreateStringSink(
+    const base::Callback<void(base::RefCountedMemory*)>& callback) {
+  return new StringTraceDataSink(callback);
 }
 
-void TracingControllerImpl::ResultFile::OpenTask() {
-  if (path_.empty())
-    base::CreateTemporaryFile(&path_);
-  file_ = base::OpenFile(path_, "w");
-  if (!file_) {
-    LOG(ERROR) << "Failed to open " << path_.value();
-    return;
-  }
-  const char* preamble = "{\"traceEvents\": [";
-  size_t written = fwrite(preamble, strlen(preamble), 1, file_);
-  DCHECK(written == 1);
+TracingController::TraceDataSink* TracingControllerImpl::CreateFileSink(
+    const base::FilePath& file_path, const base::Closure& callback) {
+  return new FileTraceDataSink(file_path, callback);
 }
 
-void TracingControllerImpl::ResultFile::WriteTask(
-    const scoped_refptr<base::RefCountedString>& events_str_ptr) {
-  if (!file_ || !events_str_ptr->data().size())
-    return;
-
-  // If there is already a result in the file, then put a comma
-  // before the next batch of results.
-  if (has_at_least_one_result_) {
-    size_t written = fwrite(",", 1, 1, file_);
-    DCHECK(written == 1);
-  }
-  has_at_least_one_result_ = true;
-  size_t written = fwrite(events_str_ptr->data().c_str(),
-                          events_str_ptr->data().size(), 1,
-                          file_);
-  DCHECK(written == 1);
-}
-
-void TracingControllerImpl::ResultFile::WriteSystemTraceTask(
-    const scoped_refptr<base::RefCountedString>& events_str_ptr) {
-  system_trace_ = events_str_ptr;
-}
-
-void TracingControllerImpl::ResultFile::CloseTask(
-    const base::Closure& callback) {
-  if (!file_)
-    return;
-
-  const char* trailevents = "]";
-  size_t written = fwrite(trailevents, strlen(trailevents), 1, file_);
-  DCHECK(written == 1);
-
-  if (system_trace_.get()) {
-#if defined(OS_WIN)
-    // The Windows kernel events are kept into a JSon format stored as string
-    // and must not be escaped.
-    std::string json_string = system_trace_->data();
-#else
-    std::string json_string = base::GetQuotedJSONString(system_trace_->data());
-#endif
-
-    const char* systemTraceHead = ",\n\"systemTraceEvents\": ";
-    written = fwrite(systemTraceHead, strlen(systemTraceHead), 1, file_);
-    DCHECK(written == 1);
-
-    written = fwrite(json_string.data(), json_string.size(), 1, file_);
-    DCHECK(written == 1);
-
-    system_trace_ = NULL;
-  }
-
-  const char* trailout = "}";
-  written = fwrite(trailout, strlen(trailout), 1, file_);
-  DCHECK(written == 1);
-  base::CloseFile(file_);
-  file_ = NULL;
-
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
-}
-
-
 TracingControllerImpl::TracingControllerImpl() :
     pending_disable_recording_ack_count_(0),
     pending_capture_monitoring_snapshot_ack_count_(0),
     pending_trace_buffer_percent_full_ack_count_(0),
     maximum_trace_buffer_percent_full_(0),
     // Tracing may have been enabled by ContentMainRunner if kTraceStartup
     // is specified in command line.
 #if defined(OS_CHROMEOS) || defined(OS_WIN)
     is_system_tracing_(false),
 #endif
     is_recording_(TraceLog::GetInstance()->IsEnabled()),
-    is_monitoring_(false) {
+    is_monitoring_(false),
+    trace_data_sink_(NULL),
+    monitoring_data_sink_(NULL) {
 }
 
 TracingControllerImpl::~TracingControllerImpl() {
   // This is a Leaky instance.
   NOTREACHED();
 }
 
 TracingControllerImpl* TracingControllerImpl::GetInstance() {
   return g_controller.Pointer();
 }
 
 bool TracingControllerImpl::GetCategories(
     const GetCategoriesDoneCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Known categories come back from child processes with the EndTracingAck
   // message. So to get known categories, just begin and end tracing immediately
   // afterwards. This will ping all the child processes for categories.
   pending_get_categories_done_callback_ = callback;
   if (!EnableRecording(
           CategoryFilter("*"), TraceOptions(), EnableRecordingDoneCallback())) {
     pending_get_categories_done_callback_.Reset();
     return false;
   }
 
-  bool ok = DisableRecording(base::FilePath(), TracingFileResultCallback());
+  bool ok = DisableRecording(NULL);
   DCHECK(ok);
   return true;
 }
 
 void TracingControllerImpl::SetEnabledOnFileThread(
     const CategoryFilter& category_filter,
     int mode,
     const TraceOptions& trace_options,
     const base::Closure& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ skipping 65 matching lines @@
   // Notify all child processes.
   for (TraceMessageFilterSet::iterator it = trace_message_filters_.begin();
       it != trace_message_filters_.end(); ++it) {
     it->get()->SendBeginTracing(category_filter, trace_options);
   }
 
   if (!callback.is_null())
     callback.Run();
 }
 
-bool TracingControllerImpl::DisableRecording(
-    const base::FilePath& result_file_path,
-    const TracingFileResultCallback& callback) {
+bool TracingControllerImpl::DisableRecording(TraceDataSink* trace_data_sink) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!can_disable_recording())
     return false;
 
   trace_options_ = TraceOptions();
   // Disable local trace early to avoid traces during end-tracing process from
   // interfering with the process.
   base::Closure on_disable_recording_done_callback =
       base::Bind(&TracingControllerImpl::OnDisableRecordingDone,
-                 base::Unretained(this),
-                 result_file_path, callback);
+                 base::Unretained(this), trace_data_sink);
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
       base::Bind(&TracingControllerImpl::SetDisabledOnFileThread,
                  base::Unretained(this),
                  on_disable_recording_done_callback));
   return true;
 }
 
 void TracingControllerImpl::OnDisableRecordingDone(
-    const base::FilePath& result_file_path,
-    const TracingFileResultCallback& callback) {
+    TraceDataSink* trace_data_sink) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  pending_disable_recording_done_callback_ = callback;
-
 #if defined(OS_ANDROID)
   if (pending_get_categories_done_callback_.is_null())
     TraceLog::GetInstance()->AddClockSyncMetadataEvent();
 #endif
 
-  if (!callback.is_null() || !result_file_path.empty())
-    result_file_.reset(new ResultFile(result_file_path));
+  trace_data_sink_ = trace_data_sink;
 
   // Count myself (local trace) in pending_disable_recording_ack_count_,
   // acked below.
   pending_disable_recording_ack_count_ = trace_message_filters_.size() + 1;
   pending_disable_recording_filters_ = trace_message_filters_;
 
 #if defined(OS_CHROMEOS) || defined(OS_WIN)
   if (is_system_tracing_) {
     // Disable system tracing.
     is_system_tracing_ = false;
@@ skipping 101 matching lines @@
     const DisableMonitoringDoneCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   OnMonitoringStateChanged(false);
 
   // Notify all child processes.
   for (TraceMessageFilterSet::iterator it = trace_message_filters_.begin();
       it != trace_message_filters_.end(); ++it) {
     it->get()->SendDisableMonitoring();
   }
-
   if (!callback.is_null())
     callback.Run();
 }
 
 void TracingControllerImpl::GetMonitoringStatus(
     bool* out_enabled,
     CategoryFilter* out_category_filter,
     TraceOptions* out_trace_options) {
   *out_enabled = is_monitoring_;
   *out_category_filter = TraceLog::GetInstance()->GetCurrentCategoryFilter();
   *out_trace_options = trace_options_;
 }
 
 bool TracingControllerImpl::CaptureMonitoringSnapshot(
-    const base::FilePath& result_file_path,
-    const TracingFileResultCallback& callback) {
+    TraceDataSink* monitoring_data_sink) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!can_disable_monitoring())
     return false;
 
-  if (callback.is_null() && result_file_path.empty())
+  if (!monitoring_data_sink)
     return false;
 
-  pending_capture_monitoring_snapshot_done_callback_ = callback;
-  monitoring_snapshot_file_.reset(new ResultFile(result_file_path));
+  monitoring_data_sink_ = monitoring_data_sink;
 
   // Count myself in pending_capture_monitoring_snapshot_ack_count_,
   // acked below.
   pending_capture_monitoring_snapshot_ack_count_ =
       trace_message_filters_.size() + 1;
   pending_capture_monitoring_filters_ = trace_message_filters_;
 
   // Handle special case of zero child processes by immediately flushing the
   // trace log. Once the flush has completed the caller will be notified that
   // the capture snapshot has ended.
@@ skipping 196 matching lines @@
     // called with the last of the local trace data.
     TraceLog::GetInstance()->Flush(
         base::Bind(&TracingControllerImpl::OnLocalTraceDataCollected,
                    base::Unretained(this)));
     return;
   }
 
   if (pending_disable_recording_ack_count_ != 0)
     return;
 
-  OnDisableRecordingComplete();
-}
-
-void TracingControllerImpl::OnDisableRecordingComplete() {
   // All acks (including from the subprocesses and the local trace) have been
   // received.
   is_recording_ = false;
 
   // Trigger callback if one is set.
   if (!pending_get_categories_done_callback_.is_null()) {
     pending_get_categories_done_callback_.Run(known_category_groups_);
     pending_get_categories_done_callback_.Reset();
-  } else if (result_file_) {
-    result_file_->Close(
-        base::Bind(&TracingControllerImpl::OnResultFileClosed,
-                   base::Unretained(this)));
+  } else if (trace_data_sink_) {
+    trace_data_sink_->Close();
+    trace_data_sink_ = 0;
   }
 }
 
-void TracingControllerImpl::OnResultFileClosed() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (!result_file_)
-    return;
-
-  if (!pending_disable_recording_done_callback_.is_null()) {
-    pending_disable_recording_done_callback_.Run(result_file_->path());
-    pending_disable_recording_done_callback_.Reset();
-  }
-  result_file_.reset();
-}
-
 #if defined(OS_CHROMEOS) || defined(OS_WIN)
 void TracingControllerImpl::OnEndSystemTracingAcked(
     const scoped_refptr<base::RefCountedString>& events_str_ptr) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (result_file_)
-    result_file_->WriteSystemTrace(events_str_ptr);
-
+  if (trace_data_sink_) {
+#if defined(OS_WIN)
+    // The Windows kernel events are kept into a JSon format stored as string
+    // and must not be escaped.
+    std::string json_string = events_str_ptr_->data();
+#else
+    std::string json_string =
+        base::GetQuotedJSONString(events_str_ptr_->data());
+#endif
+    trace_data_sink_->SetSystemTrace(json_string);
+  }
   DCHECK(!is_system_tracing_);
   std::vector<std::string> category_groups;
   OnDisableRecordingAcked(NULL, category_groups);
 }
 #endif
 
 void TracingControllerImpl::OnCaptureMonitoringSnapshotAcked(
     TraceMessageFilter* trace_message_filter) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
@@ skipping 18 matching lines @@
     // will be called with the last of the local trace data.
     TraceLog::GetInstance()->FlushButLeaveBufferIntact(
         base::Bind(&TracingControllerImpl::OnLocalMonitoringTraceDataCollected,
                    base::Unretained(this)));
     return;
   }
 
   if (pending_capture_monitoring_snapshot_ack_count_ != 0)
     return;
 
-  if (monitoring_snapshot_file_) {
-    monitoring_snapshot_file_->Close(
-        base::Bind(&TracingControllerImpl::OnMonitoringSnapshotFileClosed,
-                   base::Unretained(this)));
+  if (monitoring_data_sink_) {
+      monitoring_data_sink_->Close();
+      monitoring_data_sink_ =  NULL;
   }
 }
 
-void TracingControllerImpl::OnMonitoringSnapshotFileClosed() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (!monitoring_snapshot_file_)
-    return;
-
-  if (!pending_capture_monitoring_snapshot_done_callback_.is_null()) {
-    pending_capture_monitoring_snapshot_done_callback_.Run(
-        monitoring_snapshot_file_->path());
-    pending_capture_monitoring_snapshot_done_callback_.Reset();
-  }
-  monitoring_snapshot_file_.reset();
-}
-
 void TracingControllerImpl::OnTraceDataCollected(
     const scoped_refptr<base::RefCountedString>& events_str_ptr) {
   // OnTraceDataCollected may be called from any browser thread, either by the
   // local event trace system or from child processes via TraceMessageFilter.
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
         base::Bind(&TracingControllerImpl::OnTraceDataCollected,
                    base::Unretained(this), events_str_ptr));
     return;
   }
 
-  if (result_file_)
-    result_file_->Write(events_str_ptr);
+  if (trace_data_sink_)
+    trace_data_sink_->AddTraceChunk(events_str_ptr->data());
 }
 
 void TracingControllerImpl::OnMonitoringTraceDataCollected(
     const scoped_refptr<base::RefCountedString>& events_str_ptr) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
         base::Bind(&TracingControllerImpl::OnMonitoringTraceDataCollected,
                    base::Unretained(this), events_str_ptr));
     return;
   }
 
-  if (monitoring_snapshot_file_)
-    monitoring_snapshot_file_->Write(events_str_ptr);
+  if (monitoring_data_sink_)
+    monitoring_data_sink_->AddTraceChunk(events_str_ptr->data());
 }
 
 void TracingControllerImpl::OnLocalTraceDataCollected(
     const scoped_refptr<base::RefCountedString>& events_str_ptr,
     bool has_more_events) {
   if (events_str_ptr->data().size())
     OnTraceDataCollected(events_str_ptr);
 
   if (has_more_events)
     return;
@@ skipping 79 matching lines @@
   is_monitoring_ = is_monitoring;
 #if !defined(OS_ANDROID)
   for (std::set<TracingUI*>::iterator it = tracing_uis_.begin();
        it != tracing_uis_.end(); it++) {
     (*it)->OnMonitoringStateChanged(is_monitoring);
   }
 #endif
 }
 
 }  // namespace content