Propagate creator frame's loader_factories_ earlier.

`loader_factories_` from the creator's RenderFrameImpl need to be
propagated into the created child/popup frame.  This is important to
ensure that subresource loads work in the initial empty document - e.g.
in one created via |window.open('', '_blank')|.  See also the earlier
r792963 and https://crbug.com/1106995.

Before this CL, this propagation was taking place in
RenderFrameImpl::DidCommitNavigation and was broken if `window.opener`
was already cleared at this point (as shown by the new test added in
this CL).

After this CL, this propagation is taking place earlier - right after
creating the new frame (by making sure the new
InheritLoaderFactoriesFrom is called from RenderViewImpl::CreateView and
RenderFrameImpl::CreateChildFrame).  This fixes the new test.

This CL can remove the GetLoaderFactoryBundleFromCreator method, because
after the CL all the creator->new_frame propagation scenarios are
covered by the calls to the new InheritLoaderFactoriesFrom method.

Bug: 1191203
Change-Id: I35ad4b95ac7170d839ad2138e60ab742e783ab6b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2783353
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#868510}

content/browser/navigation_browsertest.cc

@@ -9,6 +9,7 @@
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/guid.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
@@ -4557,73 +4558,105 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
   console_observer.Wait();
 }
 
-namespace {
+class SubresourceLoadingTest : public NavigationBrowserTest {
+ public:
+  SubresourceLoadingTest() = default;
+  SubresourceLoadingTest(const SubresourceLoadingTest&) = delete;
+  SubresourceLoadingTest& operator=(const SubresourceLoadingTest&) = delete;
 
-void VerifyImageSubresourceLoad(
-    const ToRenderFrameHost& target,
-    const GURL& image_url,
-    const std::string& target_document = "document") {
-  const char kScriptTemplate[] = R"(
-      new Promise(resolve => {
-          let img = document.createElement('img');
-          img.src = $1;  // `$1` will be replaced with the value of `image_url`.
-          img.addEventListener('load', () => {
-              resolve('allowed');
-          });
-          img.addEventListener('error', () => {
-              resolve('blocked');
-          });
+  void DontTestNetworkServiceCrashes() {
+    test_network_service_crashes_ = false;
+  }
 
-          // `%%s` will be replaced with the value of `target_document`.
-          %s.body.appendChild(img);
-      }); )";
-  std::string script = base::StringPrintf(
-      JsReplace(kScriptTemplate, image_url).c_str(), target_document.c_str());
-  EXPECT_EQ("allowed", EvalJs(target, script));
-}
+  void VerifyResultsOfAboutBlankNavigation(RenderFrameHost* target_frame,
+                                           RenderFrameHost* initiator_frame) {
+    // Verify that `target_frame` has been navigated to "about:blank".
+    EXPECT_EQ(GURL(url::kAboutBlankURL), target_frame->GetLastCommittedURL());
 
-void VerifyResultsOfAboutBlankNavigation(RenderFrameHostImpl* target_frame,
-                                         RenderFrameHostImpl* initiator_frame,
-                                         const GURL& image_url) {
-  // Verify that `target_frame` has been navigated to "about:blank".
-  EXPECT_EQ(GURL(url::kAboutBlankURL), target_frame->GetLastCommittedURL());
+    // Verify that "about:blank" committed with the expected origin, and in the
+    // expected SiteInstance.
+    EXPECT_EQ(target_frame->GetLastCommittedOrigin(),
+              initiator_frame->GetLastCommittedOrigin());
+    EXPECT_EQ(target_frame->GetSiteInstance(),
+              initiator_frame->GetSiteInstance());
 
-  // Verify that "about:blank" committed with the expected origin, and in the
-  // expected SiteInstance.
-  EXPECT_EQ(target_frame->GetLastCommittedOrigin(),
-            initiator_frame->GetLastCommittedOrigin());
-  EXPECT_EQ(target_frame->GetSiteInstance(),
-            initiator_frame->GetSiteInstance());
+    // Start monitoring NetworkService for crashes.
+    //
+    // TODO(https://crbug.com/1169431): This should be part of BrowserTestBase.
+    // (with optional opt-out for things like NetworkServiceRestartBrowserTest).
+    bool did_network_service_crash = false;
+    base::CallbackListSubscription crash_monitoring_subscription =
+        RegisterNetworkServiceCrashHandler(base::BindLambdaForTesting(
+            [&]() { did_network_service_crash = true; }));
+    // Ask for cookies in the `target_frame`.  One implicit verification here
+    // is whether this step will hit any `cookie_url`-related NOTREACHED or DwoC
+    // in RestrictedCookieManager::ValidateAccessToCookiesAt.  This verification
+    // is non-racey, because `document.cookie` must have heard back from the
+    // RestrictedCookieManager before returning the value of cookies (this
+    // ignores possible Blink-side caching, but this is the first time the
+    // renderer needs the cookies and so this is okay for this test).
+    EXPECT_EQ("", EvalJs(target_frame, "document.cookie"));
+    // |network_context| might receive an error notification, but it's not
+    // guaranteed to have arrived at this point. Flush the remote to make sure
+    // the notification has been received.
+    //
+    // We flush via `initiator_frame`, because in the current set of tests, the
+    // `initiator_frame` always has a mojo connection to the NetworkService via
+    // the `network_service_disconnect_handler_holder_mojo` field of
+    // RenderFrameHostImpl.  (This is not true for the `target_frame` in tests
+    // where that frame uses the process-wide URLLoaderFactory fallback rather
+    // than creating a URLLoaderFactory via RenderFrameHostImpl.)
+    //
+    // TODO(https://crbug.com/1169431): This should be part of BrowserTestBase.
+    if (!IsInProcessNetworkService())
+      initiator_frame->FlushNetworkAndNavigationInterfacesForTesting();
+    EXPECT_FALSE(did_network_service_crash);
 
-  // Start monitoring NetworkService for crashes.
-  //
-  // TODO(https://crbug.com/1169431): This should be part of BrowserTestBase.
-  // (with optional opt-out for things like NetworkServiceRestartBrowserTest).
-  bool did_network_service_crash = false;
-  base::CallbackListSubscription crash_monitoring_subscription =
-      RegisterNetworkServiceCrashHandler(base::BindLambdaForTesting(
-          [&]() { did_network_service_crash = true; }));
-  // Ask for cookies in the `target_frame`.  One implicit verification here
-  // is whether this step will hit any `cookie_url`-related NOTREACHED or DwoC
-  // in RestrictedCookieManager::ValidateAccessToCookiesAt.  This verification
-  // is non-racey, because `document.cookie` must have heard back from the
-  // RestrictedCookieManager before returning the value of cookies (this ignores
-  // possible Blink-side caching, but this is the first time the renderer needs
-  // the cookies and so this is okay for this test).
-  EXPECT_EQ("", EvalJs(target_frame, "document.cookie"));
-  // |network_context| might receive an error notification, but it's not
-  // guaranteed to have arrived at this point. Flush the remote to make sure
-  // the notification has been received.
-  // TODO(https://crbug.com/1169431): This should be part of BrowserTestBase.
-  if (!IsInProcessNetworkService())
-    target_frame->FlushNetworkAndNavigationInterfacesForTesting();
-  EXPECT_FALSE(did_network_service_crash);
+    // Verify that the "about:blank" frame is able to load an image.
+    VerifyImageSubresourceLoads(target_frame);
+  }
 
-  // Verify that the "about:blank" frame is able to load an image.
-  VerifyImageSubresourceLoad(target_frame, image_url);
-}
+  void VerifyImageSubresourceLoads(
+      const ToRenderFrameHost& target,
+      const std::string& target_document = "document") {
+    VerifySingleImageSubresourceLoad(target, target_document);
 
-}  // namespace
+    // Verify detecting and recovering from a NetworkService crash (e.g. via the
+    // `network_service_disconnect_handler_holder_mojo` field and the
+    // UpdateSubresourceLoaderFactories method of RenderFrameHostImpl).
+    if (!IsInProcessNetworkService() && test_network_service_crashes_) {
+      SimulateNetworkServiceCrash();
+      VerifySingleImageSubresourceLoad(target, target_document);
+    }
+  }
+
+ private:
+  void VerifySingleImageSubresourceLoad(const ToRenderFrameHost& target,
+                                        const std::string& target_document) {
+    // Use a random, GUID-based hostname, to avoid hitting the network cache.
+    GURL image_url = embedded_test_server()->GetURL(
+        base::GenerateGUID() + ".com", "/blank.jpg");
+    const char kScriptTemplate[] = R"(
+        new Promise(resolve => {
+            let img = document.createElement('img');
+            img.src = $1;  // `$1` is replaced with the value of `image_url`.
+            img.addEventListener('load', () => {
+                resolve('allowed');
+            });
+            img.addEventListener('error', () => {
+                resolve('blocked');
+            });
+
+            // `%%s` is replaced with the value of `target_document`.
+            %s.body.appendChild(img);
+        }); )";
+    std::string script = base::StringPrintf(
+        JsReplace(kScriptTemplate, image_url).c_str(), target_document.c_str());
+    EXPECT_EQ("allowed", EvalJs(target, script));
+  }
+
+  bool test_network_service_crashes_ = true;
+};
 
 // The test below verifies that an "about:blank" navigation commits with the
 // right origin, even when the initiator of the navigation is not the parent or
@@ -4635,7 +4668,7 @@ void VerifyResultsOfAboutBlankNavigation(RenderFrameHostImpl* target_frame,
 // frame is a local frame (even in presence of site-per-process).  See also
 // GrandchildToAboutBlank_ABA_CrossSite and
 // GrandchildToAboutBlank_ABB_CrossSite.
-IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
+IN_PROC_BROWSER_TEST_F(SubresourceLoadingTest,
                        GrandchildToAboutBlank_ABA_SameSite) {
   GURL url(embedded_test_server()->GetURL(
       "a.example.com",
@@ -4671,8 +4704,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
       shell()->web_contents()->GetMainFrame());
   child_frame = main_frame->child_at(0)->current_frame_host();
   grandchild_frame = child_frame->child_at(0)->current_frame_host();
-  GURL image_url = embedded_test_server()->GetURL("b.com", "/blank.jpg");
-  VerifyResultsOfAboutBlankNavigation(grandchild_frame, main_frame, image_url);
+  VerifyResultsOfAboutBlankNavigation(grandchild_frame, main_frame);
 }
 
 // The test below verifies that an "about:blank" navigation commits with the
@@ -4684,7 +4716,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
 // In this test case there are no process swaps and the parent of the navigated
 // frame is a remote frame (in presence of site-per-process).  See also
 // GrandchildToAboutBlank_ABA_SameSite and GrandchildToAboutBlank_ABB_CrossSite.
-IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
+IN_PROC_BROWSER_TEST_F(SubresourceLoadingTest,
                        GrandchildToAboutBlank_ABA_CrossSite) {
   GURL url(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(b(a))"));
@@ -4722,8 +4754,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
       shell()->web_contents()->GetMainFrame());
   child_frame = main_frame->child_at(0)->current_frame_host();
   grandchild_frame = child_frame->child_at(0)->current_frame_host();
-  GURL image_url = embedded_test_server()->GetURL("b.com", "/blank.jpg");
-  VerifyResultsOfAboutBlankNavigation(grandchild_frame, main_frame, image_url);
+  VerifyResultsOfAboutBlankNavigation(grandchild_frame, main_frame);
 }
 
 // The test below verifies that an "about:blank" navigation commits with the
@@ -4735,7 +4766,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
 // In this test case the navigation forces a process swap of the target frame.
 // See also GrandchildToAboutBlank_ABA_SameSite and
 // GrandchildToAboutBlank_ABA_CrossSite.
-IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
+IN_PROC_BROWSER_TEST_F(SubresourceLoadingTest,
                        GrandchildToAboutBlank_ABB_CrossSite) {
   GURL url(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(b(b))"));
@@ -4774,8 +4805,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
       shell()->web_contents()->GetMainFrame());
   child_frame = main_frame->child_at(0)->current_frame_host();
   grandchild_frame = child_frame->child_at(0)->current_frame_host();
-  GURL image_url = embedded_test_server()->GetURL("b.com", "/blank.jpg");
-  VerifyResultsOfAboutBlankNavigation(grandchild_frame, main_frame, image_url);
+  VerifyResultsOfAboutBlankNavigation(grandchild_frame, main_frame);
 }
 
 // The test below verifies that an "about:blank" navigation commits with the
@@ -4783,7 +4813,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
 // opener of the frame targeted by the navigation.  In the
 // TopToAboutBlank_CrossSite testcase, the top-level navigation is initiated by
 // a cross-site subframe.
-IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, TopToAboutBlank_CrossSite) {
+IN_PROC_BROWSER_TEST_F(SubresourceLoadingTest, TopToAboutBlank_CrossSite) {
   GURL url(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(b)"));
   EXPECT_TRUE(NavigateToURL(shell(), url));
@@ -4832,7 +4862,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, TopToAboutBlank_CrossSite) {
 // SameSiteSiblingToAboutBlank_CrossSiteTop testcase, the navigation is
 // initiated by a same-origin sibling (notably, not by one of target frame's
 // ancestors) and both siblings are subframes of a cross-site main frame.
-IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
+IN_PROC_BROWSER_TEST_F(SubresourceLoadingTest,
                        SameSiteSiblingToAboutBlank_CrossSiteTop) {
   GURL url(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(b,b)"));
@@ -4863,14 +4893,109 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
       shell()->web_contents()->GetMainFrame());
   child_frame1 = main_frame->child_at(0)->current_frame_host();
   child_frame2 = main_frame->child_at(1)->current_frame_host();
-  GURL image_url = embedded_test_server()->GetURL("b.com", "/blank.jpg");
-  VerifyResultsOfAboutBlankNavigation(child_frame2, child_frame1, image_url);
+  VerifyResultsOfAboutBlankNavigation(child_frame2, child_frame1);
+}
+
+// The test below verifies that an initial empty document has a functional
+// URLLoaderFactory.  Note that some aspects of the current behavior (e.g. the
+// synchronous re-navigation) are not spec-compliant - see
+// https://crbug.com/778318 and https://github.com/whatwg/html/issues/3267.
+// Note that the same behavior is expected in the ...NewFrameWithoutSrc and
+// in the ...NewFrameWithAboutBlank testcases.
+IN_PROC_BROWSER_TEST_F(SubresourceLoadingTest,
+                       URLLoaderFactoryInInitialEmptyDoc_NewFrameWithoutSrc) {
+  GURL opener_url(embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), opener_url));
+
+  // This inserts an `iframe` element without an `src` attribute.  According to
+  // some specs "the browsing context will remain at the initial about:blank
+  // page", although other specs suggest that there is an explicit, separate
+  // navigation.  See:
+  // https://html.spec.whatwg.org/dev/iframe-embed-object.html#the-iframe-element
+  // https://html.spec.whatwg.org/multipage/iframe-embed-object.html#shared-attribute-processing-steps-for-iframe-and-frame-elements
+  ASSERT_TRUE(ExecJs(shell(), R"( let ifr = document.createElement('iframe');
+                                  document.body.appendChild(ifr); )"));
+  WaitForLoadStop(shell()->web_contents());
+  RenderFrameHost* main_frame = shell()->web_contents()->GetMainFrame();
+  RenderFrameHost* subframe = ChildFrameAt(main_frame, 0);
+
+  VerifyResultsOfAboutBlankNavigation(subframe, main_frame);
+}
+
+// See the doc comment for the
+// URLLoaderFactoryInInitialEmptyDoc_NewFrameWithoutSrc test case.
+IN_PROC_BROWSER_TEST_F(
+    SubresourceLoadingTest,
+    URLLoaderFactoryInInitialEmptyDoc_NewFrameWithAboutBlank) {
+  GURL opener_url(embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), opener_url));
+
+  ASSERT_TRUE(ExecJs(shell(), R"( ifr = document.createElement('iframe');
+                                  ifr.src = 'about:blank';
+                                  document.body.appendChild(ifr); )"));
+  WaitForLoadStop(shell()->web_contents());
+  RenderFrameHost* main_frame = shell()->web_contents()->GetMainFrame();
+  RenderFrameHost* subframe = ChildFrameAt(main_frame, 0);
+
+  VerifyResultsOfAboutBlankNavigation(subframe, main_frame);
+}
+
+// The test below verifies that an initial empty document has a functional
+// URLLoaderFactory.  Note that the same behavior is expected in the
+// ...NewPopupToEmptyUrl and in the ...NewPopupToAboutBlank testcases - the
+// differences in test expectations (around `GetController().GetEntryCount()`)
+// are unexpected and would need to be fixed as part of https://crbug.com/524208
+// (or maybe more broadly https://crbug.com/778318 and/or
+// https://github.com/whatwg/html/issues/3267).
+IN_PROC_BROWSER_TEST_F(SubresourceLoadingTest,
+                       URLLoaderFactoryInInitialEmptyDoc_NewPopupToEmptyUrl) {
+  GURL opener_url(embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), opener_url));
+
+  content::WebContents* popup = nullptr;
+  {
+    WebContentsAddedObserver popup_observer;
+    ASSERT_TRUE(ExecJs(shell(), "window.open('', '_blank')"));
+    popup = popup_observer.GetWebContents();
+  }
+  WaitForLoadStop(popup);
+
+  // Verify that we are at the initial empty document.
+  EXPECT_EQ(0, popup->GetController().GetEntryCount());
+
+  // Verify that the `popup` is at "about:blank", with expected origin, with
+  // working `document.cookie`, and with working subresource loads.
+  VerifyResultsOfAboutBlankNavigation(popup->GetMainFrame(),
+                                      shell()->web_contents()->GetMainFrame());
+}
+
+// See the doc comment for the
+// URLLoaderFactoryInInitialEmptyDoc_NewPopupToEmptyUrl test case.
+IN_PROC_BROWSER_TEST_F(SubresourceLoadingTest,
+                       URLLoaderFactoryInInitialEmptyDoc_NewPopupToAboutBlank) {
+  GURL opener_url(embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), opener_url));
+
+  content::WebContents* popup = nullptr;
+  {
+    WebContentsAddedObserver popup_observer;
+    ASSERT_TRUE(ExecJs(shell(), "window.open('about:blank', '_blank')"));
+    popup = popup_observer.GetWebContents();
+  }
+  WaitForLoadStop(popup);
+
+  // Verify that we are not at the initial empty document anymore.
+  EXPECT_EQ(1, popup->GetController().GetEntryCount());
+
+  // Verify other about:blank things.
+  VerifyResultsOfAboutBlankNavigation(popup->GetMainFrame(),
+                                      shell()->web_contents()->GetMainFrame());
 }
 
 // The test below verifies that an initial empty document has a functional
 // URLLoaderFactory.
 IN_PROC_BROWSER_TEST_F(
-    NavigationBrowserTest,
+    SubresourceLoadingTest,
     URLLoaderFactoryInInitialEmptyDoc_LongNavigationInSubframe) {
   ASSERT_TRUE(NavigateToURL(
       shell(), embedded_test_server()->GetURL("a.com", "/title1.html")));
@@ -4884,6 +5009,14 @@ IN_PROC_BROWSER_TEST_F(
                                    document.body.appendChild(ifr); )",
                                 hung_url)));
 
+  // No process swaps are expected before ReadyToCommit (which will never happen
+  // for a navigation to "/hung").  This test assertion double-checks that the
+  // test will cover inheriting URLLoaderFactory from the creator/opener/parent
+  // frame.
+  RenderFrameHost* main_frame = shell()->web_contents()->GetMainFrame();
+  RenderFrameHost* subframe = ChildFrameAt(main_frame, 0);
+  EXPECT_EQ(main_frame->GetProcess()->GetID(), subframe->GetProcess()->GetID());
+
   // Ask the parent to script the same-origin subframe and trigger some HTTP
   // subresource loads within the subframe.
   //
@@ -4891,14 +5024,13 @@ IN_PROC_BROWSER_TEST_F(
   // initial empty document.  In this test, the `request_initiator` will be a
   // non-opaque origin - it requires that the URLLoaderFactory will have a
   // matching `request_initiator_origin_lock` (e.g. inherited from the parent).
-  GURL image_url = embedded_test_server()->GetURL("b.com", "/blank.jpg");
-  VerifyImageSubresourceLoad(shell(), image_url, "ifr.contentDocument");
+  VerifyImageSubresourceLoads(shell(), "ifr.contentDocument");
 }
 
 // The test below verifies that an initial empty document has a functional
 // URLLoaderFactory.
 IN_PROC_BROWSER_TEST_F(
-    NavigationBrowserTest,
+    SubresourceLoadingTest,
     URLLoaderFactoryInInitialEmptyDoc_LongNavigationInPopup) {
   ASSERT_TRUE(NavigateToURL(
       shell(), embedded_test_server()->GetURL("a.com", "/title1.html")));
@@ -4906,7 +5038,22 @@ IN_PROC_BROWSER_TEST_F(
   // Open a popup window that will never commit a navigation (i.e. that will be
   // stuck on the initial empty document).
   const GURL hung_url = embedded_test_server()->GetURL("a.com", "/hung");
-  ASSERT_TRUE(ExecJs(shell(), JsReplace("popup = window.open($1)", hung_url)));
+  WebContents* popup = nullptr;
+  {
+    WebContentsAddedObserver popup_observer;
+    ASSERT_TRUE(
+        ExecJs(shell(), JsReplace("popup = window.open($1)", hung_url)));
+    popup = popup_observer.GetWebContents();
+  }
+
+  // No process swaps are expected before ReadyToCommit (which will never happen
+  // for a navigation to "/hung").  This test assertion double-checks that the
+  // test will cover inheriting URLLoaderFactory from the creator/opener/parent
+  // frame.
+  RenderFrameHost* opener_frame = shell()->web_contents()->GetMainFrame();
+  RenderFrameHost* popup_frame = popup->GetMainFrame();
+  EXPECT_EQ(opener_frame->GetProcess()->GetID(),
+            popup_frame->GetProcess()->GetID());
 
   // Ask the opener to script the (same-origin) popup window and trigger some
   // HTTP subresource loads within the popup.
@@ -4915,13 +5062,82 @@ IN_PROC_BROWSER_TEST_F(
   // initial empty document.  In this test, the `request_initiator` will be a
   // non-opaque origin - it requires that the URLLoaderFactory will have a
   // matching `request_initiator_origin_lock` (e.g. inherited from the opener).
-  GURL image_url = embedded_test_server()->GetURL("b.com", "/blank.jpg");
-  VerifyImageSubresourceLoad(shell(), image_url, "popup.document");
+  VerifyImageSubresourceLoads(shell(), "popup.document");
+
+  // TODO(https://crbug.com/1194763): Crash recovery doesn't work when there is
+  // no opener.
+  DontTestNetworkServiceCrashes();
+  // Test again after closing the opener..
+  shell()->Close();
+  VerifyImageSubresourceLoads(popup);
+}
+
+// The test below verifies that an initial empty document has a functional
+// URLLoaderFactory.  The ...WithClearedOpener testcase is a regression test for
+// https://crbug.com/1191203.
+IN_PROC_BROWSER_TEST_F(
+    SubresourceLoadingTest,
+    URLLoaderFactoryInInitialEmptyDoc_LongNavigationInPopupWithClearedOpener) {
+  ASSERT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL("a.com", "/title1.html")));
+
+  // Open a new window that will never commit a navigation (i.e. that will be
+  // stuck on the initial empty document).  Clearing of `popup.opener` tests if
+  // inheriting of URLLoaderFactory from the opener will work when the opener
+  // has been cleared in DOM/Javascript.
+  const GURL hung_url = embedded_test_server()->GetURL("a.com", "/hung");
+  const char kScriptTemplate[] = R"(
+      popup = window.open($1);
+      popup.opener = null;
+  )";
+  content::WebContents* popup = nullptr;
+  {
+    WebContentsAddedObserver popup_observer;
+    ASSERT_TRUE(ExecJs(shell(), JsReplace(kScriptTemplate, hung_url)));
+    popup = popup_observer.GetWebContents();
+  }
+
+  // No process swaps are expected before ReadyToCommit (which will never happen
+  // for a navigation to "/hung").  This test assertion double-checks that the
+  // test will cover inheriting URLLoaderFactory from the creator/opener/parent
+  // frame.  This differentiates the test from the "noopener" case covered in
+  // another testcase.
+  RenderFrameHost* opener_frame = shell()->web_contents()->GetMainFrame();
+  RenderFrameHost* popup_frame = popup->GetMainFrame();
+  EXPECT_EQ(opener_frame->GetProcess()->GetID(),
+            popup_frame->GetProcess()->GetID());
+
+  // Double-check that the popup didn't commit any navigation and that it has
+  // an the same origin as the initial opener.
+  EXPECT_EQ(GURL(), popup->GetMainFrame()->GetLastCommittedURL());
+  EXPECT_NE("null", EvalJs(popup, "window.origin"));
+  EXPECT_EQ(shell()
+                ->web_contents()
+                ->GetMainFrame()
+                ->GetLastCommittedOrigin()
+                .Serialize(),
+            EvalJs(popup, "window.origin"));
+
+  // Use the parent frame's `popup` reference to script the same-origin popup
+  // window and trigger some HTTP subresource loads within the popup.
+  //
+  // This tests the functionality of the URLLoaderFactory that gets used by the
+  // initial empty document.  In this test, the `request_initiator` will be a
+  // non-opaque origin - it requires that the URLLoaderFactory will have a
+  // matching `request_initiator_origin_lock` (e.g. inherited from the opener).
+  VerifyImageSubresourceLoads(popup);
+
+  // TODO(https://crbug.com/1194763): Crash recovery doesn't work when there is
+  // no opener.
+  DontTestNetworkServiceCrashes();
+  // Test again after closing the opener..
+  shell()->Close();
+  VerifyImageSubresourceLoads(popup);
 }
 
 // The test below verifies that an initial empty document has a functional
 // URLLoaderFactory.
-IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
+IN_PROC_BROWSER_TEST_F(SubresourceLoadingTest,
                        URLLoaderFactoryInInitialEmptyDoc_204NoOpenerPopup) {
   ASSERT_TRUE(NavigateToURL(
       shell(), embedded_test_server()->GetURL("a.com", "/title1.html")));
@@ -4951,6 +5167,19 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
   EXPECT_EQ(GURL(), popup->GetMainFrame()->GetLastCommittedURL());
   EXPECT_EQ("null", EvalJs(popup, "window.origin"));
 
+  // Process swap is expected because of 'noopener'.  This test assertion
+  // double-checks that in the test it is not possible to inheriting
+  // URLLoaderFactory from the creator/opener/parent frame (because the popup is
+  // in another process).
+  RenderFrameHost* opener_frame = shell()->web_contents()->GetMainFrame();
+  RenderFrameHost* popup_frame = popup->GetMainFrame();
+  EXPECT_NE(opener_frame->GetProcess()->GetID(),
+            popup_frame->GetProcess()->GetID());
+
+  // TODO(https://crbug.com/1194763): Crash recovery doesn't work when there is
+  // no opener.
+  DontTestNetworkServiceCrashes();
+
   // Inject Javascript that triggers some subresource loads over HTTP.
   //
   // To some extent, this simulates an ability of 1) Android WebView (see
@@ -4962,8 +5191,51 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
   // initial empty document.  In this test, the `request_initiator` will be an
   // opaque, unique origin (since nothing has committed yet) and will be
   // compatible with `request_initiator_origin_lock` of the URLLoaderFactory.
-  GURL image_url = embedded_test_server()->GetURL("b.com", "/blank.jpg");
-  VerifyImageSubresourceLoad(popup, image_url);
+  VerifyImageSubresourceLoads(popup);
+}
+
+// The test below verifies that an initial empty document has a functional
+// URLLoaderFactory.
+IN_PROC_BROWSER_TEST_F(SubresourceLoadingTest,
+                       URLLoaderFactoryInInitialEmptyDoc_HungNewWindow) {
+  // Open a new shell, starting at the "/hung" URL.
+  const GURL hung_url = embedded_test_server()->GetURL("a.com", "/hung");
+  Shell* new_shell =
+      Shell::CreateNewWindow(shell()->web_contents()->GetBrowserContext(),
+                             hung_url, nullptr, gfx::Size());
+
+  // Wait until the renderer process launches (this will flush the CreateView
+  // IPC and make sure that ExecJs and EvalJs are able to work).
+  RenderFrameHost* main_frame = new_shell->web_contents()->GetMainFrame();
+  {
+    RenderProcessHostWatcher process_watcher(
+        main_frame->GetProcess(),
+        RenderProcessHostWatcher::WATCH_FOR_PROCESS_READY);
+    process_watcher.Wait();
+  }
+
+  // Double-check that the new shell didn't commit any navigation and that it
+  // has an opaque origin.
+  ASSERT_EQ(0, new_shell->web_contents()->GetController().GetEntryCount());
+  EXPECT_EQ(GURL(), main_frame->GetLastCommittedURL());
+  EXPECT_EQ("null", EvalJs(main_frame, "window.origin"));
+
+  // TODO(https://crbug.com/1194763): Crash recovery doesn't work when there is
+  // no opener.
+  DontTestNetworkServiceCrashes();
+
+  // Inject Javascript that triggers some subresource loads over HTTP.
+  //
+  // To some extent, this simulates an ability of 1) Android WebView (see
+  // https://crbug.com/1189838) and 2) Chrome Extensions, to inject Javascript
+  // into an initial empty document (even when no web/renderer content has
+  // access to the document).
+  //
+  // This tests the functionality of the URLLoaderFactory that gets used by the
+  // initial empty document.  In this test, the `request_initiator` will be an
+  // opaque, unique origin (since nothing has committed yet) and will be
+  // compatible with `request_initiator_origin_lock` of the URLLoaderFactory.
+  VerifyImageSubresourceLoads(main_frame);
 }
 
 IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, Bug838348) {

content/renderer/render_frame_impl.cc

@@ -3052,7 +3052,15 @@ void RenderFrameImpl::CommitNavigationWithParams(
     DCHECK(!subresource_overrides);
     DCHECK(!prefetch_loader_factory);
 
-    new_loader_factories = GetLoaderFactoryBundleFromCreator();
+    // Presence of the parent is verified by the browser process before
+    // committing.
+    //
+    // TODO(arthursonzogni, dcheng): If `inherit_loaders_from_creator` is ever
+    // extended to 'about:blank', then `creator` might also need to come from
+    // the opener.
+    auto* creator = RenderFrameImpl::FromWebFrame(frame_->Parent());
+    DCHECK(creator);
+    new_loader_factories = CloneLoaderFactoriesFrom(*creator);
   } else {
     new_loader_factories = CreateLoaderFactoryBundle(
         std::move(subresource_loader_factories),
@@ -3459,8 +3467,6 @@ void RenderFrameImpl::UpdateSubresourceLoaderFactories(
   // non-null after a frame commits (and UpdateSubresourceLoaderFactories should
   // only be called after a commit).  The check below is just a temporary
   // workaround to paper-over the crash in https://crbug.com/1013254.
-  if (!loader_factories_)
-    loader_factories_ = GetLoaderFactoryBundleFromCreator();
   if (!loader_factories_)
     loader_factories_ = GetLoaderFactoryBundleFallback();
 
@@ -3790,6 +3796,7 @@ blink::WebLocalFrame* RenderFrameImpl::CreateChildFrame(
       agent_scheduling_group_, render_view_, child_routing_id,
       std::move(pending_frame_receiver), std::move(browser_interface_broker),
       devtools_frame_token);
+  child_render_frame->InheritLoaderFactoriesFrom(*this);
   child_render_frame->unique_name_helper_.set_propagated_name(
       frame_unique_name);
   if (is_created_by_script)
@@ -3992,17 +3999,24 @@ void RenderFrameImpl::DidCommitNavigation(
     // Commits triggered by the browser process should always provide
     // |pending_loader_factories_|.
     loader_factories_ = std::move(pending_loader_factories_);
+    DCHECK(loader_factories_->HasBoundDefaultFactory());
   } else if (!loader_factories_) {
-    // When committing an initial empty document synchronously (e,g, in response
-    // to |window.open('', '_blank')|) we won't get |pending_loader_factories_|
-    // from the browser.  In such cases we expect to always have a local parent
-    // or opener - we should eagerly inherit the factories from them (so that
-    // even if the opener gets closed, our factories will be correctly
-    // initialized).
-    loader_factories_ = GetLoaderFactoryBundleFromCreator();
+    // For renderer-initiated frame creation, the factories should be inherited
+    // using the InheritLoaderFactoriesFrom method, but browser-created initial
+    // empty document currently won't get any factories.  For now, the
+    // process-wide fallback factory will be used for such browser-created
+    // initial empty documents.
+    //
+    // TODO(https://crbug.com/1114822): This case should be covered by always
+    // providing a bundle of factories via a new field of
+    // `mojom::CreateLocalMainFrameParams`.  See also
+    // https://crrev.com/c/2787689.
+
+    // This should only happen for a browser-created initial empty document.
+    DCHECK(GetWebFrame()->GetSecurityOrigin().IsOpaque());
+    DCHECK(document_loader->GetUrl().IsEmpty());
+    DCHECK(!navigation_state->common_params().initiator_origin.has_value());
   }
-  DCHECK(loader_factories_);
-  DCHECK(loader_factories_->HasBoundDefaultFactory());
 
   // TODO(dgozman): call DidStartNavigation in various places where we call
   // CommitNavigation() on the frame.
@@ -5566,8 +5580,6 @@ void RenderFrameImpl::OpenURL(std::unique_ptr<blink::WebNavigationInfo> info) {
 }
 
 blink::ChildURLLoaderFactoryBundle* RenderFrameImpl::GetLoaderFactoryBundle() {
-  if (!loader_factories_)
-    loader_factories_ = GetLoaderFactoryBundleFromCreator();
   if (!loader_factories_)
     loader_factories_ = GetLoaderFactoryBundleFallback();
   return loader_factories_.get();
@@ -5581,17 +5593,12 @@ RenderFrameImpl::GetLoaderFactoryBundleFallback() {
 }
 
 scoped_refptr<blink::ChildURLLoaderFactoryBundle>
-RenderFrameImpl::GetLoaderFactoryBundleFromCreator() {
-  RenderFrameImpl* creator = RenderFrameImpl::FromWebFrame(
-      frame_->Parent() ? frame_->Parent() : frame_->Opener());
-  if (creator) {
-    auto bundle_info = base::WrapUnique(
-        static_cast<blink::TrackedChildPendingURLLoaderFactoryBundle*>(
-            creator->GetLoaderFactoryBundle()->Clone().release()));
-    return base::MakeRefCounted<blink::TrackedChildURLLoaderFactoryBundle>(
-        std::move(bundle_info));
-  }
-  return nullptr;
+RenderFrameImpl::CloneLoaderFactoriesFrom(RenderFrameImpl& frame) {
+  auto bundle_info = base::WrapUnique(
+      static_cast<blink::TrackedChildPendingURLLoaderFactoryBundle*>(
+          frame.GetLoaderFactoryBundle()->Clone().release()));
+  return base::MakeRefCounted<blink::TrackedChildURLLoaderFactoryBundle>(
+      std::move(bundle_info));
 }
 
 scoped_refptr<blink::ChildURLLoaderFactoryBundle>
@@ -5601,7 +5608,7 @@ RenderFrameImpl::CreateLoaderFactoryBundle(
         subresource_overrides,
     mojo::PendingRemote<network::mojom::URLLoaderFactory>
         prefetch_loader_factory) {
-  scoped_refptr<blink::ChildURLLoaderFactoryBundle> loader_factories =
+  auto loader_factories =
       base::MakeRefCounted<blink::HostChildURLLoaderFactoryBundle>(
           GetTaskRunner(blink::TaskType::kInternalLoading));
 
@@ -5621,6 +5628,7 @@ RenderFrameImpl::CreateLoaderFactoryBundle(
         std::make_unique<blink::ChildPendingURLLoaderFactoryBundle>(
             std::move(info)));
   }
+
   if (subresource_overrides) {
     loader_factories->UpdateSubresourceOverrides(&*subresource_overrides);
   }
@@ -6197,6 +6205,11 @@ void RenderFrameImpl::SetWebURLLoaderFactoryOverrideForTest(
   web_url_loader_factory_override_for_test_ = std::move(factory);
 }
 
+void RenderFrameImpl::InheritLoaderFactoriesFrom(RenderFrameImpl& frame) {
+  CHECK(!loader_factories_);
+  loader_factories_ = CloneLoaderFactoriesFrom(frame);
+}
+
 blink::scheduler::WebAgentGroupScheduler&
 RenderFrameImpl::GetAgentGroupScheduler() {
   return agent_scheduling_group_.agent_group_scheduler();

content/renderer/render_frame_impl.h

@@ -385,6 +385,7 @@ class CONTENT_EXPORT RenderFrameImpl
   gfx::RectF ElementBoundsInWindow(const blink::WebElement& element) override;
   void ConvertViewportToWindow(gfx::Rect* rect) override;
   float GetDeviceScaleFactor() override;
+  blink::scheduler::WebAgentGroupScheduler& GetAgentGroupScheduler() override;
 
   // blink::mojom::AutoplayConfigurationClient implementation:
   void AddAutoplayFlags(const url::Origin& origin,
@@ -711,7 +712,7 @@ class CONTENT_EXPORT RenderFrameImpl
   void SetWebURLLoaderFactoryOverrideForTest(
       std::unique_ptr<blink::WebURLLoaderFactoryForTest> factory);
 
-  blink::scheduler::WebAgentGroupScheduler& GetAgentGroupScheduler() override;
+  void InheritLoaderFactoriesFrom(RenderFrameImpl& frame);
 
   url::Origin GetSecurityOriginOfTopFrame();
 
@@ -884,17 +885,12 @@ class CONTENT_EXPORT RenderFrameImpl
 
   // Returns a blink::ChildURLLoaderFactoryBundle which can be used to request
   // subresources for this frame.
-  // For frames with committed navigations, this bundle is created with the
-  // factories provided by the browser at navigation time. For any other frames
-  // (i.e. frames on the initial about:blank Document), the bundle returned here
-  // is lazily cloned from the parent or opener's own bundle.
+  //
+  // The returned bundle was typically sent by the browser process when
+  // committing a navigation, but in some cases (about:srcdoc, initial empty
+  // document) it may be inherited from the parent or opener.
   blink::ChildURLLoaderFactoryBundle* GetLoaderFactoryBundle();
 
-  // Clones and returns the creator's (parent's or opener's)
-  // blink::ChildURLLoaderFactoryBundle.
-  scoped_refptr<blink::ChildURLLoaderFactoryBundle>
-  GetLoaderFactoryBundleFromCreator();
-
   // Returns a mostly empty bundle, with a fallback that uses a process-wide,
   // direct-network factory.
   //
@@ -903,6 +899,10 @@ class CONTENT_EXPORT RenderFrameImpl
   scoped_refptr<blink::ChildURLLoaderFactoryBundle>
   GetLoaderFactoryBundleFallback();
 
+  // Clones and returns the `frame`'s blink::ChildURLLoaderFactoryBundle.
+  scoped_refptr<blink::ChildURLLoaderFactoryBundle> CloneLoaderFactoriesFrom(
+      RenderFrameImpl& frame);
+
   scoped_refptr<blink::ChildURLLoaderFactoryBundle> CreateLoaderFactoryBundle(
       std::unique_ptr<blink::PendingURLLoaderFactoryBundle> info,
       base::Optional<std::vector<blink::mojom::TransferrableURLLoaderPtr>>

content/renderer/render_view_impl.cc

@@ -411,6 +411,7 @@ WebView* RenderViewImpl::CreateView(
       agent_scheduling_group_, compositor_deps_, std::move(view_params),
       /*was_created_by_renderer=*/true,
       creator->GetTaskRunner(blink::TaskType::kInternalDefault));
+  view->GetMainRenderFrame()->InheritLoaderFactoriesFrom(*creator_frame);
 
   if (reply->wait_for_debugger) {
     blink::WebFrameWidget* frame_widget =