Remove TestRunner reliance on the |main_view_| during test start and end

During startup, we want to disable the V8 cache if we're loading a
devtools test. That can be done directly from BlinkTestRunner where
the main frame is being loaded.

During test completion, we want to check the main frame's mime type
for text/plain, in order to change our output to text without a pixel
dump. This can be done directly against the main frame, since
BlinkTestRunner::TestFinished() runs against that main test window's
main frame.

This removes 2 more places causing problematic tracking of the "main
view" in the renderer, when it may change over the course of a test
due to frame swaps on cross-site navigations, and in the future on
same-site navigations as well.

R=avi@chromium.org

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

content/shell/renderer/web_test/blink_test_runner.cc

@@ -20,6 +20,7 @@
 #include "third_party/blink/public/platform/web_url_request.h"
 #include "third_party/blink/public/web/web_frame_widget.h"
 #include "third_party/blink/public/web/web_local_frame.h"
+#include "third_party/blink/public/web/web_settings.h"
 #include "third_party/blink/public/web/web_view.h"
 
 namespace content {
@@ -45,6 +46,9 @@ blink::WebString BlinkTestRunner::GetAbsoluteWebStringFromUTF8Path(
 void BlinkTestRunner::TestFinished() {
   TestRunner* test_runner = web_view_test_proxy_->GetTestRunner();
 
+  DCHECK(is_main_window_);
+  DCHECK(web_view_test_proxy_->GetMainRenderFrame());
+
   // Avoid a situation where TestFinished is called twice, because
   // of a racey test where renderers both call notifyDone(), or a test that
   // calls notifyDone() more than once.
@@ -52,13 +56,6 @@ void BlinkTestRunner::TestFinished() {
     return;
   test_runner->SetTestIsRunning(false);
 
-  // If we're not in the main frame, then ask the browser to redirect the call
-  // to the main frame instead.
-  if (!is_main_window_ || !web_view_test_proxy_->GetMainRenderFrame()) {
-    GetWebTestControlHostRemote()->TestFinishedInSecondaryRenderer();
-    return;
-  }
-
   // Now we know that we're in the main frame, we should generate dump results.
   // Clean out the lifecycle if needed before capturing the web tree
   // dump and pixels from the compositor.
@@ -77,14 +74,33 @@ void BlinkTestRunner::TestFinished() {
   if (test_runner->ShouldDumpAsAudio()) {
     dump_result->audio = CaptureLocalAudioDump();
   } else {
+    TextResultType text_result_type = test_runner->ShouldGenerateTextResults();
+    bool pixel_result = test_runner->ShouldGeneratePixelResults();
+
+    std::string spec = GURL(test_config_->test_url).spec();
+    size_t path_start = spec.rfind("web_tests/");
+    if (path_start != std::string::npos)
+      spec = spec.substr(path_start);
+
+    std::string mime_type =
+        web_frame->GetDocumentLoader()->GetResponse().MimeType().Utf8();
+
+    // In a text/plain document, and in a dumpAsText/ subdirectory, we generate
+    // text results no matter what the test may previously have requested.
+    if (mime_type == "text/plain" ||
+        spec.find("/dumpAsText/") != std::string::npos) {
+      text_result_type = TextResultType::kText;
+      pixel_result = false;
+    }
+
     // If possible we grab the layout dump locally because a round trip through
     // the browser would give javascript a chance to run and change the layout.
     // We only go to the browser if we can not do it locally, because we want to
     // dump more than just the local main frame. Those tests must be written to
     // not modify layout after signalling the test is finished.
-    dump_result->layout = CaptureLocalLayoutDump();
+    dump_result->layout = CaptureLocalLayoutDump(text_result_type);
 
-    if (test_runner->ShouldGeneratePixelResults()) {
+    if (pixel_result) {
       if (test_runner->CanDumpPixelsFromRenderer()) {
         SkBitmap actual = CaptureLocalPixelsDump();
 
@@ -115,17 +131,28 @@ std::vector<uint8_t> BlinkTestRunner::CaptureLocalAudioDump() {
   return test_runner->GetAudioData();
 }
 
-base::Optional<std::string> BlinkTestRunner::CaptureLocalLayoutDump() {
+base::Optional<std::string> BlinkTestRunner::CaptureLocalLayoutDump(
+    TextResultType type) {
   TRACE_EVENT0("shell", "BlinkTestRunner::CaptureLocalLayoutDump");
   TestRunner* test_runner = web_view_test_proxy_->GetTestRunner();
-  std::string layout;
-  if (test_runner->HasCustomTextDump(&layout)) {
-    return layout + "\n";
-  } else if (!test_runner->IsRecursiveLayoutDumpRequested()) {
-    return test_runner->DumpLayout(
-        web_view_test_proxy_->GetMainRenderFrame()->GetWebFrame());
+
+  {
+    // The CustomTextDump always takes precedence, and is also only available
+    // for a local dump of the main frame.
+    std::string layout;
+    if (test_runner->HasCustomTextDump(&layout))
+      return layout + "\n";
+  }
+
+  // If doing a recursive dump, it's done asynchronously from the browser.
+  if (test_runner->IsRecursiveLayoutDumpRequested()) {
+    return base::nullopt;
   }
-  return base::nullopt;
+
+  // Otherwise, in the common case, we do a synchronous text dump of the main
+  // frame here.
+  RenderFrame* main_frame = web_view_test_proxy_->GetMainRenderFrame();
+  return DumpLayoutAsString(main_frame->GetWebFrame(), type);
 }
 
 SkBitmap BlinkTestRunner::CaptureLocalPixelsDump() {
@@ -207,7 +234,7 @@ void BlinkTestRunner::ApplyTestConfiguration(
   test_runner->SetMainView(web_view_test_proxy_->GetWebView());
   test_runner->SetTestIsRunning(true);
 
-  std::string spec = GURL(params->test_url).spec();
+  std::string spec = GURL(test_config_->test_url).spec();
   size_t path_start = spec.rfind("web_tests/");
   if (path_start != std::string::npos)
     spec = spec.substr(path_start);
@@ -231,17 +258,16 @@ void BlinkTestRunner::ApplyTestConfiguration(
   // For http/tests/loading/, which is served via httpd and becomes /loading/.
   if (spec.find("/loading/") != std::string::npos)
     test_runner->SetShouldDumpFrameLoadCallbacks(true);
-  if (spec.find("/dumpAsText/") != std::string::npos) {
-    test_runner->SetShouldDumpAsText(true);
-    test_runner->SetShouldGeneratePixelResults(false);
-  }
-  test_runner->SetV8CacheDisabled(is_devtools_test);
 
   if (spec.find("/external/wpt/") != std::string::npos ||
       spec.find("/external/csswg-test/") != std::string::npos ||
       spec.find("://web-platform.test") != std::string::npos ||
       spec.find("/harness-tests/wpt/") != std::string::npos)
     test_runner->SetIsWebPlatformTestsMode();
+
+  web_view_test_proxy_->GetWebView()->GetSettings()->SetV8CacheOptions(
+      is_devtools_test ? blink::WebSettings::V8CacheOptions::kNone
+                       : blink::WebSettings::V8CacheOptions::kDefault);
 }
 
 void BlinkTestRunner::OnReplicateTestConfiguration(
@@ -285,14 +311,6 @@ void BlinkTestRunner::OnResetRendererAfterWebTest() {
 }
 
 void BlinkTestRunner::OnFinishTestInMainWindow() {
-  DCHECK(is_main_window_ && web_view_test_proxy_->GetMainRenderFrame());
-
-  // Avoid a situation where TestFinished is called twice, because
-  // of a racey test finish in 2 secondary renderers.
-  TestRunner* test_runner = web_view_test_proxy_->GetTestRunner();
-  if (!test_runner->TestIsRunning())
-    return;
-
   TestFinished();
 }
 

content/shell/renderer/web_test/blink_test_runner.h

@@ -18,6 +18,7 @@
 #include "base/strings/string16.h"
 #include "content/public/common/page_state.h"
 #include "content/shell/common/web_test/web_test.mojom.h"
+#include "content/shell/renderer/web_test/layout_dump.h"
 #include "mojo/public/cpp/bindings/associated_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "third_party/blink/public/platform/web_string.h"
@@ -82,7 +83,7 @@ class BlinkTestRunner {
   std::vector<uint8_t> CaptureLocalAudioDump();
   // Returns a string if able to capture the dump locally. If not, then the
   // browser must do the capture.
-  base::Optional<std::string> CaptureLocalLayoutDump();
+  base::Optional<std::string> CaptureLocalLayoutDump(TextResultType type);
   // Grabs the pixel results. This is only called when pixel results are being
   // captured in the renderer (aka CanDumpPixelsFromRenderer() is true), such as
   // to grab the current image being dragged by the mouse.

content/shell/renderer/web_test/layout_dump.cc

@@ -56,31 +56,37 @@ std::string DumpFrameScrollPosition(WebLocalFrame* frame) {
 
 }  // namespace
 
-std::string DumpLayoutAsString(WebLocalFrame* frame,
-                               const WebTestRuntimeFlags& flags) {
+std::string DumpLayoutAsString(WebLocalFrame* frame, TextResultType type) {
   DCHECK(frame);
   std::string result;
 
-  if (flags.dump_as_text()) {
-    result = DumpFrameHeaderIfNeeded(frame);
-    result += frame->GetDocument().ContentAsTextForTesting().Utf8();
-    result += "\n";
-  } else if (flags.dump_as_markup()) {
-    DCHECK(!flags.is_printing());
-    result = DumpFrameHeaderIfNeeded(frame);
-    result += WebFrameContentDumper::DumpAsMarkup(frame).Utf8();
-    result += "\n";
-  } else if (flags.dump_as_layout()) {
-    if (frame->Parent() == nullptr) {
-      WebFrameContentDumper::LayoutAsTextControls layout_text_behavior =
-          WebFrameContentDumper::kLayoutAsTextNormal;
-      if (flags.is_printing())
-        layout_text_behavior |= WebFrameContentDumper::kLayoutAsTextPrinting;
-      result = WebFrameContentDumper::DumpLayoutTreeAsText(frame,
-                                                           layout_text_behavior)
-                   .Utf8();
-    }
-    result += DumpFrameScrollPosition(frame);
+  switch (type) {
+    case TextResultType::kEmpty:
+      break;
+    case TextResultType::kText:
+      result += DumpFrameHeaderIfNeeded(frame);
+      result += frame->GetDocument().ContentAsTextForTesting().Utf8();
+      result += "\n";
+      break;
+    case TextResultType::kMarkup:
+      result += DumpFrameHeaderIfNeeded(frame);
+      result += WebFrameContentDumper::DumpAsMarkup(frame).Utf8();
+      result += "\n";
+      break;
+    case TextResultType::kLayout:
+    case TextResultType::kLayoutAsPrinting:
+      if (!frame->Parent()) {
+        WebFrameContentDumper::LayoutAsTextControls layout_text_behavior =
+            WebFrameContentDumper::kLayoutAsTextNormal;
+        if (type == TextResultType::kLayoutAsPrinting)
+          layout_text_behavior |= WebFrameContentDumper::kLayoutAsTextPrinting;
+
+        result += WebFrameContentDumper::DumpLayoutTreeAsText(
+                      frame, layout_text_behavior)
+                      .Utf8();
+      }
+      result += DumpFrameScrollPosition(frame);
+      break;
   }
 
   return result;

content/shell/renderer/web_test/layout_dump.h

@@ -7,18 +7,24 @@
 
 #include <string>
 
-#include "content/shell/renderer/web_test/web_test_runtime_flags.h"
-
 namespace blink {
 class WebLocalFrame;
 }  // namespace blink
 
 namespace content {
 
+enum class TextResultType {
+  kEmpty,
+  kText,
+  kMarkup,
+  kLayout,
+  kLayoutAsPrinting,
+};
+
 // Dumps textual representation of |frame| contents.  Exact dump mode depends
 // on |flags| (i.e. dump_as_text VS dump_as_markup and/or is_printing).
 std::string DumpLayoutAsString(blink::WebLocalFrame* frame,
-                               const WebTestRuntimeFlags& flags);
+                               TextResultType type);
 
 }  // namespace content
 

content/shell/renderer/web_test/test_runner.cc

@@ -30,7 +30,6 @@
 #include "content/shell/renderer/web_test/app_banner_service.h"
 #include "content/shell/renderer/web_test/blink_test_helpers.h"
 #include "content/shell/renderer/web_test/blink_test_runner.h"
-#include "content/shell/renderer/web_test/layout_dump.h"
 #include "content/shell/renderer/web_test/mock_web_document_subresource_filter.h"
 #include "content/shell/renderer/web_test/pixel_dump.h"
 #include "content/shell/renderer/web_test/spell_check_client.h"
@@ -70,7 +69,6 @@
 #include "third_party/blink/public/web/web_script_source.h"
 #include "third_party/blink/public/web/web_security_policy.h"
 #include "third_party/blink/public/web/web_serialized_script_value.h"
-#include "third_party/blink/public/web/web_settings.h"
 #include "third_party/blink/public/web/web_testing_support.h"
 #include "third_party/blink/public/web/web_view.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -2223,8 +2221,6 @@ void TestRunner::Install(WebFrameTestProxy* frame,
 
 void TestRunner::SetMainView(blink::WebView* web_view) {
   main_view_ = web_view;
-  if (disable_v8_cache_)
-    SetV8CacheDisabled(true);
 }
 
 void TestRunner::Reset(WebFrameTestProxy* main_frame) {
@@ -2302,11 +2298,6 @@ bool TestRunner::ShouldDumpEditingCallbacks() const {
   return web_test_runtime_flags_.dump_editting_callbacks();
 }
 
-void TestRunner::SetShouldDumpAsText(bool value) {
-  web_test_runtime_flags_.set_dump_as_text(value);
-  OnWebTestRuntimeFlagsChanged();
-}
-
 void TestRunner::SetShouldDumpAsLayout(bool value) {
   web_test_runtime_flags_.set_dump_as_layout(value);
   OnWebTestRuntimeFlagsChanged();
@@ -2327,10 +2318,23 @@ void TestRunner::SetCustomTextOutput(const std::string& text) {
 }
 
 bool TestRunner::ShouldGeneratePixelResults() {
-  CheckResponseMimeType();
   return web_test_runtime_flags_.generate_pixel_results();
 }
 
+TextResultType TestRunner::ShouldGenerateTextResults() {
+  if (web_test_runtime_flags_.dump_as_text()) {
+    return TextResultType::kText;
+  } else if (web_test_runtime_flags_.dump_as_markup()) {
+    DCHECK(!web_test_runtime_flags_.is_printing());
+    return TextResultType::kMarkup;
+  } else if (web_test_runtime_flags_.dump_as_layout()) {
+    if (web_test_runtime_flags_.is_printing())
+      return TextResultType::kLayoutAsPrinting;
+    return TextResultType::kLayout;
+  }
+  return TextResultType::kEmpty;
+}
+
 bool TestRunner::ShouldStayOnPageAfterHandlingBeforeUnload() const {
   return web_test_runtime_flags_.stay_on_page_after_handling_before_unload();
 }
@@ -2349,15 +2353,9 @@ const std::vector<uint8_t>& TestRunner::GetAudioData() const {
 }
 
 bool TestRunner::IsRecursiveLayoutDumpRequested() {
-  CheckResponseMimeType();
   return web_test_runtime_flags_.dump_child_frames();
 }
 
-std::string TestRunner::DumpLayout(blink::WebLocalFrame* frame) {
-  CheckResponseMimeType();
-  return DumpLayoutAsString(frame, web_test_runtime_flags_);
-}
-
 bool TestRunner::CanDumpPixelsFromRenderer() const {
   return web_test_runtime_flags_.dump_drag_image() ||
          web_test_runtime_flags_.is_printing();
@@ -2481,7 +2479,6 @@ void TestRunner::AddLoadingFrame(blink::WebFrame* frame) {
     OnWebTestRuntimeFlagsChanged();
   }
 
-  LOG(ERROR) << "AddLoadingFrame " << frame;
   loading_frames_.push_back(frame);
   frame_will_start_load_ = false;
 }
@@ -2599,16 +2596,6 @@ bool TestRunner::ShouldDumpNavigationPolicy() const {
   return web_test_runtime_flags_.dump_navigation_policy();
 }
 
-void TestRunner::SetV8CacheDisabled(bool disabled) {
-  if (!main_view_) {
-    disable_v8_cache_ = disabled;
-    return;
-  }
-  main_view_->GetSettings()->SetV8CacheOptions(
-      disabled ? blink::WebSettings::V8CacheOptions::kNone
-               : blink::WebSettings::V8CacheOptions::kDefault);
-}
-
 class WorkItemBackForward : public TestRunner::WorkItem {
  public:
   explicit WorkItemBackForward(int distance) : distance_(distance) {}
@@ -3138,33 +3125,6 @@ void TestRunner::OnWebTestRuntimeFlagsChanged() {
   web_test_runtime_flags_.tracked_dictionary().ResetChangeTracking();
 }
 
-void TestRunner::CheckResponseMimeType() {
-  // Text output: the test page can request different types of output which we
-  // handle here.
-
-  if (web_test_runtime_flags_.dump_as_text())
-    return;
-
-  if (!main_view_)
-    return;
-
-  if (!main_view_->MainFrame()->IsWebLocalFrame())
-    return;
-
-  blink::WebDocumentLoader* document_loader =
-      main_view_->MainFrame()->ToWebLocalFrame()->GetDocumentLoader();
-  if (!document_loader)
-    return;
-
-  std::string mimeType = document_loader->GetResponse().MimeType().Utf8();
-  if (mimeType != "text/plain")
-    return;
-
-  web_test_runtime_flags_.set_dump_as_text(true);
-  web_test_runtime_flags_.set_generate_pixel_results(false);
-  OnWebTestRuntimeFlagsChanged();
-}
-
 void TestRunner::FinishTest() {
   WebFrameTestProxy* main_frame = FindInProcessMainWindowMainFrame();
 

content/shell/renderer/web_test/test_runner.h

@@ -23,6 +23,7 @@
 #include "content/shell/common/web_test/web_test.mojom.h"
 #include "content/shell/common/web_test/web_test_bluetooth_fake_adapter_setter.mojom.h"
 #include "content/shell/renderer/web_test/gamepad_controller.h"
+#include "content/shell/renderer/web_test/layout_dump.h"
 #include "content/shell/renderer/web_test/mock_content_settings_client.h"
 #include "content/shell/renderer/web_test/mock_screen_orientation_client.h"
 #include "content/shell/renderer/web_test/web_test_runtime_flags.h"
@@ -40,7 +41,6 @@ class DictionaryValue;
 namespace blink {
 class WebContentSettingsClient;
 class WebFrame;
-class WebLocalFrame;
 class WebString;
 class WebView;
 }  // namespace blink
@@ -133,10 +133,6 @@ class TestRunner {
   // (i.e. by calling testRunner.dumpChildFramesAsText() from javascript).
   bool IsRecursiveLayoutDumpRequested();
 
-  // Dumps layout of |frame| using the mode requested by the current test
-  // (i.e. text mode if testRunner.dumpAsText() was called from javascript).
-  std::string DumpLayout(blink::WebLocalFrame* frame);
-
   // Returns true if the selection window should be painted onto captured
   // pixels.
   bool ShouldDumpSelectionRect() const;
@@ -167,6 +163,8 @@ class TestRunner {
   // Returns true if pixel results should be generated at the end of the test.
   bool ShouldGeneratePixelResults();
 
+  TextResultType ShouldGenerateTextResults();
+
   // Activate the window holding the given main frame, and set focus on the
   // frame's widget.
   void FocusWindow(RenderFrame* main_frame, bool focus);
@@ -179,8 +177,6 @@ class TestRunner {
   std::string CustomDumpText() const;
   void ShowDevTools(const std::string& settings,
                     const std::string& frontend_url);
-  void SetV8CacheDisabled(bool);
-  void SetShouldDumpAsText(bool);
   void SetShouldDumpAsLayout(bool);
   void SetCustomTextOutput(const std::string& text);
   void SetShouldGeneratePixelResults(bool);
@@ -499,8 +495,6 @@ class TestRunner {
   ///////////////////////////////////////////////////////////////////////////
   // Internal helpers
 
-  void CheckResponseMimeType();
-
   mojo::AssociatedRemote<mojom::WebTestControlHost>&
   GetWebTestControlHostRemote();
   void HandleWebTestControlHostDisconnected();
@@ -585,9 +579,6 @@ class TestRunner {
   blink::WebEffectiveConnectionType effective_connection_type_ =
       blink::WebEffectiveConnectionType::kTypeUnknown;
 
-  // Forces v8 compilation cache to be disabled (used for inspector tests).
-  bool disable_v8_cache_ = false;
-
   base::WeakPtrFactory<TestRunner> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(TestRunner);

content/shell/renderer/web_test/web_frame_test_proxy.cc

@@ -12,6 +12,7 @@
 #include "content/shell/renderer/web_test/blink_test_runner.h"
 #include "content/shell/renderer/web_test/event_sender.h"
 #include "content/shell/renderer/web_test/gc_controller.h"
+#include "content/shell/renderer/web_test/layout_dump.h"
 #include "content/shell/renderer/web_test/spell_check_client.h"
 #include "content/shell/renderer/web_test/test_interfaces.h"
 #include "content/shell/renderer/web_test/test_plugin.h"
@@ -737,7 +738,8 @@ void WebFrameTestProxy::SynchronouslyCompositeAfterTest(
 }
 
 void WebFrameTestProxy::DumpFrameLayout(DumpFrameLayoutCallback callback) {
-  std::string dump = test_runner()->DumpLayout(GetWebFrame());
+  std::string dump = DumpLayoutAsString(
+      GetWebFrame(), test_runner()->ShouldGenerateTextResults());
   std::move(callback).Run(std::move(dump));
 }