InitRenderViewHostForExtensions would only be called when a new RenderViewHost

was created, but when reloading a crashed tab we would be reusing the RVH and just
making a new RenderView, the process would not be sent the ActivateExtension/Application
IPCs.

Move the process-specific initialization to ChromeRenderViewHostObserver::RenderViewHostInitialized,
which is called every time a new renderer process is created.

BUG=89607
TEST=no
R=creis@chromium.org,jam@chromium.org

Review URL: http://codereview.chromium.org/7612016

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

chrome/browser/chrome_content_browser_client.cc

@@ -45,14 +45,12 @@
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
-#include "chrome/common/extensions/extension_messages.h"
 #include "chrome/common/logging_chrome.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/common/url_constants.h"
 #include "content/browser/browser_url_handler.h"
 #include "content/browser/browsing_instance.h"
-#include "content/browser/child_process_security_policy.h"
 #include "content/browser/debugger/devtools_handler.h"
 #include "content/browser/plugin_process_host.h"
 #include "content/browser/renderer_host/browser_render_process_host.h"
@@ -63,7 +61,6 @@
 #include "content/browser/ssl/ssl_client_auth_handler.h"
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/browser/worker_host/worker_process_host.h"
-#include "content/common/bindings_policy.h"
 #include "content/common/desktop_notification_messages.h"
 #include "grit/ui_resources.h"
 #include "net/base/cookie_monster.h"
@@ -81,69 +78,6 @@
 
 namespace {
 
-void InitRenderViewHostForExtensions(RenderViewHost* render_view_host) {
-  // Note that due to GetEffectiveURL(), even hosted apps will have a
-  // chrome-extension:// URL for their site, so we can ignore that wrinkle here.
-  SiteInstance* site_instance = render_view_host->site_instance();
-  const GURL& site = site_instance->site();
-
-  if (!site.SchemeIs(chrome::kExtensionScheme))
-    return;
-
-  Profile* profile = Profile::FromBrowserContext(
-      site_instance->browsing_instance()->browser_context());
-  ExtensionService* service = profile->GetExtensionService();
-  if (!service)
-    return;
-
-  ExtensionProcessManager* process_manager =
-      profile->GetExtensionProcessManager();
-  CHECK(process_manager);
-
-  // This can happen if somebody typos a chrome-extension:// URL.
-  const Extension* extension = service->GetExtensionByURL(site);
-  if (!extension)
-    return;
-
-  site_instance->GetProcess()->mark_is_extension_process();
-
-  // Register the association between extension and SiteInstance with
-  // ExtensionProcessManager.
-  // TODO(creis): Use this to replace SetInstalledAppForRenderer below.
-  process_manager->RegisterExtensionSiteInstance(site_instance->id(),
-                                                 extension->id());
-
-  RenderProcessHost* process = render_view_host->process();
-
-  if (extension->is_app()) {
-    render_view_host->Send(
-        new ExtensionMsg_ActivateApplication(extension->id()));
-    // Record which, if any, installed app is associated with this process.
-    // TODO(aa): Totally lame to store this state in a global map in extension
-    // service. Can we get it from EPM instead?
-    service->SetInstalledAppForRenderer(process->id(), extension);
-  }
-
-  // Some extensions use chrome:// URLs.
-  Extension::Type type = extension->GetType();
-  if (type == Extension::TYPE_EXTENSION ||
-      type == Extension::TYPE_PACKAGED_APP) {
-    ChildProcessSecurityPolicy::GetInstance()->GrantScheme(
-        process->id(), chrome::kChromeUIScheme);
-  }
-
-  // Enable extension bindings for the renderer. Currently only extensions,
-  // packaged apps, and hosted component apps use extension bindings.
-  if (type == Extension::TYPE_EXTENSION ||
-      type == Extension::TYPE_USER_SCRIPT ||
-      type == Extension::TYPE_PACKAGED_APP ||
-      (type == Extension::TYPE_HOSTED_APP &&
-       extension->location() == Extension::COMPONENT)) {
-    render_view_host->Send(new ExtensionMsg_ActivateExtension(extension->id()));
-    render_view_host->AllowBindings(BindingsPolicy::EXTENSION);
-  }
-}
-
 // Handles rewriting Web UI URLs.
 static bool HandleWebUI(GURL* url, content::BrowserContext* browser_context) {
   if (!ChromeWebUIFactory::GetInstance()->UseWebUIForURL(browser_context, *url))
@@ -170,8 +104,6 @@ void ChromeContentBrowserClient::RenderViewHostCreated(
   new ChromeRenderViewHostObserver(render_view_host);
   new DevToolsHandler(render_view_host);
   new ExtensionMessageHandler(render_view_host);
-
-  InitRenderViewHostForExtensions(render_view_host);
 }
 
 void ChromeContentBrowserClient::BrowserRenderProcessHostCreated(

chrome/browser/extensions/app_process_apitest.cc

@@ -338,3 +338,33 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, OpenAppFromIframe) {
   EXPECT_TRUE(last_active_browser->GetTabContentsAt(0)->render_view_host()->
       process()->is_extension_process());
 }
+
+IN_PROC_BROWSER_TEST_F(AppApiTest, ReloadAppAfterCrash) {
+  host_resolver()->AddRule("*", "127.0.0.1");
+  ASSERT_TRUE(test_server()->Start());
+
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("app_process")));
+
+  GURL base_url = GetTestBaseURL("app_process");
+
+  // Load the app, chrome.app.isInstalled should be true.
+  ui_test_utils::NavigateToURL(browser(), base_url.Resolve("path1/empty.html"));
+  TabContents* contents = browser()->GetTabContentsAt(0);
+  EXPECT_TRUE(contents->render_view_host()->process()->is_extension_process());
+  bool is_installed = false;
+  ASSERT_TRUE(ui_test_utils::ExecuteJavaScriptAndExtractBool(
+      contents->render_view_host(), L"",
+      L"window.domAutomationController.send(chrome.app.isInstalled)",
+      &is_installed));
+  ASSERT_TRUE(is_installed);
+
+  // Crash the tab and reload it, chrome.app.isInstalled should still be true.
+  ui_test_utils::CrashTab(browser()->GetSelectedTabContents());
+  browser()->Reload(CURRENT_TAB);
+  ASSERT_TRUE(ui_test_utils::WaitForNavigationInCurrentTab(browser()));
+  ASSERT_TRUE(ui_test_utils::ExecuteJavaScriptAndExtractBool(
+      contents->render_view_host(), L"",
+      L"window.domAutomationController.send(chrome.app.isInstalled)",
+      &is_installed));
+  ASSERT_TRUE(is_installed);
+}

chrome/browser/extensions/isolated_app_apitest.cc

@@ -90,27 +90,36 @@ IN_PROC_BROWSER_TEST_F(IsolatedAppApiTest, MAYBE_CookieIsolation) {
   ASSERT_TRUE(!GetInstalledApp(tab3));
 
   // Check that each tab sees its own cookie.
-  ASSERT_TRUE(HasCookie(tab1, "app1=3"));
-  ASSERT_TRUE(HasCookie(tab2, "app2=4"));
-  ASSERT_TRUE(HasCookie(tab3, "normalPage=5"));
+  EXPECT_TRUE(HasCookie(tab1, "app1=3"));
+  EXPECT_TRUE(HasCookie(tab2, "app2=4"));
+  EXPECT_TRUE(HasCookie(tab3, "normalPage=5"));
 
   // Check that app1 tab cannot see the other cookies.
-  ASSERT_FALSE(HasCookie(tab1, "app2"));
-  ASSERT_FALSE(HasCookie(tab1, "normalPage"));
+  EXPECT_FALSE(HasCookie(tab1, "app2"));
+  EXPECT_FALSE(HasCookie(tab1, "normalPage"));
 
   // Check that app2 tab cannot see the other cookies.
-  ASSERT_FALSE(HasCookie(tab2, "app1"));
-  ASSERT_FALSE(HasCookie(tab2, "normalPage"));
+  EXPECT_FALSE(HasCookie(tab2, "app1"));
+  EXPECT_FALSE(HasCookie(tab2, "normalPage"));
 
   // Check that normal tab cannot see the other cookies.
-  ASSERT_FALSE(HasCookie(tab3, "app1"));
-  ASSERT_FALSE(HasCookie(tab3, "app2"));
+  EXPECT_FALSE(HasCookie(tab3, "app1"));
+  EXPECT_FALSE(HasCookie(tab3, "app2"));
 
   // Check that the non_app iframe cookie is associated with app1 and not the
   // normal tab.  (For now, iframes are always rendered in their parent
   // process, even if they aren't in the app manifest.)
-  ASSERT_TRUE(HasCookie(tab1, "nonAppFrame=6"));
-  ASSERT_FALSE(HasCookie(tab3, "nonAppFrame"));
+  EXPECT_TRUE(HasCookie(tab1, "nonAppFrame=6"));
+  EXPECT_FALSE(HasCookie(tab3, "nonAppFrame"));
+
+  // Check that isolation persists even if the tab crashes and is reloaded.
+  browser()->SelectNumberedTab(1);
+  ui_test_utils::CrashTab(tab1);
+  browser()->Reload(CURRENT_TAB);
+  ASSERT_TRUE(ui_test_utils::WaitForNavigationInCurrentTab(browser()));
+  EXPECT_TRUE(HasCookie(tab1, "app1=3"));
+  EXPECT_FALSE(HasCookie(tab1, "app2"));
+  EXPECT_FALSE(HasCookie(tab1, "normalPage"));
 }
 
 // Ensure that cookies are not isolated if the isolated apps are not installed.

chrome/browser/renderer_host/chrome_render_view_host_observer.cc

@@ -6,12 +6,19 @@
 
 #include "base/command_line.h"
 #include "chrome/browser/dom_operation_notification_details.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/net/predictor_api.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/extension_messages.h"
 #include "chrome/common/render_messages.h"
-#include "content/browser/renderer_host/render_view_host.h"
+#include "content/browser/browsing_instance.h"
+#include "content/browser/child_process_security_policy.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
-#include "chrome/common/chrome_notification_types.h"
+#include "content/browser/renderer_host/render_view_host.h"
+#include "content/browser/site_instance.h"
+#include "content/common/bindings_policy.h"
 #include "content/common/notification_service.h"
 #include "content/common/url_constants.h"
 #include "content/common/view_messages.h"
@@ -19,11 +26,16 @@
 ChromeRenderViewHostObserver::ChromeRenderViewHostObserver(
     RenderViewHost* render_view_host)
     : RenderViewHostObserver(render_view_host) {
+  InitRenderViewHostForExtensions();
 }
 
 ChromeRenderViewHostObserver::~ChromeRenderViewHostObserver() {
 }
 
+void ChromeRenderViewHostObserver::RenderViewHostInitialized() {
+  InitRenderViewForExtensions();
+}
+
 void ChromeRenderViewHostObserver::Navigate(
     const ViewMsg_Navigate_Params& params) {
   const GURL& url = params.url;
@@ -43,6 +55,103 @@ bool ChromeRenderViewHostObserver::OnMessageReceived(
   return handled;
 }
 
+void ChromeRenderViewHostObserver::InitRenderViewHostForExtensions() {
+  const Extension* extension = GetExtension();
+  if (!extension)
+    return;
+
+  SiteInstance* site_instance = render_view_host()->site_instance();
+  Profile* profile = Profile::FromBrowserContext(
+      site_instance->browsing_instance()->browser_context());
+  ExtensionProcessManager* process_manager =
+      profile->GetExtensionProcessManager();
+  CHECK(process_manager);
+
+  site_instance->GetProcess()->mark_is_extension_process();
+
+  // Register the association between extension and SiteInstance with
+  // ExtensionProcessManager.
+  // TODO(creis): Use this to replace SetInstalledAppForRenderer.
+  process_manager->RegisterExtensionSiteInstance(site_instance->id(),
+                                                 extension->id());
+
+  if (extension->is_app()) {
+    // Record which, if any, installed app is associated with this process.
+    // TODO(aa): Totally lame to store this state in a global map in extension
+    // service. Can we get it from EPM instead?
+    profile->GetExtensionService()->SetInstalledAppForRenderer(
+        render_view_host()->process()->id(), extension);
+  }
+
+  // Enable extension bindings for the renderer. Currently only extensions,
+  // packaged apps, and hosted component apps use extension bindings.
+  Extension::Type type = extension->GetType();
+  if (type == Extension::TYPE_EXTENSION ||
+      type == Extension::TYPE_USER_SCRIPT ||
+      type == Extension::TYPE_PACKAGED_APP ||
+      (type == Extension::TYPE_HOSTED_APP &&
+       extension->location() == Extension::COMPONENT)) {
+    render_view_host()->AllowBindings(BindingsPolicy::EXTENSION);
+  }
+}
+
+void ChromeRenderViewHostObserver::InitRenderViewForExtensions() {
+  const Extension* extension = GetExtension();
+  if (!extension)
+    return;
+
+  SiteInstance* site_instance = render_view_host()->site_instance();
+  Profile* profile = Profile::FromBrowserContext(
+      site_instance->browsing_instance()->browser_context());
+  RenderProcessHost* process = render_view_host()->process();
+
+  if (extension->is_app()) {
+    Send(new ExtensionMsg_ActivateApplication(extension->id()));
+    // Though we already record the associated process ID for the renderer in
+    // InitRenderViewHostForExtensions, the process might have crashed and been
+    // restarted (hence the re-initialization), so we need to update that
+    // mapping.
+    profile->GetExtensionService()->SetInstalledAppForRenderer(
+        process->id(), extension);
+  }
+
+  // Some extensions use chrome:// URLs.
+  Extension::Type type = extension->GetType();
+  if (type == Extension::TYPE_EXTENSION ||
+      type == Extension::TYPE_PACKAGED_APP) {
+    ChildProcessSecurityPolicy::GetInstance()->GrantScheme(
+        process->id(), chrome::kChromeUIScheme);
+  }
+
+  if (type == Extension::TYPE_EXTENSION ||
+      type == Extension::TYPE_USER_SCRIPT ||
+      type == Extension::TYPE_PACKAGED_APP ||
+      (type == Extension::TYPE_HOSTED_APP &&
+       extension->location() == Extension::COMPONENT)) {
+    Send(new ExtensionMsg_ActivateExtension(extension->id()));
+  }
+}
+
+const Extension* ChromeRenderViewHostObserver::GetExtension() {
+  // Note that due to ChromeContentBrowserClient::GetEffectiveURL(), even hosted
+  // apps will have a chrome-extension:// URL for their site, so we can ignore
+  // that wrinkle here.
+  SiteInstance* site_instance = render_view_host()->site_instance();
+  const GURL& site = site_instance->site();
+
+  if (!site.SchemeIs(chrome::kExtensionScheme))
+    return NULL;
+
+  Profile* profile = Profile::FromBrowserContext(
+      site_instance->browsing_instance()->browser_context());
+  ExtensionService* service = profile->GetExtensionService();
+  if (!service)
+    return NULL;
+
+  // May be null if somebody typos a chrome-extension:// URL.
+  return service->GetExtensionByURL(site);
+}
+
 void ChromeRenderViewHostObserver::OnDomOperationResponse(
     const std::string& json_string, int automation_id) {
   DomOperationNotificationDetails details(json_string, automation_id);

chrome/browser/renderer_host/chrome_render_view_host_observer.h

@@ -8,6 +8,8 @@
 
 #include "content/browser/renderer_host/render_view_host_observer.h"
 
+class Extension;
+
 // This class holds the Chrome specific parts of RenderViewHost, and has the
 // same lifetime.
 class ChromeRenderViewHostObserver : public RenderViewHostObserver {
@@ -16,10 +18,20 @@ class ChromeRenderViewHostObserver : public RenderViewHostObserver {
   virtual ~ChromeRenderViewHostObserver();
 
   // RenderViewHostObserver overrides.
+  virtual void RenderViewHostInitialized() OVERRIDE;
   virtual void Navigate(const ViewMsg_Navigate_Params& params) OVERRIDE;
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
 
  private:
+   // Does extension-specific initialization when a new RenderViewHost is
+   // created.
+  void InitRenderViewHostForExtensions();
+   // Does extension-specific initialization when a new renderer process is
+   // created by a RenderViewHost.
+  void InitRenderViewForExtensions();
+  // Gets the extension or app (if any) that is associated with the RVH.
+  const Extension* GetExtension();
+
   void OnDomOperationResponse(const std::string& json_string,
                               int automation_id);