Kill bad apps.

chrome/browser/apps/guest_view/app_view_browsertest.cc

Index: chrome/browser/apps/guest_view/app_view_browsertest.cc
diff --git a/chrome/browser/apps/guest_view/app_view_browsertest.cc b/chrome/browser/apps/guest_view/app_view_browsertest.cc
index 0278507d3b520be64442f238bf2ee3cfe3dab465..243668cfc3f5cea6ad491b5647e4d9790e9d6641 100644
--- a/chrome/browser/apps/guest_view/app_view_browsertest.cc
+++ b/chrome/browser/apps/guest_view/app_view_browsertest.cc
@@ -1,16 +1,21 @@
 // Copyright 2014 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 "base/strings/stringprintf.h"
 #include "chrome/browser/apps/app_browsertest_util.h"
 #include "components/guest_view/browser/guest_view_manager.h"
 #include "components/guest_view/browser/guest_view_manager_factory.h"
 #include "components/guest_view/browser/test_guest_view_manager.h"
+#include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_process_host_observer.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/app_window/app_window_registry.h"
+#include "extensions/browser/guest_view/app_view/app_view_guest.h"
+#include "extensions/browser/guest_view/extensions_guest_view_manager_delegate.h"
+#include "extensions/browser/process_manager.h"
 #include "extensions/common/switches.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -20,6 +25,52 @@
 using guest_view::GuestViewManager;
 using guest_view::TestGuestViewManagerFactory;
 
+namespace {
+
+class RenderProcessHostObserverForExit
+    : public content::RenderProcessHostObserver {
+ public:
+  explicit RenderProcessHostObserverForExit(
+      content::RenderProcessHost* observed_host)
+      : render_process_host_exited_(false),
+        observed_host_(observed_host),
+        message_loop_runner_(nullptr) {
+    observed_host->AddObserver(this);
+    LOG(INFO) << "Listening for RPH Exit (ID = " << observed_host->GetID()
+              << ")";
+  }
+
+  void WaitUntilRenderProcessHostKilled() {
+    if (render_process_host_exited_)
+      return;
+    message_loop_runner_ = new content::MessageLoopRunner;
+    message_loop_runner_->Run();
+  }
+
+  base::TerminationStatus termination_status() { return status_; }
+
+ private:
+  void RenderProcessExited(content::RenderProcessHost* host,
+                           base::TerminationStatus status,
+                           int exit_code) override {
+    LOG(INFO) << "RPH is exiting. (ID = " << host->GetID() << ").";
+    DCHECK(observed_host_ == host);
+    render_process_host_exited_ = true;
+    status_ = status;
+    observed_host_->RemoveObserver(this);
+    if (message_loop_runner_.get()) {
+      message_loop_runner_->Quit();
+    }
+  }
+
+  bool render_process_host_exited_;
+  content::RenderProcessHost* observed_host_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+  base::TerminationStatus status_;
+};
+
+}  // namespace
+
 class AppViewTest : public extensions::PlatformAppBrowserTest {
  public:
   AppViewTest() {
@@ -59,8 +110,7 @@ class AppViewTest : public extensions::PlatformAppBrowserTest {
     done_listener.set_failure_message("TEST_FAILED");
     if (!content::ExecuteScript(
             embedder_web_contents,
-            base::StringPrintf("runTest('%s', '%s')",
-                               test_name.c_str(),
+            base::StringPrintf("runTest('%s', '%s')", test_name.c_str(),
                                app_to_embed.c_str()))) {
       LOG(ERROR) << "UNABLE TO START TEST.";
       return;
@@ -68,6 +118,22 @@ class AppViewTest : public extensions::PlatformAppBrowserTest {
     ASSERT_TRUE(done_listener.WaitUntilSatisfied());
   }
 
+  guest_view::TestGuestViewManager* GetGuestViewManager() {
+    guest_view::TestGuestViewManager* manager =
+        static_cast<guest_view::TestGuestViewManager*>(
+            guest_view::TestGuestViewManager::FromBrowserContext(
+                browser()->profile()));
+    if (!manager) {
+      manager = static_cast<guest_view::TestGuestViewManager*>(
+          GuestViewManager::CreateWithDelegate(
+              browser()->profile(),
+              scoped_ptr<guest_view::GuestViewManagerDelegate>(
+                  new extensions::ExtensionsGuestViewManagerDelegate(
+                      browser()->profile()))));
+    }
+    return manager;
+  }
+
  private:
   void SetUpCommandLine(base::CommandLine* command_line) override {
     extensions::PlatformAppBrowserTest::SetUpCommandLine(command_line);
@@ -125,3 +191,126 @@ IN_PROC_BROWSER_TEST_F(AppViewTest, TestAppViewEmbedSelfShouldFail) {
              skeleton_app->id(),
              NO_TEST_SERVER);
 }
+
+IN_PROC_BROWSER_TEST_F(AppViewTest, TestKillGuestWithInvalidInstanceID) {
+  const extensions::Extension* mock_bad_app =
+      LoadAndLaunchPlatformApp("app_view/bad_app", "AppViewTest.LAUNCHED");
+
+  content::RenderProcessHost* bad_app_render_process_host =
+      extensions::AppWindowRegistry::Get(browser()->profile())
+          ->GetCurrentAppWindowForApp(mock_bad_app->id())
+          ->web_contents()
+          ->GetRenderProcessHost();
+
+  // Monitor |mock_bad_app|'s "RenderProcessHost" for its exiting.
+  RenderProcessHostObserverForExit exit_observer(bad_app_render_process_host);
+
+  // Get/Create the instance of "GuestViewManager".
+  guest_view::GuestViewManager* guest_view_manager = GetGuestViewManager();
+  EXPECT_TRUE(guest_view_manager != nullptr);
+
+  // Choosing a |guest_instance_id| which does not exist.
+  int invalid_guest_instance_id = guest_view_manager->GetNextInstanceID();
+  LOG(INFO) << "Guest instance ID: " << invalid_guest_instance_id;
+
+  // Call the desired function to verify that the |mock_bad_app| gets killed if
+  // the provided |guest_instance_id| is not mapped to any "GuestView"'s.
+  extensions::AppViewGuest::CompletePendingRequest(
+      browser()->profile(), GURL("about:blank"), invalid_guest_instance_id,
+      mock_bad_app->id(), bad_app_render_process_host->GetID());
+  exit_observer.WaitUntilRenderProcessHostKilled();
+  EXPECT_EQ(exit_observer.termination_status(),
+            base::TERMINATION_STATUS_PROCESS_WAS_KILLED);
+}
+
+IN_PROC_BROWSER_TEST_F(AppViewTest,
+                       TestKillGuestCommunicatingWithWrongAppView) {
+  const extensions::Extension* host_app =
+      LoadAndLaunchPlatformApp("app_view/host_app", "AppViewTest.LAUNCHED");
+  const extensions::Extension* mock_guest_extension =
+      InstallPlatformApp("app_view/guest_app");
+  const extensions::Extension* mock_bad_app =
+      LoadAndLaunchPlatformApp("app_view/bad_app", "AppViewTest.LAUNCHED");
+  // Ask the host to create and load an <appview>
+  EXPECT_TRUE(content::ExecuteScript(
+      extensions::AppWindowRegistry::Get(browser()->profile())
+          ->GetCurrentAppWindowForApp(host_app->id())
+          ->web_contents(),
+      base::StringPrintf("onAppCommand('%s', '%s');", "EMBED",
+                         mock_guest_extension->id().c_str())));
+  // Now listen for the guest content to announce that it is being requested to
+  // be embedded.
+  ExtensionTestMessageListener on_embed_requested_listener(
+      "AppViewTest.EmbedRequested", true);
+  EXPECT_TRUE(on_embed_requested_listener.WaitUntilSatisfied());
+  // Now assume the bad application is somehow sending a message to complete a
+  // pending request to attach to <appview>. It should be killed.
+  content::RenderProcessHost* bad_app_render_process_host =
+      extensions::ProcessManager::Get(browser()->profile())
+          ->GetBackgroundHostForExtension(mock_bad_app->id())
+          ->render_process_host();
+  RenderProcessHostObserverForExit bad_app_obs(bad_app_render_process_host);
+  // Make the false request.
+  int guest_instance_id =
+      extensions::AppViewGuest::GetAllRegisteredInstanceIdsForTesting()[0];
+  extensions::AppViewGuest::CompletePendingRequest(
+      browser()->profile(), GURL("about:blank"), guest_instance_id,
+      mock_bad_app->id(), bad_app_render_process_host->GetID());
+  bad_app_obs.WaitUntilRenderProcessHostKilled();
+
+  EXPECT_EQ(bad_app_obs.termination_status(),
+            base::TERMINATION_STATUS_PROCESS_WAS_KILLED);
+  // Proceed with the rest of embedding of the guest app.
+  LOG(INFO) << "Forcing the 'guest_app' to finalize embedding process by"
+            << " executing script.";
+  EXPECT_TRUE(content::ExecuteScript(
+      extensions::ProcessManager::Get(browser()->profile())
+          ->GetBackgroundHostForExtension(mock_guest_extension->id())
+          ->host_contents(),
+      "continueEmbedding();"));
+}
+
+IN_PROC_BROWSER_TEST_F(
+    AppViewTest,
+    TestKillGuestCommunicatingWithWrongAppViewUsingChromeSendMessageAPI) {

[lazyboy, 2015/06/29 20:05:33]

Here and in other test, You should remove the prefix "Test" before the test
name, as this already belongs to AppViewTest. I know we're not consistent about
this :), but given the epic function name, I'd look for opportunities to shorten
it w/o loosing clarity.

"UsingChromeSendMessageAPI" is really an internal details and shouldn't be part
of the test name. Something and Something2 as test names are also fine.

[EhsanK, 2015/06/30 16:53:10]

Done.

+  const extensions::Extension* host_app =
+      LoadAndLaunchPlatformApp("app_view/host_app", "AppViewTest.LAUNCHED");
+  const extensions::Extension* mock_guest_extension =
+      InstallPlatformApp("app_view/guest_app");
+  const extensions::Extension* mock_bad_app =
+      LoadAndLaunchPlatformApp("app_view/bad_app", "AppViewTest.LAUNCHED");
+  // Ask the host to create and load an <appview>
+  EXPECT_TRUE(content::ExecuteScript(
+      extensions::AppWindowRegistry::Get(browser()->profile())
+          ->GetCurrentAppWindowForApp(host_app->id())
+          ->web_contents(),
+      base::StringPrintf("onAppCommand('%s', '%s');", "EMBED",
+                         mock_guest_extension->id().c_str())));
+  // Now listen for the guest content to announce that it is being requested to
+  // be embedded.
+  ExtensionTestMessageListener on_embed_requested_listener(
+      "AppViewTest.EmbedRequested", true);
+  EXPECT_TRUE(on_embed_requested_listener.WaitUntilSatisfied());
+  // Now assume the bad application is somehow sending a message to complete a
+  // pending request to attach to <appview>. It should be killed.
+  content::RenderProcessHost* bad_app_render_process_host =
+      extensions::ProcessManager::Get(browser()->profile())
+          ->GetBackgroundHostForExtension(mock_bad_app->id())
+          ->render_process_host();
+  RenderProcessHostObserverForExit bad_app_obs(bad_app_render_process_host);
+  // Make the false request.
+  int guest_instance_id =
+      extensions::AppViewGuest::GetAllRegisteredInstanceIdsForTesting()[0];
+  extensions::AppViewGuest::CompletePendingRequest(
+      browser()->profile(), GURL("about:blank"), guest_instance_id,
+      mock_bad_app->id(), bad_app_render_process_host->GetID());
+  bad_app_obs.WaitUntilRenderProcessHostKilled();

[lazyboy, 2015/06/29 20:05:33]

I got a chance to run this test locally.
I think this is the problem you have, you have a message loop started above in
line 291 and one right here before the previous one finished.

You can just call on_embed_requested_listener.Reply() right before this line?
That would fix the nested message loop issue.

[EhsanK, 2015/06/30 16:53:11]

You are right. Thanks!. Done.

+
+  EXPECT_EQ(bad_app_obs.termination_status(),
+            base::TERMINATION_STATUS_PROCESS_WAS_KILLED);
+  // Proceed with the rest of embedding of the guest app.
+  LOG(INFO) << "Replying the message from 'guest_app' so that it can finalize"
+            << " It's embedding.";
+  on_embed_requested_listener.Reply("continue");
+  LOG(INFO) << "Message sent!";
+}