Browser Plugin: Fixed browser process crash on embedder reload.

The browser process crashed because a CHECK that verifies that an existing browser_plugin_embedder_ does not yet exist fails.

This happens because CreateGuest should only be handled in WebContentsImpl on the first call to CreateGuest. On all subsequent calls, CreateGuest should be handled in BrowserPluginEmbedderHelper.

This patch makes this change.

I've also cleaned up some dead code.

I've also written a test that verifies that reloading the embedder
doesn't cause the browser plugin to misbehave.


BUG=153561
Test=BrowserPluginHostTest.ReloadEmbedder


Review URL: https://chromiumcodereview.appspot.com/11046002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@159854 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 107 insertions, 15 deletions

diff --git a/content/browser/browser_plugin/browser_plugin_embedder_helper.cc b/content/browser/browser_plugin/browser_plugin_embedder_helper.cc
index 7039b2b..6c15c98 100644
--- a/content/browser/browser_plugin/browser_plugin_embedder_helper.cc
+++ b/content/browser/browser_plugin/browser_plugin_embedder_helper.cc
@@ -33,6 +33,8 @@ bool BrowserPluginEmbedderHelper::OnMessageReceived(
     const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(BrowserPluginEmbedderHelper, message)
+    IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_CreateGuest,
+                        OnCreateGuest);
     IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_NavigateGuest,
                         OnNavigateGuest);
     IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_ResizeGuest, OnResizeGuest)
@@ -97,6 +99,19 @@ void BrowserPluginEmbedderHelper::OnHandleInputEvent(
                               reply_message);
 }
 
+void BrowserPluginEmbedderHelper::OnCreateGuest(
+    int instance_id,
+    const std::string& storage_partition_id,
+    bool persist_storage) {
+  // The first BrowserPluginHostMsg_CreateGuest message is handled in
+  // WebContentsImpl. All subsequent BrowserPluginHostMsg_CreateGuest
+  // messages are handled here.
+  embedder_->CreateGuest(render_view_host(),
+                         instance_id,
+                         storage_partition_id,
+                         persist_storage);
+}
+
 void BrowserPluginEmbedderHelper::OnNavigateGuest(
     int instance_id,
     const std::string& src,
diff --git a/content/browser/browser_plugin/browser_plugin_embedder_helper.h b/content/browser/browser_plugin/browser_plugin_embedder_helper.h
index c5e6c8e..6e90c9d 100644
--- a/content/browser/browser_plugin/browser_plugin_embedder_helper.h
+++ b/content/browser/browser_plugin/browser_plugin_embedder_helper.h
@@ -51,6 +51,9 @@ class BrowserPluginEmbedderHelper : public RenderViewHostObserver {
 
  private:
   // Message handlers.
+  void OnCreateGuest(int instance_id,
+                     const std::string& storage_partition_id,
+                     bool persist_storage);
   void OnNavigateGuest(int instance_id,
                        const std::string& src,
                        const BrowserPluginHostMsg_ResizeGuest_Params& params);
diff --git a/content/browser/browser_plugin/browser_plugin_host_browsertest.cc b/content/browser/browser_plugin/browser_plugin_host_browsertest.cc
index d868e8f7..368ee233 100644
--- a/content/browser/browser_plugin/browser_plugin_host_browsertest.cc
+++ b/content/browser/browser_plugin/browser_plugin_host_browsertest.cc
@@ -795,5 +795,83 @@ IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest, Renavigate) {
   }
 }
 
+// This tests verifies that reloading the embedder does not crash the browser
+// and that the guest is reset.
+IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest, ReloadEmbedder) {
+  ASSERT_TRUE(test_server()->Start());
+  GURL test_url(test_server()->GetURL(
+      "files/browser_plugin_embedder.html"));
+  NavigateToURL(shell(), test_url);
+
+  WebContentsImpl* embedder_web_contents = static_cast<WebContentsImpl*>(
+      shell()->web_contents());
+  RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(
+      embedder_web_contents->GetRenderViewHost());
+
+  rvh->ExecuteJavascriptAndGetValue(string16(), ASCIIToUTF16(
+      StringPrintf("SetSrc('%s');", kHTMLForGuest)));
+
+  // Wait to make sure embedder is created/attached to WebContents.
+  TestBrowserPluginHostFactory::GetInstance()->WaitForEmbedderCreation();
+
+  TestBrowserPluginEmbedder* test_embedder =
+      static_cast<TestBrowserPluginEmbedder*>(
+          embedder_web_contents->GetBrowserPluginEmbedder());
+  ASSERT_TRUE(test_embedder);
+  test_embedder->WaitForGuestAdded();
+
+  // Verify that we have exactly one guest.
+  const BrowserPluginEmbedder::ContainerInstanceMap& instance_map =
+      test_embedder->guest_web_contents_for_testing();
+  EXPECT_EQ(1u, instance_map.size());
+
+  WebContentsImpl* test_guest_web_contents = static_cast<WebContentsImpl*>(
+      instance_map.begin()->second);
+  TestBrowserPluginGuest* test_guest = static_cast<TestBrowserPluginGuest*>(
+      test_guest_web_contents->GetBrowserPluginGuest());
+
+  // Wait for the guest to send an UpdateRectMsg, meaning it is ready.
+  test_guest->WaitForUpdateRectMsg();
+
+  // Change the title of the page to 'modified' so that we know that
+  // the page has successfully reloaded when it goes back to 'embedder'
+  // in the next step.
+  {
+    const string16 expected_title = ASCIIToUTF16("modified");
+    content::TitleWatcher title_watcher(embedder_web_contents,
+                                        expected_title);
+
+    rvh->ExecuteJavascriptAndGetValue(string16(), ASCIIToUTF16(
+        StringPrintf("SetTitle('%s');", "modified")));
+
+    string16 actual_title = title_watcher.WaitAndGetTitle();
+    EXPECT_EQ(expected_title, actual_title);
+  }
+
+  // Reload the embedder page, and verify that the reload was successful.
+  // Then navigate the guest to verify that the browser process does not crash.
+  {
+    const string16 expected_title = ASCIIToUTF16("embedder");
+    content::TitleWatcher title_watcher(embedder_web_contents,
+                                        expected_title);
+
+    embedder_web_contents->GetController().Reload(false);
+    string16 actual_title = title_watcher.WaitAndGetTitle();
+    EXPECT_EQ(expected_title, actual_title);
+
+    embedder_web_contents->GetRenderViewHost()->
+        ExecuteJavascriptAndGetValue(string16(), ASCIIToUTF16(
+            StringPrintf("SetSrc('%s');", kHTMLForGuest)));
+
+    WebContentsImpl* test_guest_web_contents = static_cast<WebContentsImpl*>(
+        instance_map.begin()->second);
+    TestBrowserPluginGuest* test_guest = static_cast<TestBrowserPluginGuest*>(
+        test_guest_web_contents->GetBrowserPluginGuest());
+
+    // Wait for the guest to send an UpdateRectMsg, meaning it is ready.
+    test_guest->WaitForUpdateRectMsg();
+  }
+}
+
 
 }  // namespace content
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 9291e9f..beeed5e6 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -2378,8 +2378,13 @@ void WebContentsImpl::OnBrowserPluginCreateGuest(
   // plugin renderer prior to CreateGuest will be ignored.
   // For more info, see comment above classes BrowserPluginEmbedder and
   // BrowserPluginGuest.
+  // The first BrowserPluginHostMsg_CreateGuest message from this WebContents'
+  // embedder render process is handled here. Once BrowserPluginEmbedder is
+  // created, all subsequent BrowserPluginHostMsg_CreateGuest messages are
+  // intercepted by the BrowserPluginEmbedderHelper and handled by the
+  // BrowserPluginEmbedder. Thus, this code will not be executed if a
+  // BrowserPluginEmbedder exists for this WebContents.
   CHECK(!browser_plugin_embedder_.get());
-
   browser_plugin_embedder_.reset(
       content::BrowserPluginEmbedder::Create(this, GetRenderViewHost()));
   browser_plugin_embedder_->CreateGuest(GetRenderViewHost(),
@@ -2388,16 +2393,6 @@ void WebContentsImpl::OnBrowserPluginCreateGuest(
                                         persist_storage);
 }
 
-void WebContentsImpl::OnBrowserPluginNavigateGuest(
-    int instance_id,
-    const std::string& src,
-    const BrowserPluginHostMsg_ResizeGuest_Params& resize_params) {
-  browser_plugin_embedder_->NavigateGuest(GetRenderViewHost(),
-                                          instance_id,
-                                          src,
-                                          resize_params);
-}
-
 // Notifies the RenderWidgetHost instance about the fact that the page is
 // loading, or done loading and calls the base implementation.
 void WebContentsImpl::SetIsLoading(bool is_loading,
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index dffb971..635a406 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -574,10 +574,6 @@ class CONTENT_EXPORT WebContentsImpl
   void OnBrowserPluginCreateGuest(int instance_id,
                                   const std::string& storage_partition_id,
                                   bool persist_storage);
-  void OnBrowserPluginNavigateGuest(
-      int instance_id,
-      const std::string& src,
-      const BrowserPluginHostMsg_ResizeGuest_Params& resize_params);
 
   // Changes the IsLoading state and notifies delegate as needed
   // |details| is used to provide details on the load that just finished
diff --git a/content/test/data/browser_plugin_embedder.html b/content/test/data/browser_plugin_embedder.html
index fb70190..dc2e780 100644
--- a/content/test/data/browser_plugin_embedder.html
+++ b/content/test/data/browser_plugin_embedder.html
@@ -20,6 +20,11 @@ function Go(relativeIndex) {
   var plugin = document.getElementById('plugin');
   plugin.go(relativeIndex);
 }
+function SetTitle(str) {
+  document.title = str;
+}
+
+window.document.title = 'embedder';
 </script>
 
 <object id="plugin"