Fix process selection for chrome.tabs.update(... about:blank ...).

chrome.tabs.update is treated as renderer-initiated to address
omnibox/url spoofing problems (see r698617).  All renderer-initiated
navigations need to have a non-null |initiator_origin| - the extension
origin is used in case of chrome.tabs.update.

The problem fixed by this CL, is that without explicitly specifying
LoadURLParams::source_site_instance, the old SiteInstance of the
navigated web contents might be used.  In particular, when navigating
a tab with https://example.com to about:blank, reusing the SiteInstance
of https://example.com means that about:blank (with an origin associated
with the extension) would commit in a process locked to
https://example.com - this is undesirable from Site Isolation
perspective.

The CL fixes the problem above, by making sure that chrome.tabs.update
populates LoadURLParams::source_site_instance in a way consistent
with 1) initiator_origin and 2) the BrowserContext of the target tab.

Note that when discussing this CL, we've explicitly decided against
setting the |initiator_origin| to the (old/current) origin of the target
tab.  This is because such initiator would prevent the extension from
being able to use chrome.tabs.update to navigate to
non-web-accessible-resources owned by the extension.  Additionally,
using the origin of the target tab is subject to races with other
navigations of the tab that might be happening in parallel.

Note that this CL does *not* change the fact that using
chrome.tabs.update to navigate a non-extension tab to about:blank will
commit an *opaque* origin (with precursor set to the extension origin),
rather than committing about:blank with the (non-opaque) extension origin.
This aspect of behavior is not changed because:
1. There are no compelling reasons to make a change at this point.
2. Changing this behavior is possible when chrome.tabs.update is called
   from an extension frame in the same BrowserContext as the target tab
   (in this case |source_site_instance| can be set to the SiteInstance
   of the extension frame calling chrome.tabs.update).  Unfortunately
   sometimes there is no extension frame (when chrome.tabs.update is
   called from an extension service worker) and sometimes the target
   tab is in a different BrowserContext (e.g. in an Incognito profile).

Relevant tests:
- (new) ExtensionApiTest.TabsUpdate_WebToAboutBlank
- (new) ExtensionApiTest.TabsUpdate_WebToNonWAR
- IncognitoApiTest.Incognito
- ServiceWorkerBasedBackgroundTest.FilteredEvents
- ServiceWorkerBasedBackgroundTest.TabsEvents

Change-Id: I31b298d9a118de45ba4a255d2384d82f029c8b7a
Bug: 991607
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1850355
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708283}

chrome/browser/extensions/api/tabs/tabs_api.cc

@@ -1362,8 +1362,14 @@ bool TabsUpdateFunction::UpdateURL(const std::string& url_string,
   // does not show in the omnibox until it commits.  This avoids URL spoofs
   // since URLs can be opened on behalf of untrusted content.
   load_params.is_renderer_initiated = true;
+  // All renderer-initiated navigations need to have an initiator origin.
   load_params.initiator_origin = url::Origin::Create(
       Extension::GetBaseURLFromExtensionId(extension()->id()));
+  // |source_site_instance| needs to be set so that a renderer process
+  // compatible with |initiator_origin| is picked by Site Isolation.
+  load_params.source_site_instance = content::SiteInstance::CreateForURL(
+      web_contents_->GetBrowserContext(),
+      load_params.initiator_origin->GetURL());
 
   web_contents_->GetController().LoadURLWithParams(load_params);
 

chrome/browser/extensions/api/tabs/tabs_test.cc

@@ -2318,4 +2318,138 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowsCreate_OpenerAndOrigin) {
   }
 }
 
+// Tests updating a URL of a web tab to an about:blank.  Verify that the new
+// frame is placed in the correct process, has the correct origin and that no
+// DCHECKs are hit anywhere.
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsUpdate_WebToAboutBlank) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const extensions::Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("../simple_with_file"));
+  ASSERT_TRUE(extension);
+  GURL extension_url = extension->GetResourceURL("file.html");
+  url::Origin extension_origin = url::Origin::Create(extension_url);
+  GURL web_url = embedded_test_server()->GetURL("/title1.html");
+  url::Origin web_origin = url::Origin::Create(web_url);
+  GURL about_blank_url = GURL(url::kAboutBlankURL);
+
+  // Navigate a tab to an extension page.
+  ui_test_utils::NavigateToURL(browser(), extension_url);
+  content::WebContents* extension_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(extension_origin,
+            extension_contents->GetMainFrame()->GetLastCommittedOrigin());
+
+  // Create another tab and navigate it to a web page.
+  content::WebContents* test_contents = nullptr;
+  {
+    content::WebContentsAddedObserver test_contents_observer;
+    content::TestNavigationObserver nav_observer(nullptr);
+    nav_observer.StartWatchingNewWebContents();
+    content::ExecuteScriptAsync(
+        extension_contents,
+        content::JsReplace("window.open($1, '_blank')", web_url));
+
+    test_contents = test_contents_observer.GetWebContents();
+    nav_observer.WaitForNavigationFinished();
+    EXPECT_TRUE(nav_observer.last_navigation_succeeded());
+    EXPECT_EQ(web_url, nav_observer.last_navigation_url());
+  }
+  EXPECT_EQ(web_origin,
+            test_contents->GetMainFrame()->GetLastCommittedOrigin());
+  EXPECT_NE(extension_contents->GetMainFrame()->GetProcess(),
+            test_contents->GetMainFrame()->GetProcess());
+
+  // Use |chrome.tabs.update| API to navigate |test_contents| to an about:blank
+  // URL.
+  {
+    content::TestNavigationObserver nav_observer(test_contents, 1);
+    int test_tab_id = ExtensionTabUtil::GetTabId(test_contents);
+    content::ExecuteScriptAsync(
+        extension_contents,
+        content::JsReplace("chrome.tabs.update($1, { url: $2 })", test_tab_id,
+                           about_blank_url));
+    nav_observer.WaitForNavigationFinished();
+    EXPECT_TRUE(nav_observer.last_navigation_succeeded());
+    EXPECT_EQ(about_blank_url, nav_observer.last_navigation_url());
+  }
+
+  // Verify the origin and process of the about:blank tab.
+  content::RenderFrameHost* test_frame = test_contents->GetMainFrame();
+  EXPECT_EQ(about_blank_url, test_frame->GetLastCommittedURL());
+  EXPECT_EQ(extension_contents->GetMainFrame()->GetProcess(),
+            test_contents->GetMainFrame()->GetProcess());
+
+  // The expectations below preserve the behavior at r704251.  It is not clear
+  // whether these are the right expectations - maybe about:blank should commit
+  // with an extension origin?  OTOH, committing with the extension origin
+  // wouldn't be possible when targeting an incognito window (see also
+  // IncognitoApiTest.Incognito test).
+  EXPECT_TRUE(test_frame->GetLastCommittedOrigin().opaque());
+  EXPECT_EQ(
+      extension_origin.GetTupleOrPrecursorTupleIfOpaque(),
+      test_frame->GetLastCommittedOrigin().GetTupleOrPrecursorTupleIfOpaque());
+}
+
+// Tests updating a URL of a web tab to a non-web-accessible-resource of an
+// extension - such navigation should be allowed.
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsUpdate_WebToNonWAR) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const extensions::Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("../simple_with_file"));
+  ASSERT_TRUE(extension);
+  GURL extension_url = extension->GetResourceURL("file.html");
+  url::Origin extension_origin = url::Origin::Create(extension_url);
+  GURL web_url = embedded_test_server()->GetURL("/title1.html");
+  url::Origin web_origin = url::Origin::Create(web_url);
+  GURL non_war_url = extension_url;
+
+  // Navigate a tab to an extension page.
+  ui_test_utils::NavigateToURL(browser(), extension_url);
+  content::WebContents* extension_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(extension_origin,
+            extension_contents->GetMainFrame()->GetLastCommittedOrigin());
+
+  // Create another tab and navigate it to a web page.
+  content::WebContents* test_contents = nullptr;
+  {
+    content::WebContentsAddedObserver test_contents_observer;
+    content::TestNavigationObserver nav_observer(nullptr);
+    nav_observer.StartWatchingNewWebContents();
+    content::ExecuteScriptAsync(
+        extension_contents,
+        content::JsReplace("window.open($1, '_blank')", web_url));
+    test_contents = test_contents_observer.GetWebContents();
+    nav_observer.WaitForNavigationFinished();
+
+    EXPECT_TRUE(nav_observer.last_navigation_succeeded());
+    EXPECT_EQ(web_url, nav_observer.last_navigation_url());
+  }
+  EXPECT_EQ(web_origin,
+            test_contents->GetMainFrame()->GetLastCommittedOrigin());
+  EXPECT_NE(extension_contents->GetMainFrame()->GetProcess(),
+            test_contents->GetMainFrame()->GetProcess());
+
+  // Use |chrome.tabs.update| API to navigate |test_contents| to a
+  // non-web-accessible-resource of an extension.
+  {
+    content::TestNavigationObserver nav_observer(test_contents, 1);
+    int test_tab_id = ExtensionTabUtil::GetTabId(test_contents);
+    content::ExecuteScriptAsync(
+        extension_contents,
+        content::JsReplace("chrome.tabs.update($1, { url: $2 })", test_tab_id,
+                           non_war_url));
+    nav_observer.WaitForNavigationFinished();
+    EXPECT_TRUE(nav_observer.last_navigation_succeeded());
+    EXPECT_EQ(non_war_url, nav_observer.last_navigation_url());
+  }
+
+  // Verify the origin and process of the navigated tab.
+  content::RenderFrameHost* test_frame = test_contents->GetMainFrame();
+  EXPECT_EQ(non_war_url, test_frame->GetLastCommittedURL());
+  EXPECT_EQ(extension_origin, test_frame->GetLastCommittedOrigin());
+  EXPECT_EQ(extension_contents->GetMainFrame()->GetProcess(),
+            test_contents->GetMainFrame()->GetProcess());
+}
+
 }  // namespace extensions

content/browser/child_process_security_policy_impl.cc

@@ -1253,14 +1253,6 @@ CanCommitStatus ChildProcessSecurityPolicyImpl::CanCommitOriginAndUrl(
     if (actual_origin_lock == expected_origin_lock)
       return CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL;
 
-    // Allow about: pages to commit in a process that does not match the opaque
-    // origin's precursor information.
-    // TODO(acolwell): Remove this once process selection for about: URLs has
-    // been fixed to always match the precursor info.
-    if (url_origin.opaque() && (url.IsAboutBlank() || url.IsAboutSrcdoc()) &&
-        !actual_origin_lock.is_empty()) {
-      return CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL;
-    }
     return CanCommitStatus::CANNOT_COMMIT_URL;
   }