[UserTimingL3] Centralize PerformanceMark construction logic

Before this change, running javascript like `performance.mark(...)`
would result in a callstack like

  MakeGarbageCollected<PerformanceMark>(...)
  PerformanceMark::Create(<overload-1>)
  UserTiming::CreatePerformanceMark(...)
  Performance::mark()
  <v8-bindings-layer>
  `performance.mark(...)`

while running script like `new PerformanceMark(...)` would result in a
callstack like

  MakeGarbageCollected<PerformanceMark>(...)
  PerformanceMark::Create(<overload-1>)
  UserTiming::CreatePerformanceMark(...)
  PerformanceMark::Create(<overload-2>)
  `new PerformanceMark(...)`

Note that, since the bindings layer calls into PerformanceMark::Create
directly, PerformanceMark had to call UserTiming which had to call back
to PerformanceMark. Apart from being a cyclic dependency, this also lead
to confusion around which constraints needed to be checked at which
points.

This CL breaks the cyclic dependency between UserTiming and
PerformanceMark by pushing all the validation into a single
PerformanceMark::Create function.

Along with this change comes better UseCount tracking for UserTimingL3 in
the case where script runs `new PerformanceMark(...)`.

Bug: 805566
Change-Id: Ic648b097734655b2c0e55db95e738b48bc8826c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003552
Commit-Queue: Tom McKee <tommckee@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744117}

third_party/blink/renderer/core/timing/performance_mark.cc

@@ -11,7 +11,7 @@
 #include "third_party/blink/renderer/core/performance_entry_names.h"
 #include "third_party/blink/renderer/core/timing/dom_window_performance.h"
 #include "third_party/blink/renderer/core/timing/performance.h"
-#include "third_party/blink/renderer/core/timing/performance_user_timing.h"
+#include "third_party/blink/renderer/core/timing/performance_mark_options.h"
 #include "third_party/blink/renderer/core/timing/worker_global_scope_performance.h"
 #include "third_party/blink/renderer/core/workers/worker_global_scope.h"
 
@@ -25,48 +25,63 @@ PerformanceMark::PerformanceMark(
     : PerformanceEntry(name, start_time, start_time),
       serialized_detail_(std::move(serialized_detail)) {}
 
-// static
-PerformanceMark* PerformanceMark::Create(ScriptState* script_state,
-                                         const AtomicString& name,
-                                         double start_time,
-                                         const ScriptValue& detail,
-                                         ExceptionState& exception_state) {
-  scoped_refptr<SerializedScriptValue> serialized_detail;
-  if (detail.IsEmpty()) {
-    serialized_detail = SerializedScriptValue::NullValue();
-  } else {
-    serialized_detail = SerializedScriptValue::Serialize(
-        script_state->GetIsolate(), detail.V8Value(),
-        SerializedScriptValue::SerializeOptions(), exception_state);
-    if (exception_state.HadException())
-      return nullptr;
-  }
-  return MakeGarbageCollected<PerformanceMark>(
-      name, start_time, std::move(serialized_detail), exception_state);
-}
-
 // static
 PerformanceMark* PerformanceMark::Create(ScriptState* script_state,
                                          const AtomicString& mark_name,
                                          PerformanceMarkOptions* mark_options,
                                          ExceptionState& exception_state) {
   Performance* performance = nullptr;
+  bool is_worker_global_scope = false;
   if (LocalDOMWindow* window = LocalDOMWindow::From(script_state)) {
     performance = DOMWindowPerformance::performance(*window);
-    DCHECK(performance);
   } else if (auto* scope = DynamicTo<WorkerGlobalScope>(
                  ExecutionContext::From(script_state))) {
     performance = WorkerGlobalScopePerformance::performance(*scope);
-    DCHECK(performance);
+    is_worker_global_scope = true;
   }
+  DCHECK(performance);
 
-  if (performance) {
-    return performance->GetUserTiming().CreatePerformanceMark(
-        script_state, mark_name, mark_options, exception_state);
+  DOMHighResTimeStamp start = 0.0;
+  ScriptValue detail = ScriptValue::CreateNull(script_state->GetIsolate());
+  if (mark_options) {
+    if (mark_options->hasStartTime()) {
+      start = mark_options->startTime();
+      if (start < 0.0) {
+        exception_state.ThrowTypeError("'" + mark_name +
+                                       "' cannot have a negative start time.");
+        return nullptr;
+      }
+    } else {
+      start = performance->now();
+    }
+
+    detail = mark_options->detail();
+  } else {
+    start = performance->now();
+  }
+
+  if (!is_worker_global_scope &&
+      PerformanceTiming::GetAttributeMapping().Contains(mark_name)) {
+    exception_state.ThrowDOMException(
+        DOMExceptionCode::kSyntaxError,
+        "'" + mark_name +
+            "' is part of the PerformanceTiming interface, and "
+            "cannot be used as a mark name.");
+    return nullptr;
+  }
+
+  scoped_refptr<SerializedScriptValue> serialized_detail;
+  if (detail.IsEmpty()) {
+    serialized_detail = SerializedScriptValue::NullValue();
+  } else {
+    serialized_detail = SerializedScriptValue::Serialize(
+        script_state->GetIsolate(), detail.V8Value(),
+        SerializedScriptValue::SerializeOptions(), exception_state);
+    if (exception_state.HadException())
+      return nullptr;
   }
-  exception_state.ThrowTypeError(
-      "PerformanceMark: no 'worker' or 'window' in current context.");
-  return nullptr;
+  return MakeGarbageCollected<PerformanceMark>(
+      mark_name, start, std::move(serialized_detail), exception_state);
 }
 
 AtomicString PerformanceMark::entryType() const {

third_party/blink/renderer/core/timing/performance_mark.h

@@ -43,18 +43,17 @@ class CORE_EXPORT PerformanceMark final : public PerformanceEntry {
   DEFINE_WRAPPERTYPEINFO();
 
  public:
-  static PerformanceMark* Create(ScriptState* script_state,
-                                 const AtomicString& name,
-                                 double start_time,
-                                 const ScriptValue& detail,
-                                 ExceptionState& exception_state);
-
-  // This method is required by the constructor defined in performance_mark.idl.
+  // This method corresponds to the PerformanceMark constructor defined in the
+  // User Timing L3 spec
+  // (https://w3c.github.io/user-timing/#the-performancemark-constructor). It
+  // gets called as a subroutine of the `performance.mark` method as well as
+  // whenever script runs `new PerformanceMark(..)`.
   static PerformanceMark* Create(ScriptState*,
                                  const AtomicString& mark_name,
                                  PerformanceMarkOptions*,
                                  ExceptionState&);
 
+  // This constructor is only public so that MakeGarbageCollected can call it.
   PerformanceMark(const AtomicString& name,
                   double start_time,
                   scoped_refptr<SerializedScriptValue>,

third_party/blink/renderer/core/timing/performance_mark.idl

@@ -28,6 +28,6 @@
 [
 Exposed=(Window,Worker)
 ] interface PerformanceMark : PerformanceEntry {
-    [CallWith=ScriptState, RaisesException] constructor(DOMString markName, optional PerformanceMarkOptions markOptions = {});
+    [MeasureAs=UserTimingL3, CallWith=ScriptState, RaisesException] constructor(DOMString markName, optional PerformanceMarkOptions markOptions = {});
     [CallWith=ScriptState] readonly attribute any detail;
 };

third_party/blink/renderer/core/timing/performance_mark_test.cc

@@ -14,26 +14,6 @@
 
 namespace blink {
 
-TEST(PerformanceMarkTest, CreateWithScriptValue) {
-  V8TestingScope scope;
-
-  ExceptionState& exception_state = scope.GetExceptionState();
-  ScriptState* script_state = scope.GetScriptState();
-  v8::Isolate* isolate = scope.GetIsolate();
-  scoped_refptr<SerializedScriptValue> payload_string =
-      SerializedScriptValue::Create(String("some-payload"));
-  ScriptValue script_value(isolate, payload_string->Deserialize(isolate));
-
-  PerformanceMark* pm = PerformanceMark::Create(script_state, "mark-name",
-                                                /*start_time=*/0.0,
-                                                script_value, exception_state);
-
-  ASSERT_EQ(pm->entryType(), performance_entry_names::kMark);
-  ASSERT_EQ(pm->EntryTypeEnum(), PerformanceEntry::EntryType::kMark);
-  ASSERT_EQ(payload_string->Deserialize(isolate),
-            pm->detail(script_state).V8Value());
-}
-
 TEST(PerformanceMarkTest, CreateWithOptions) {
   V8TestingScope scope;
 
@@ -49,7 +29,6 @@ TEST(PerformanceMarkTest, CreateWithOptions) {
 
   PerformanceMark* pm = PerformanceMark::Create(script_state, "mark-name",
                                                 options, exception_state);
-
   ASSERT_EQ(pm->entryType(), performance_entry_names::kMark);
   ASSERT_EQ(pm->EntryTypeEnum(), PerformanceEntry::EntryType::kMark);
   ASSERT_EQ(payload_string->Deserialize(isolate),

third_party/blink/renderer/core/timing/performance_observer_test.cc

@@ -13,6 +13,7 @@
 #include "third_party/blink/renderer/core/timing/layout_shift.h"
 #include "third_party/blink/renderer/core/timing/performance.h"
 #include "third_party/blink/renderer/core/timing/performance_mark.h"
+#include "third_party/blink/renderer/core/timing/performance_mark_options.h"
 #include "third_party/blink/renderer/core/timing/window_performance.h"
 
 namespace blink {
@@ -88,9 +89,10 @@ TEST_F(PerformanceObserverTest, Enqueue) {
   NonThrowableExceptionState exception_state;
   Initialize(scope.GetScriptState());
 
-  ScriptValue empty_value;
+  PerformanceMarkOptions* options = PerformanceMarkOptions::Create();
+  options->setStartTime(1234);
   Persistent<PerformanceEntry> entry = PerformanceMark::Create(
-      scope.GetScriptState(), "m", 1234, empty_value, exception_state);
+      scope.GetScriptState(), "m", options, exception_state);
   EXPECT_EQ(0, NumPerformanceEntries());
 
   observer_->EnqueuePerformanceEntry(*entry);
@@ -102,9 +104,10 @@ TEST_F(PerformanceObserverTest, Deliver) {
   NonThrowableExceptionState exception_state;
   Initialize(scope.GetScriptState());
 
-  ScriptValue empty_value;
+  PerformanceMarkOptions* options = PerformanceMarkOptions::Create();
+  options->setStartTime(1234);
   Persistent<PerformanceEntry> entry = PerformanceMark::Create(
-      scope.GetScriptState(), "m", 1234, empty_value, exception_state);
+      scope.GetScriptState(), "m", options, exception_state);
   EXPECT_EQ(0, NumPerformanceEntries());
 
   observer_->EnqueuePerformanceEntry(*entry);
@@ -119,9 +122,10 @@ TEST_F(PerformanceObserverTest, Disconnect) {
   NonThrowableExceptionState exception_state;
   Initialize(scope.GetScriptState());
 
-  ScriptValue empty_value;
+  PerformanceMarkOptions* options = PerformanceMarkOptions::Create();
+  options->setStartTime(1234);
   Persistent<PerformanceEntry> entry = PerformanceMark::Create(
-      scope.GetScriptState(), "m", 1234, empty_value, exception_state);
+      scope.GetScriptState(), "m", options, exception_state);
   EXPECT_EQ(0, NumPerformanceEntries());
 
   observer_->EnqueuePerformanceEntry(*entry);

third_party/blink/renderer/core/timing/performance_user_timing.cc

@@ -65,36 +65,7 @@ PerformanceMark* UserTiming::CreatePerformanceMark(
     const AtomicString& mark_name,
     PerformanceMarkOptions* mark_options,
     ExceptionState& exception_state) {
-  DOMHighResTimeStamp start = 0.0;
-  if (mark_options && mark_options->hasStartTime()) {
-    start = mark_options->startTime();
-    if (start < 0.0) {
-      exception_state.ThrowTypeError("'" + mark_name +
-                                     "' cannot have a negative start time.");
-      return nullptr;
-    }
-  } else {
-    start = performance_->now();
-  }
-
-  ScriptValue detail = ScriptValue::CreateNull(script_state->GetIsolate());
-  if (mark_options)
-    detail = mark_options->detail();
-
-  bool is_worker_global_scope =
-      performance_->GetExecutionContext() &&
-      performance_->GetExecutionContext()->IsWorkerGlobalScope();
-  if (!is_worker_global_scope &&
-      PerformanceTiming::GetAttributeMapping().Contains(mark_name)) {
-    exception_state.ThrowDOMException(
-        DOMExceptionCode::kSyntaxError,
-        "'" + mark_name +
-            "' is part of the PerformanceTiming interface, and "
-            "cannot be used as a mark name.");
-    return nullptr;
-  }
-
-  return PerformanceMark::Create(script_state, mark_name, start, detail,
+  return PerformanceMark::Create(script_state, mark_name, mark_options,
                                  exception_state);
 }