diff --git a/DEPS b/DEPS index a7d7a58..55ac4bed 100644 --- a/DEPS +++ b/DEPS
@@ -130,7 +130,7 @@ # Three lines of non-changing comments so that # the commit queue can handle CLs rolling catapult # and whatever else without interference from each other. - 'catapult_revision': 'c8947ecc4964bf055ddab8598981f7c83c546f7d', + 'catapult_revision': 'c796cbc3a6e6c45f95d9dd7bf8769f7407719dcf', # Three lines of non-changing comments so that # the commit queue can handle CLs rolling libFuzzer # and whatever else without interference from each other.
diff --git a/base/bind_internal.h b/base/bind_internal.h index 172ce53..9af3ed23 100644 --- a/base/bind_internal.h +++ b/base/bind_internal.h
@@ -50,7 +50,12 @@ struct ExtractCallableRunTypeImpl; template <typename Callable, typename R, typename... Args> -struct ExtractCallableRunTypeImpl<Callable, R(Callable::*)(Args...) const> { +struct ExtractCallableRunTypeImpl<Callable, R (Callable::*)(Args...)> { + using Type = R(Args...); +}; + +template <typename Callable, typename R, typename... Args> +struct ExtractCallableRunTypeImpl<Callable, R (Callable::*)(Args...) const> { using Type = R(Args...); }; @@ -64,27 +69,23 @@ using ExtractCallableRunType = typename ExtractCallableRunTypeImpl<Callable>::Type; -// IsConvertibleToRunType<Functor> is std::true_type if |Functor| has operator() -// and convertible to the corresponding function pointer. Otherwise, it's -// std::false_type. +// IsCallableObject<Functor> is std::true_type if |Functor| has operator(). +// Otherwise, it's std::false_type. // Example: -// IsConvertibleToRunType<void(*)()>::value is false. +// IsCallableObject<void(*)()>::value is false. // // struct Foo {}; -// IsConvertibleToRunType<void(Foo::*)()>::value is false. -// -// auto f = []() {}; -// IsConvertibleToRunType<decltype(f)>::value is true. +// IsCallableObject<void(Foo::*)()>::value is false. // // int i = 0; -// auto g = [i]() {}; -// IsConvertibleToRunType<decltype(g)>::value is false. +// auto f = [i]() {}; +// IsCallableObject<decltype(f)>::value is false. template <typename Functor, typename SFINAE = void> -struct IsConvertibleToRunType : std::false_type {}; +struct IsCallableObject : std::false_type {}; template <typename Callable> -struct IsConvertibleToRunType<Callable, void_t<decltype(&Callable::operator())>> - : std::is_convertible<Callable, ExtractCallableRunType<Callable>*> {}; +struct IsCallableObject<Callable, void_t<decltype(&Callable::operator())>> + : std::true_type {}; // HasRefCountedTypeAsRawPtr selects true_type when any of the |Args| is a raw // pointer to a RefCounted type. @@ -119,21 +120,37 @@ template <typename Functor, typename SFINAE> struct FunctorTraits; -// For a callable type that is convertible to the corresponding function type. +// For empty callable types. // This specialization is intended to allow binding captureless lambdas by -// base::Bind(), based on the fact that captureless lambdas can be convertible -// to the function type while capturing lambdas can't. +// base::Bind(), based on the fact that captureless lambdas are empty while +// capturing lambdas are not. This also allows any functors as far as it's an +// empty class. +// Example: +// +// // Captureless lambdas are allowed. +// []() {return 42;}; +// +// // Capturing lambdas are *not* allowed. +// int x; +// [x]() {return x;}; +// +// // Any empty class with operator() is allowed. +// struct Foo { +// void operator()() const {} +// // No non-static member variable and no virtual functions. +// }; template <typename Functor> struct FunctorTraits<Functor, - std::enable_if_t<IsConvertibleToRunType<Functor>::value>> { + std::enable_if_t<IsCallableObject<Functor>::value && + std::is_empty<Functor>::value>> { using RunType = ExtractCallableRunType<Functor>; static constexpr bool is_method = false; static constexpr bool is_nullable = false; - template <typename... RunArgs> - static ExtractReturnType<RunType> - Invoke(const Functor& functor, RunArgs&&... args) { - return functor(std::forward<RunArgs>(args)...); + template <typename RunFunctor, typename... RunArgs> + static ExtractReturnType<RunType> Invoke(RunFunctor&& functor, + RunArgs&&... args) { + return std::forward<RunFunctor>(functor)(std::forward<RunArgs>(args)...); } }; @@ -437,8 +454,7 @@ : BindState(IsCancellable{}, invoke_func, std::forward<ForwardFunctor>(functor), - std::forward<ForwardBoundArgs>(bound_args)...) { - } + std::forward<ForwardBoundArgs>(bound_args)...) {} Functor functor_; std::tuple<BoundArgs...> bound_args_;
diff --git a/base/bind_unittest.cc b/base/bind_unittest.cc index 7deba47..fed0433 100644 --- a/base/bind_unittest.cc +++ b/base/bind_unittest.cc
@@ -13,6 +13,7 @@ #include "base/memory/ptr_util.h" #include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" +#include "base/test/bind_test_util.h" #include "base/test/gtest_util.h" #include "build/build_config.h" #include "testing/gmock/include/gmock/gmock.h" @@ -1228,17 +1229,17 @@ } TEST_F(BindTest, CapturelessLambda) { - EXPECT_FALSE(internal::IsConvertibleToRunType<void>::value); - EXPECT_FALSE(internal::IsConvertibleToRunType<int>::value); - EXPECT_FALSE(internal::IsConvertibleToRunType<void(*)()>::value); - EXPECT_FALSE(internal::IsConvertibleToRunType<void(NoRef::*)()>::value); + EXPECT_FALSE(internal::IsCallableObject<void>::value); + EXPECT_FALSE(internal::IsCallableObject<int>::value); + EXPECT_FALSE(internal::IsCallableObject<void (*)()>::value); + EXPECT_FALSE(internal::IsCallableObject<void (NoRef::*)()>::value); auto f = []() {}; - EXPECT_TRUE(internal::IsConvertibleToRunType<decltype(f)>::value); + EXPECT_TRUE(internal::IsCallableObject<decltype(f)>::value); int i = 0; auto g = [i]() { (void)i; }; - EXPECT_FALSE(internal::IsConvertibleToRunType<decltype(g)>::value); + EXPECT_TRUE(internal::IsCallableObject<decltype(g)>::value); auto h = [](int, double) { return 'k'; }; EXPECT_TRUE((std::is_same< @@ -1257,6 +1258,36 @@ EXPECT_EQ(42, x); } +TEST_F(BindTest, EmptyFunctor) { + struct NonEmptyFunctor { + int operator()() const { return x; } + int x = 42; + }; + + struct EmptyFunctor { + int operator()() { return 42; } + }; + + struct EmptyFunctorConst { + int operator()() const { return 42; } + }; + + EXPECT_TRUE(internal::IsCallableObject<NonEmptyFunctor>::value); + EXPECT_TRUE(internal::IsCallableObject<EmptyFunctor>::value); + EXPECT_TRUE(internal::IsCallableObject<EmptyFunctorConst>::value); + EXPECT_EQ(42, BindOnce(EmptyFunctor()).Run()); + EXPECT_EQ(42, BindOnce(EmptyFunctorConst()).Run()); + EXPECT_EQ(42, BindRepeating(EmptyFunctorConst()).Run()); +} + +TEST_F(BindTest, CapturingLambdaForTesting) { + int x = 42; + EXPECT_EQ(42, BindLambdaForTesting([=] { return x; }).Run()); + + auto f = [x] { return x; }; + EXPECT_EQ(42, BindLambdaForTesting(f).Run()); +} + TEST_F(BindTest, Cancellation) { EXPECT_CALL(no_ref_, VoidMethodWithIntArg(_)).Times(2);
diff --git a/base/bind_unittest.nc b/base/bind_unittest.nc index 8e26c1c8..e4ac609 100644 --- a/base/bind_unittest.nc +++ b/base/bind_unittest.nc
@@ -11,6 +11,7 @@ #include "base/callback.h" #include "base/macros.h" #include "base/memory/ref_counted.h" +#include "base/test/bind_test_util.h" namespace base { @@ -73,6 +74,11 @@ void TakesMoveOnly(std::unique_ptr<int>) { } +struct NonEmptyFunctor { + int x; + void operator()() const {} +}; + // TODO(hans): Remove .* and update the static_assert expectations once we roll // past Clang r313315. https://crbug.com/765692. @@ -304,6 +310,11 @@ Bind(&TakesMoveOnly, std::move(x)); } +#elif defined(NCTEST_BIND_NON_EMPTY_FUNCTOR) // [r"fatal error: implicit instantiation of undefined template 'base::internal::FunctorTraits<base::NonEmptyFunctor, void>'"] + +void WontCompile() { + Bind(NonEmptyFunctor()); +} #endif
diff --git a/base/test/BUILD.gn b/base/test/BUILD.gn index 5871b9f..12b34f4 100644 --- a/base/test/BUILD.gn +++ b/base/test/BUILD.gn
@@ -28,6 +28,7 @@ "../trace_event/trace_config_memory_test_util.h", "android/java_handler_thread_helpers.cc", "android/java_handler_thread_helpers.h", + "bind_test_util.h", "copy_only_int.h", "fuzzed_data_provider.cc", "fuzzed_data_provider.h",
diff --git a/base/test/bind_test_util.h b/base/test/bind_test_util.h new file mode 100644 index 0000000..c6d52fb3 --- /dev/null +++ b/base/test/bind_test_util.h
@@ -0,0 +1,23 @@ +// Copyright 2017 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. + +#ifndef BASE_TEST_BIND_TEST_UTIL_H_ +#define BASE_TEST_BIND_TEST_UTIL_H_ + +#include "base/bind.h" + +namespace base { + +// A variant of Bind() that can bind capturing lambdas for testing. +// This doesn't support extra arguments binding as the lambda itself can do. +template <typename F> +RepeatingCallback<internal::ExtractCallableRunType<std::decay_t<F>>> +BindLambdaForTesting(F&& f) { + return BindRepeating([](const std::decay_t<F>& f) { return f(); }, + std::forward<F>(f)); +} + +} // namespace base + +#endif // BASE_TEST_BIND_TEST_UTIL_H_
diff --git a/chrome/browser/extensions/app_process_apitest.cc b/chrome/browser/extensions/app_process_apitest.cc index 4da3174..6eadd740 100644 --- a/chrome/browser/extensions/app_process_apitest.cc +++ b/chrome/browser/extensions/app_process_apitest.cc
@@ -617,42 +617,6 @@ process_map->Contains(contents->GetMainFrame()->GetProcess()->GetID())); } -// Tests that if we have a non-app process (path3/container.html) that has an -// iframe with a URL in the app's extent (path1/iframe.html), then opening a -// link from that iframe to a new window to a URL in the app's extent (path1/ -// empty.html) results in the new window being in an app process. See -// http://crbug.com/89272 for more details. -IN_PROC_BROWSER_TEST_F(AppApiTest, OpenAppFromIframe) { - extensions::ProcessMap* process_map = - extensions::ProcessMap::Get(browser()->profile()); - - GURL base_url = GetTestBaseURL("app_process"); - - // Load app and start URL (not in the app). - const Extension* app = - LoadExtension(test_data_dir_.AppendASCII("app_process")); - ASSERT_TRUE(app); - - ui_test_utils::NavigateToURL(browser(), - base_url.Resolve("path3/container.html")); - EXPECT_FALSE(process_map->Contains(browser() - ->tab_strip_model() - ->GetWebContentsAt(0) - ->GetMainFrame() - ->GetProcess() - ->GetID())); - - const BrowserList* active_browser_list = BrowserList::GetInstance(); - EXPECT_EQ(2U, active_browser_list->size()); - content::WebContents* popup_contents = - active_browser_list->get(1)->tab_strip_model()->GetActiveWebContents(); - content::WaitForLoadStop(popup_contents); - - // Popup window should be in the app's process. - RenderViewHost* popup_host = popup_contents->GetRenderViewHost(); - EXPECT_TRUE(process_map->Contains(popup_host->GetProcess()->GetID())); -} - // Similar to the previous test, but ensure that popup blocking bypass // isn't granted to the iframe. See crbug.com/117446. #if defined(OS_CHROMEOS)
diff --git a/chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc b/chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc index 2905ac2b..88e511d2 100644 --- a/chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc +++ b/chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc
@@ -70,7 +70,7 @@ template <class C> void data_reduction_proxy::DataReductionProxySettingsTestBase::ResetSettings( - std::unique_ptr<base::Clock> clock) { + base::Clock* clock) { MockDataReductionProxySettings<C>* settings = new MockDataReductionProxySettings<C>(); settings->config_ = test_context_->config(); @@ -86,9 +86,8 @@ test_context_->CreateDataReductionProxyService(settings_.get()); } -template void -data_reduction_proxy::DataReductionProxySettingsTestBase::ResetSettings< - DataReductionProxyChromeSettings>(std::unique_ptr<base::Clock> clock); +template void data_reduction_proxy::DataReductionProxySettingsTestBase:: + ResetSettings<DataReductionProxyChromeSettings>(base::Clock* clock); namespace {
diff --git a/chrome/browser/resources/settings/internet_page/network_summary.js b/chrome/browser/resources/settings/internet_page/network_summary.js index a755721b..37dd8e4cf 100644 --- a/chrome/browser/resources/settings/internet_page/network_summary.js +++ b/chrome/browser/resources/settings/internet_page/network_summary.js
@@ -159,12 +159,13 @@ getActiveStateCallback_: function(id, state) { if (chrome.runtime.lastError) { var message = chrome.runtime.lastError.message; - if (message != 'Error.NetworkUnavailable') { + if (message != 'Error.NetworkUnavailable' && + message != 'Error.InvalidNetworkGuid') { console.error( 'Unexpected networkingPrivate.getState error: ' + message + ' For: ' + id); + return; } - return; } // Async call, ensure id still exists. if (!this.activeNetworkIds_.has(id)) @@ -174,14 +175,12 @@ return; } // Find the active state for the type and update it. - for (var i = 0; i < this.activeNetworkStates_.length; ++i) { - if (this.activeNetworkStates_[i].type == state.type) { - this.activeNetworkStates_[i] = state; - return; - } + var idx = this.activeNetworkStates_.findIndex((s) => s.Type == state.Type); + if (idx == -1) { + console.error('Active state not found: ' + state.Name); + return; } - // Not found - console.error('Active state not found: ' + state.Name); + this.set(['activeNetworkStates_', idx], state); }, /**
diff --git a/chrome/browser/ui/extensions/hosted_app_browsertest.cc b/chrome/browser/ui/extensions/hosted_app_browsertest.cc index 2122b83..2d54cc7 100644 --- a/chrome/browser/ui/extensions/hosted_app_browsertest.cc +++ b/chrome/browser/ui/extensions/hosted_app_browsertest.cc
@@ -49,6 +49,7 @@ #include "chrome/common/chrome_features.h" #endif +using content::RenderFrameHost; using content::WebContents; using extensions::Extension; @@ -335,8 +336,7 @@ // Ensure that the frame navigated successfully and that it has correct // content. - content::RenderFrameHost* subframe = - content::ChildFrameAt(tab->GetMainFrame(), 0); + RenderFrameHost* subframe = content::ChildFrameAt(tab->GetMainFrame(), 0); EXPECT_EQ(app_url, subframe->GetLastCommittedURL()); std::string result; EXPECT_TRUE(ExecuteScriptAndExtractString( @@ -445,28 +445,24 @@ ASSERT_EQ(1u, chrome::GetBrowserCount(browser()->profile())); } -class HostedAppVsTdiTest : public HostedAppTest { - public: - HostedAppVsTdiTest() {} - ~HostedAppVsTdiTest() override {} +// Common app manifest for HostedAppProcessModelTests. +constexpr const char kHostedAppProcessModelManifest[] = + R"( { "name": "Hosted App Process Model Test", + "version": "1", + "manifest_version": 2, + "app": { + "launch": { + "web_url": "%s" + }, + "urls": ["*://app.site.com/frame_tree", "*://isolated.site.com/"] + } + } )"; - void SetUpOnMainThread() override { - scoped_feature_list_.InitAndEnableFeature(features::kTopDocumentIsolation); - HostedAppTest::SetUpOnMainThread(); - ASSERT_TRUE(embedded_test_server()->Start()); - } - - private: - base::test::ScopedFeatureList scoped_feature_list_; - - DISALLOW_COPY_AND_ASSIGN(HostedAppVsTdiTest); -}; - -// Tests that even with --top-document-isolation, app.site.com (covered by app's -// web extent) main frame and foo.site.com (not covered by app's web extent) -// share the same renderer process. See also https://crbug.com/679011. +// This set of tests verifies the hosted app process model behavior in various +// isolation modes. They can be run by default, with --site-per-process, or +// with --top-document-isolation. In each mode, they contain an isolated origin. // -// Relevant frames in the test: +// Relevant frames in the tests: // - |app| - app.site.com/frame_tree/cross_origin_but_same_site_frames.html // Main frame, launch URL of the hosted app (i.e. app.launch.web_url). // - |same_dir| - app.site.com/frame_tree/simple.htm @@ -478,53 +474,169 @@ // - |same_site| - other.site.com/title1.htm // Different origin, but same site as |app|, |same_dir|, // |diff_dir|. +// - |isolated| - isolated.site.com/title1.htm +// Within app's extent, but belongs to an isolated origin. // - |cross_site| - cross.domain.com/title1.htm // Cross-site from all the other frames. -// -// Verifications of |*_site| (e.g. EXPECT_EQ(same_dir_site, app_site) are -// sanity checks of the test setup. -// -// First real verification in the test is whether |same_dir| and |diff_dir| can -// script each other (despite their effective URLs being cross-site, because -// |same_dir| is mapped to a chrome-extension URL). This was a functionality -// problem caused by https://crbug.com/679011. -// -// The test also verifies that all same-site frames (i.e. |app|, |same_dir|, -// |diff_dir|, |same_site|) share the same renderer process. This was a small -// performance problem caused by https://crbug.com/679011. -IN_PROC_BROWSER_TEST_P(HostedAppVsTdiTest, ProcessAllocation) { - // Setup and launch the hosted app. + +class HostedAppProcessModelTest : public HostedAppTest { + public: + HostedAppProcessModelTest() {} + ~HostedAppProcessModelTest() override {} + + void SetUpCommandLine(base::CommandLine* command_line) override { + ASSERT_TRUE(embedded_test_server()->InitializeAndListen()); + std::string origin_list = + embedded_test_server()->GetURL("isolated.site.com", "/").spec(); + command_line->AppendSwitchASCII(switches::kIsolateOrigins, origin_list); + } + + void SetUpOnMainThread() override { + HostedAppTest::SetUpOnMainThread(); + host_resolver()->AddRule("*", "127.0.0.1"); + embedded_test_server()->StartAcceptingConnections(); + + // TODO(alexmos): This should also be true for TDI, if + // base::FeatureList::IsEnabled(features::kTopDocumentIsolation) is true. + // However, we can't do that yet, because + // ChromeContentBrowserClientExtensionsPart:: + // ShouldFrameShareParentSiteInstanceDespiteTopDocumentIsolation() returns + // true for all hosted apps and hence forces even cross-site subframes to + // be kept in the app process. + should_swap_for_cross_site_ = content::AreAllSitesIsolatedForTesting(); + + process_map_ = extensions::ProcessMap::Get(browser()->profile()); + + same_dir_url_ = embedded_test_server()->GetURL("app.site.com", + "/frame_tree/simple.htm"); + diff_dir_url_ = + embedded_test_server()->GetURL("app.site.com", "/save_page/a.htm"); + same_site_url_ = + embedded_test_server()->GetURL("other.site.com", "/title1.html"); + isolated_url_ = + embedded_test_server()->GetURL("isolated.site.com", "/title1.html"); + cross_site_url_ = + embedded_test_server()->GetURL("cross.domain.com", "/title1.html"); + } + + // Opens a popup from |rfh| to |url|, verifies whether it should stay in the + // same process as |rfh| and whether it should be in an app process, and then + // closes the popup. + void TestPopupProcess(RenderFrameHost* rfh, + const GURL& url, + bool expect_same_process, + bool expect_app_process) { + content::WebContentsAddedObserver tab_added_observer; + ASSERT_TRUE( + content::ExecuteScript(rfh, "window.open('" + url.spec() + "');")); + content::WebContents* new_tab = tab_added_observer.GetWebContents(); + ASSERT_TRUE(new_tab); + EXPECT_TRUE(WaitForLoadStop(new_tab)); + EXPECT_EQ(url, new_tab->GetLastCommittedURL()); + RenderFrameHost* new_rfh = new_tab->GetMainFrame(); + + EXPECT_EQ(expect_same_process, rfh->GetProcess() == new_rfh->GetProcess()) + << " for " << url << " from " << rfh->GetLastCommittedURL(); + + EXPECT_EQ(expect_app_process, + process_map_->Contains(new_rfh->GetProcess()->GetID())) + << " for " << url << " from " << rfh->GetLastCommittedURL(); + EXPECT_EQ(expect_app_process, + new_rfh->GetSiteInstance()->GetSiteURL().SchemeIs( + extensions::kExtensionScheme)) + << " for " << url << " from " << rfh->GetLastCommittedURL(); + + ASSERT_TRUE(content::ExecuteScript(new_rfh, "window.close();")); + } + + // Creates a subframe underneath |parent_rfh| to |url|, verifies whether it + // should stay in the same process as |parent_rfh| and whether it should be in + // an app process, and returns the subframe RFH. + RenderFrameHost* TestSubframeProcess(RenderFrameHost* parent_rfh, + const GURL& url, + bool expect_same_process, + bool expect_app_process) { + return TestSubframeProcess(parent_rfh, url, "", expect_same_process, + expect_app_process); + } + + RenderFrameHost* TestSubframeProcess(RenderFrameHost* parent_rfh, + const GURL& url, + const std::string& element_id, + bool expect_same_process, + bool expect_app_process) { + WebContents* web_contents = WebContents::FromRenderFrameHost(parent_rfh); + content::TestNavigationObserver nav_observer(web_contents, 1); + std::string script = "var f = document.createElement('iframe');"; + if (!element_id.empty()) + script += "f.id = '" + element_id + "';"; + script += "f.src = '" + url.spec() + "';"; + script += "document.body.appendChild(f);"; + EXPECT_TRUE(ExecuteScript(parent_rfh, script)); + nav_observer.Wait(); + + RenderFrameHost* subframe = content::FrameMatchingPredicate( + web_contents, base::Bind(&content::FrameHasSourceUrl, url)); + + EXPECT_EQ(expect_same_process, + parent_rfh->GetProcess() == subframe->GetProcess()) + << " for " << url << " from " << parent_rfh->GetLastCommittedURL(); + + EXPECT_EQ(expect_app_process, + process_map_->Contains(subframe->GetProcess()->GetID())) + << " for " << url << " from " << parent_rfh->GetLastCommittedURL(); + EXPECT_EQ(expect_app_process, + subframe->GetSiteInstance()->GetSiteURL().SchemeIs( + extensions::kExtensionScheme)) + << " for " << url << " from " << parent_rfh->GetLastCommittedURL(); + + return subframe; + } + + protected: + bool should_swap_for_cross_site_; + + extensions::ProcessMap* process_map_; + + GURL same_dir_url_; + GURL diff_dir_url_; + GURL same_site_url_; + GURL isolated_url_; + GURL cross_site_url_; + + private: + DISALLOW_COPY_AND_ASSIGN(HostedAppProcessModelTest); +}; + +// Tests that same-site iframes stay inside the hosted app process, even when +// they are not within the hosted app's extent. This allows same-site scripting +// to work and avoids unnecessary OOPIFs. Also tests that isolated origins in +// iframes do not stay in the app's process, nor do cross-site iframes in modes +// that require them to swap. +IN_PROC_BROWSER_TEST_P(HostedAppProcessModelTest, IframesInsideHostedApp) { + // Set up and launch the hosted app. GURL url = embedded_test_server()->GetURL( "app.site.com", "/frame_tree/cross_origin_but_same_site_frames.html"); extensions::TestExtensionDir test_app_dir; - test_app_dir.WriteManifest(base::StringPrintf( - R"( { "name": "Hosted App vs TDI Test", - "version": "1", - "manifest_version": 2, - "app": { - "launch": { - "web_url": "%s" - }, - "urls": ["*://app.site.com/frame_tree"] - } - } )", - url.spec().c_str())); + test_app_dir.WriteManifest( + base::StringPrintf(kHostedAppProcessModelManifest, url.spec().c_str())); SetupApp(test_app_dir.UnpackedPath(), false); content::WebContents* web_contents = app_browser_->tab_strip_model()->GetActiveWebContents(); - content::WaitForLoadStop(web_contents); + EXPECT_TRUE(content::WaitForLoadStop(web_contents)); auto find_frame = [web_contents](const std::string& name) { return content::FrameMatchingPredicate( web_contents, base::Bind(&content::FrameMatchesName, name)); }; - content::RenderFrameHost* app = web_contents->GetMainFrame(); - content::RenderFrameHost* same_dir = find_frame("SameOrigin-SamePath"); - content::RenderFrameHost* diff_dir = find_frame("SameOrigin-DifferentPath"); - content::RenderFrameHost* same_site = find_frame("OtherSubdomain-SameSite"); - content::RenderFrameHost* cross_site = find_frame("CrossSite"); + RenderFrameHost* app = web_contents->GetMainFrame(); + RenderFrameHost* same_dir = find_frame("SameOrigin-SamePath"); + RenderFrameHost* diff_dir = find_frame("SameOrigin-DifferentPath"); + RenderFrameHost* same_site = find_frame("OtherSubdomain-SameSite"); + RenderFrameHost* isolated = find_frame("Isolated-SameSite"); + RenderFrameHost* cross_site = find_frame("CrossSite"); // Sanity-check sites of all relevant frames to verify test setup. GURL app_site = content::SiteInstance::GetSiteForURL( @@ -547,6 +659,12 @@ EXPECT_NE(same_site_site, app_site); EXPECT_EQ(same_site_site, diff_dir_site); + GURL isolated_site = content::SiteInstance::GetSiteForURL( + app_browser_->profile(), isolated->GetLastCommittedURL()); + EXPECT_NE(extensions::kExtensionScheme, isolated_site.scheme()); + EXPECT_NE(isolated_site, app_site); + EXPECT_NE(isolated_site, diff_dir_site); + GURL cross_site_site = content::SiteInstance::GetSiteForURL( app_browser_->profile(), cross_site->GetLastCommittedURL()); EXPECT_NE(cross_site_site, app_site); @@ -559,125 +677,281 @@ EXPECT_TRUE(content::ExecuteScriptAndExtractString( same_dir, "domAutomationController.send(window.origin)", &same_dir_origin)); - std::string diff_dir_origin; EXPECT_TRUE(content::ExecuteScriptAndExtractString( diff_dir, "domAutomationController.send(window.origin)", &diff_dir_origin)); - EXPECT_EQ(diff_dir_origin, same_dir_origin); - // Verify scriptability and process placement. + // Verify that (1) all same-site iframes stay in the process, (2) isolated + // origin iframe does not, and (3) cross-site iframe leaves if the process + // model calls for it. EXPECT_EQ(same_dir->GetProcess(), app->GetProcess()); - if (!content::AreAllSitesIsolatedForTesting()) { - // Verify that |same_dir| and |diff_dir| can script each other. - // (they should - they have the same origin). - std::string inner_text_from_other_frame; - ASSERT_TRUE(content::ExecuteScriptAndExtractString( - diff_dir, - R"( var w = window.open('', 'SameOrigin-SamePath'); - domAutomationController.send(w.document.body.innerText); )", - &inner_text_from_other_frame)); - EXPECT_EQ("Simple test page.", inner_text_from_other_frame); + EXPECT_EQ(diff_dir->GetProcess(), app->GetProcess()); + EXPECT_EQ(same_site->GetProcess(), app->GetProcess()); + EXPECT_NE(isolated->GetProcess(), app->GetProcess()); + if (should_swap_for_cross_site_) + EXPECT_NE(cross_site->GetProcess(), app->GetProcess()); + else + EXPECT_EQ(cross_site->GetProcess(), app->GetProcess()); - // Verify there are no additional processes for same-site frames. - EXPECT_EQ(diff_dir->GetProcess(), app->GetProcess()); - EXPECT_EQ(same_site->GetProcess(), app->GetProcess()); - // TODO(lukasza): https://crbug.com/718516: For now it is okay for - // |cross_site| to be in any process, but we should probably revisit - // before launching --top-document-isolation more broadly. - } else { - // TODO(lukasza): https://crbug.com/718516: Process policy is not - // well-defined / settled wrt relationship between 1) hosted apps and 2) - // same-site web content outside of hosted app's extent. When this test was - // authored --site-per-process would put |app| in a separate renderer - // process from |diff_dir| and |same_site|, even though such process - // placement can be problematic (if |app| tries to synchronously script - // |diff_dir| and/or |same_site|). + // The isolated origin iframe's process should not be in the ProcessMap. If + // we swapped processes for the |cross_site| iframe, its process should also + // not be on the ProcessMap. + EXPECT_FALSE(process_map_->Contains(isolated->GetProcess()->GetID())); + if (should_swap_for_cross_site_) + EXPECT_FALSE(process_map_->Contains(cross_site->GetProcess()->GetID())); + + // Verify that |same_dir| and |diff_dir| can script each other. + // (they should - they have the same origin). + std::string inner_text_from_other_frame; + ASSERT_TRUE(content::ExecuteScriptAndExtractString( + diff_dir, + R"( var w = window.open('', 'SameOrigin-SamePath'); + domAutomationController.send(w.document.body.innerText); )", + &inner_text_from_other_frame)); + EXPECT_EQ("Simple test page.", inner_text_from_other_frame); +} + +// Check that if a hosted app has an iframe, and that iframe navigates to URLs +// that are same-site with the app, these navigations ends up in the app +// process. +IN_PROC_BROWSER_TEST_P(HostedAppProcessModelTest, + IframeNavigationsInsideHostedApp) { + // Set up and launch the hosted app. + GURL app_url = + embedded_test_server()->GetURL("app.site.com", "/frame_tree/simple.htm"); + + extensions::TestExtensionDir test_app_dir; + test_app_dir.WriteManifest(base::StringPrintf(kHostedAppProcessModelManifest, + app_url.spec().c_str())); + SetupApp(test_app_dir.UnpackedPath(), false); + + content::WebContents* web_contents = + app_browser_->tab_strip_model()->GetActiveWebContents(); + EXPECT_TRUE(content::WaitForLoadStop(web_contents)); + + RenderFrameHost* app = web_contents->GetMainFrame(); + + // Add a data: URL subframe. This should stay in the app process. + TestSubframeProcess(app, GURL("data:text/html,foo"), "test_iframe", + true /* expect_same_process */, + true /* expect_app_process */); + + // Navigate iframe to a non-app-but-same-site-with-app URL and check that it + // stays in the parent process. + { + SCOPED_TRACE("... for data: -> diff_dir"); + EXPECT_TRUE( + NavigateIframeToURL(web_contents, "test_iframe", diff_dir_url_)); + EXPECT_EQ(ChildFrameAt(app, 0)->GetProcess(), app->GetProcess()); + } + + // Navigate the iframe to an isolated origin to force an OOPIF. + { + SCOPED_TRACE("... for diff_dir -> isolated"); + EXPECT_TRUE( + NavigateIframeToURL(web_contents, "test_iframe", isolated_url_)); + EXPECT_NE(ChildFrameAt(app, 0)->GetProcess(), app->GetProcess()); + } + + // Navigate the iframe to an app URL. This should go back to the app process. + { + SCOPED_TRACE("... for isolated -> same_dir"); + EXPECT_TRUE( + NavigateIframeToURL(web_contents, "test_iframe", same_dir_url_)); + EXPECT_EQ(ChildFrameAt(app, 0)->GetProcess(), app->GetProcess()); + } + + // Navigate the iframe back to the OOPIF again. + { + SCOPED_TRACE("... for same_dir -> isolated"); + EXPECT_TRUE( + NavigateIframeToURL(web_contents, "test_iframe", isolated_url_)); + EXPECT_NE(ChildFrameAt(app, 0)->GetProcess(), app->GetProcess()); + } + + // Navigate iframe to a non-app-but-same-site-with-app URL and check that it + // also goes back to the parent process. + { + SCOPED_TRACE("... for isolated -> diff_dir"); + EXPECT_TRUE( + NavigateIframeToURL(web_contents, "test_iframe", diff_dir_url_)); + EXPECT_EQ(ChildFrameAt(app, 0)->GetProcess(), app->GetProcess()); } } -class HostedAppWithIsolatedOriginsTest : public HostedAppTest { - public: - HostedAppWithIsolatedOriginsTest() {} - ~HostedAppWithIsolatedOriginsTest() override {} +// Tests that popups opened within a hosted app behave as expected. +// TODO(creis): The goal is for same-site popups to stay in the app's process so +// that they can be scripted, but this is not yet supported. See +// https://crbug.com/718516. +IN_PROC_BROWSER_TEST_P(HostedAppProcessModelTest, PopupsInsideHostedApp) { + // Set up and launch the hosted app. + GURL url = embedded_test_server()->GetURL( + "app.site.com", "/frame_tree/cross_origin_but_same_site_frames.html"); - void SetUpCommandLine(base::CommandLine* command_line) override { - ASSERT_TRUE(embedded_test_server()->InitializeAndListen()); - std::string origin_list = - embedded_test_server()->GetURL("isolated.foo.com", "/").spec(); - command_line->AppendSwitchASCII(switches::kIsolateOrigins, origin_list); - } - - void SetUpOnMainThread() override { - HostedAppTest::SetUpOnMainThread(); - host_resolver()->AddRule("*", "127.0.0.1"); - embedded_test_server()->StartAcceptingConnections(); - } - - private: - DISALLOW_COPY_AND_ASSIGN(HostedAppWithIsolatedOriginsTest); -}; - -// Verify that when navigating to an isolated origin which is also part of -// a hosted app's web extent, the isolated origin takes precedence for -// SiteInstance determination and still ends up in a dedicated process. -IN_PROC_BROWSER_TEST_P(HostedAppWithIsolatedOriginsTest, - IsolatedOriginTakesPrecedence) { - // Launch a hosted app which covers an isolated origin in its web extent. - GURL url = embedded_test_server()->GetURL("app.foo.com", "/iframe.html"); extensions::TestExtensionDir test_app_dir; - test_app_dir.WriteManifest(base::StringPrintf( - R"( { "name": "Hosted App vs IsolatedOrigins Test", - "version": "1", - "manifest_version": 2, - "app": { - "launch": { - "web_url": "%s" - }, - "urls": ["*://app.foo.com", "*://isolated.foo.com/"] - } - } )", - url.spec().c_str())); + test_app_dir.WriteManifest( + base::StringPrintf(kHostedAppProcessModelManifest, url.spec().c_str())); SetupApp(test_app_dir.UnpackedPath(), false); - content::WebContents* app_contents = - app_browser_->tab_strip_model()->GetActiveWebContents(); - content::WaitForLoadStop(app_contents); - - content::RenderFrameHost* app = app_contents->GetMainFrame(); - - // A subframe on an app for an isolated origin should be kicked out to its - // own process. - GURL isolated_url = - embedded_test_server()->GetURL("isolated.foo.com", "/title1.html"); - EXPECT_TRUE(NavigateIframeToURL(app_contents, "test", isolated_url)); - - content::RenderFrameHost* app_subframe = content::ChildFrameAt(app, 0); - EXPECT_EQ(isolated_url, app_subframe->GetLastCommittedURL()); - EXPECT_TRUE(app_subframe->IsCrossProcessSubframe()); - EXPECT_NE(app->GetSiteInstance(), app_subframe->GetSiteInstance()); - EXPECT_EQ(isolated_url.GetOrigin(), - app_subframe->GetSiteInstance()->GetSiteURL()); - GURL isolated_site = content::SiteInstance::GetSiteForURL( - app_browser_->profile(), app_subframe->GetLastCommittedURL()); - EXPECT_NE(extensions::kExtensionScheme, isolated_site.scheme()); - EXPECT_FALSE(extensions::ProcessMap::Get(app_browser_->profile()) - ->Contains(app_subframe->GetProcess()->GetID())); - - // Navigating a regular tab to an isolated origin which is also part of an - // app's web extent should use the isolated origin's SiteInstance and not the - // app's. - NavigateToURLAndWait(browser(), isolated_url); content::WebContents* web_contents = - browser()->tab_strip_model()->GetActiveWebContents(); - EXPECT_EQ(isolated_url.GetOrigin(), - web_contents->GetMainFrame()->GetSiteInstance()->GetSiteURL()); - EXPECT_NE(web_contents->GetMainFrame()->GetSiteInstance(), - app->GetSiteInstance()); - EXPECT_FALSE( - extensions::ProcessMap::Get(browser()->profile()) - ->Contains(web_contents->GetMainFrame()->GetProcess()->GetID())); + app_browser_->tab_strip_model()->GetActiveWebContents(); + EXPECT_TRUE(content::WaitForLoadStop(web_contents)); + + auto find_frame = [web_contents](const std::string& name) { + return content::FrameMatchingPredicate( + web_contents, base::Bind(&content::FrameMatchesName, name)); + }; + RenderFrameHost* app = web_contents->GetMainFrame(); + RenderFrameHost* same_dir = find_frame("SameOrigin-SamePath"); + RenderFrameHost* diff_dir = find_frame("SameOrigin-DifferentPath"); + RenderFrameHost* same_site = find_frame("OtherSubdomain-SameSite"); + RenderFrameHost* isolated = find_frame("Isolated-SameSite"); + RenderFrameHost* cross_site = find_frame("CrossSite"); + + { + SCOPED_TRACE("... for same_dir popup"); + TestPopupProcess(app, same_dir_url_, true, true); + } + { + SCOPED_TRACE("... for diff_dir popup"); + // TODO(creis): This should stay in the app's process, but that's not yet + // supported in modes that swap for cross-site navigations. + TestPopupProcess(app, diff_dir_url_, !should_swap_for_cross_site_, + !should_swap_for_cross_site_); + } + { + SCOPED_TRACE("... for same_site popup"); + // TODO(creis): This should stay in the app's process, but that's not yet + // supported in modes that swap for cross-site navigations. + TestPopupProcess(app, same_site_url_, !should_swap_for_cross_site_, + !should_swap_for_cross_site_); + } + { + SCOPED_TRACE("... for isolated_url popup"); + TestPopupProcess(app, isolated_url_, false, false); + } + // For cross-site, the resulting process is only in the app process if we + // don't swap processes. + { + SCOPED_TRACE("... for cross_site popup"); + TestPopupProcess(app, cross_site_url_, !should_swap_for_cross_site_, + !should_swap_for_cross_site_); + } + + // If the iframes open popups that are same-origin with themselves, the popups + // should be in the same process as the respective iframes. + { + SCOPED_TRACE("... for same_dir iframe popup"); + TestPopupProcess(same_dir, same_dir_url_, true, true); + } + { + SCOPED_TRACE("... for diff_dir iframe popup"); + // TODO(creis): This should stay in the app's process, but that's not yet + // supported in modes that swap for cross-site navigations. + TestPopupProcess(diff_dir, diff_dir_url_, !should_swap_for_cross_site_, + !should_swap_for_cross_site_); + } + { + SCOPED_TRACE("... for same_site iframe popup"); + // TODO(creis): This should stay in the app's process, but that's not yet + // supported in modes that swap for cross-site navigations. + TestPopupProcess(same_site, same_site_url_, !should_swap_for_cross_site_, + !should_swap_for_cross_site_); + } + { + SCOPED_TRACE("... for isolated_url iframe popup"); + TestPopupProcess(isolated, isolated_url_, true, false); + } + { + SCOPED_TRACE("... for cross_site iframe popup"); + TestPopupProcess(cross_site, cross_site_url_, true, + !should_swap_for_cross_site_); + } +} + +// Tests that hosted app URLs loaded in iframes of non-app pages won't cause an +// OOPIF unless there is another reason to create it, but popups from outside +// the app will swap into the app. +IN_PROC_BROWSER_TEST_P(HostedAppProcessModelTest, FromOutsideHostedApp) { + // Set up and launch the hosted app. + GURL app_url = + embedded_test_server()->GetURL("app.site.com", "/frame_tree/simple.htm"); + + extensions::TestExtensionDir test_app_dir; + test_app_dir.WriteManifest(base::StringPrintf(kHostedAppProcessModelManifest, + app_url.spec().c_str())); + SetupApp(test_app_dir.UnpackedPath(), false); + + content::WebContents* web_contents = + app_browser_->tab_strip_model()->GetActiveWebContents(); + EXPECT_TRUE(content::WaitForLoadStop(web_contents)); + + // Starting same-origin but outside the app, popups should swap to the app. + { + SCOPED_TRACE("... from diff_dir"); + ui_test_utils::NavigateToURL(app_browser_, diff_dir_url_); + RenderFrameHost* main_frame = web_contents->GetMainFrame(); + EXPECT_FALSE(main_frame->GetSiteInstance()->GetSiteURL().SchemeIs( + extensions::kExtensionScheme)); + TestPopupProcess(main_frame, app_url, false, true); + // Subframes in the app should not swap. + RenderFrameHost* diff_dir_rfh = + TestSubframeProcess(main_frame, app_url, true, false); + // Popups from the subframe, though same-origin, should swap to the app. + // See https://crbug.com/89272. + TestPopupProcess(diff_dir_rfh, app_url, false, true); + } + + // Starting same-site but outside the app, popups should swap to the app. + { + SCOPED_TRACE("... from same_site"); + ui_test_utils::NavigateToURL(app_browser_, same_site_url_); + RenderFrameHost* main_frame = web_contents->GetMainFrame(); + EXPECT_FALSE(main_frame->GetSiteInstance()->GetSiteURL().SchemeIs( + extensions::kExtensionScheme)); + TestPopupProcess(main_frame, app_url, false, true); + // Subframes in the app should not swap. + RenderFrameHost* same_site_rfh = + TestSubframeProcess(main_frame, app_url, true, false); + // Popups from the subframe should swap to the app, as above. + TestPopupProcess(same_site_rfh, app_url, false, true); + } + + // Starting on an isolated origin, popups should swap to the app. + { + SCOPED_TRACE("... from isolated_url"); + ui_test_utils::NavigateToURL(app_browser_, isolated_url_); + RenderFrameHost* main_frame = web_contents->GetMainFrame(); + EXPECT_FALSE(main_frame->GetSiteInstance()->GetSiteURL().SchemeIs( + extensions::kExtensionScheme)); + TestPopupProcess(main_frame, app_url, false, true); + // Subframes in the app should swap process. + // TODO(creis): Perhaps this OOPIF should not be an app process? + RenderFrameHost* isolated_rfh = + TestSubframeProcess(main_frame, app_url, false, true); + // Popups from the subframe into the app should be in the app process. + TestPopupProcess(isolated_rfh, app_url, true, true); + } + + // Starting cross-site, popups should swap to the app. + { + SCOPED_TRACE("... from cross_site"); + ui_test_utils::NavigateToURL(app_browser_, cross_site_url_); + RenderFrameHost* main_frame = web_contents->GetMainFrame(); + EXPECT_FALSE(main_frame->GetSiteInstance()->GetSiteURL().SchemeIs( + extensions::kExtensionScheme)); + TestPopupProcess(main_frame, app_url, false, true); + // Subframes in the app should swap if the process model needs it. + // TODO(creis): Perhaps this OOPIF should not be an app process? + RenderFrameHost* cross_site_rfh = + TestSubframeProcess(main_frame, app_url, !should_swap_for_cross_site_, + should_swap_for_cross_site_); + // Popups from the subframe into the app should be in the app process. + TestPopupProcess(cross_site_rfh, app_url, should_swap_for_cross_site_, + true); + } } INSTANTIATE_TEST_CASE_P(/* no prefix */, HostedAppTest, ::testing::Bool()); @@ -685,6 +959,5 @@ HostedAppPWAOnlyTest, ::testing::Values(true)); INSTANTIATE_TEST_CASE_P(/* no prefix */, - HostedAppWithIsolatedOriginsTest, + HostedAppProcessModelTest, ::testing::Bool()); -INSTANTIATE_TEST_CASE_P(/* no prefix */, HostedAppVsTdiTest, ::testing::Bool());
diff --git a/chrome/test/data/frame_tree/cross_origin_but_same_site_frames.html b/chrome/test/data/frame_tree/cross_origin_but_same_site_frames.html index eb6a8b9c..cf1edb4 100644 --- a/chrome/test/data/frame_tree/cross_origin_but_same_site_frames.html +++ b/chrome/test/data/frame_tree/cross_origin_but_same_site_frames.html
@@ -16,6 +16,10 @@ <iframe name="OtherSubdomain-SameSite" src="/cross-site/other.site.com/title1.html"></iframe> <hr> + isolated.site.com site: + <iframe name="Isolated-SameSite" + src="/cross-site/isolated.site.com/title1.html"></iframe> + <hr> cross.domain.com site: <iframe name="CrossSite" src="/cross-site/cross.domain.com/title1.html"></iframe>
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc index 090f7c96..474ff0f0 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
@@ -43,7 +43,7 @@ event_creator_(event_creator), bypass_stats_(bypass_stats), alternative_proxies_broken_(false), - tick_clock_(new base::DefaultTickClock()), + tick_clock_(base::DefaultTickClock::GetInstance()), first_data_saver_request_recorded_(false), io_data_(nullptr), net_log_(net_log) { @@ -154,8 +154,8 @@ } void DataReductionProxyDelegate::SetTickClockForTesting( - std::unique_ptr<base::TickClock> tick_clock) { - tick_clock_ = std::move(tick_clock); + base::TickClock* tick_clock) { + tick_clock_ = tick_clock; // Update |last_network_change_time_| to the provided tick clock's current // time for testing. last_network_change_time_ = tick_clock_->NowTicks();
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h index 32ab61f..4047720 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h
@@ -70,7 +70,7 @@ const net::HostPortPair& proxy_server, const net::HttpResponseHeaders& response_headers) override; - void SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock); + void SetTickClockForTesting(base::TickClock* tick_clock); protected: // Protected so that these methods are accessible for testing. @@ -112,7 +112,7 @@ bool alternative_proxies_broken_; // Tick clock used for obtaining the current time. - std::unique_ptr<base::TickClock> tick_clock_; + base::TickClock* tick_clock_; // True if the metrics related to the first request whose resolved proxy was a // data saver proxy has been recorded. |first_data_saver_request_recorded_| is
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc index 0bfbc6c8..f444ab31 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc
@@ -838,10 +838,8 @@ } TEST_F(DataReductionProxyDelegateTest, TimeToFirstHttpDataSaverRequest) { - std::unique_ptr<base::SimpleTestTickClock> tick_clock( - new base::SimpleTestTickClock()); - base::SimpleTestTickClock* tick_clock_ptr = tick_clock.get(); - proxy_delegate()->SetTickClockForTesting(std::move(tick_clock)); + base::SimpleTestTickClock tick_clock; + proxy_delegate()->SetTickClockForTesting(&tick_clock); const char kResponseHeaders[] = "HTTP/1.1 200 OK\r\n" @@ -852,7 +850,7 @@ { base::HistogramTester histogram_tester; base::TimeDelta advance_time(base::TimeDelta::FromSeconds(1)); - tick_clock_ptr->Advance(advance_time); + tick_clock.Advance(advance_time); FetchURLRequest(GURL("http://example.com/path/"), nullptr, kResponseHeaders, 10); @@ -874,7 +872,7 @@ net::NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests(); base::RunLoop().RunUntilIdle(); - tick_clock_ptr->Advance(advance_time); + tick_clock.Advance(advance_time); FetchURLRequest(GURL("http://example.com/path/"), nullptr, kResponseHeaders, 10); histogram_tester.ExpectUniqueSample(
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc index 38e986d..bc58d38 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
@@ -54,7 +54,7 @@ data_reduction_proxy_enabled_pref_name_(), prefs_(nullptr), config_(nullptr), - clock_(new base::DefaultClock()) {} + clock_(base::DefaultClock::GetInstance()) {} DataReductionProxySettings::~DataReductionProxySettings() { spdy_proxy_auth_enabled_.Destroy();
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h index 362238a..2e2005c 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
@@ -265,7 +265,7 @@ SyntheticFieldTrialRegistrationCallback register_synthetic_field_trial_; // Should not be null. - std::unique_ptr<base::Clock> clock_; + base::Clock* clock_; base::ThreadChecker thread_checker_;
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc index 124a47a..b64c3f1 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc
@@ -72,8 +72,7 @@ } template <class C> -void DataReductionProxySettingsTestBase::ResetSettings( - std::unique_ptr<base::Clock> clock) { +void DataReductionProxySettingsTestBase::ResetSettings(base::Clock* clock) { MockDataReductionProxySettings<C>* settings = new MockDataReductionProxySettings<C>(); settings->config_ = test_context_->config(); @@ -81,7 +80,7 @@ settings->data_reduction_proxy_service_ = test_context_->CreateDataReductionProxyService(settings); if (clock) - settings->clock_ = std::move(clock); + settings->clock_ = clock; EXPECT_CALL(*settings, GetOriginalProfilePrefs()) .Times(AnyNumber()) .WillRepeatedly(Return(test_context_->pref_service())); @@ -93,7 +92,7 @@ // Explicitly generate required instantiations. template void DataReductionProxySettingsTestBase::ResetSettings< - DataReductionProxySettings>(std::unique_ptr<base::Clock> clock); + DataReductionProxySettings>(base::Clock* clock); void DataReductionProxySettingsTestBase::ExpectSetProxyPrefs( bool expected_enabled,
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.h index eb864a6..a61b9c0d 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.h
@@ -50,8 +50,8 @@ void SetUp() override; template <class C> - void ResetSettings(std::unique_ptr<base::Clock> clock); - virtual void ResetSettings(std::unique_ptr<base::Clock> clock) = 0; + void ResetSettings(base::Clock* clock); + virtual void ResetSettings(base::Clock* clock) = 0; void ExpectSetProxyPrefs(bool expected_enabled, bool expected_at_startup); @@ -84,9 +84,8 @@ : public DataReductionProxySettingsTestBase { public: typedef MockDataReductionProxySettings<C> MockSettings; - void ResetSettings(std::unique_ptr<base::Clock> clock) override { - return DataReductionProxySettingsTestBase::ResetSettings<C>( - std::move(clock)); + void ResetSettings(base::Clock* clock) override { + return DataReductionProxySettingsTestBase::ResetSettings<C>(clock); } };
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc index e4d3204..5b072d5 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc
@@ -385,12 +385,11 @@ // enables the data reduction proxy. TEST_F(DataReductionProxySettingsTest, TestDaysSinceEnabledWithTestClock) { const char kUMAEnabledState[] = "DataReductionProxy.DaysSinceEnabled"; - std::unique_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock()); - base::SimpleTestClock* clock_ptr = clock.get(); - clock_ptr->Advance(base::TimeDelta::FromDays(1)); - ResetSettings(std::move(clock)); + base::SimpleTestClock clock; + clock.Advance(base::TimeDelta::FromDays(1)); + ResetSettings(&clock); - base::Time last_enabled_time = clock_ptr->Now(); + base::Time last_enabled_time = clock.Now(); InitPrefMembers(); { @@ -405,7 +404,7 @@ settings_->SetDataReductionProxyEnabled(true /* enabled */); test_context_->RunUntilIdle(); - last_enabled_time = clock_ptr->Now(); + last_enabled_time = clock.Now(); EXPECT_EQ( last_enabled_time, @@ -418,9 +417,9 @@ // Simulate turning off and on of data reduction proxy while Chromium is // running. settings_->SetDataReductionProxyEnabled(false /* enabled */); - clock_ptr->Advance(base::TimeDelta::FromDays(1)); + clock.Advance(base::TimeDelta::FromDays(1)); base::HistogramTester histogram_tester; - last_enabled_time = clock_ptr->Now(); + last_enabled_time = clock.Now(); settings_->spdy_proxy_auth_enabled_.SetValue(true); settings_->MaybeActivateDataReductionProxy(false); @@ -435,7 +434,7 @@ { // Advance clock by a random number of days. int advance_clock_days = 42; - clock_ptr->Advance(base::TimeDelta::FromDays(advance_clock_days)); + clock.Advance(base::TimeDelta::FromDays(advance_clock_days)); base::HistogramTester histogram_tester; // Simulate Chromium start up. Data reduction proxy was enabled // |advance_clock_days| ago. @@ -469,22 +468,21 @@ } TEST_F(DataReductionProxySettingsTest, TestDaysSinceSavingsCleared) { - std::unique_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock()); - base::SimpleTestClock* clock_ptr = clock.get(); - clock_ptr->Advance(base::TimeDelta::FromDays(1)); - ResetSettings(std::move(clock)); + base::SimpleTestClock clock; + clock.Advance(base::TimeDelta::FromDays(1)); + ResetSettings(&clock); InitPrefMembers(); base::HistogramTester histogram_tester; test_context_->pref_service()->SetInt64( prefs::kDataReductionProxySavingsClearedNegativeSystemClock, - clock_ptr->Now().ToInternalValue()); + clock.Now().ToInternalValue()); settings_->data_reduction_proxy_service_->SetIOData( test_context_->io_data()->GetWeakPtr()); test_context_->RunUntilIdle(); - clock_ptr->Advance(base::TimeDelta::FromDays(100)); + clock.Advance(base::TimeDelta::FromDays(100)); // Simulate Chromium startup with data reduction proxy already enabled. settings_->spdy_proxy_auth_enabled_.SetValue(true);
diff --git a/components/previews/content/previews_io_data.cc b/components/previews/content/previews_io_data.cc index 3b0d487..d25744db 100644 --- a/components/previews/content/previews_io_data.cc +++ b/components/previews/content/previews_io_data.cc
@@ -133,7 +133,7 @@ DCHECK(io_task_runner_->BelongsToCurrentThread()); previews_black_list_.reset( new PreviewsBlackList(std::move(previews_opt_out_store), - base::MakeUnique<base::DefaultClock>(), this)); + base::DefaultClock::GetInstance(), this)); ui_task_runner_->PostTask( FROM_HERE, base::Bind(&PreviewsUIService::SetIOData, previews_ui_service_, weak_factory_.GetWeakPtr()));
diff --git a/components/previews/content/previews_io_data_unittest.cc b/components/previews/content/previews_io_data_unittest.cc index d9ca1d8..cf064ff 100644 --- a/components/previews/content/previews_io_data_unittest.cc +++ b/components/previews/content/previews_io_data_unittest.cc
@@ -77,7 +77,7 @@ TestPreviewsBlackList(PreviewsEligibilityReason status, PreviewsBlacklistDelegate* blacklist_delegate) : PreviewsBlackList(nullptr, - base::MakeUnique<base::DefaultClock>(), + base::DefaultClock::GetInstance(), blacklist_delegate), status_(status) {} ~TestPreviewsBlackList() override {}
diff --git a/components/previews/core/previews_black_list.cc b/components/previews/core/previews_black_list.cc index 5251321..e4edfd0a 100644 --- a/components/previews/core/previews_black_list.cc +++ b/components/previews/core/previews_black_list.cc
@@ -54,11 +54,11 @@ PreviewsBlackList::PreviewsBlackList( std::unique_ptr<PreviewsOptOutStore> opt_out_store, - std::unique_ptr<base::Clock> clock, + base::Clock* clock, PreviewsBlacklistDelegate* blacklist_delegate) : loaded_(false), opt_out_store_(std::move(opt_out_store)), - clock_(std::move(clock)), + clock_(clock), blacklist_delegate_(blacklist_delegate), weak_factory_(this) { DCHECK(blacklist_delegate_);
diff --git a/components/previews/core/previews_black_list.h b/components/previews/core/previews_black_list.h index 2959ac8b..75a5d46 100644 --- a/components/previews/core/previews_black_list.h +++ b/components/previews/core/previews_black_list.h
@@ -82,7 +82,7 @@ // |blacklist_delegate| is a single object listening for blacklist events, and // it is guaranteed to overlive the life time of |this|. PreviewsBlackList(std::unique_ptr<PreviewsOptOutStore> opt_out_store, - std::unique_ptr<base::Clock> clock, + base::Clock* clock, PreviewsBlacklistDelegate* blacklist_delegate); virtual ~PreviewsBlackList(); @@ -165,7 +165,7 @@ // completed. base::queue<base::Closure> pending_callbacks_; - std::unique_ptr<base::Clock> clock_; + base::Clock* clock_; // The delegate listening to this blacklist. |blacklist_delegate_| lifetime is // guaranteed to overlive |this|.
diff --git a/components/previews/core/previews_black_list_unittest.cc b/components/previews/core/previews_black_list_unittest.cc index b52ac0f..a2871caa 100644 --- a/components/previews/core/previews_black_list_unittest.cc +++ b/components/previews/core/previews_black_list_unittest.cc
@@ -157,15 +157,12 @@ "Enabled")); params_.clear(); } - std::unique_ptr<base::SimpleTestClock> test_clock = - base::MakeUnique<base::SimpleTestClock>(); - test_clock_ = test_clock.get(); std::unique_ptr<TestPreviewsOptOutStore> opt_out_store = null_opt_out ? nullptr : base::MakeUnique<TestPreviewsOptOutStore>(); opt_out_store_ = opt_out_store.get(); black_list_ = base::MakeUnique<PreviewsBlackList>( - std::move(opt_out_store), std::move(test_clock), &blacklist_delegate_); - start_ = test_clock_->Now(); + std::move(opt_out_store), &test_clock_, &blacklist_delegate_); + start_ = test_clock_.Now(); } void SetHostHistoryParam(size_t host_history) { @@ -216,13 +213,13 @@ StartTest(false /* null_opt_out */); if (!short_time) - test_clock_->Advance( + test_clock_.Advance( base::TimeDelta::FromSeconds(single_opt_out_duration)); black_list_->AddPreviewNavigation(url, true /* opt_out */, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); - black_list_->ClearBlackList(start_, test_clock_->Now()); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); + black_list_->ClearBlackList(start_, test_clock_.Now()); base::RunLoop().RunUntilIdle(); } @@ -232,8 +229,7 @@ // Observer to |black_list_|. TestPreviewsBlacklistDelegate blacklist_delegate_; - // Unowned raw pointers tied to the lifetime of |black_list_|. - base::SimpleTestClock* test_clock_; + base::SimpleTestClock test_clock_; TestPreviewsOptOutStore* opt_out_store_; base::Time start_; std::map<std::string, std::string> params_; @@ -263,7 +259,7 @@ StartTest(true /* null_opt_out */); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(PreviewsEligibilityReason::ALLOWED, black_list_->IsLoadedAndAllowed(url_a, PreviewsType::OFFLINE)); @@ -271,9 +267,9 @@ black_list_->IsLoadedAndAllowed(url_b, PreviewsType::OFFLINE)); black_list_->AddPreviewNavigation(url_a, true, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_a, true, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(PreviewsEligibilityReason::HOST_BLACKLISTED, black_list_->IsLoadedAndAllowed(url_a, PreviewsType::OFFLINE)); @@ -281,9 +277,9 @@ black_list_->IsLoadedAndAllowed(url_b, PreviewsType::OFFLINE)); black_list_->AddPreviewNavigation(url_b, true, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_b, true, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(PreviewsEligibilityReason::HOST_BLACKLISTED, black_list_->IsLoadedAndAllowed(url_a, PreviewsType::OFFLINE)); @@ -291,18 +287,18 @@ black_list_->IsLoadedAndAllowed(url_b, PreviewsType::OFFLINE)); black_list_->AddPreviewNavigation(url_b, false, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_b, false, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_b, false, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(PreviewsEligibilityReason::HOST_BLACKLISTED, black_list_->IsLoadedAndAllowed(url_a, PreviewsType::OFFLINE)); EXPECT_EQ(PreviewsEligibilityReason::ALLOWED, black_list_->IsLoadedAndAllowed(url_b, PreviewsType::OFFLINE)); - black_list_->ClearBlackList(start_, test_clock_->Now()); + black_list_->ClearBlackList(start_, test_clock_.Now()); EXPECT_EQ(PreviewsEligibilityReason::ALLOWED, black_list_->IsLoadedAndAllowed(url_a, PreviewsType::OFFLINE)); @@ -329,7 +325,7 @@ StartTest(false /* null_opt_out */); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(PreviewsEligibilityReason::BLACKLIST_DATA_NOT_LOADED, black_list_->IsLoadedAndAllowed(url_a1, PreviewsType::OFFLINE)); @@ -348,9 +344,9 @@ black_list_->IsLoadedAndAllowed(url_b, PreviewsType::OFFLINE)); black_list_->AddPreviewNavigation(url_a1, true, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_a1, true, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(PreviewsEligibilityReason::HOST_BLACKLISTED, black_list_->IsLoadedAndAllowed(url_a1, PreviewsType::OFFLINE)); @@ -360,9 +356,9 @@ black_list_->IsLoadedAndAllowed(url_b, PreviewsType::OFFLINE)); black_list_->AddPreviewNavigation(url_b, true, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_b, true, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(PreviewsEligibilityReason::HOST_BLACKLISTED, black_list_->IsLoadedAndAllowed(url_a1, PreviewsType::OFFLINE)); @@ -372,11 +368,11 @@ black_list_->IsLoadedAndAllowed(url_b, PreviewsType::OFFLINE)); black_list_->AddPreviewNavigation(url_b, false, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_b, false, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_b, false, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(PreviewsEligibilityReason::HOST_BLACKLISTED, black_list_->IsLoadedAndAllowed(url_a1, PreviewsType::OFFLINE)); @@ -427,7 +423,7 @@ SetSingleOptOutDurationParam(0); StartTest(true /* null_opt_out */); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(PreviewsEligibilityReason::ALLOWED, black_list_->IsLoadedAndAllowed(urls[0], PreviewsType::OFFLINE)); @@ -443,7 +439,7 @@ EXPECT_EQ(i != 3 ? PreviewsEligibilityReason::ALLOWED : PreviewsEligibilityReason::USER_BLACKLISTED, black_list_->IsLoadedAndAllowed(urls[0], PreviewsType::OFFLINE)); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); } EXPECT_EQ(PreviewsEligibilityReason::USER_BLACKLISTED, @@ -456,7 +452,7 @@ black_list_->IsLoadedAndAllowed(urls[3], PreviewsType::OFFLINE)); black_list_->AddPreviewNavigation(urls[3], false, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); // New non-opt-out entry will cause these to be allowed now. EXPECT_EQ(PreviewsEligibilityReason::ALLOWED, @@ -493,9 +489,9 @@ EXPECT_EQ(PreviewsEligibilityReason::BLACKLIST_DATA_NOT_LOADED, black_list_->IsLoadedAndAllowed(url, PreviewsType::OFFLINE)); black_list_->AddPreviewNavigation(url, opt_out, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url, opt_out, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(PreviewsEligibilityReason::BLACKLIST_DATA_NOT_LOADED, black_list_->IsLoadedAndAllowed(url, PreviewsType::OFFLINE)); base::RunLoop().RunUntilIdle(); @@ -503,17 +499,17 @@ : PreviewsEligibilityReason::ALLOWED, black_list_->IsLoadedAndAllowed(url, PreviewsType::OFFLINE)); black_list_->AddPreviewNavigation(url, opt_out, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url, opt_out, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); EXPECT_EQ(0, opt_out_store_->clear_blacklist_count()); black_list_->ClearBlackList( - start_, test_clock_->Now() + base::TimeDelta::FromSeconds(1)); + start_, test_clock_.Now() + base::TimeDelta::FromSeconds(1)); EXPECT_EQ(1, opt_out_store_->clear_blacklist_count()); black_list_->AddPreviewNavigation(url2, opt_out, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url2, opt_out, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); base::RunLoop().RunUntilIdle(); EXPECT_EQ(1, opt_out_store_->clear_blacklist_count()); @@ -550,9 +546,9 @@ StartTest(true /* null_opt_out */); black_list_->AddPreviewNavigation(url_a, true, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_b, false, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_c, false, PreviewsType::OFFLINE); // url_a should stay in the map, since it has an opt out time. EXPECT_EQ(PreviewsEligibilityReason::HOST_BLACKLISTED, @@ -562,9 +558,9 @@ EXPECT_EQ(PreviewsEligibilityReason::ALLOWED, black_list_->IsLoadedAndAllowed(url_c, PreviewsType::OFFLINE)); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_d, true, PreviewsType::OFFLINE); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_e, true, PreviewsType::OFFLINE); // url_d and url_e should remain in the map, but url_a should be evicted. EXPECT_EQ(PreviewsEligibilityReason::ALLOWED, @@ -602,7 +598,7 @@ EXPECT_EQ(PreviewsEligibilityReason::ALLOWED, black_list_->IsLoadedAndAllowed(url_c, PreviewsType::OFFLINE)); - test_clock_->Advance( + test_clock_.Advance( base::TimeDelta::FromSeconds(single_opt_out_duration + 1)); black_list_->AddPreviewNavigation(url_b, true, PreviewsType::OFFLINE); @@ -611,7 +607,7 @@ EXPECT_EQ(PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT, black_list_->IsLoadedAndAllowed(url_c, PreviewsType::OFFLINE)); - test_clock_->Advance( + test_clock_.Advance( base::TimeDelta::FromSeconds(single_opt_out_duration - 1)); EXPECT_EQ(PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT, @@ -619,7 +615,7 @@ EXPECT_EQ(PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT, black_list_->IsLoadedAndAllowed(url_c, PreviewsType::OFFLINE)); - test_clock_->Advance( + test_clock_.Advance( base::TimeDelta::FromSeconds(single_opt_out_duration + 1)); EXPECT_EQ(PreviewsEligibilityReason::ALLOWED, @@ -686,22 +682,22 @@ black_list_->IsLoadedAndAllowed(url_, PreviewsType::OFFLINE)); // Observer is not notified as blacklisted when the threshold does not met. - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_, true, PreviewsType::OFFLINE); base::RunLoop().RunUntilIdle(); EXPECT_THAT(blacklist_delegate_.blacklisted_hosts(), ::testing::SizeIs(0)); // Observer is notified as blacklisted when the threshold is met. - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_, true, PreviewsType::OFFLINE); base::RunLoop().RunUntilIdle(); - const base::Time blacklisted_time = test_clock_->Now(); + const base::Time blacklisted_time = test_clock_.Now(); EXPECT_THAT(blacklist_delegate_.blacklisted_hosts(), ::testing::SizeIs(1)); EXPECT_EQ(blacklisted_time, blacklist_delegate_.blacklisted_hosts().find(url_.host())->second); // Observer is not notified when the host is already blacklisted. - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(url_, true, PreviewsType::OFFLINE); base::RunLoop().RunUntilIdle(); EXPECT_THAT(blacklist_delegate_.blacklisted_hosts(), ::testing::SizeIs(1)); @@ -711,12 +707,12 @@ // Observer is notified when blacklist is cleared. EXPECT_FALSE(blacklist_delegate_.blacklist_cleared()); - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); - black_list_->ClearBlackList(start_, test_clock_->Now()); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); + black_list_->ClearBlackList(start_, test_clock_.Now()); base::RunLoop().RunUntilIdle(); EXPECT_TRUE(blacklist_delegate_.blacklist_cleared()); - EXPECT_EQ(test_clock_->Now(), blacklist_delegate_.blacklist_cleared_time()); + EXPECT_EQ(test_clock_.Now(), blacklist_delegate_.blacklist_cleared_time()); } TEST_F(PreviewsBlackListTest, ObserverIsNotifiedOnUserBlacklisted) { @@ -745,7 +741,7 @@ EXPECT_FALSE(blacklist_delegate_.user_blacklisted()); for (size_t i = 0; i < host_indifferent_threshold; ++i) { - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(urls[i], true, PreviewsType::OFFLINE); base::RunLoop().RunUntilIdle(); @@ -757,7 +753,7 @@ } // Observer is notified when the user is no longer blacklisted. - test_clock_->Advance(base::TimeDelta::FromSeconds(1)); + test_clock_.Advance(base::TimeDelta::FromSeconds(1)); black_list_->AddPreviewNavigation(urls[3], false, PreviewsType::OFFLINE); base::RunLoop().RunUntilIdle(); @@ -784,12 +780,11 @@ std::unique_ptr<PreviewsBlackListItem> host_indifferent_item = PreviewsBlackList::CreateHostIndifferentBlackListItem(); - std::unique_ptr<base::SimpleTestClock> test_clock = - base::MakeUnique<base::SimpleTestClock>(); + base::SimpleTestClock test_clock; for (size_t i = 0; i < host_indifferent_threshold; ++i) { - test_clock->Advance(base::TimeDelta::FromSeconds(1)); - host_indifferent_item->AddPreviewNavigation(true, test_clock->Now()); + test_clock.Advance(base::TimeDelta::FromSeconds(1)); + host_indifferent_item->AddPreviewNavigation(true, test_clock.Now()); } std::unique_ptr<TestPreviewsOptOutStore> opt_out_store = @@ -799,7 +794,7 @@ EXPECT_FALSE(blacklist_delegate_.user_blacklisted()); auto black_list = base::MakeUnique<PreviewsBlackList>( - std::move(opt_out_store), std::move(test_clock), &blacklist_delegate_); + std::move(opt_out_store), &test_clock, &blacklist_delegate_); base::RunLoop().RunUntilIdle(); EXPECT_TRUE(blacklist_delegate_.user_blacklisted()); } @@ -822,8 +817,7 @@ StartTest(false /* null_opt_out */); - std::unique_ptr<base::SimpleTestClock> test_clock = - base::MakeUnique<base::SimpleTestClock>(); + base::SimpleTestClock test_clock; PreviewsBlackListItem* item_a = new PreviewsBlackListItem( params::MaxStoredHistoryLengthForPerHostBlackList(), @@ -835,18 +829,18 @@ params::PerHostBlackListDuration()); // Host |url_a| is blacklisted. - test_clock->Advance(base::TimeDelta::FromSeconds(1)); - item_a->AddPreviewNavigation(true, test_clock->Now()); - test_clock->Advance(base::TimeDelta::FromSeconds(1)); - item_a->AddPreviewNavigation(true, test_clock->Now()); - base::Time blacklisted_time = test_clock->Now(); + test_clock.Advance(base::TimeDelta::FromSeconds(1)); + item_a->AddPreviewNavigation(true, test_clock.Now()); + test_clock.Advance(base::TimeDelta::FromSeconds(1)); + item_a->AddPreviewNavigation(true, test_clock.Now()); + base::Time blacklisted_time = test_clock.Now(); base::RunLoop().RunUntilIdle(); - EXPECT_TRUE(item_a->IsBlackListed(test_clock->Now())); + EXPECT_TRUE(item_a->IsBlackListed(test_clock.Now())); // Host |url_b| is not blacklisted. - test_clock->Advance(base::TimeDelta::FromSeconds(1)); - item_b->AddPreviewNavigation(true, test_clock->Now()); + test_clock.Advance(base::TimeDelta::FromSeconds(1)); + item_b->AddPreviewNavigation(true, test_clock.Now()); std::unique_ptr<BlackListItemMap> item_map = base::MakeUnique<BlackListItemMap>(); @@ -858,7 +852,7 @@ opt_out_store->SetBlacklistItemMap(std::move(item_map)); auto black_list = base::MakeUnique<PreviewsBlackList>( - std::move(opt_out_store), std::move(test_clock), &blacklist_delegate_); + std::move(opt_out_store), &test_clock, &blacklist_delegate_); base::RunLoop().RunUntilIdle(); ASSERT_THAT(blacklist_delegate_.blacklisted_hosts(), ::testing::SizeIs(1));
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc index 0663535..53b0f5b 100644 --- a/content/browser/frame_host/render_frame_host_manager.cc +++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -976,9 +976,10 @@ BrowserContext* browser_context, GURL dest_url, SiteInstanceRelation relation_to_current) - : existing_site_instance(nullptr), relation(relation_to_current) { - new_site_url = SiteInstance::GetSiteForURL(browser_context, dest_url); -} + : existing_site_instance(nullptr), + dest_url(dest_url), + browser_context(browser_context), + relation(relation_to_current) {} void RenderFrameHostManager::RenderProcessGone(SiteInstanceImpl* instance) { GetRenderFrameProxyHost(instance)->set_render_frame_proxy_created(false); @@ -1424,22 +1425,31 @@ if (IsCurrentlySameSite(render_frame_host_.get(), dest_url)) return SiteInstanceDescriptor(render_frame_host_->GetSiteInstance()); - if (SiteIsolationPolicy::IsTopDocumentIsolationEnabled()) { - // TODO(nick): Looking at the main frame and openers is required for TDI - // mode, but should be safe to enable unconditionally. - if (!frame_tree_node_->IsMainFrame()) { - RenderFrameHostImpl* main_frame = - frame_tree_node_->frame_tree()->root()->current_frame_host(); - if (IsCurrentlySameSite(main_frame, dest_url)) - return SiteInstanceDescriptor(main_frame->GetSiteInstance()); - } - - if (frame_tree_node_->opener()) { - RenderFrameHostImpl* opener_frame = - frame_tree_node_->opener()->current_frame_host(); - if (IsCurrentlySameSite(opener_frame, dest_url)) - return SiteInstanceDescriptor(opener_frame->GetSiteInstance()); - } + // Shortcut some common cases for reusing an existing frame's SiteInstance. + // Looking at the main frame and openers is required for TDI mode. It also + // helps with hosted apps, allowing same-site, non-app subframes to be kept + // inside the hosted app process. + // + // TODO(alexmos): Normally, we'd find these SiteInstances later, as part of + // creating a new related SiteInstance from + // BrowsingInstance::GetSiteInstanceForURL(), but the lookup there does not + // properly deal with hosted apps. Once that's refactored to skip effective + // URLs when necessary, this can be removed. See https://crbug.com/718516. + if (!frame_tree_node_->IsMainFrame()) { + RenderFrameHostImpl* main_frame = + frame_tree_node_->frame_tree()->root()->current_frame_host(); + if (IsCurrentlySameSite(main_frame, dest_url)) + return SiteInstanceDescriptor(main_frame->GetSiteInstance()); + RenderFrameHostImpl* parent = + frame_tree_node_->parent()->current_frame_host(); + if (IsCurrentlySameSite(parent, dest_url)) + return SiteInstanceDescriptor(parent->GetSiteInstance()); + } + if (frame_tree_node_->opener()) { + RenderFrameHostImpl* opener_frame = + frame_tree_node_->opener()->current_frame_host(); + if (IsCurrentlySameSite(opener_frame, dest_url)) + return SiteInstanceDescriptor(opener_frame->GetSiteInstance()); } if (!frame_tree_node_->IsMainFrame() && @@ -1547,7 +1557,7 @@ // Note: If the |candidate_instance| matches the descriptor, // GetRelatedSiteInstance will return it. if (descriptor.relation == SiteInstanceRelation::RELATED) - return current_instance->GetRelatedSiteInstance(descriptor.new_site_url); + return current_instance->GetRelatedSiteInstance(descriptor.dest_url); if (descriptor.relation == SiteInstanceRelation::RELATED_DEFAULT_SUBFRAME) return current_instance->GetDefaultSubframeSiteInstance(); @@ -1556,14 +1566,16 @@ // check if the candidate matches. if (candidate_instance && !current_instance->IsRelatedSiteInstance(candidate_instance) && - candidate_instance->GetSiteURL() == descriptor.new_site_url) { + candidate_instance->GetSiteURL() == + SiteInstance::GetSiteForURL(descriptor.browser_context, + descriptor.dest_url)) { return candidate_instance; } // Otherwise return a newly created one. return SiteInstance::CreateForURL( delegate_->GetControllerForRenderManager().GetBrowserContext(), - descriptor.new_site_url); + descriptor.dest_url); } bool RenderFrameHostManager::IsCurrentlySameSite(RenderFrameHostImpl* candidate, @@ -1571,29 +1583,49 @@ BrowserContext* browser_context = delegate_->GetControllerForRenderManager().GetBrowserContext(); + // Don't compare effective URLs for subframe navigations, since we don't want + // to create OOPIFs based on that mechanism (e.g., for hosted apps). + // See https://crbug.com/718516. + // TODO(creis): This should eventually call out to embedder to help decide, + // if we can find a way to make decisions about popups based on their opener. + bool should_compare_effective_urls = frame_tree_node_->IsMainFrame(); + // If the process type is incorrect, reject the candidate even if |dest_url| // is same-site. (The URL may have been installed as an app since // the last time we visited it.) - if (candidate->GetSiteInstance()->HasWrongProcessForURL(dest_url)) + // + // This check must be skipped to keep same-site subframe navigations from a + // hosted app to non-hosted app, and vice versa, in the same process. + // Otherwise, this would return false due to a process privilege level + // mismatch. + bool src_or_dest_has_effective_url = + (SiteInstanceImpl::HasEffectiveURL(browser_context, dest_url) || + SiteInstanceImpl::HasEffectiveURL( + browser_context, candidate->GetSiteInstance()->original_url())); + bool should_check_for_wrong_process = + should_compare_effective_urls || !src_or_dest_has_effective_url; + if (should_check_for_wrong_process && + candidate->GetSiteInstance()->HasWrongProcessForURL(dest_url)) return false; // If we don't have a last successful URL, we can't trust the origin or URL - // stored on the frame, so we fall back to GetSiteURL(). This case occurs - // after commits of net errors, since net errors do not currently swap - // processes for transfer navigations. Note: browser-initiated net errors do - // swap processes, but the frame's last successful URL will still be empty in - // that case. + // stored on the frame, so we fall back to the SiteInstance URL. This case + // matters for newly created frames which haven't committed a navigation yet, + // as well as for net errors. Note that we use the SiteInstance's + // original_url() and not the site URL, so that we can do this comparison + // without the effective URL resolution if needed. if (candidate->last_successful_url().is_empty()) { - // TODO(creis): GetSiteURL() is not 100% accurate. Eliminate this fallback. - return SiteInstance::IsSameWebSite( - browser_context, candidate->GetSiteInstance()->GetSiteURL(), dest_url); + return SiteInstanceImpl::IsSameWebSite( + browser_context, candidate->GetSiteInstance()->original_url(), dest_url, + should_compare_effective_urls); } // In the common case, we use the RenderFrameHost's last successful URL. Thus, // we compare against the last successful commit when deciding whether to swap // this time. - if (SiteInstance::IsSameWebSite(browser_context, - candidate->last_successful_url(), dest_url)) { + if (SiteInstanceImpl::IsSameWebSite( + browser_context, candidate->last_successful_url(), dest_url, + should_compare_effective_urls)) { return true; } @@ -1601,9 +1633,10 @@ // example, "about:blank"). If so, examine the replicated origin to determine // the site. if (!candidate->GetLastCommittedOrigin().unique() && - SiteInstance::IsSameWebSite( + SiteInstanceImpl::IsSameWebSite( browser_context, - GURL(candidate->GetLastCommittedOrigin().Serialize()), dest_url)) { + GURL(candidate->GetLastCommittedOrigin().Serialize()), dest_url, + should_compare_effective_urls)) { return true; }
diff --git a/content/browser/frame_host/render_frame_host_manager.h b/content/browser/frame_host/render_frame_host_manager.h index 75cf8ae..804c4fba 100644 --- a/content/browser/frame_host/render_frame_host_manager.h +++ b/content/browser/frame_host/render_frame_host_manager.h
@@ -551,8 +551,12 @@ // Set with an existing SiteInstance to be reused. content::SiteInstance* existing_site_instance; - // In case |existing_site_instance| is null, specify a new site URL. - GURL new_site_url; + // In case |existing_site_instance| is null, specify a destination URL. + GURL dest_url; + + // In case |existing_site_instance| is null, specify a BrowsingContext, to + // be used with |dest_url| to resolve the site URL. + BrowserContext* browser_context; // In case |existing_site_instance| is null, specify how the new site is // related to the current BrowsingInstance.
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc index 2e0e2bf0..334e84d 100644 --- a/content/browser/frame_host/render_frame_host_manager_unittest.cc +++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -3236,4 +3236,36 @@ main_test_rfh()->frame_tree_node()->active_sandbox_flags()); } +// Check that after a navigation, the final SiteInstance has the correct +// original URL that was used to determine its site URL. +TEST_F(RenderFrameHostManagerTest, + SiteInstanceOriginalURLIsPreservedAfterNavigation) { + const GURL kFooUrl("https://foo.com"); + const GURL kOriginalUrl("https://original.com"); + const GURL kTranslatedUrl("https://translated.com"); + EffectiveURLContentBrowserClient modified_client(kOriginalUrl, + kTranslatedUrl); + ContentBrowserClient* regular_client = + SetBrowserClientForTesting(&modified_client); + + NavigationSimulator::NavigateAndCommitFromBrowser(contents(), kFooUrl); + scoped_refptr<SiteInstanceImpl> initial_instance = + main_test_rfh()->GetSiteInstance(); + EXPECT_EQ(kFooUrl, initial_instance->original_url()); + EXPECT_EQ(kFooUrl, initial_instance->GetSiteURL()); + + // Simulate a browser-initiated navigation to an app URL, which should swap + // processes and create a new related SiteInstance in the same + // BrowsingInstance. This new SiteInstance should have correct site URL and + // |original_url()|. + NavigationSimulator::NavigateAndCommitFromBrowser(contents(), kOriginalUrl); + EXPECT_NE(initial_instance.get(), main_test_rfh()->GetSiteInstance()); + EXPECT_TRUE(initial_instance->IsRelatedSiteInstance( + main_test_rfh()->GetSiteInstance())); + EXPECT_EQ(kOriginalUrl, main_test_rfh()->GetSiteInstance()->original_url()); + EXPECT_EQ(kTranslatedUrl, main_test_rfh()->GetSiteInstance()->GetSiteURL()); + + SetBrowserClientForTesting(regular_client); +} + } // namespace content
diff --git a/content/browser/renderer_host/render_process_host_unittest.cc b/content/browser/renderer_host/render_process_host_unittest.cc index d2855208..a6ecd7a 100644 --- a/content/browser/renderer_host/render_process_host_unittest.cc +++ b/content/browser/renderer_host/render_process_host_unittest.cc
@@ -649,26 +649,6 @@ EXPECT_EQ(speculative_process_host_id, site_instance->GetProcess()->GetID()); } -class EffectiveURLContentBrowserClient : public ContentBrowserClient { - public: - EffectiveURLContentBrowserClient(const GURL& url_to_modify, - const GURL& url_to_return) - : url_to_modify_(url_to_modify), url_to_return_(url_to_return) {} - ~EffectiveURLContentBrowserClient() override {} - - private: - GURL GetEffectiveURL(BrowserContext* browser_context, - const GURL& url, - bool is_isolated_origin) override { - if (url == url_to_modify_) - return url_to_return_; - return url; - } - - GURL url_to_modify_; - GURL url_to_return_; -}; - // Tests that RenderProcessHost reuse works correctly even if the site URL of a // URL changes. TEST_F(RenderProcessHostUnitTest, ReuseSiteURLChanges) {
diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc index 9d9415f..827e674c 100644 --- a/content/browser/site_instance_impl.cc +++ b/content/browser/site_instance_impl.cc
@@ -165,6 +165,7 @@ has_site_ = true; BrowserContext* browser_context = browsing_instance_->browser_context(); site_ = GetSiteForURL(browser_context, url); + original_url_ = url; // Now that we have a site, register it with the BrowsingInstance. This // ensures that we won't create another SiteInstance for this site within @@ -316,10 +317,22 @@ bool SiteInstance::IsSameWebSite(BrowserContext* browser_context, const GURL& real_src_url, const GURL& real_dest_url) { - GURL src_url = SiteInstanceImpl::GetEffectiveURL(browser_context, - real_src_url); - GURL dest_url = SiteInstanceImpl::GetEffectiveURL(browser_context, - real_dest_url); + return SiteInstanceImpl::IsSameWebSite(browser_context, real_src_url, + real_dest_url, true); +} + +bool SiteInstanceImpl::IsSameWebSite(BrowserContext* browser_context, + const GURL& real_src_url, + const GURL& real_dest_url, + bool should_compare_effective_urls) { + GURL src_url = + should_compare_effective_urls + ? SiteInstanceImpl::GetEffectiveURL(browser_context, real_src_url) + : real_src_url; + GURL dest_url = + should_compare_effective_urls + ? SiteInstanceImpl::GetEffectiveURL(browser_context, real_dest_url) + : real_dest_url; // We infer web site boundaries based on the registered domain name of the // top-level page and the scheme. We do not pay attention to the port if @@ -428,6 +441,12 @@ } // static +bool SiteInstanceImpl::HasEffectiveURL(BrowserContext* browser_context, + const GURL& url) { + return GetEffectiveURL(browser_context, url) != url; +} + +// static bool SiteInstanceImpl::DoesSiteRequireDedicatedProcess( BrowserContext* browser_context, const GURL& url) {
diff --git a/content/browser/site_instance_impl.h b/content/browser/site_instance_impl.h index 6236ae3a..7548752 100644 --- a/content/browser/site_instance_impl.h +++ b/content/browser/site_instance_impl.h
@@ -39,6 +39,15 @@ const GURL& url); static bool ShouldAssignSiteForURL(const GURL& url); + // See SiteInstance::IsSameWebSite. + // This version allows comparing URLs without converting them to effective + // URLs first, which is useful for avoiding OOPIFs when otherwise same-site + // URLs may look cross-site via their effective URLs. + static bool IsSameWebSite(content::BrowserContext* browser_context, + const GURL& src_url, + const GURL& dest_url, + bool should_compare_effective_urls); + // SiteInstance interface overrides. int32_t GetId() override; bool HasProcess() const override; @@ -97,6 +106,15 @@ void set_is_for_service_worker() { is_for_service_worker_ = true; } bool is_for_service_worker() const { return is_for_service_worker_; } + // Returns the URL which was used to set the |site_| for this SiteInstance. + // May be empty if this SiteInstance does not have a |site_|. + const GURL& original_url() { return original_url_; } + + // True if |url| resolves to an effective URL that is different from |url|. + // See GetEffectiveURL(). This will be true for hosted apps as well as NTP + // URLs. + static bool HasEffectiveURL(BrowserContext* browser_context, const GURL& url); + // Returns the SiteInstance, related to this one, that should be used // for subframes when an oopif is required, but a dedicated process is not. // This SiteInstance will be created if it doesn't already exist. There is @@ -229,6 +247,9 @@ // Whether SetSite has been called. bool has_site_; + // The URL which was used to set the |site_| for this SiteInstance. + GURL original_url_; + // The ProcessReusePolicy to use when creating a RenderProcessHost for this // SiteInstance. ProcessReusePolicy process_reuse_policy_;
diff --git a/content/browser/site_instance_impl_unittest.cc b/content/browser/site_instance_impl_unittest.cc index 5b5919f..efcc82f 100644 --- a/content/browser/site_instance_impl_unittest.cc +++ b/content/browser/site_instance_impl_unittest.cc
@@ -1146,4 +1146,50 @@ policy->RemoveIsolatedOriginForTesting(url::Origin::Create(baz_bar_foo_url)); } +// Check that new SiteInstances correctly preserve the full URL that was used +// to initialize their site URL. +TEST_F(SiteInstanceTest, OriginalURL) { + GURL original_url("https://foo.com/"); + GURL app_url("https://app.com/"); + EffectiveURLContentBrowserClient modified_client(original_url, app_url); + ContentBrowserClient* regular_client = + SetBrowserClientForTesting(&modified_client); + std::unique_ptr<TestBrowserContext> browser_context(new TestBrowserContext()); + + // New SiteInstance in a new BrowsingInstance with a predetermined URL. + { + scoped_refptr<SiteInstanceImpl> site_instance = + SiteInstanceImpl::CreateForURL(browser_context.get(), original_url); + EXPECT_EQ(app_url, site_instance->GetSiteURL()); + EXPECT_EQ(original_url, site_instance->original_url()); + } + + // New related SiteInstance from an existing SiteInstance with a + // predetermined URL. + { + scoped_refptr<SiteInstanceImpl> bar_site_instance = + SiteInstanceImpl::CreateForURL(browser_context.get(), + GURL("https://bar.com/")); + scoped_refptr<SiteInstance> site_instance = + bar_site_instance->GetRelatedSiteInstance(original_url); + EXPECT_EQ(app_url, site_instance->GetSiteURL()); + EXPECT_EQ( + original_url, + static_cast<SiteInstanceImpl*>(site_instance.get())->original_url()); + } + + // New SiteInstance with a lazily assigned site URL. + { + scoped_refptr<SiteInstanceImpl> site_instance = + SiteInstanceImpl::Create(browser_context.get()); + EXPECT_FALSE(site_instance->HasSite()); + EXPECT_TRUE(site_instance->original_url().is_empty()); + site_instance->SetSite(original_url); + EXPECT_EQ(app_url, site_instance->GetSiteURL()); + EXPECT_EQ(original_url, site_instance->original_url()); + } + + SetBrowserClientForTesting(regular_client); +} + } // namespace content
diff --git a/content/public/test/test_utils.cc b/content/public/test/test_utils.cc index 56b541a..2784313 100644 --- a/content/public/test/test_utils.cc +++ b/content/public/test/test_utils.cc
@@ -408,4 +408,13 @@ run_loop_.Quit(); } +GURL EffectiveURLContentBrowserClient::GetEffectiveURL( + BrowserContext* browser_context, + const GURL& url, + bool is_isolated_origin) { + if (url == url_to_modify_) + return url_to_return_; + return url; +} + } // namespace content
diff --git a/content/public/test/test_utils.h b/content/public/test/test_utils.h index adbfae1a..9436bdce 100644 --- a/content/public/test/test_utils.h +++ b/content/public/test/test_utils.h
@@ -15,6 +15,7 @@ #include "build/build_config.h" #include "content/public/browser/browser_child_process_observer.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/content_browser_client.h" #include "content/public/browser/notification_details.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" @@ -317,6 +318,26 @@ DISALLOW_COPY_AND_ASSIGN(WebContentsDestroyedWatcher); }; +// A custom ContentBrowserClient that simulates GetEffectiveURL() translation +// for a single URL. +class EffectiveURLContentBrowserClient : public ContentBrowserClient { + public: + EffectiveURLContentBrowserClient(const GURL& url_to_modify, + const GURL& url_to_return) + : url_to_modify_(url_to_modify), url_to_return_(url_to_return) {} + ~EffectiveURLContentBrowserClient() override {} + + private: + GURL GetEffectiveURL(BrowserContext* browser_context, + const GURL& url, + bool is_isolated_origin) override; + + GURL url_to_modify_; + GURL url_to_return_; + + DISALLOW_COPY_AND_ASSIGN(EffectiveURLContentBrowserClient); +}; + } // namespace content #endif // CONTENT_PUBLIC_TEST_TEST_UTILS_H_
diff --git a/gpu/config/gpu_info.cc b/gpu/config/gpu_info.cc index e01e17a9..96cf6dbc 100644 --- a/gpu/config/gpu_info.cc +++ b/gpu/config/gpu_info.cc
@@ -100,7 +100,7 @@ if (secondary_gpu.active) return secondary_gpu; } - DLOG(ERROR) << "No active GPU found, returning primary GPU."; + DLOG(WARNING) << "No active GPU found, returning primary GPU."; return gpu; }
diff --git a/gpu/config/gpu_info_collector.cc b/gpu/config/gpu_info_collector.cc index 6f60582..ebb1c1eb 100644 --- a/gpu/config/gpu_info_collector.cc +++ b/gpu/config/gpu_info_collector.cc
@@ -27,6 +27,10 @@ #include "ui/gl/gl_version_info.h" #include "ui/gl/init/gl_factory.h" +#if defined(OS_ANDROID) +#include "ui/gl/gl_surface_egl.h" +#endif // OS_ANDROID + #if defined(USE_X11) #include "ui/gl/gl_visual_picker_glx.h" #endif @@ -159,6 +163,13 @@ gpu_info->max_msaa_samples = base::IntToString(max_samples); UMA_HISTOGRAM_SPARSE_SLOWLY("GPU.MaxMSAASampleCount", max_samples); +#if defined(OS_ANDROID) + gpu_info->can_support_threaded_texture_mailbox = + gl::GLSurfaceEGL::HasEGLExtension("EGL_KHR_fence_sync") && + gl::GLSurfaceEGL::HasEGLExtension("EGL_KHR_image_base") && + gl::GLSurfaceEGL::HasEGLExtension("EGL_KHR_gl_texture_2D_image") && + gl::HasExtension(extension_set, "GL_OES_EGL_image"); +#else gl::GLWindowSystemBindingInfo window_system_binding_info; if (gl::init::GetGLWindowSystemBindingInfo(&window_system_binding_info)) { gpu_info->gl_ws_vendor = window_system_binding_info.vendor; @@ -166,6 +177,7 @@ gpu_info->gl_ws_extensions = window_system_binding_info.extensions; gpu_info->direct_rendering = window_system_binding_info.direct_rendering; } +#endif // OS_ANDROID bool supports_robustness = gl::HasExtension(extension_set, "GL_EXT_robustness") ||
diff --git a/gpu/config/gpu_info_collector_android.cc b/gpu/config/gpu_info_collector_android.cc index 0ca1fdb..d57fc35 100644 --- a/gpu/config/gpu_info_collector_android.cc +++ b/gpu/config/gpu_info_collector_android.cc
@@ -282,11 +282,18 @@ namespace gpu { CollectInfoResult CollectContextGraphicsInfo(GPUInfo* gpu_info) { - /// TODO(tobiasjs) Check if CollectGraphicsInfo in gpu_main.cc - /// really only needs basic graphics info on all platforms, and if - /// so switch it to using that and make this the NOP that it really - /// should be, to avoid potential double collection of info. - return CollectBasicGraphicsInfo(gpu_info); + // When command buffer is compiled as a standalone library, the process might + // not have a Java environment. + if (base::android::IsVMInitialized()) { + gpu_info->machine_model_name = + base::android::BuildInfo::GetInstance()->model(); + } + + // At this point GL bindings have been initialized already. + CollectInfoResult result = CollectGraphicsInfoGL(gpu_info); + gpu_info->basic_info_state = result; + gpu_info->context_info_state = result; + return result; } CollectInfoResult CollectBasicGraphicsInfo(GPUInfo* gpu_info) {
diff --git a/gpu/ipc/service/gpu_init.cc b/gpu/ipc/service/gpu_init.cc index 03e40b0..56da3b4 100644 --- a/gpu/ipc/service/gpu_init.cc +++ b/gpu/ipc/service/gpu_init.cc
@@ -102,19 +102,13 @@ bool GpuInit::InitializeAndStartSandbox(base::CommandLine* command_line, const GpuPreferences& gpu_preferences) { gpu_preferences_ = gpu_preferences; -#if defined(OS_ANDROID) - // Android doesn't have PCI vendor/device IDs, so collecting GL strings early - // is necessary. - CollectGraphicsInfo(&gpu_info_); - if (gpu_info_.context_info_state == gpu::kCollectInfoFatalFailure) - return false; -#else + gpu_info_.in_process_gpu = false; +#if !defined(OS_ANDROID) // Get vendor_id, device_id, driver_version from browser process through // commandline switches. // TODO(zmo): Collect basic GPU info (without a context) here instead of // passing from browser process. GetGpuInfoFromCommandLine(*command_line, &gpu_info_); -#endif // OS_ANDROID // Set keys for crash logging based on preliminary gpu info, in case we // crash during feature collection. @@ -125,7 +119,6 @@ gpu_info_.driver_vendor == "NVIDIA" && !CanAccessNvidiaDeviceFile()) return false; #endif - gpu_info_.in_process_gpu = false; // Compute blacklist and driver bug workaround decisions based on basic GPU // info. @@ -139,6 +132,7 @@ .status_values[GPU_FEATURE_TYPE_ACCELERATED_VIDEO_DECODE]) { gpu_preferences_.disable_accelerated_video_decode = true; } +#endif // OS_ANDROID // In addition to disabling the watchdog if the command line switch is // present, disable the watchdog on valgrind because the code is expected @@ -222,10 +216,11 @@ // multiple seconds to finish, which in turn cause the GPU process to crash. // By skipping the following code on Mac, we don't really lose anything, // because the basic GPU information is passed down from the host process. -#if !defined(OS_MACOSX) && !defined(OS_ANDROID) +#if !defined(OS_MACOSX) CollectGraphicsInfo(&gpu_info_); if (gpu_info_.context_info_state == gpu::kCollectInfoFatalFailure) return false; + gpu::SetKeysForCrashLogging(gpu_info_); gpu_feature_info_ = gpu::ComputeGpuFeatureInfo(gpu_info_, command_line); if (kGpuFeatureStatusEnabled != gpu_feature_info_ @@ -290,13 +285,11 @@ gpu_info_ = *gpu_info; gpu_feature_info_ = *gpu_feature_info; } else { -#if defined(OS_ANDROID) - gpu::CollectContextGraphicsInfo(&gpu_info_); -#else +#if !defined(OS_ANDROID) // TODO(zmo): Collect basic GPU info here instead. gpu::GetGpuInfoFromCommandLine(*command_line, &gpu_info_); -#endif gpu_feature_info_ = gpu::ComputeGpuFeatureInfo(gpu_info_, command_line); +#endif } if (gpu::SwitchableGPUsSupported(gpu_info_, *command_line)) { gpu::InitializeSwitchableGPUs( @@ -308,10 +301,8 @@ return; } -#if !defined(OS_ANDROID) gpu::CollectContextGraphicsInfo(&gpu_info_); gpu_feature_info_ = gpu::ComputeGpuFeatureInfo(gpu_info_, command_line); -#endif if (!gpu_feature_info_.disabled_extensions.empty()) { gl::init::SetDisabledExtensionsPlatform( gpu_feature_info_.disabled_extensions);