Re-fix http://crbug.com/87176, and add a test.

content/browser/renderer_host/render_process_host_impl_unittest.cc

Index: content/browser/renderer_host/render_process_host_impl_unittest.cc
diff --git a/content/browser/renderer_host/render_process_host_impl_unittest.cc b/content/browser/renderer_host/render_process_host_impl_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5e22b71c5dce538a7ce941596974c3921054e83d
--- /dev/null
+++ b/content/browser/renderer_host/render_process_host_impl_unittest.cc
@@ -0,0 +1,257 @@
+// Copyright 2013 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/browser/renderer_host/render_process_host_impl.h"
+
+#include "base/command_line.h"
+#include "base/process_util.h"
+#include "base/run_loop.h"
+#include "content/browser/child_process_launcher.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/browser/site_instance_impl.h"
+#include "content/browser/storage_partition_impl.h"
+#include "content/browser/webui/web_ui_controller_factory_registry.h"
+#include "content/common/child_process_messages.h"
+#include "content/common/view_messages.h"
+#include "content/public/browser/render_process_host_factory.h"
+#include "content/public/common/sandboxed_process_launcher_delegate.h"
+#include "content/public/test/test_browser_context.h"
+#include "content/public/test/test_browser_thread.h"
+#include "content/test/test_content_browser_client.h"
+#include "content/test/test_render_view_host_factory.h"
+#include "content/test/test_web_contents.h"
+#include "ipc/ipc_switches.h"
+#include "ipc/ipc_test_sink.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+namespace {
+
+class FakeChildProcessLauncher;
+
+struct LaunchedChildProcess {
+  // Becomes NULL after the FakeChildProcessLauncher is destroyed.
+  FakeChildProcessLauncher* launcher_;
+  // Collects messages sent to the child process.
+  IPC::TestSink messages_;
+};
+
+class FakeChildProcessLauncher : public ChildProcessLauncher {
+ public:
+  FakeChildProcessLauncher(LaunchedChildProcess* info,
+                           const IPC::ChannelHandle& channel,
+                           Client* client)
+      : info_(info),
+        client_(client),
+        starting_(true),
+        termination_status_(base::TERMINATION_STATUS_NORMAL_TERMINATION),
+        channel_(new IPC::ChannelProxy(
+            channel,
+            IPC::Channel::MODE_CLIENT,
+            &info->messages_,
+            BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO))),
+        weak_this_factory_(this) {
+    info_->launcher_ = this;
+
+    // Automatically start the child process so the RenderProcessHostImpl sends
+    // messages to it.  To mimic real child processes, don't start instantly.
+    base::MessageLoopProxy::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&FakeChildProcessLauncher::Start,
+                   weak_this_factory_.GetWeakPtr()));
+  }
+
+  virtual ~FakeChildProcessLauncher() { info_->launcher_ = NULL; }
+
+  void Send(IPC::Message* msg) {
+    EXPECT_TRUE(channel_->Send(msg));
+  }
+
+  // Implementation of ChildProcessLauncher.
+  virtual bool IsStarting() OVERRIDE { return starting_; }
+  virtual base::ProcessHandle GetHandle() OVERRIDE {
+    return base::Process::Current().handle();
+  }
+  virtual base::TerminationStatus GetChildTerminationStatus(bool known_dead,
+                                                            int* exit_code)
+      OVERRIDE {
+    return termination_status_;
+  }
+  virtual void SetProcessBackgrounded(bool background) OVERRIDE {}
+  virtual void SetTerminateChildOnShutdown(bool terminate_on_shutdown)
+      OVERRIDE {}
+
+ private:
+  void Start() {
+    starting_ = false;
+    client_->OnProcessLaunched();
+  }
+
+  LaunchedChildProcess* info_;
+  Client* client_;
+  bool starting_;
+  base::TerminationStatus termination_status_;
+  scoped_ptr<IPC::ChannelProxy> channel_;
+  base::WeakPtrFactory<FakeChildProcessLauncher> weak_this_factory_;
+};
+
+scoped_ptr<ChildProcessLauncher> NewFakeChildProcessLauncher(
+    ScopedVector<LaunchedChildProcess>* child_processes,
+#if defined(OS_WIN)
+    SandboxedProcessLauncherDelegate* delegate,
+#elif defined(OS_POSIX)
+    bool use_zygote,
+    const base::EnvironmentVector& environ,
+    int ipcfd,
+#endif
+    CommandLine* cmd_line,
+    int child_process_id,
+    ChildProcessLauncher::Client* client) {
+#if defined(OS_WIN)
+  delete delegate;
+#endif
+  scoped_ptr<CommandLine> cmd_line_deleter(cmd_line);
+  child_processes->push_back(new LaunchedChildProcess());
+  std::string channel_id =
+      cmd_line->GetSwitchValueASCII(switches::kProcessChannelID);
+  return scoped_ptr<ChildProcessLauncher>(new FakeChildProcessLauncher(
+      child_processes->back(),
+      IPC::ChannelHandle(channel_id
+#if defined(OS_POSIX)
+                         ,
+                         base::FileDescriptor(ipcfd, false)
+#endif
+                         ),
+      client));
+}
+
+class RPHIBrowserContext : public TestBrowserContext {
+ private:
+  virtual net::URLRequestContextGetter* GetRequestContextForRenderProcess(
+      int renderer_child_id) OVERRIDE {
+    return GetRequestContext();
+  }
+};
+
+class RPHIBrowserClient : public TestContentBrowserClient {
+ public:
+  RPHIBrowserClient()
+      : request_context_getter_(new net::TestURLRequestContextGetter(
+            BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO))) {}
+
+  virtual net::URLRequestContextGetter* CreateRequestContext(
+      BrowserContext* browser_context,
+      ProtocolHandlerMap* protocol_handlers) OVERRIDE {
+    return request_context_getter_.get();
+  }
+
+ private:
+  scoped_refptr<net::TestURLRequestContextGetter> request_context_getter_;
+};
+
+class FakeChildProcessRPHFactory : public RenderProcessHostFactory {
+ public:
+  FakeChildProcessRPHFactory(
+      ScopedVector<LaunchedChildProcess>* child_processes,
+      SiteInstance* site_instance)
+      : child_processes_(child_processes), site_instance_(site_instance) {
+    SiteInstanceImpl::set_render_process_host_factory(this);
+  }
+  virtual ~FakeChildProcessRPHFactory() {
+    SiteInstanceImpl::set_render_process_host_factory(NULL);
+  }
+
+  virtual RenderProcessHost* CreateRenderProcessHost(
+      BrowserContext* browser_context) const OVERRIDE {
+    RenderProcessHostImpl* host =
+        new RenderProcessHostImpl(browser_context,
+                                  static_cast<StoragePartitionImpl*>(
+                                      TestBrowserContext::GetStoragePartition(
+                                          browser_context, site_instance_)),
+                                  false,
+                                  false);
+    host->SetChildProcessLauncherFactory(
+        base::Bind(&NewFakeChildProcessLauncher, child_processes_));
+    return host;
+  }
+
+ private:
+  ScopedVector<LaunchedChildProcess>* child_processes_;
+  SiteInstance* site_instance_;

[Charlie Reis, 2013/06/05 19:07:10]

It's not clear why this needs a SiteInstance, since processes aren't created
with a SiteInstance.  Looks like we're only using it to get the
StoragePartition.  Perhaps we should take that in instead?

[Jeffrey Yasskin, 2013/06/05 21:23:27]

Sure, done.

Another possible refactoring would be to pass the SiteInstance and possibly the
ContentBrowserClient to CreateRenderProcessHost() at
https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/s...,
so that both branches have access to the same information.

[Charlie Reis, 2013/06/05 21:54:09]

Good point!  That seems like a better solution, so we don't have to store either
here.

[Jeffrey Yasskin, 2013/06/05 22:53:20]

I've put that in https://codereview.chromium.org/16490003/. Assuming that one
gets approved soon, I'll rebase this on top of it; otherwise, I'll add this file
to that CL.

+};
+
+class RenderProcessHostImplTest : public testing::Test {
+ public:
+  RenderProcessHostImplTest()
+      : ui_thread_(BrowserThread::UI, &message_loop_),
+        webkit_thread_(BrowserThread::WEBKIT_DEPRECATED, &message_loop_),
+        file_user_blocking_thread_(BrowserThread::FILE_USER_BLOCKING,
+                                   &message_loop_),
+        io_thread_(BrowserThread::IO, &message_loop_),
+        old_browser_client_(SetBrowserClientForTesting(&test_browser_client_)) {
+  }
+
+  virtual ~RenderProcessHostImplTest() {
+    base::RunLoop().RunUntilIdle();
+    SetBrowserClientForTesting(old_browser_client_);
+  }
+
+ protected:
+  ScopedVector<LaunchedChildProcess> child_processes_;
+  base::MessageLoopForIO message_loop_;
+  TestBrowserThread ui_thread_;
+  TestBrowserThread webkit_thread_;
+  TestBrowserThread file_user_blocking_thread_;
+  TestBrowserThread io_thread_;
+
+  RPHIBrowserContext browser_context_;
+  RPHIBrowserClient test_browser_client_;
+  ContentBrowserClient* old_browser_client_;
+  ResourceDispatcherHostImpl resource_dispatcher_host_;
+
+  DISALLOW_COPY_AND_ASSIGN(RenderProcessHostImplTest);
+};
+
+// http://crbug.com/87176
+TEST_F(RenderProcessHostImplTest,
+       RejectShutdownFromChildProcessWithOneActiveView) {
+  const GURL kUrl("http://example.com");
+
+  scoped_refptr<SiteInstance> site(
+      SiteInstance::CreateForURL(&browser_context_, kUrl));
+  FakeChildProcessRPHFactory rph_factory(&child_processes_, site.get());
+
+  WebContents::CreateParams params(&browser_context_, site.get());
+  scoped_ptr<WebContentsImpl> tab(
+      WebContentsImpl::CreateWithOpener(params, NULL));
+  tab->GetController().LoadURL(kUrl, Referrer(), PAGE_TRANSITION_TYPED, "");

[Charlie Reis, 2013/06/05 19:07:10]

How does example.com commit?  Am I missing a part where we return a hard-coded
response for it?

[Jeffrey Yasskin, 2013/06/05 21:23:27]

No, AFAIK, there's no hard-coded response for it, and since there's no actual
renderer process, it probably wouldn't request the response anyway. I don't
understand the RPH's flow very well, but I suspect that it's not currently
committing, and that I'd need to fake one or more ViewHostMsg_Something in order
to push the RVH through the expected sequence of events.

Can you describe the extra assertion that'll check if example.com has committed?
After that, I can add enough messages to make it happen.

[Charlie Reis, 2013/06/05 21:54:09]

Well, we usually use a TestWebContents and call TestDidNavigate to simulate
commit.  There's examples of that in web_contents_impl_unittest.cc (e.g.,
SimpleNavigation).  I'm not sure if that's compatible with what you're doing
here, though.

Perhaps it doesn't matter if it commits for the purpose of this test, though. 
The new tab will be considered an active view, and that's all we need.  That
means your comment/check below about "the loading tab isn't pending" is a bit of
a red herring.  We just care that the ShutdownRequest message is sent to the
tab's current process, so you could check that
tab->GetSiteInstance()->GetProcess() is equal to child_processes_[0].

[Jeffrey Yasskin, 2013/06/05 22:53:20]

I started this test with a TestWebContents, but TestWebContents casts its
RenderViewHost to a TestRenderViewHost in CommitPendingNavigation(), and I can't
use TestRenderViewHost because it almost entirely avoids using the
RenderProcessHost that I'm trying to test.
That double-check was intended to make sure that pending_views_==0, but of
course the pending_render_view_host() isn't directly connected to that field
either. Oh well; I've removed it.
To some extent, knowing that child_processes_.size()==1 already tells us that,
since every RenderProcessHostImpl creates a ChildProcess, so there aren't any
other RenderProcessHostImpl instances to send anything to. But this double-check
doesn't hurt anything; done.

+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(1U, child_processes_.size());
+
+  child_processes_[0]->messages_.ClearMessages();
+
+  // Double-check that the loading tab isn't pending.
+  EXPECT_EQ(NULL,

[Charlie Reis, 2013/06/05 19:07:10]

Any reason not to use EXPECT_FALSE?  That seems to be a common pattern
elsewhere.

[Jeffrey Yasskin, 2013/06/05 21:23:27]

No particular reason. Done.

+            tab->GetRenderManagerForTesting()->pending_render_view_host());
+  EXPECT_EQ(1,
+            static_cast<RenderProcessHostImpl*>(
+                tab->GetSiteInstance()->GetProcess())->GetActiveViewCount());
+
+  // Sometimes the renderer process's ShutdownRequest (corresponding to the
+  // ViewMsg_WasSwappedOut from a previous navigation) doesn't arrive until

[Charlie Reis, 2013/06/05 19:07:10]

Interesting.  I like that we don't have to simulate the previous navigation,
since just sending the ShutdownRequest is sufficient.

+  // after the browser process decides to re-use the renderer for a new purpose.
+  // This test makes sure the browser doesn't let the renderer die in that case.
+  child_processes_[0]->launcher_
+      ->Send(new ChildProcessHostMsg_ShutdownRequest());

[Charlie Reis, 2013/06/05 19:07:10]

nit: -> on previous line

[Jeffrey Yasskin, 2013/06/05 21:23:27]

Done. Since clang-format puts the -> at the beginning of the line, I've filed
http://b/9299359 too.

[Charlie Reis, 2013/06/05 21:54:09]

Thanks.  Not sure if there's an official rule on this, but there's certainly
lots of code in content/ that puts it at the end, so that will be good for
consistency.

+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(NULL,

[Charlie Reis, 2013/06/05 19:07:10]

EXPECT_FALSE?

[Jeffrey Yasskin, 2013/06/05 21:23:27]

Done.

+            child_processes_[0]->messages_.GetFirstMessageMatching(
+                ChildProcessMsg_Shutdown::ID))
+      << "Host sent a message confirming that the renderer process should shut "
+      << "down even though it was still being used.";
+}
+
+}  // namespace
+}  // namespace content