Check for already existing entry when adding to SiteProcessMap.

chrome/browser/extensions/process_management_browsertest.cc

 // Copyright (c) 2012 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 <stddef.h>
 
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_process_policy.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/process_manager.h"
 #include "extensions/browser/process_map.h"
@@ skipping 354 matching lines @@
   // Verify that we really have the Chrome Web Store app loaded in the Web
   // Contents.
   content::RenderProcessHost* new_process_host =
       web_contents->GetMainFrame()->GetProcess();
   EXPECT_TRUE(extensions::ProcessMap::Get(profile())->Contains(
       extensions::kWebStoreAppId, new_process_host->GetID()));
 
   // Verify that Chrome Web Store is isolated in a separate renderer process.
   EXPECT_NE(old_process_host, new_process_host);
 }
+
+// This test verifies that blocked navigations to extensions pages do not
+// overwrite process-per-site map inside content/.
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest,
+                       NavigateToBlockedExtensionPageInNewTab) {
+  host_resolver()->AddRule("*", "127.0.0.1");
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Load an extension, which will block a request for a specifig page in it.

[Devlin, 2017/03/11 01:52:17]

*specific

[nasko, 2017/03/13 17:22:03]

Done.

+  const extensions::Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("web_request_blocking"));
+  ASSERT_TRUE(extension);
+
+  WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  WebContents* new_web_contents = nullptr;
+  GURL blocked_url(extension->GetResourceURL("/blocked.html"));
+
+  // Navigating to the blocked extension URL should be done through a redirect,
+  // otherwise it will result in an OpenURL IPC from the renderer process, which
+  // will initiate a navigation through the browser process.
+  GURL redirect_url(
+      embedded_test_server()->GetURL("/server-redirect?" + blocked_url.spec()));
+
+  // Navigate the current tab to the test page in the extension, which will
+  // create the extension process and register the webRequest blocking listener.
+  {
+    content::TestNavigationObserver observer(web_contents);

[Devlin, 2017/03/11 01:52:17]

Doesn't NavigateToURL by default block until navigation is complete?

Also, from my (more limited) investigation, it seemed like if the extension had
a background page, this wouldn't have failed (because the site instance would
have been created).  It might be good to mention that somewhere, since normally
we'd have this handled by the background page.

[nasko, 2017/03/13 17:22:03]

Good point :). I had so many iterations of this code that required an observer
that forgot to remove it at the end.
 
I don't know if background pages are accounted differently, but SiteInstance is
already created by this navigation. I can experiment with background pages as a
follow up.

+    ui_test_utils::NavigateToURL(browser(),
+                                 extension->GetResourceURL("/test.html"));
+    observer.Wait();
+  }
+
+  // Open a new tab to about:blank, which will result in a new SiteInstance
+  // without an explicit site set.
+  {
+    content::WindowedNotificationObserver tab_added_observer(
+        chrome::NOTIFICATION_TAB_ADDED,
+        content::NotificationService::AllSources());
+    EXPECT_TRUE(content::ExecuteScript(
+        web_contents, "chrome.tabs.create({url: 'about:blank'});"));

[Devlin, 2017/03/11 01:52:18]

Does this have to be done through the extension API?  It might be clearer to
just use e.g. NavigateToURL() with disposition of
NEW_[FOREGROUND|BACKGROUND]_TAB.

[nasko, 2017/03/13 17:22:03]

I was trying to mimic as closely what I observed. It is a good point that these
should be equivalent.

+    tab_added_observer.Wait();
+    new_web_contents = browser()->tab_strip_model()->GetActiveWebContents();
+
+    // If the navigation hasn't committed yet, the test needs to wait for it.
+    if (new_web_contents->GetLastCommittedURL() != GURL(url::kAboutBlankURL)) {

[Devlin, 2017/03/11 01:52:17]

The above might also solve this, since NavigateToURL should block until
complete.

[nasko, 2017/03/13 17:22:03]

Done.

+      content::TestNavigationObserver observer(new_web_contents);
+      observer.Wait();
+    }
+  }
+
+  // Navigate the new tab to an extension URL that will be blocked by
+  // webRequest.
+  {
+    std::string script = base::StringPrintf("location.href = '%s';",
+                                            redirect_url.spec().c_str());
+    content::TestNavigationObserver observer(new_web_contents);
+    EXPECT_TRUE(content::ExecuteScript(new_web_contents, script));

[Devlin, 2017/03/11 01:52:17]

Same question - does this need to be through ExecuteScript?  If so (possible),
why? (extend comments)  If not, prefer NavigateToURL().

[nasko, 2017/03/13 17:22:03]

This needs to be a renderer initiated navigation and NavigateToURL is
browser-initiated. I have a comment about it on the redirect_url declaration,
but I can add a bit more here too.

+    observer.Wait();
+
+    EXPECT_EQ(observer.last_navigation_url(), blocked_url);
+    EXPECT_FALSE(observer.last_navigation_succeeded());
+  }
+
+  // Very subtle check for content/ internal functionality :(.
+  // When a navigation is blocked, it still commits an error page. Since
+  // extensions use the process-per-site model, each extension URL is registered

[Devlin, 2017/03/11 01:52:17]

nit: process-per-extension?  process-per-origin?  Does "site" have a defined
meaning?

[nasko, 2017/03/13 17:22:03]

Our codebase and docs already talk about process-per-site process model, so I'd
rather use that instead of inventing new name for it. 

https://www.chromium.org/developers/design-documents/process-models
https://cs.chromium.org/chromium/src/content/public/browser/site_instance.h?r...

+  // in a map from URL to a process. Creating a brand new SiteInstance for the
+  // extension URL should always result in a SiteInstance that has a process and
+  // the process is the same for all SiteInstances.
+  scoped_refptr<content::SiteInstance> new_site_instance =
+      content::SiteInstance::CreateForURL(web_contents->GetBrowserContext(),
+                                          extension->GetResourceURL(""));

[Devlin, 2017/03/11 01:52:17]

nit: extension->url()

[nasko, 2017/03/13 17:22:03]

Done.

+  EXPECT_TRUE(new_site_instance->HasProcess());
+  EXPECT_EQ(new_site_instance->GetProcess(),
+            web_contents->GetSiteInstance()->GetProcess());
+}