Skip to content

Commit

Permalink
Support initiator-specific factories in about:blank frames.
Browse files Browse the repository at this point in the history
This CL tweaks RenderFrameImpl::UpdateSubresourceLoaderFactories so that
it also works for about:blank frames.  Such frames inherit their
factories from their opener/parent and therefore need to use a different
code path for storing initiator-specific factories.  This fixes the
crash in 943685.

This change is a prerequsite for fixing 948751, but is not a sufficient
fix on its own.

This CL should also stop crashes capture in 916625, although this bug is
not entirely understood.

Bug: 916625, 943685, 948751
Change-Id: I6b9c108b4d545643474ef1c9631195603d365edd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1549490
Commit-Queue: Łukasz Anforowicz <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: anthonyvd <[email protected]>
Cr-Commit-Position: refs/heads/master@{#647432}
  • Loading branch information
anforowicz authored and Commit Bot committed Apr 3, 2019
1 parent cb9a891 commit 137300a
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 4 deletions.
35 changes: 35 additions & 0 deletions chrome/browser/translate/translate_manager_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,41 @@ IN_PROC_BROWSER_TEST_F(TranslateManagerBrowserTest, PageTranslationSuccess) {
EXPECT_EQ(translate::TranslateErrors::NONE, GetPageTranslatedResult());
}

// Test that the translation was successful in an about:blank page.
// This is a regression test for https://crbug.com/943685.
IN_PROC_BROWSER_TEST_F(TranslateManagerBrowserTest, PageTranslationAboutBlank) {
SetTranslateScript(kTestValidScript);
ResetObserver();
AddTabAtIndex(0, GURL(embedded_test_server()->GetURL("/french_page.html")),
ui::PAGE_TRANSITION_TYPED);

// Open a pop-up window and leave it at the initial about:blank URL.
content::WebContentsAddedObserver popup_observer;
ASSERT_TRUE(
content::ExecJs(browser()->tab_strip_model()->GetActiveWebContents(),
"window.open('about:blank', 'popup')"));
content::WebContents* popup = popup_observer.GetWebContents();

// A round-trip to the renderer process helps avoid a race where the
// browser-side translate structures are not yet ready for the translate call.
EXPECT_EQ("ping", content::EvalJs(popup, "'ping'"));

// Translate the about:blank page.
ChromeTranslateClient* chrome_translate_client =
ChromeTranslateClient::FromWebContents(popup);
translate::TranslateManager* manager =
chrome_translate_client->GetTranslateManager();
manager->TranslatePage("fr", "en", true);

// Verify that the crash from https://crbug.com/943685 didn't happen.
EXPECT_EQ("still alive", content::EvalJs(popup, "'still alive'"));

// Wait for translation to finish and verify it was successful.
WaitUntilPageTranslated();
EXPECT_FALSE(chrome_translate_client->GetLanguageState().translation_error());
EXPECT_EQ(translate::TranslateErrors::NONE, GetPageTranslatedResult());
}

// Test that hrefTranslate is propagating properly
IN_PROC_BROWSER_TEST_F(TranslateManagerBrowserTest, HrefTranslateSuccess) {
ChromeTranslateClient* chrome_translate_client = GetChromeTranslateClient();
Expand Down
26 changes: 22 additions & 4 deletions content/renderer/render_frame_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3847,10 +3847,28 @@ void RenderFrameImpl::UpdateSubresourceLoaderFactories(
std::unique_ptr<blink::URLLoaderFactoryBundleInfo>
subresource_loader_factories) {
DCHECK(loader_factories_);
// TODO(crbug/916625): CHECKing for crbug.com/916625.
CHECK(loader_factories_->IsHostChildURLLoaderFactoryBundle());
static_cast<HostChildURLLoaderFactoryBundle*>(loader_factories_.get())
->UpdateThisAndAllClones(std::move(subresource_loader_factories));
if (loader_factories_->IsHostChildURLLoaderFactoryBundle()) {
static_cast<HostChildURLLoaderFactoryBundle*>(loader_factories_.get())
->UpdateThisAndAllClones(std::move(subresource_loader_factories));
} else {
#if DCHECK_IS_ON()
// In presence of the NetworkService, this situation should happen only if
// the frame hosts a document that isn't related to a real navigation (i.e.
// if the frame should "inherit" the factories from its opener/parent - for
// example for about:blank or about:srcdoc or about:blank#someHref frames,
// or for frames with no URL - like the initial frame opened by window('',
// 'popup')).
if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
WebURL url = GetWebFrame()->GetDocument().Url();
if (url.IsValid() && !url.IsEmpty())
DCHECK(url.ProtocolIs(url::kAboutScheme));
}
#endif
auto partial_bundle = base::MakeRefCounted<ChildURLLoaderFactoryBundle>();
static_cast<blink::URLLoaderFactoryBundle*>(partial_bundle.get())
->Update(std::move(subresource_loader_factories));
loader_factories_->Update(partial_bundle->PassInterface());
}
}

void RenderFrameImpl::MarkInitiatorAsRequiringSeparateURLLoaderFactory(
Expand Down

0 comments on commit 137300a

Please sign in to comment.