Add tests for string refcount thrashing in setAttribute and getAttribute

These APIs were previously identified as hot paths that exhibited
regressions when attempting to make StringImpl's refcount atomic. We'll
use these tests to drive down refcount thrashing on these paths and
ensure it stays low.

Bug: 1083392
Change-Id: I5e92687b49cbf8da12f1676609e3f7755965530c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3117006
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kevin Babbitt <kbabbitt@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#916479}

third_party/blink/renderer/bindings/bindings.gni

@@ -234,5 +234,6 @@ bindings_unittest_files = get_path_info(
           "core/v8/script_promise_tester.h",
           "modules/v8/serialization/v8_script_value_serializer_for_modules_test.cc",
           "modules/v8/v8_binding_for_modules_test.cc",
+          "modules/v8/v8_element_test.cc",
         ],
         "abspath")

third_party/blink/renderer/bindings/modules/v8/v8_element_test.cc

@@ -0,0 +1,102 @@
+// Copyright (c) 2021 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 "third_party/blink/renderer/bindings/core/v8/script_source_code.h"
+#include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_testing.h"
+#include "third_party/blink/renderer/core/script/classic_script.h"
+#include "third_party/blink/renderer/platform/wtf/text/atomic_string.h"
+#include "third_party/blink/renderer/platform/wtf/text/atomic_string_table.h"
+
+namespace blink {
+
+class V8ElementTest : public BindingTestSupportingGC {
+ protected:
+  void SetUp() override {
+    // Precondition: test strings should not be in the AtomicStringTable yet.
+    DCHECK(AtomicStringTable::Instance().WeakFind("test-attribute").IsNull());
+    DCHECK(AtomicStringTable::Instance().WeakFind("test-value").IsNull());
+  }
+
+  void TearDown() override {
+    PreciselyCollectGarbage();
+
+    // Postcondition: test strings should have been released from the
+    // AtomicStringTable
+    DCHECK(AtomicStringTable::Instance().WeakFind("test-attribute").IsNull());
+    DCHECK(AtomicStringTable::Instance().WeakFind("test-value").IsNull());
+  }
+};
+
+v8::Local<v8::Value> Eval(const String& source, V8TestingScope& scope) {
+  return ClassicScript::CreateUnspecifiedScript(ScriptSourceCode(source))
+      ->RunScriptAndReturnValue(&scope.GetWindow());
+}
+
+TEST_F(V8ElementTest, SetAttributeOperationCallback) {
+  V8TestingScope scope;
+
+  Eval("document.body.setAttribute('test-attribute', 'test-value')", scope);
+  EXPECT_FALSE(
+      AtomicStringTable::Instance().WeakFind("test-attribute").IsNull());
+  EXPECT_FALSE(AtomicStringTable::Instance().WeakFind("test-value").IsNull());
+
+#if DCHECK_IS_ON()
+  AtomicString test_attribute("test-attribute");
+  EXPECT_EQ(test_attribute.Impl()->RefCountChangeCountForTesting(), 27u);
+  AtomicString test_value("test-value");
+  EXPECT_EQ(test_value.Impl()->RefCountChangeCountForTesting(), 15u);
+#endif
+
+  // Trigger a low memory notification. This will signal V8 to clear its
+  // CompilationCache, which is needed because cached compiled code may be
+  // holding references to externalized AtomicStrings.
+  scope.GetIsolate()->LowMemoryNotification();
+}
+
+TEST_F(V8ElementTest, GetAttributeOperationCallback_NonExisting) {
+  V8TestingScope scope;
+
+  Eval("document.body.getAttribute('test-attribute')", scope);
+  EXPECT_FALSE(
+      AtomicStringTable::Instance().WeakFind("test-attribute").IsNull());
+  EXPECT_TRUE(AtomicStringTable::Instance().WeakFind("test-value").IsNull());
+
+#if DCHECK_IS_ON()
+  AtomicString test_attribute("test-attribute");
+  EXPECT_EQ(test_attribute.Impl()->RefCountChangeCountForTesting(), 14u);
+#endif
+
+  // Trigger a low memory notification. This will signal V8 to clear its
+  // CompilationCache, which is needed because cached compiled code may be
+  // holding references to externalized AtomicStrings.
+  scope.GetIsolate()->LowMemoryNotification();
+}
+
+TEST_F(V8ElementTest, GetAttributeOperationCallback_Existing) {
+  V8TestingScope scope;
+
+  Eval("document.body.setAttribute('test-attribute', 'test-value')", scope);
+  EXPECT_FALSE(
+      AtomicStringTable::Instance().WeakFind("test-attribute").IsNull());
+  EXPECT_FALSE(AtomicStringTable::Instance().WeakFind("test-value").IsNull());
+
+  AtomicString test_attribute("test-attribute");
+  test_attribute.Impl()->ResetRefCountChangeCountForTesting();
+  AtomicString test_value("test-value");
+  test_value.Impl()->ResetRefCountChangeCountForTesting();
+
+  Eval("document.body.getAttribute('test-attribute')", scope);
+
+#if DCHECK_IS_ON()
+  EXPECT_EQ(test_attribute.Impl()->RefCountChangeCountForTesting(), 9u);
+  EXPECT_EQ(test_value.Impl()->RefCountChangeCountForTesting(), 6u);
+#endif
+
+  // Trigger a low memory notification. This will signal V8 to clear its
+  // CompilationCache, which is needed because cached compiled code may be
+  // holding references to externalized AtomicStrings.
+  scope.GetIsolate()->LowMemoryNotification();
+}
+
+}  // namespace blink

third_party/blink/renderer/platform/wtf/text/string_impl.cc

@@ -53,6 +53,7 @@ namespace {
 struct SameSizeAsStringImpl {
 #if DCHECK_IS_ON()
   ThreadRestrictionVerifier verifier;
+  unsigned int ref_count_change_count;
 #endif
   int fields[3];
 };

third_party/blink/renderer/platform/wtf/text/string_impl.h

@@ -272,14 +272,19 @@ class WTF_EXPORT StringImpl {
 #if DCHECK_IS_ON()
     DCHECK(IsStatic() || verifier_.OnRef(ref_count_)) << AsciiForDebugging();
 #endif
-    if (!IsStatic())
+    if (!IsStatic()) {
       ref_count_ = base::CheckAdd(ref_count_, 1).ValueOrDie();
+#if DCHECK_IS_ON()
+      ref_count_change_count_++;
+#endif
+    }
   }
 
   ALWAYS_INLINE void Release() const {
 #if DCHECK_IS_ON()
     DCHECK(IsStatic() || verifier_.OnDeref(ref_count_))
         << AsciiForDebugging() << " " << CurrentThread();
+    ref_count_change_count_++;
 #endif
 
     if (!IsStatic()) {
@@ -289,6 +294,7 @@ class WTF_EXPORT StringImpl {
       // enough to catch implementation bugs, and that implementation bugs are
       // the only way we'd experience underflow.
       ref_count_ = base::CheckSub(ref_count_, 1).ValueOrDie();
+      ref_count_change_count_++;
 #else
       --ref_count_;
 #endif
@@ -297,6 +303,13 @@ class WTF_EXPORT StringImpl {
       DestroyIfNotStatic();
   }
 
+#if DCHECK_IS_ON()
+  unsigned int RefCountChangeCountForTesting() const {
+    return ref_count_change_count_;
+  }
+  void ResetRefCountChangeCountForTesting() { ref_count_change_count_ = 0; }
+#endif
+
   ALWAYS_INLINE void Adopted() const {}
 
   // FIXME: Does this really belong in StringImpl?
@@ -568,6 +581,7 @@ class WTF_EXPORT StringImpl {
 
 #if DCHECK_IS_ON()
   mutable ThreadRestrictionVerifier verifier_;
+  mutable unsigned int ref_count_change_count_{0};
 #endif
   mutable unsigned ref_count_{1};
   const unsigned length_;