Ensure web test C++ code Reset()s v8::Persistents before destroying

v8::Persistent will leak on destruction by default, unless marked
as copyable. As such, we must call Reset() before destroying any
v8::Persistent with the default, non-copyable, traits.

Then we can remove the JS cleanup code added to the spellcheck tests.

R=avi@chromium.org

Bug: 866140, 1069111
Change-Id: I3da663bda02c8916c1a4ae5d21234c54311a1dab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2161652
Auto-Submit: danakj <danakj@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#762025}

content/shell/renderer/web_test/blink_test_runner.cc

@@ -311,8 +311,7 @@ void BlinkTestRunner::OnWebTestRuntimeFlagsChanged(
   // Ignore changes that happen before we got the initial, accumulated
   // web flag changes in either OnReplicateTestConfiguration or
   // OnSetTestConfiguration.
-  TestInterfaces* interfaces =
-      WebTestRenderThreadObserver::GetInstance()->test_interfaces();
+  TestInterfaces* interfaces = web_view_test_proxy_->test_interfaces();
   if (!interfaces->TestIsRunning())
     return;
 
@@ -320,8 +319,7 @@ void BlinkTestRunner::OnWebTestRuntimeFlagsChanged(
 }
 
 void BlinkTestRunner::TestFinished() {
-  TestInterfaces* interfaces =
-      WebTestRenderThreadObserver::GetInstance()->test_interfaces();
+  TestInterfaces* interfaces = web_view_test_proxy_->test_interfaces();
   TestRunner* test_runner = interfaces->GetTestRunner();
 
   // We might get multiple TestFinished calls, ensure to only process the dump
@@ -395,16 +393,14 @@ void BlinkTestRunner::TestFinished() {
 
 void BlinkTestRunner::CaptureLocalAudioDump() {
   TRACE_EVENT0("shell", "BlinkTestRunner::CaptureLocalAudioDump");
-  TestInterfaces* interfaces =
-      WebTestRenderThreadObserver::GetInstance()->test_interfaces();
+  TestInterfaces* interfaces = web_view_test_proxy_->test_interfaces();
   dump_result_->audio.emplace();
   interfaces->GetTestRunner()->GetAudioData(&*dump_result_->audio);
 }
 
 void BlinkTestRunner::CaptureLocalLayoutDump() {
   TRACE_EVENT0("shell", "BlinkTestRunner::CaptureLocalLayoutDump");
-  TestInterfaces* interfaces =
-      WebTestRenderThreadObserver::GetInstance()->test_interfaces();
+  TestInterfaces* interfaces = web_view_test_proxy_->test_interfaces();
   TestRunner* test_runner = interfaces->GetTestRunner();
   std::string layout;
   if (test_runner->HasCustomTextDump(&layout)) {
@@ -429,8 +425,7 @@ void BlinkTestRunner::CaptureLocalPixelsDump() {
 
   waiting_for_pixels_dump_result_ = true;
 
-  TestInterfaces* interfaces =
-      WebTestRenderThreadObserver::GetInstance()->test_interfaces();
+  TestInterfaces* interfaces = web_view_test_proxy_->test_interfaces();
   interfaces->GetTestRunner()->DumpPixelsAsync(
       web_view_test_proxy_,
       base::BindOnce(&BlinkTestRunner::OnPixelsDumpCompleted,
@@ -650,8 +645,7 @@ void BlinkTestRunner::HandleWebTestClientDisconnected() {
 void BlinkTestRunner::OnSetupRendererProcessForNonTestWindow() {
   DCHECK(!is_main_window_);
 
-  TestInterfaces* interfaces =
-      WebTestRenderThreadObserver::GetInstance()->test_interfaces();
+  TestInterfaces* interfaces = web_view_test_proxy_->test_interfaces();
   // Allows the window to receive replicated WebTestRuntimeFlags and to
   // control or end the test.
   interfaces->SetTestIsRunning(true);
@@ -670,8 +664,7 @@ void BlinkTestRunner::OnSetupRendererProcessForNonTestWindow() {
 
 void BlinkTestRunner::ApplyTestConfiguration(
     mojom::ShellTestConfigurationPtr params) {
-  TestInterfaces* interfaces =
-      WebTestRenderThreadObserver::GetInstance()->test_interfaces();
+  TestInterfaces* interfaces = web_view_test_proxy_->test_interfaces();
 
   test_config_ = params.Clone();
 
@@ -701,8 +694,7 @@ void BlinkTestRunner::OnSetTestConfiguration(
                         window_size.width(), window_size.height());
   widget->SetWindowRectSynchronouslyForTesting(window_rect);
 
-  TestInterfaces* interfaces =
-      WebTestRenderThreadObserver::GetInstance()->test_interfaces();
+  TestInterfaces* interfaces = web_view_test_proxy_->test_interfaces();
   TestRunner* test_runner = interfaces->GetTestRunner();
   test_runner->SetFocus(web_view_test_proxy_->GetWebView(), true);
 }
@@ -712,7 +704,8 @@ void BlinkTestRunner::OnReset() {
   DCHECK(web_view_test_proxy_->GetMainRenderFrame());
 
   prefs_.Reset();
-  WebTestRenderThreadObserver::GetInstance()->test_interfaces()->ResetAll();
+  TestInterfaces* interfaces = web_view_test_proxy_->test_interfaces();
+  interfaces->ResetAll();
 
   // Navigating to about:blank will make sure that no new loads are initiated
   // by the renderer.
@@ -732,8 +725,7 @@ void BlinkTestRunner::OnTestFinishedInSecondaryRenderer() {
 
   // Avoid a situation where TestFinished is called twice, because
   // of a racey test finish in 2 secondary renderers.
-  TestInterfaces* interfaces =
-      WebTestRenderThreadObserver::GetInstance()->test_interfaces();
+  TestInterfaces* interfaces = web_view_test_proxy_->test_interfaces();
   if (!interfaces->TestIsRunning())
     return;
 

content/shell/test_runner/accessibility_controller.cc

@@ -149,7 +149,11 @@ AccessibilityController::AccessibilityController(
     : log_accessibility_events_(false),
       web_view_test_proxy_(web_view_test_proxy) {}
 
-AccessibilityController::~AccessibilityController() {}
+AccessibilityController::~AccessibilityController() {
+  // v8::Persistent will leak on destroy, due to the default
+  // NonCopyablePersistentTraits (it claims this may change in the future).
+  notification_callback_.Reset();
+}
 
 void AccessibilityController::Reset() {
   elements_.Clear();

content/shell/test_runner/spell_check_client.cc

@@ -22,7 +22,11 @@ namespace content {
 SpellCheckClient::SpellCheckClient(blink::WebLocalFrame* frame)
     : frame_(frame) {}
 
-SpellCheckClient::~SpellCheckClient() = default;
+SpellCheckClient::~SpellCheckClient() {
+  // v8::Persistent will leak on destroy, due to the default
+  // NonCopyablePersistentTraits (it claims this may change in the future).
+  resolved_callback_.Reset();
+}
 
 void SpellCheckClient::SetEnabled(bool enabled) {
   enabled_ = enabled;

content/shell/test_runner/test_interfaces.cc

@@ -49,16 +49,12 @@ void TestInterfaces::Install(blink::WebLocalFrame* frame) {
   gamepad_controller_->Install(frame);
 }
 
-void TestInterfaces::ResetTestHelperControllers() {
+void TestInterfaces::ResetAll() {
   gamepad_controller_->Reset();
   blink::WebCache::Clear();
 
   for (WebViewTestProxy* web_view_test_proxy : window_list_)
     web_view_test_proxy->Reset();
-}
-
-void TestInterfaces::ResetAll() {
-  ResetTestHelperControllers();
   test_runner_->Reset();
 }
 

content/shell/test_runner/test_interfaces.h

@@ -30,7 +30,6 @@ class TestInterfaces {
 
   void SetMainView(blink::WebView* web_view);
   void Install(blink::WebLocalFrame* frame);
-  void ResetTestHelperControllers();
   void ResetAll();
   bool TestIsRunning();
   void SetTestIsRunning(bool running);

content/shell/test_runner/test_runner.cc

@@ -200,7 +200,6 @@ class TestRunnerBindings : public gin::Wrappable<TestRunnerBindings> {
   void QueueReload();
   void RemoveSpellCheckResolvedCallback();
   void RemoveWebPageOverlay();
-  void ResetTestHelperControllers();
   void ResolveBeforeInstallPromptPromise(const std::string& platform);
   void RunIdleTasks(v8::Local<v8::Function> callback);
   void SendBluetoothManualChooserEvent(const std::string& event,
@@ -507,8 +506,6 @@ gin::ObjectTemplateBuilder TestRunnerBindings::GetObjectTemplateBuilder(
                  &TestRunnerBindings::RemoveSpellCheckResolvedCallback)
       .SetMethod("removeWebPageOverlay",
                  &TestRunnerBindings::RemoveWebPageOverlay)
-      .SetMethod("resetTestHelperControllers",
-                 &TestRunnerBindings::ResetTestHelperControllers)
       .SetMethod("resolveBeforeInstallPromptPromise",
                  &TestRunnerBindings::ResolveBeforeInstallPromptPromise)
       .SetMethod("runIdleTasks", &TestRunnerBindings::RunIdleTasks)
@@ -706,11 +703,6 @@ void TestRunnerBindings::SetCloseRemainingWindowsWhenComplete(
     runner_->SetCloseRemainingWindowsWhenComplete(close_remaining_windows);
 }
 
-void TestRunnerBindings::ResetTestHelperControllers() {
-  if (runner_)
-    runner_->ResetTestHelperControllers();
-}
-
 void TestRunnerBindings::SetTabKeyCyclesThroughElements(
     bool tab_key_cycles_through_elements) {
   view_runner_->SetTabKeyCyclesThroughElements(tab_key_cycles_through_elements);
@@ -2043,10 +2035,6 @@ void TestRunner::SetCloseRemainingWindowsWhenComplete(
   close_remaining_windows_ = close_remaining_windows;
 }
 
-void TestRunner::ResetTestHelperControllers() {
-  test_interfaces_->ResetTestHelperControllers();
-}
-
 void TestRunner::AddOriginAccessAllowListEntry(
     const std::string& source_origin,
     const std::string& destination_protocol,

content/shell/test_runner/test_runner.h

@@ -295,7 +295,6 @@ class TestRunner {
   // Functions for dealing with windows. By default we block all new windows.
   int WindowCount();
   void SetCloseRemainingWindowsWhenComplete(bool close_remaining_windows);
-  void ResetTestHelperControllers();
 
   // Allows web tests to manage origins' allow list.
   void AddOriginAccessAllowListEntry(const std::string& source_origin,

content/shell/test_runner/web_ax_object_proxy.h

@@ -13,7 +13,6 @@
 #include "gin/object_template_builder.h"
 #include "gin/wrappable.h"
 #include "third_party/blink/public/web/web_ax_object.h"
-#include "v8/include/v8-util.h"
 #include "v8/include/v8.h"
 
 namespace blink {
@@ -253,8 +252,13 @@ class WebAXObjectProxyList : public WebAXObjectProxy::Factory {
   v8::Local<v8::Object> GetOrCreate(const blink::WebAXObject&) override;
 
  private:
-  typedef v8::PersistentValueVector<v8::Object> ElementList;
-  ElementList elements_;
+  // Defines the Persistents as copyable because v8 does not support moving
+  // in non-copyable (default) traits either.
+  using CopyablePersistentObject =
+      v8::Persistent<v8::Object, v8::CopyablePersistentTraits<v8::Object>>;
+  // Because the v8::Persistent in this container uses CopyablePersistentObject
+  // traits, it will not leak on destruction.
+  std::vector<CopyablePersistentObject> elements_;
 };
 
 }  // namespace content

content/shell/test_runner/web_frame_test_client.cc

@@ -145,7 +145,10 @@ WebFrameTestClient::WebFrameTestClient(WebViewTestProxy* web_view_test_proxy,
 WebFrameTestClient::~WebFrameTestClient() = default;
 
 void WebFrameTestClient::Reset() {
-  spell_check_->Reset();
+  // If this frame failed to navigate then it won't have set up the
+  // SpellCheckClient in DidClearWindowObject().
+  if (spell_check_)
+    spell_check_->Reset();
 }
 
 // static

content/shell/test_runner/web_frame_test_proxy.cc

@@ -153,13 +153,13 @@ void WebFrameTestProxy::Reset() {
     // Resetting the internals object also overrides the WebPreferences, so we
     // have to sync them to WebKit again.
     render_view()->SetWebkitPreferences(render_view()->GetWebkitPreferences());
-
-    test_client_->Reset();
   }
   if (IsLocalRoot()) {
     GetLocalRootWebWidgetTestProxy()->Reset();
     GetLocalRootWebWidgetTestProxy()->EndSyntheticGestures();
   }
+
+  test_client_->Reset();
 }
 
 std::string WebFrameTestProxy::GetFrameNameForWebTests() {

third_party/blink/web_tests/editing/assert_selection.js

@@ -767,14 +767,10 @@ class Sample {
   /**
    * @public
    * Enables or disables the test runner's spell checker.
-   * When enabled, this must later be disabled before ending the test to prevent
-   * leak detection due to the resolved callback keeping the |iframe_| alive.
    */
   setMockSpellCheckerEnabled(enabled) {
     this.iframe_.contentWindow.eval(
       "testRunner.setMockSpellCheckerEnabled(" + enabled + ");");
-    if (!enabled)
-      this.setSpellCheckResolvedCallback(null);
   }
 
   /**
@@ -796,11 +792,6 @@ class Sample {
            window.parent.postMessage('resolved_spellcheck', '*'); \
          });");
     } else if (remove) {
-      // Drops the stored closure's reference to the iframe allowing it to be
-      // destroyed before the web test harness is reset for the next test.
-      // This is important for leak detection at the end of this test.
-      this.iframe_.contentWindow.eval(
-        "testRunner.removeSpellCheckResolvedCallback();");
       window.removeEventListener("message", this.listener_, false);
       this.listener_ = null;
     }

third_party/blink/web_tests/editing/spelling/spellcheck_test.js

@@ -355,7 +355,6 @@ function invokeSpellcheckTest(testObject, input, tester, expectedText) {
           grammar: '~'});
 
         assert_equals(serializer.serialize(sample.document), expectedText);
-        testObject.sample.setMockSpellCheckerEnabled(false);
         testObject.done();
       });
     };