[LongTasks] Ship the buffered flag try 2

This change ships the buffered flag and implements the UseCounter to
track how often the buffer becomes full. WindowPerformance is subscribed
to observe longtasks upon construction, and only unsubscribed when the
GC kicks in. This allows us to remove UpdateLongTaskInstrumentation()
and should be performant assuming the counter confirms that most page
loads do not fill up the buffer.

First attempt:
https://chromium-review.googlesource.com/c/chromium/src/+/1956075

Intent:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/cX5ahS7nCFw

Bug: 1016815
Change-Id: I1003c6cf8fc34dbf9dd1cca5d60e33c8df841b91
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1975994
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Reviewed-by: Annie Sullivan <sullivan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726519}

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

@@ -107,6 +107,7 @@ constexpr size_t kDefaultEventTimingBufferSize = 150;
 constexpr size_t kDefaultElementTimingBufferSize = 150;
 constexpr size_t kDefaultLayoutShiftBufferSize = 150;
 constexpr size_t kDefaultLargestContenfulPaintSize = 150;
+constexpr size_t kDefaultLongTaskBufferSize = 200;
 
 Performance::Performance(
     base::TimeTicks time_origin,
@@ -262,11 +263,11 @@ PerformanceEntryVector Performance::getEntriesByTypeInternal(
       if (first_contentful_paint_timing_)
         entries.push_back(first_contentful_paint_timing_);
       break;
-    // Unsupported for LongTask, TaskAttribution.
-    // Per the spec, these entries can only be accessed via
-    // Performance Observer. No separate buffer is maintained.
     case PerformanceEntry::kLongTask:
+      for (const auto& entry : longtask_buffer_)
+        entries.push_back(entry);
       break;
+    // TaskAttribution entries are only associated to longtask entries.
     case PerformanceEntry::kTaskAttribution:
       break;
     case PerformanceEntry::kLayoutShift:
@@ -660,10 +661,16 @@ void Performance::AddLongTaskTiming(base::TimeTicks start_time,
   if (!HasObserverFor(PerformanceEntry::kLongTask))
     return;
 
+  UseCounter::Count(GetExecutionContext(), WebFeature::kLongTaskObserver);
   auto* entry = MakeGarbageCollected<PerformanceLongTaskTiming>(
       MonotonicTimeToDOMHighResTimeStamp(start_time),
       MonotonicTimeToDOMHighResTimeStamp(end_time), name, container_type,
       container_src, container_id, container_name);
+  if (longtask_buffer_.size() < kDefaultLongTaskBufferSize) {
+    longtask_buffer_.push_back(entry);
+  } else {
+    UseCounter::Count(GetExecutionContext(), WebFeature::kLongTaskBufferFull);
+  }
   NotifyObserversOfEntry(*entry);
 }
 
@@ -853,14 +860,12 @@ ScriptPromise Performance::profile(ScriptState* script_state,
 void Performance::RegisterPerformanceObserver(PerformanceObserver& observer) {
   observer_filter_options_ |= observer.FilterOptions();
   observers_.insert(&observer);
-  UpdateLongTaskInstrumentation();
 }
 
 void Performance::UnregisterPerformanceObserver(
     PerformanceObserver& old_observer) {
   observers_.erase(&old_observer);
   UpdatePerformanceObserverFilterOptions();
-  UpdateLongTaskInstrumentation();
 }
 
 void Performance::UpdatePerformanceObserverFilterOptions() {
@@ -868,7 +873,6 @@ void Performance::UpdatePerformanceObserverFilterOptions() {
   for (const auto& observer : observers_) {
     observer_filter_options_ |= observer->FilterOptions();
   }
-  UpdateLongTaskInstrumentation();
 }
 
 void Performance::NotifyObserversOfEntry(PerformanceEntry& entry) const {
@@ -965,6 +969,7 @@ void Performance::Trace(blink::Visitor* visitor) {
   visitor->Trace(event_timing_buffer_);
   visitor->Trace(layout_shift_buffer_);
   visitor->Trace(largest_contentful_paint_buffer_);
+  visitor->Trace(longtask_buffer_);
   visitor->Trace(navigation_timing_);
   visitor->Trace(user_timing_);
   visitor->Trace(first_paint_timing_);

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

@@ -99,8 +99,6 @@ class CORE_EXPORT Performance : public EventTargetWithInlineData {
   virtual ScriptPromise measureMemory(ScriptState*,
                                       MeasureMemoryOptions*) const;
 
-  virtual void UpdateLongTaskInstrumentation() {}
-
   // Reduce the resolution to prevent timing attacks. See:
   // http://www.w3.org/TR/hr-time-2/#privacy-security
   static double ClampTimeResolution(double time_seconds);
@@ -365,6 +363,7 @@ class CORE_EXPORT Performance : public EventTargetWithInlineData {
   unsigned element_timing_buffer_max_size_;
   PerformanceEntryVector layout_shift_buffer_;
   PerformanceEntryVector largest_contentful_paint_buffer_;
+  PerformanceEntryVector longtask_buffer_;
   Member<PerformanceEntry> navigation_timing_;
   Member<UserTiming> user_timing_;
   Member<PerformanceEntry> first_paint_timing_;

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

@@ -177,22 +177,13 @@ void PerformanceObserver::observe(const PerformanceObserverInit* observer_init,
       return;
     }
     if (observer_init->buffered()) {
-      if (entry_type == PerformanceEntry::kLongTask) {
-        String message =
-            "Buffered flag does not support the 'longtask' entry type.";
-        GetExecutionContext()->AddConsoleMessage(ConsoleMessage::Create(
-            mojom::ConsoleMessageSource::kJavaScript,
-            mojom::ConsoleMessageLevel::kWarning, message));
-      } else {
-        // Append all entries of this type to the current performance_entries_
-        // to be returned on the next callback.
-        performance_entries_.AppendVector(
-            performance_->getBufferedEntriesByType(
-                AtomicString(observer_init->type())));
-        std::sort(performance_entries_.begin(), performance_entries_.end(),
-                  PerformanceEntry::StartTimeCompareLessThan);
-        is_buffered = true;
-      }
+      // Append all entries of this type to the current performance_entries_
+      // to be returned on the next callback.
+      performance_entries_.AppendVector(performance_->getBufferedEntriesByType(
+          AtomicString(observer_init->type())));
+      std::sort(performance_entries_.begin(), performance_entries_.end(),
+                PerformanceEntry::StartTimeCompareLessThan);
+      is_buffered = true;
     }
     filter_options_ |= entry_type;
   }

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

@@ -158,7 +158,12 @@ WindowPerformance::WindowPerformance(LocalDOMWindow* window)
     : Performance(
           ToTimeOrigin(window),
           window->document()->GetTaskRunner(TaskType::kPerformanceTimeline)),
-      DOMWindowClient(window) {}
+      DOMWindowClient(window) {
+  DCHECK(GetFrame());
+  DCHECK(GetFrame()->GetPerformanceMonitor());
+  GetFrame()->GetPerformanceMonitor()->Subscribe(
+      PerformanceMonitor::kLongTask, kLongTaskObserverThreshold, this);
+}
 
 WindowPerformance::~WindowPerformance() = default;
 
@@ -213,19 +218,6 @@ WindowPerformance::CreateNavigationTimingInstance() {
       GetFrame(), info, time_origin_, server_timing);
 }
 
-void WindowPerformance::UpdateLongTaskInstrumentation() {
-  if (!GetFrame() || !GetFrame()->GetDocument())
-    return;
-
-  if (HasObserverFor(PerformanceEntry::kLongTask)) {
-    UseCounter::Count(GetFrame()->GetDocument(), WebFeature::kLongTaskObserver);
-    GetFrame()->GetPerformanceMonitor()->Subscribe(
-        PerformanceMonitor::kLongTask, kLongTaskObserverThreshold, this);
-  } else {
-    GetFrame()->GetPerformanceMonitor()->UnsubscribeAll(this);
-  }
-}
-
 void WindowPerformance::BuildJSONValue(V8ObjectBuilder& builder) const {
   Performance::BuildJSONValue(builder);
   builder.Add("timing", timing()->toJSONForBinding(builder.GetScriptState()));

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

@@ -61,8 +61,6 @@ class CORE_EXPORT WindowPerformance final : public Performance,
 
   MemoryInfo* memory() const override;
 
-  void UpdateLongTaskInstrumentation() override;
-
   bool FirstInputDetected() const { return !!first_input_timing_; }
 
   // This method creates a PerformanceEventTiming and if needed creates a swap

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

@@ -96,18 +96,17 @@ class WindowPerformanceTest : public testing::Test {
 };
 
 TEST_F(WindowPerformanceTest, LongTaskObserverInstrumentation) {
-  performance_->UpdateLongTaskInstrumentation();
-  EXPECT_FALSE(ObservingLongTasks());
+  // Check that we're always observing longtasks
+  EXPECT_TRUE(ObservingLongTasks());
 
-  // Adding LongTask observer (with filer option) enables instrumentation.
+  // Adding LongTask observer.
   AddLongTaskObserver();
-  performance_->UpdateLongTaskInstrumentation();
   EXPECT_TRUE(ObservingLongTasks());
 
-  // Removing LongTask observer disables instrumentation.
+  // Removing LongTask observer doeos not cause us to stop observing. We still
+  // observe because entries should still be added to the longtasks buffer.
   RemoveLongTaskObserver();
-  performance_->UpdateLongTaskInstrumentation();
-  EXPECT_FALSE(ObservingLongTasks());
+  EXPECT_TRUE(ObservingLongTasks());
 }
 
 TEST_F(WindowPerformanceTest, SanitizedLongTaskName) {
@@ -141,7 +140,6 @@ TEST_F(WindowPerformanceTest, SanitizedLongTaskName_CrossOrigin) {
 // to the old window do not cause a crash.
 TEST_F(WindowPerformanceTest, NavigateAway) {
   AddLongTaskObserver();
-  performance_->UpdateLongTaskInstrumentation();
   EXPECT_TRUE(ObservingLongTasks());
 
   // Simulate navigation commit.

third_party/blink/web_tests/TestExpectations

@@ -5978,8 +5978,6 @@ crbug.com/1025944 [ Linux ] external/wpt/pointerevents/pointerlock/pointerevent_
 # Timeout media preload test until preloading media destinations gets enabled.
 crbug.com/977033 http/tests/priorities/resource-load-priorities-media-preload.html [ Timeout ]
 
-crbug.com/1016815 external/wpt/longtask-timing/buffered-flag.window.html [ Failure ]
-
 # Sheriff 2019-11-21
 crbug.com/1027287 css3/filters/effect-reference-image-lazy-attach.html [ Pass Failure ]