Remove SecondaryTestWindowObserver

This drops one dependency from shell/browser/ to shell/browser/web_test
by going directly to WebTestControlHost and having it register a
WebContentsObserver to hear about new RenderFrames.

This still has Shell using WebTestControlHost, but we're narrowing scope
down to just that one class, at which point we can insert a delegate
instead of the concrete type.

We remove the WebTestControlHost::current_pid_ and the notification
code around it, which is was partly tied to RenderFrame creation
notification and was both wholly unnecessary as well as problematic or
incorrect if the main frame moved to a new renderer process on
navigation during a test.

R=avi@chromium.org

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

content/shell/BUILD.gn

@@ -300,8 +300,6 @@ static_library("content_shell_lib") {
     "browser/web_test/mock_client_hints_controller_delegate.h",
     "browser/web_test/mojo_web_test_helper.cc",
     "browser/web_test/mojo_web_test_helper.h",
-    "browser/web_test/secondary_test_window_observer.cc",
-    "browser/web_test/secondary_test_window_observer.h",
     "browser/web_test/test_info_extractor.cc",
     "browser/web_test/test_info_extractor.h",
     "browser/web_test/web_test_background_fetch_delegate.cc",

content/shell/browser/shell.cc

@@ -37,7 +37,6 @@
 #include "content/shell/browser/shell_content_browser_client.h"
 #include "content/shell/browser/shell_devtools_frontend.h"
 #include "content/shell/browser/shell_javascript_dialog_manager.h"
-#include "content/shell/browser/web_test/secondary_test_window_observer.h"
 #include "content/shell/browser/web_test/web_test_bluetooth_chooser_factory.h"
 #include "content/shell/browser/web_test/web_test_control_host.h"
 #include "content/shell/browser/web_test/web_test_javascript_dialog_manager.h"
@@ -151,6 +150,10 @@ Shell* Shell::CreateShell(std::unique_ptr<WebContents> web_contents,
             switches::kForceWebRtcIPHandlingPolicy);
   }
 
+  WebTestControlHost* web_test_control_host = WebTestControlHost::Get();
+  if (web_test_control_host)
+    web_test_control_host->DidOpenNewWindowOrTab(shell->web_contents());
+
   return shell;
 }
 
@@ -222,6 +225,7 @@ Shell* Shell::CreateNewWindow(BrowserContext* browser_context,
   Shell* shell =
       CreateShell(std::move(web_contents), AdjustWindowSize(initial_size),
                   true /* should_set_delegate */);
+
   if (!url.is_empty())
     shell->LoadURL(url);
   return shell;
@@ -297,12 +301,9 @@ void Shell::AddNewContents(WebContents* source,
                            const gfx::Rect& initial_rect,
                            bool user_gesture,
                            bool* was_blocked) {
-  WebContents* raw_new_contents = new_contents.get();
   CreateShell(
       std::move(new_contents), AdjustWindowSize(initial_rect.size()),
       !delay_popup_contents_delegate_for_testing_ /* should_set_delegate */);
-  if (switches::IsRunWebTestsSwitchPresent())
-    SecondaryTestWindowObserver::CreateForWebContents(raw_new_contents);
 }
 
 void Shell::GoBackOrForward(int offset) {
@@ -422,8 +423,6 @@ WebContents* Shell::OpenURLFromTab(WebContents* source,
                                  params.source_site_instance,
                                  gfx::Size());  // Use default size.
       target = new_window->web_contents();
-      if (switches::IsRunWebTestsSwitchPresent())
-        SecondaryTestWindowObserver::CreateForWebContents(target);
       break;
     }
 
@@ -582,8 +581,9 @@ bool Shell::DidAddMessageToConsole(WebContents* source,
 }
 
 void Shell::PortalWebContentsCreated(WebContents* portal_web_contents) {
-  if (switches::IsRunWebTestsSwitchPresent())
-    SecondaryTestWindowObserver::CreateForWebContents(portal_web_contents);
+  WebTestControlHost* web_test_control_host = WebTestControlHost::Get();
+  if (web_test_control_host)
+    web_test_control_host->DidOpenNewWindowOrTab(portal_web_contents);
 }
 
 void Shell::RendererUnresponsive(

content/shell/browser/shell_devtools_manager_delegate.cc

@@ -29,7 +29,6 @@
 #include "content/public/common/url_constants.h"
 #include "content/public/common/user_agent.h"
 #include "content/shell/browser/shell.h"
-#include "content/shell/browser/web_test/secondary_test_window_observer.h"
 #include "content/shell/common/shell_content_client.h"
 #include "content/shell/common/shell_switches.h"
 #include "content/shell/grit/shell_resources.h"
@@ -202,8 +201,6 @@ scoped_refptr<DevToolsAgentHost>
 ShellDevToolsManagerDelegate::CreateNewTarget(const GURL& url) {
   Shell* shell = Shell::CreateNewWindow(browser_context_, url, nullptr,
                                         Shell::GetShellDefaultSize());
-  if (switches::IsRunWebTestsSwitchPresent())
-    SecondaryTestWindowObserver::CreateForWebContents(shell->web_contents());
   return DevToolsAgentHost::GetOrCreateFor(shell->web_contents());
 }
 

content/shell/browser/web_test/secondary_test_window_observer.cc

-// Copyright 2016 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 "content/shell/browser/web_test/secondary_test_window_observer.h"
-
-#include "content/public/browser/render_frame_host.h"
-#include "content/shell/browser/web_test/web_test_control_host.h"
-
-namespace content {
-
-SecondaryTestWindowObserver::SecondaryTestWindowObserver(
-    WebContents* web_contents)
-    : WebContentsObserver(web_contents) {
-  WebTestControlHost* web_test_control_host = WebTestControlHost::Get();
-  if (!web_test_control_host)
-    return;
-  DCHECK(!web_test_control_host->IsMainWindow(web_contents));
-
-  // Ensure that any preexisting frames (likely just the main frame) are handled
-  // as well.
-  for (RenderFrameHost* frame : web_contents->GetAllFrames()) {
-    if (frame->IsRenderFrameLive())
-      web_test_control_host->HandleNewRenderFrameHost(frame);
-  }
-}
-
-SecondaryTestWindowObserver::~SecondaryTestWindowObserver() {}
-
-void SecondaryTestWindowObserver::RenderFrameCreated(
-    RenderFrameHost* render_frame_host) {
-  WebTestControlHost* web_test_control_host = WebTestControlHost::Get();
-  if (!web_test_control_host)
-    return;
-  DCHECK(!web_test_control_host->IsMainWindow(
-      WebContents::FromRenderFrameHost(render_frame_host)));
-  web_test_control_host->HandleNewRenderFrameHost(render_frame_host);
-}
-
-WEB_CONTENTS_USER_DATA_KEY_IMPL(SecondaryTestWindowObserver)
-
-}  // namespace content

content/shell/browser/web_test/secondary_test_window_observer.h

-// Copyright 2016 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.
-
-#ifndef CONTENT_SHELL_BROWSER_WEB_TEST_SECONDARY_TEST_WINDOW_OBSERVER_H_
-#define CONTENT_SHELL_BROWSER_WEB_TEST_SECONDARY_TEST_WINDOW_OBSERVER_H_
-
-#include "base/macros.h"
-#include "content/public/browser/web_contents_observer.h"
-#include "content/public/browser/web_contents_user_data.h"
-
-namespace content {
-
-class SecondaryTestWindowObserver
-    : public WebContentsObserver,
-      public WebContentsUserData<SecondaryTestWindowObserver> {
- public:
-  ~SecondaryTestWindowObserver() override;
-
-  // WebContentsObserver implementation.
-  void RenderFrameCreated(RenderFrameHost* render_frame_host) override;
-
- private:
-  friend class WebContentsUserData<SecondaryTestWindowObserver>;
-  explicit SecondaryTestWindowObserver(WebContents* web_contents);
-
-  WEB_CONTENTS_USER_DATA_KEY_DECL();
-
-  DISALLOW_COPY_AND_ASSIGN(SecondaryTestWindowObserver);
-};
-
-}  // namespace content
-
-#endif  // CONTENT_SHELL_BROWSER_WEB_TEST_SECONDARY_TEST_WINDOW_OBSERVER_H_

content/shell/browser/web_test/web_test_control_host.cc

@@ -426,6 +426,37 @@ void WebTestResultPrinter::CloseStderr() {
   }
 }
 
+// WebTestWindowObserver -----------------------------------------------------
+
+class WebTestControlHost::WebTestWindowObserver : WebContentsObserver {
+ public:
+  WebTestWindowObserver(WebContents* web_contents,
+                        WebTestControlHost* web_test_control)
+      : WebContentsObserver(web_contents), web_test_control_(web_test_control) {
+    // If the WebContents was already set up before given to the Shell, it may
+    // have a set of RenderFrames already, and we need to notify about them
+    // here.
+    if (web_contents->GetMainFrame()->IsRenderFrameLive()) {
+      for (RenderFrameHost* frame : web_contents->GetAllFrames()) {
+        if (frame->IsRenderFrameLive())
+          RenderFrameCreated(frame);
+      }
+    }
+  }
+
+  void WebContentsDestroyed() override {
+    // Deletes |this| and removes the pointer to it from WebTestControlHost.
+    web_test_control_->test_opened_window_observers_.erase(web_contents());
+  }
+
+  void RenderFrameCreated(RenderFrameHost* render_frame_host) override {
+    web_test_control_->HandleNewRenderFrameHost(render_frame_host);
+  }
+
+ private:
+  WebTestControlHost* const web_test_control_;
+};
+
 // WebTestControlHost -------------------------------------------------------
 
 WebTestControlHost* WebTestControlHost::instance_ = nullptr;
@@ -465,8 +496,6 @@ WebTestControlHost::WebTestControlHost()
   InjectTestSharedWorkerService(BrowserContext::GetStoragePartition(
       ShellContentBrowserClient::Get()->browser_context(), nullptr));
 
-  registrar_.Add(this, NOTIFICATION_RENDERER_PROCESS_CREATED,
-                 NotificationService::AllSources());
   GpuDataManager::GetInstance()->AddObserver(this);
   ResetBrowserAfterWebTest();
 }
@@ -482,7 +511,6 @@ WebTestControlHost::~WebTestControlHost() {
 
 bool WebTestControlHost::PrepareForWebTest(const TestInfo& test_info) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  test_phase_ = DURING_TEST;
   current_working_directory_ = test_info.current_working_directory;
   expected_pixel_hash_ = test_info.expected_pixel_hash;
   bool is_devtools_js_test = false;
@@ -521,8 +549,6 @@ bool WebTestControlHost::PrepareForWebTest(const TestInfo& test_info) {
         browser_context, GURL(url::kAboutBlankURL), nullptr, window_size);
     WebContentsObserver::Observe(main_window_->web_contents());
 
-    current_pid_ = base::kNullProcessId;
-
     main_render_view_host = main_window_->web_contents()->GetRenderViewHost();
     default_prefs_ = main_render_view_host->GetWebkitPreferences();
   } else {
@@ -543,18 +569,12 @@ bool WebTestControlHost::PrepareForWebTest(const TestInfo& test_info) {
     main_window_->web_contents()->WasShown();
   }
 
-  if (is_devtools_js_test && !secondary_window_) {
-    secondary_window_ = content::Shell::CreateNewWindow(
-        ShellContentBrowserClient::Get()->browser_context(),
-        GURL(url::kAboutBlankURL), nullptr, window_size);
-  }
-
-  // The main frame is constructed along with the Shell, which is before we can
-  // observe it happening. Further, we clear all observers and re-add the main
-  // frame here before each test.
+  // We did not track the |main_window_| RenderFrameHost during the creation of
+  // |main_window_|, since we need the pointer value in this class set first. So
+  // we update the |test_phase_| here allowing us to now track the RenderFrames
+  // in that window, and call HandleNewRenderFrameHost() explicitly.
+  test_phase_ = DURING_TEST;
   HandleNewRenderFrameHost(main_window_->web_contents()->GetMainFrame());
-  if (secondary_window_)
-    HandleNewRenderFrameHost(secondary_window_->web_contents()->GetMainFrame());
 
   if (is_devtools_protocol_test) {
     devtools_protocol_test_bindings_ =
@@ -578,6 +598,11 @@ bool WebTestControlHost::PrepareForWebTest(const TestInfo& test_info) {
   main_render_view_host->GetWidget()->FlushForTesting();
 
   if (is_devtools_js_test) {
+    if (!secondary_window_) {
+      secondary_window_ = content::Shell::CreateNewWindow(
+          ShellContentBrowserClient::Get()->browser_context(),
+          GURL(url::kAboutBlankURL), nullptr, window_size);
+    }
     // This navigates the secondary (devtools inspector) window, and then
     // navigates the main window once that has loaded to a devtools html test
     // page, based on the test url.
@@ -640,6 +665,13 @@ bool WebTestControlHost::ResetBrowserAfterWebTest() {
   return true;
 }
 
+void WebTestControlHost::DidOpenNewWindowOrTab(WebContents* web_contents) {
+  auto result = test_opened_window_observers_.emplace(
+      web_contents,
+      std::make_unique<WebTestWindowObserver>(web_contents, this));
+  CHECK(result.second);  // The WebContents should not already be in the map!
+}
+
 void WebTestControlHost::SetTempPath(const base::FilePath& temp_path) {
   temp_path_ = temp_path;
 }
@@ -924,12 +956,6 @@ void WebTestControlHost::PluginCrashed(const base::FilePath& plugin_path,
                      weak_factory_.GetWeakPtr()));
 }
 
-void WebTestControlHost::RenderFrameCreated(
-    RenderFrameHost* render_frame_host) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  HandleNewRenderFrameHost(render_frame_host);
-}
-
 void WebTestControlHost::TitleWasSet(NavigationEntry* entry) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   std::vector<std::string> logs = DumpTitleWasSet(main_window_->web_contents());
@@ -1004,30 +1030,6 @@ void WebTestControlHost::RenderProcessExited(
   }
 }
 
-void WebTestControlHost::Observe(int type,
-                                 const NotificationSource& source,
-                                 const NotificationDetails& details) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  switch (type) {
-    case NOTIFICATION_RENDERER_PROCESS_CREATED: {
-      if (!main_window_)
-        return;
-      RenderViewHost* render_view_host =
-          main_window_->web_contents()->GetRenderViewHost();
-      if (!render_view_host)
-        return;
-      RenderProcessHost* render_process_host =
-          Source<RenderProcessHost>(source).ptr();
-      if (render_process_host != render_view_host->GetProcess())
-        return;
-      current_pid_ = render_process_host->GetProcess().Pid();
-      break;
-    }
-    default:
-      NOTREACHED();
-  }
-}
-
 void WebTestControlHost::OnGpuProcessCrashed(
     base::TerminationStatus exit_code) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -1050,21 +1052,20 @@ void WebTestControlHost::DiscardMainWindow() {
     main_window_->Close();
   }
   main_window_ = nullptr;
-  current_pid_ = base::kNullProcessId;
 }
 
 void WebTestControlHost::HandleNewRenderFrameHost(RenderFrameHost* frame) {
-  RenderProcessHost* process_host = frame->GetProcess();
-  RenderViewHost* view_host = frame->GetRenderViewHost();
-  bool main_window =
+  // When creating the main window, we don't have a |main_window_| pointer yet.
+  // So we will explicitly call this for the main window after moving  to
+  // DURING_TEST.
+  if (test_phase_ != DURING_TEST)
+    return;
+
+  const bool main_window =
       WebContents::FromRenderFrameHost(frame) == main_window_->web_contents();
 
-  // Track pid of the renderer handling the main frame.
-  if (main_window && frame->GetParent() == nullptr) {
-    const base::Process& process = process_host->GetProcess();
-    if (process.IsValid())
-      current_pid_ = process.Pid();
-  }
+  RenderProcessHost* process_host = frame->GetProcess();
+  RenderViewHost* view_host = frame->GetRenderViewHost();
 
   // If this the first time this renderer contains parts of the main test
   // window, we need to make sure that it gets configured correctly (including
@@ -1474,10 +1475,12 @@ void WebTestControlHost::ResetRendererAfterWebTestDone() {
   if (leak_detector_) {
     if (main_window_ && main_window_->web_contents()) {
       RenderViewHost* rvh = main_window_->web_contents()->GetRenderViewHost();
+      RenderProcessHost* rph = rvh->GetProcess();
+      CHECK(rph->GetProcess().IsValid());
       leak_detector_->TryLeakDetection(
-          rvh->GetProcess(),
+          rph,
           base::BindOnce(&WebTestControlHost::OnLeakDetectionDone,
-                         weak_factory_.GetWeakPtr()));
+                         weak_factory_.GetWeakPtr(), rph->GetProcess().Pid()));
     }
     return;
   }
@@ -1486,14 +1489,15 @@ void WebTestControlHost::ResetRendererAfterWebTestDone() {
 }
 
 void WebTestControlHost::OnLeakDetectionDone(
+    int pid,
     const LeakDetector::LeakDetectionReport& report) {
   if (!report.leaked) {
     Shell::QuitMainMessageLoopForTesting();
     return;
   }
 
-  printer_->AddErrorMessage(base::StringPrintf(
-      "#LEAK - renderer pid %d (%s)", current_pid_, report.detail.c_str()));
+  printer_->AddErrorMessage(base::StringPrintf("#LEAK - renderer pid %d (%s)",
+                                               pid, report.detail.c_str()));
   CHECK(!crash_when_leak_found_);
 
   DiscardMainWindow();

content/shell/browser/web_test/web_test_control_host.h

@@ -108,7 +108,6 @@ class WebTestResultPrinter {
 
 class WebTestControlHost : public WebContentsObserver,
                            public RenderProcessHostObserver,
-                           public NotificationObserver,
                            public GpuDataManagerObserver,
                            public mojom::WebTestControlHost {
  public:
@@ -127,10 +126,7 @@ class WebTestControlHost : public WebContentsObserver,
       int sender_process_host_id,
       const base::DictionaryValue& changed_web_test_runtime_flags);
 
-  // Makes sure that the potentially new renderer associated with |frame| is 1)
-  // initialized for the test, 2) kept up to date wrt test flags and 3)
-  // monitored for crashes.
-  void HandleNewRenderFrameHost(RenderFrameHost* frame);
+  void DidOpenNewWindowOrTab(WebContents* web_contents);
 
   void SetTempPath(const base::FilePath& temp_path);
   void RendererUnresponsive();
@@ -152,7 +148,6 @@ class WebTestControlHost : public WebContentsObserver,
   // WebContentsObserver implementation.
   void PluginCrashed(const base::FilePath& plugin_path,
                      base::ProcessId plugin_pid) override;
-  void RenderFrameCreated(RenderFrameHost* render_frame_host) override;
   void TitleWasSet(NavigationEntry* entry) override;
   void DidFailLoad(RenderFrameHost* render_frame_host,
                    const GURL& validated_url,
@@ -168,11 +163,6 @@ class WebTestControlHost : public WebContentsObserver,
   void RenderProcessExited(RenderProcessHost* render_process_host,
                            const ChildProcessTerminationInfo& info) override;
 
-  // NotificationObserver implementation.
-  void Observe(int type,
-               const NotificationSource& source,
-               const NotificationDetails& details) override;
-
   // GpuDataManagerObserver implementation.
   void OnGpuProcessCrashed(base::TerminationStatus exit_code) override;
 
@@ -224,10 +214,17 @@ class WebTestControlHost : public WebContentsObserver,
     DISALLOW_COPY_AND_ASSIGN(Node);
   };
 
+  class WebTestWindowObserver;
+
   static WebTestControlHost* instance_;
 
   void DiscardMainWindow();
 
+  // Makes sure that the potentially new renderer associated with |frame| is 1)
+  // initialized for the test, 2) kept up to date wrt test flags and 3)
+  // monitored for crashes.
+  void HandleNewRenderFrameHost(RenderFrameHost* frame);
+
   // Message handlers.
   void OnAudioDump(const std::vector<unsigned char>& audio_dump);
   void OnImageDump(const std::string& actual_pixel_hash, const SkBitmap& image);
@@ -236,7 +233,8 @@ class WebTestControlHost : public WebContentsObserver,
                                  const std::string& dump);
   void OnTestFinished();
   void OnCaptureSessionHistory();
-  void OnLeakDetectionDone(const LeakDetector::LeakDetectionReport& report);
+  void OnLeakDetectionDone(int pid,
+                           const LeakDetector::LeakDetectionReport& report);
 
   void OnCleanupFinished();
   void OnCaptureDumpCompleted(mojom::WebTestDumpPtr dump);
@@ -285,9 +283,6 @@ class WebTestControlHost : public WebContentsObserver,
   std::unique_ptr<DevToolsProtocolTestBindings>
       devtools_protocol_test_bindings_;
 
-  // The PID of the render process of the render view host of main_window_.
-  int current_pid_;
-
   // Tracks if (during the current test) we have already sent *initial* test
   // configuration to a renderer process (*initial* test configuration is
   // associated with some steps that should only be executed *once* per test -
@@ -311,8 +306,6 @@ class WebTestControlHost : public WebContentsObserver,
   bool should_override_prefs_;
   WebPreferences prefs_;
 
-  NotificationRegistrar registrar_;
-
   bool crash_when_leak_found_;
   std::unique_ptr<LeakDetector> leak_detector_;
 
@@ -323,6 +316,10 @@ class WebTestControlHost : public WebContentsObserver,
   // Number of WebTestRenderFrame.DumpFrameLayout responses we are waiting for.
   int pending_layout_dumps_;
 
+  // Observe windows opened by tests.
+  base::flat_map<WebContents*, std::unique_ptr<WebTestWindowObserver>>
+      test_opened_window_observers_;
+
   // Renderer processes are observed to detect crashes.
   ScopedObserver<RenderProcessHost, RenderProcessHostObserver>
       render_process_host_observer_{this};

content/shell/browser/web_test/web_test_devtools_bindings.cc

@@ -50,12 +50,6 @@ class WebTestDevToolsBindings::SecondaryObserver : public WebContentsObserver {
     bindings_ = nullptr;
   }
 
-  // WebContentsObserver implementation.
-  void RenderFrameCreated(RenderFrameHost* render_frame_host) override {
-    if (WebTestControlHost::Get())
-      WebTestControlHost::Get()->HandleNewRenderFrameHost(render_frame_host);
-  }
-
  private:
   WebTestDevToolsBindings* bindings_;
   DISALLOW_COPY_AND_ASSIGN(SecondaryObserver);