Revert "base: Make TraceEvent a movable class."

This reverts commit 03982f12.

Reason for revert: Caused compile failure on code coverage bot https://bugs.chromium.org/p/chromium/issues/detail?id=908937

Original change's description:
> base: Make TraceEvent a movable class.
> 
> This is a small cleanup of the TraceEvent class performed
> which is part of a larget bug allowing cleaning up and
> reducing the generated machine code for TRACE_EVENTXXX()
> macro calls (see related bug).
> 
> A first CL to perform this refactor was submitted as [1], but
> later reverted because it made some tests fail mysteriously
> (see http://crbug.com/899813). So the original CL was split into
> several independent ones.
> 
> A first CL was submitted as [2], which actually fixed some
> potential dangling pointer issues that were created from the
> Initialize() and MoveFrom() methods.
> 
> This second CL removes this methods by making TraceEvent a
> proper C++11 movable type, which should prevent (or at least
> make it more difficult) introducing invalid states for its
> instances. The goal is to see if this introduces new unexpected
> test failures (which would indicate that there are still invalid
> instance states used in the code base).
> 
> The third CL is [3] and re-introduces the TraceArguments helper
> class on top of this one.
> 
> [1] https://chromium-review.googlesource.com/c/chromium/src/+/1318919
> [2] https://chromium-review.googlesource.com/c/chromium/src/+/1340308
> [3] https://chromium-review.googlesource.com/c/chromium/src/+/1318919
> 
> BUG=898794
> R=​primiano@chromium.org,oysteine@chromium.org,alexilin@chromium.org,chiniforooshan@chromium.org
> 
> Change-Id: I2b36885e2485d23cca199c48b2bd07d5745b00c5
> Reviewed-on: https://chromium-review.googlesource.com/c/1346305
> Commit-Queue: David Turner <digit@chromium.org>
> Reviewed-by: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
> Reviewed-by: Alexandr Ilin <alexilin@chromium.org>
> Reviewed-by: oysteine <oysteine@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#611185}

TBR=digit@chromium.org,primiano@chromium.org,chiniforooshan@chromium.org,oysteine@chromium.org,alexilin@chromium.org

Change-Id: I73d85745e3b67cbcecddd25cb78a8e22ba031965
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 898794
Reviewed-on: https://chromium-review.googlesource.com/c/1351557Reviewed-by: Shuotao Gao <stgao@chromium.org>
Commit-Queue: Shuotao Gao <stgao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611259}

base/trace_event/event_name_filter_unittest.cc

@@ -13,8 +13,9 @@ namespace trace_event {
 
 const TraceEvent& MakeTraceEvent(const char* name) {
   static TraceEvent event;
-  event.Reset(0, TimeTicks(), ThreadTicks(), 'b', nullptr, name, "", 0, 0, 0,
-              nullptr, nullptr, nullptr, nullptr, 0);
+  event.Reset();
+  event.Initialize(0, TimeTicks(), ThreadTicks(), 'b', nullptr, name, "", 0, 0,
+                   0, nullptr, nullptr, nullptr, nullptr, 0);
   return event;
 }
 

base/trace_event/trace_event_impl.cc

@@ -45,24 +45,49 @@ bool ConvertableToTraceFormat::AppendToProto(ProtoAppender* appender) {
   return false;
 }
 
-// See comment for name TraceEvent::scope_ definition.
-static_assert(trace_event_internal::kGlobalScope == nullptr,
-              "Invalid TraceEvent::scope default initializer value");
-
-TraceEvent::TraceEvent() {
-  for (int i = 0; i < kTraceMaxNumArgs; ++i) {
-    arg_values_[i].as_uint = 0u;
+TraceEvent::TraceEvent()
+    : duration_(TimeDelta::FromInternalValue(-1)),
+      scope_(trace_event_internal::kGlobalScope),
+      id_(0u),
+      category_group_enabled_(nullptr),
+      name_(nullptr),
+      thread_id_(0),
+      flags_(0),
+      phase_(TRACE_EVENT_PHASE_BEGIN) {
+  for (int i = 0; i < kTraceMaxNumArgs; ++i)
     arg_names_[i] = nullptr;
-    arg_types_[i] = TRACE_VALUE_TYPE_UINT;
-  }
+  memset(arg_values_, 0, sizeof(arg_values_));
 }
 
 TraceEvent::~TraceEvent() = default;
 
-TraceEvent::TraceEvent(TraceEvent&& other) noexcept = default;
-TraceEvent& TraceEvent::operator=(TraceEvent&& other) noexcept = default;
+void TraceEvent::MoveFrom(std::unique_ptr<TraceEvent> other) {
+  timestamp_ = other->timestamp_;
+  thread_timestamp_ = other->thread_timestamp_;
+  duration_ = other->duration_;
+  scope_ = other->scope_;
+  id_ = other->id_;
+  category_group_enabled_ = other->category_group_enabled_;
+  name_ = other->name_;
+  if (other->flags_ & TRACE_EVENT_FLAG_HAS_PROCESS_ID)
+    process_id_ = other->process_id_;
+  else
+    thread_id_ = other->thread_id_;
+  phase_ = other->phase_;
+  flags_ = other->flags_;
+  parameter_copy_storage_ = std::move(other->parameter_copy_storage_);
 
-TraceEvent::TraceEvent(
+  for (int i = 0; i < kTraceMaxNumArgs; ++i) {
+    arg_names_[i] = other->arg_names_[i];
+    arg_types_[i] = other->arg_types_[i];
+    arg_values_[i] = other->arg_values_[i];
+    convertable_values_[i] = std::move(other->convertable_values_[i]);
+  }
+
+  other->Reset();
+}
+
+void TraceEvent::Initialize(
     int thread_id,
     TimeTicks timestamp,
     ThreadTicks thread_timestamp,
@@ -72,33 +97,24 @@ TraceEvent::TraceEvent(
     const char* scope,
     unsigned long long id,
     unsigned long long bind_id,
-    int num_args,
-    const char* const* arg_names,
-    const unsigned char* arg_types,
-    const unsigned long long* arg_values,
-    std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
-    unsigned int flags)
-    : timestamp_(timestamp),
-      thread_timestamp_(thread_timestamp),
-      scope_(scope),
-      id_(id),
-      category_group_enabled_(category_group_enabled),
-      name_(name),
-      thread_id_(thread_id),
-      flags_(flags),
-      bind_id_(bind_id),
-      phase_(phase) {
-  InitArgs(num_args, arg_names, arg_types, arg_values, convertable_values,
-           flags);
-}
-
-void TraceEvent::InitArgs(
     int num_args,
     const char* const* arg_names,
     const unsigned char* arg_types,
     const unsigned long long* arg_values,
     std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
     unsigned int flags) {
+  timestamp_ = timestamp;
+  thread_timestamp_ = thread_timestamp;
+  duration_ = TimeDelta::FromInternalValue(-1);
+  scope_ = scope;
+  id_ = id;
+  category_group_enabled_ = category_group_enabled;
+  name_ = name;
+  thread_id_ = thread_id;
+  phase_ = phase;
+  flags_ = flags;
+  bind_id_ = bind_id;
+
   // Clamp num_args since it may have been set by a third_party library.
   num_args = (num_args > kTraceMaxNumArgs) ? kTraceMaxNumArgs : num_args;
   int i = 0;
@@ -123,7 +139,7 @@ void TraceEvent::InitArgs(
   bool copy = !!(flags & TRACE_EVENT_FLAG_COPY);
   size_t alloc_size = 0;
   if (copy) {
-    alloc_size += GetAllocLength(name_) + GetAllocLength(scope_);
+    alloc_size += GetAllocLength(name) + GetAllocLength(scope);
     for (i = 0; i < num_args; ++i) {
       alloc_size += GetAllocLength(arg_names_[i]);
       if (arg_types_[i] == TRACE_VALUE_TYPE_STRING)
@@ -166,7 +182,7 @@ void TraceEvent::InitArgs(
 }
 
 void TraceEvent::Reset() {
-  // Only reset fields that won't be initialized in Reset(int, ...), or that may
+  // Only reset fields that won't be initialized in Initialize(), or that may
   // hold references to other objects.
   duration_ = TimeDelta::FromInternalValue(-1);
 
@@ -187,38 +203,6 @@ void TraceEvent::Reset() {
     convertable_values_[i].reset();
 }
 
-void TraceEvent::Reset(
-    int thread_id,
-    TimeTicks timestamp,
-    ThreadTicks thread_timestamp,
-    char phase,
-    const unsigned char* category_group_enabled,
-    const char* name,
-    const char* scope,
-    unsigned long long id,
-    unsigned long long bind_id,
-    int num_args,
-    const char* const* arg_names,
-    const unsigned char* arg_types,
-    const unsigned long long* arg_values,
-    std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
-    unsigned int flags) {
-  Reset();
-  timestamp_ = timestamp;
-  thread_timestamp_ = thread_timestamp;
-  scope_ = scope;
-  id_ = id;
-  category_group_enabled_ = category_group_enabled;
-  name_ = name;
-  thread_id_ = thread_id;
-  flags_ = flags;
-  bind_id_ = bind_id;
-  phase_ = phase;
-
-  InitArgs(num_args, arg_names, arg_types, arg_values, convertable_values,
-           flags);
-}
-
 void TraceEvent::UpdateDuration(const TimeTicks& now,
                                 const ThreadTicks& thread_now) {
   DCHECK_EQ(duration_.ToInternalValue(), -1);

base/trace_event/trace_event_impl.h

@@ -23,7 +23,6 @@
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_local.h"
-#include "base/trace_event/common/trace_event_common.h"
 #include "base/trace_event/trace_event_memory_overhead.h"
 #include "build/build_config.h"
 
@@ -100,55 +99,28 @@ class BASE_EXPORT TraceEvent {
   };
 
   TraceEvent();
-
-  TraceEvent(int thread_id,
-             TimeTicks timestamp,
-             ThreadTicks thread_timestamp,
-             char phase,
-             const unsigned char* category_group_enabled,
-             const char* name,
-             const char* scope,
-             unsigned long long id,
-             unsigned long long bind_id,
-             int num_args,
-             const char* const* arg_names,
-             const unsigned char* arg_types,
-             const unsigned long long* arg_values,
-             std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
-             unsigned int flags);
-
   ~TraceEvent();
 
-  // Allow move operations.
-  TraceEvent(TraceEvent&&) noexcept;
-  TraceEvent& operator=(TraceEvent&&) noexcept;
+  void MoveFrom(std::unique_ptr<TraceEvent> other);
+
+  void Initialize(int thread_id,
+                  TimeTicks timestamp,
+                  ThreadTicks thread_timestamp,
+                  char phase,
+                  const unsigned char* category_group_enabled,
+                  const char* name,
+                  const char* scope,
+                  unsigned long long id,
+                  unsigned long long bind_id,
+                  int num_args,
+                  const char* const* arg_names,
+                  const unsigned char* arg_types,
+                  const unsigned long long* arg_values,
+                  std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
+                  unsigned int flags);
 
-  // Reset instance to empty state.
   void Reset();
 
-  // Reset instance to new state. This is equivalent but slightly more
-  // efficient than doing a move assignment, since it avoids creating
-  // temporary copies. I.e. compare these two statements:
-  //
-  //    event = TraceEvent(thread_id, ....);  // Create and destroy temporary.
-  //    event.Reset(thread_id, ...);  // Direct re-initialization.
-  //
-  void Reset(int thread_id,
-             TimeTicks timestamp,
-             ThreadTicks thread_timestamp,
-             char phase,
-             const unsigned char* category_group_enabled,
-             const char* name,
-             const char* scope,
-             unsigned long long id,
-             unsigned long long bind_id,
-             int num_args,
-             const char* const* arg_names,
-             const unsigned char* arg_types,
-             const unsigned long long* arg_values,
-             std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
-             unsigned int flags);
-
   void UpdateDuration(const TimeTicks& now, const ThreadTicks& thread_now);
 
   void EstimateTraceMemoryOverhead(TraceEventMemoryOverhead* overhead);
@@ -199,43 +171,32 @@ class BASE_EXPORT TraceEvent {
 #endif
 
  private:
-  void InitArgs(int num_args,
-                const char* const* arg_names,
-                const unsigned char* arg_types,
-                const unsigned long long* arg_values,
-                std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
-                unsigned int flags);
-
   // Note: these are ordered by size (largest first) for optimal packing.
-  TimeTicks timestamp_ = TimeTicks();
-  ThreadTicks thread_timestamp_ = ThreadTicks();
-  TimeDelta duration_ = TimeDelta::FromInternalValue(-1);
-  TimeDelta thread_duration_ = TimeDelta();
+  TimeTicks timestamp_;
+  ThreadTicks thread_timestamp_;
+  TimeDelta duration_;
+  TimeDelta thread_duration_;
   // scope_ and id_ can be used to store phase-specific data.
-  // The following should be default-initialized to the expression
-  // trace_event_internal::kGlobalScope, which is nullptr, but its definition
-  // cannot be included here due to cyclical header dependencies.
-  // The equivalence is checked with a static_assert() in trace_event_impl.cc.
-  const char* scope_ = nullptr;
-  unsigned long long id_ = 0u;
+  const char* scope_;
+  unsigned long long id_;
   TraceValue arg_values_[kTraceMaxNumArgs];
   const char* arg_names_[kTraceMaxNumArgs];
   std::unique_ptr<ConvertableToTraceFormat>
       convertable_values_[kTraceMaxNumArgs];
-  const unsigned char* category_group_enabled_ = nullptr;
-  const char* name_ = nullptr;
+  const unsigned char* category_group_enabled_;
+  const char* name_;
   std::unique_ptr<std::string> parameter_copy_storage_;
   // Depending on TRACE_EVENT_FLAG_HAS_PROCESS_ID the event will have either:
   //  tid: thread_id_, pid: current_process_id (default case).
   //  tid: -1, pid: process_id_ (when flags_ & TRACE_EVENT_FLAG_HAS_PROCESS_ID).
   union {
-    int thread_id_ = 0;
+    int thread_id_;
     int process_id_;
   };
-  unsigned int flags_ = 0;
-  unsigned long long bind_id_ = 0;
+  unsigned int flags_;
+  unsigned long long bind_id_;
   unsigned char arg_types_[kTraceMaxNumArgs];
-  char phase_ = TRACE_EVENT_PHASE_BEGIN;
+  char phase_;
 
   DISALLOW_COPY_AND_ASSIGN(TraceEvent);
 };

base/trace_event/trace_log.cc

@@ -111,14 +111,21 @@ void InitializeMetadataEvent(TraceEvent* trace_event,
   unsigned char arg_type;
   unsigned long long arg_value;
   ::trace_event_internal::SetTraceValue(value, &arg_type, &arg_value);
-
-  trace_event->Reset(
-      thread_id, TimeTicks(), ThreadTicks(), TRACE_EVENT_PHASE_METADATA,
-      CategoryRegistry::kCategoryMetadata->state_ptr(), metadata_name,
+  trace_event->Initialize(
+      thread_id,
+      TimeTicks(),
+      ThreadTicks(),
+      TRACE_EVENT_PHASE_METADATA,
+      CategoryRegistry::kCategoryMetadata->state_ptr(),
+      metadata_name,
       trace_event_internal::kGlobalScope,  // scope
-      trace_event_internal::kNoId,         // id
-      trace_event_internal::kNoId,         // bind_id
-      num_args, &arg_name, &arg_type, &arg_value, nullptr,
+      trace_event_internal::kNoId,  // id
+      trace_event_internal::kNoId,  // bind_id
+      num_args,
+      &arg_name,
+      &arg_type,
+      &arg_value,
+      nullptr,
       TRACE_EVENT_FLAG_NONE);
 }
 
@@ -1284,6 +1291,7 @@ TraceEventHandle TraceLog::AddTraceEventWithThreadIdAndTimestamp(
       reinterpret_cast<AddTraceEventOverrideCallback>(
           subtle::NoBarrier_Load(&trace_event_override_));
   if (trace_event_override) {
+    TraceEvent new_trace_event;
     // If we have an override in place for events, rather than sending
     // them to the tracelog, we don't have a way of going back and updating
     // the duration of _COMPLETE events. Instead, we emit separate _BEGIN
@@ -1291,7 +1299,7 @@ TraceEventHandle TraceLog::AddTraceEventWithThreadIdAndTimestamp(
     if (phase == TRACE_EVENT_PHASE_COMPLETE)
       phase = TRACE_EVENT_PHASE_BEGIN;
 
-    TraceEvent new_trace_event(thread_id, offset_event_timestamp, thread_now,
+    new_trace_event.Initialize(thread_id, offset_event_timestamp, thread_now,
                                phase, category_group_enabled, name, scope, id,
                                bind_id, num_args, arg_names, arg_types,
                                arg_values, convertable_values, flags);
@@ -1306,10 +1314,11 @@ TraceEventHandle TraceLog::AddTraceEventWithThreadIdAndTimestamp(
   std::unique_ptr<TraceEvent> filtered_trace_event;
   bool disabled_by_filters = false;
   if (*category_group_enabled & TraceCategory::ENABLED_FOR_FILTERING) {
-    auto new_trace_event = std::make_unique<TraceEvent>(
-        thread_id, offset_event_timestamp, thread_now, phase,
-        category_group_enabled, name, scope, id, bind_id, num_args, arg_names,
-        arg_types, arg_values, convertable_values, flags);
+    std::unique_ptr<TraceEvent> new_trace_event(new TraceEvent);
+    new_trace_event->Initialize(thread_id, offset_event_timestamp, thread_now,
+                                phase, category_group_enabled, name, scope, id,
+                                bind_id, num_args, arg_names, arg_types,
+                                arg_values, convertable_values, flags);
 
     disabled_by_filters = true;
     ForEachCategoryFilter(
@@ -1338,12 +1347,12 @@ TraceEventHandle TraceLog::AddTraceEventWithThreadIdAndTimestamp(
 
     if (trace_event) {
       if (filtered_trace_event) {
-        *trace_event = std::move(*filtered_trace_event);
+        trace_event->MoveFrom(std::move(filtered_trace_event));
       } else {
-        trace_event->Reset(thread_id, offset_event_timestamp, thread_now, phase,
-                           category_group_enabled, name, scope, id, bind_id,
-                           num_args, arg_names, arg_types, arg_values,
-                           convertable_values, flags);
+        trace_event->Initialize(thread_id, offset_event_timestamp, thread_now,
+                                phase, category_group_enabled, name, scope, id,
+                                bind_id, num_args, arg_names, arg_types,
+                                arg_values, convertable_values, flags);
       }
 
 #if defined(OS_ANDROID)
@@ -1374,11 +1383,12 @@ void TraceLog::AddMetadataEvent(
     std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
     unsigned int flags) {
   HEAP_PROFILER_SCOPED_IGNORE;
+  std::unique_ptr<TraceEvent> trace_event(new TraceEvent);
   int thread_id = static_cast<int>(base::PlatformThread::CurrentId());
   ThreadTicks thread_now = ThreadNow();
   TimeTicks now = OffsetNow();
   AutoLock lock(lock_);
-  auto trace_event = std::make_unique<TraceEvent>(
+  trace_event->Initialize(
       thread_id, now, thread_now, TRACE_EVENT_PHASE_METADATA,
       category_group_enabled, name,
       trace_event_internal::kGlobalScope,  // scope
@@ -1494,7 +1504,8 @@ void TraceLog::UpdateTraceEventDurationExplicit(
     // we don't have way of updating the prior event so we'll emit a
     // separate _END event instead.
     if (trace_event_override) {
-      TraceEvent new_trace_event(
+      TraceEvent new_trace_event;
+      new_trace_event.Initialize(
           static_cast<int>(base::PlatformThread::CurrentId()), now, thread_now,
           TRACE_EVENT_PHASE_END, category_group_enabled, name,
           trace_event_internal::kGlobalScope,
@@ -1580,7 +1591,7 @@ void TraceLog::AddMetadataEventsWhileLocked() {
     while (!metadata_events_.empty()) {
       TraceEvent* event =
           AddEventToThreadSharedChunkWhileLocked(nullptr, false);
-      *event = std::move(*metadata_events_.back());
+      event->MoveFrom(std::move(metadata_events_.back()));
       metadata_events_.pop_back();
     }
   }