Don't allow direct const char* trace value

Source/platform/TraceEvent.h

Index: Source/platform/TraceEvent.h
diff --git a/Source/platform/TraceEvent.h b/Source/platform/TraceEvent.h
index 5c3589342823d0e9d72c6ade1cb61ce2cee2c382..fd6d2101632f6c6b9a2e9cfaf5806be151a332c3 100644
--- a/Source/platform/TraceEvent.h
+++ b/Source/platform/TraceEvent.h
@@ -125,15 +125,18 @@
 //        macros.
 //
 // When are string argument values copied:
-// const char* arg_values are only referenced by default:
+// - Literal string is always allowed:
+//     TRACE_EVENT1("category", "name", "arg1", "literal string");
+// - Use TRACE_STR_COPY to force copying of a const char*:
 //     TRACE_EVENT1("category", "name",
-//                  "arg1", "literal string is only referenced");
-// Use TRACE_STR_COPY to force copying of a const char*:
+//                  "arg1", TRACE_STR_COPY(string_will_be_copied));
+// - Use TRACE_STR_NO_COPY to explicitly pass a const char* that is ensured
+//   long-lived without copy:
 //     TRACE_EVENT1("category", "name",
-//                  "arg1", TRACE_STR_COPY("string will be copied"));
-// std::string arg_values are always copied:
+//                  "arg1", TRACE_STR_NO_COPY(long_lived_string));
+// - std::string arg_values are always copied:
 //     TRACE_EVENT1("category", "name",
-//                  "arg1", std::string("string will be copied"));
+//                  "arg1", std::string(string_will_be_copied));
 //
 //
 // Thread Safety:
@@ -169,10 +172,13 @@
 #include "wtf/PassRefPtr.h"
 #include "wtf/text/CString.h"
 
-// By default, const char* argument values are assumed to have long-lived scope
-// and will not be copied. Use this macro to force a const char* to be copied.
+// Non-literal const char* argument values are not allowed.
+// Use this macro to force a const char* to be copied.
 #define TRACE_STR_COPY(str) \
-    WebCore::TraceEvent::TraceStringWithCopy(str)
+    WebCore::TraceEvent::TraceStringWrapper<true>(str)
+// And use this macro to explicitly pass a long-lived const char*.
+#define TRACE_STR_NO_COPY(str) \
+    WebCore::TraceEvent::TraceStringWrapper<false>(str)
 
 // By default, uint64 ID argument values are not mangled with the Process ID in
 // TRACE_EVENT_ASYNC macros. Use this macro to force Process ID mangling.
@@ -734,11 +740,13 @@ union TraceValueUnion {
     const char* m_string;
 };
 
-// Simple container for const char* that should be copied instead of retained.
-class TraceStringWithCopy {
+// Simple container for const char*. The string will be copied or retained
+// according to the Copy template parameter.
+template <bool Copy>

[dsinclair, 2014/06/17 20:47:16]

Can we make the template parameter an enum? CopyString, NoCopyString?

[Xianzhu, 2014/06/17 21:27:35]

Done with a slightly different method using TraceStringWithCopy and
TraceStringWithoutCopy. In this method, the value of the template parameter
actually doesn't matter, while the typedefed types really matters.

+class TraceStringWrapper {
 public:
-    explicit TraceStringWithCopy(const char* str) : m_str(str) { }
-    operator const char* () const { return m_str; }
+    explicit TraceStringWrapper(const char* str) : m_str(str) { }
+    const char* str() const { return m_str; }
 private:
     const char* m_str;
 };
@@ -746,24 +754,17 @@ private:
 // Define setTraceValue for each allowed type. It stores the type and
 // value in the return arguments. This allows this API to avoid declaring any
 // structures so that it is portable to third_party libraries.
-#define INTERNAL_DECLARE_SET_TRACE_VALUE(actual_type, \
-                                         union_member, \
-                                         value_type_id) \
-    static inline void setTraceValue(actual_type arg, \
-                                     unsigned char* type, \
-                                     unsigned long long* value) { \
+#define INTERNAL_DECLARE_SET_TRACE_VALUE(actualType, argExpression, unionMember, valueTypeId) \
+    static inline void setTraceValue(actualType arg, unsigned char* type, unsigned long long* value) { \
         TraceValueUnion typeValue; \
-        typeValue.union_member = arg; \
-        *type = value_type_id; \
+        typeValue.unionMember = argExpression; \
+        *type = valueTypeId; \
         *value = typeValue.m_uint; \
     }
 // Simpler form for int types that can be safely casted.
-#define INTERNAL_DECLARE_SET_TRACE_VALUE_INT(actual_type, \
-                                             value_type_id) \
-    static inline void setTraceValue(actual_type arg, \
-                                     unsigned char* type, \
-                                     unsigned long long* value) { \
-        *type = value_type_id; \
+#define INTERNAL_DECLARE_SET_TRACE_VALUE_INT(actualType, valueTypeId) \
+    static inline void setTraceValue(actualType arg, unsigned char* type, unsigned long long* value) { \
+        *type = valueTypeId; \
         *value = static_cast<unsigned long long>(arg); \
     }
 
@@ -775,19 +776,44 @@ INTERNAL_DECLARE_SET_TRACE_VALUE_INT(long long, TRACE_VALUE_TYPE_INT)
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(int, TRACE_VALUE_TYPE_INT)
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(short, TRACE_VALUE_TYPE_INT)
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(signed char, TRACE_VALUE_TYPE_INT)
-INTERNAL_DECLARE_SET_TRACE_VALUE(bool, m_bool, TRACE_VALUE_TYPE_BOOL)
-INTERNAL_DECLARE_SET_TRACE_VALUE(double, m_double, TRACE_VALUE_TYPE_DOUBLE)
-INTERNAL_DECLARE_SET_TRACE_VALUE(const void*, m_pointer,
-                                 TRACE_VALUE_TYPE_POINTER)
-INTERNAL_DECLARE_SET_TRACE_VALUE(const char*, m_string,
-                                 TRACE_VALUE_TYPE_STRING)
-INTERNAL_DECLARE_SET_TRACE_VALUE(const TraceStringWithCopy&, m_string,
-                                 TRACE_VALUE_TYPE_COPY_STRING)
+INTERNAL_DECLARE_SET_TRACE_VALUE(bool, arg, m_bool, TRACE_VALUE_TYPE_BOOL)
+INTERNAL_DECLARE_SET_TRACE_VALUE(double, arg, m_double, TRACE_VALUE_TYPE_DOUBLE)
+INTERNAL_DECLARE_SET_TRACE_VALUE(const TraceStringWrapper<false>&, arg.str(), m_string, TRACE_VALUE_TYPE_STRING)
+INTERNAL_DECLARE_SET_TRACE_VALUE(const TraceStringWrapper<true>&, arg.str(), m_string, TRACE_VALUE_TYPE_COPY_STRING)
 
 #undef INTERNAL_DECLARE_SET_TRACE_VALUE
 #undef INTERNAL_DECLARE_SET_TRACE_VALUE_INT
 
-// WTF::String version of setTraceValue so that trace arguments can be strings.
+template <typename T>
+struct TraceValuePointerTypeAllowed {
+    static const bool allowed = true;
+};
+
+template <>
+struct TraceValuePointerTypeAllowed<char> {
+    static const bool allowed = false;
+};
+
+// Pointer version of SetTraceValue, but const char* is not allowed.
+template <typename T>
+static inline void setTraceValue(const T* arg, unsigned char* type, unsigned long long* value)
+{
+    COMPILE_ASSERT(TraceValuePointerTypeAllowed<T>::allowed, invalid_pointer_type);
+    TraceValueUnion typeValue;
+    typeValue.m_pointer = arg;
+    *type = TRACE_VALUE_TYPE_POINTER;
+    *value = typeValue.m_uint;
+}
+
+// Allow literal string.
+// FIXME: This will also allow passing local char arrays. Is it possible to avoid?
+template <size_t N>
+static inline void setTraceValue(const char (&arg)[N], unsigned char* type, unsigned long long* value)

[dsinclair, 2014/06/17 20:47:16]

This is going to end up stamping out a function for every length of string that
we pass into trace event. That seems like it could get expensive?

[Xianzhu, 2014/06/17 21:27:35]

There should be no difference before and after the change, because the functions
are inlined. Each setTraceValue call site is expanded to 2 or 3 assignments
despite this change (TRACE_STR_NO_COPY() and TRACE_STR_COPY() are just no-op in
optimized code).

+{
+    setTraceValue(TRACE_STR_NO_COPY(arg), type, value);
+}
+
+// WTF::CString version of setTraceValue so that trace arguments can be strings.
 static inline void setTraceValue(const WTF::CString& arg, unsigned char* type, unsigned long long* value)
 {
     TraceValueUnion typeValue;