Commit 45d93e98 authored by rajendrant's avatar rajendrant Committed by Chromium LUCI CQ

Reland "ImageCompression: Disable on logged-in pages"

This reverts commit 9bdf89ef.

Reason for revert: Landing the failing tests separately

Original change's description:
> Revert "ImageCompression: Disable on logged-in pages"
>
> This reverts commit d672f7b5.
>
> Reason for revert: SubresourceRedirectLoginRobotsBrowserTest.TestCancelBeforeImageLoad is flaky.
> https://ci.chromium.org/p/chromium/builders/ci/Linux%20Tests/96887
>
> Original change's description:
> > ImageCompression: Disable on logged-in pages
> >
> > This CL adds a mojo to disable image compression on likely loggged-in
> > pages.
> >
> > Bug: 1160424
> > Change-Id: I42dda576947aa04e51d3cb3a5fac712945ac9651
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2598858
> > Commit-Queue: rajendrant <rajendrant@chromium.org>
> > Reviewed-by: Michael Crouse <mcrouse@chromium.org>
> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#840523}
>
> TBR=kinuko@chromium.org,rajendrant@chromium.org,mcrouse@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com
>
> Change-Id: I150cc38bc4c2fb7113e6bdcb68c9dad0f6a2fffa
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1160424
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2613025
> Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
> Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#840578}

TBR=kinuko@chromium.org,rajendrant@chromium.org,mstensho@chromium.org,mcrouse@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

Bug: 1160424
Change-Id: Iaf5ff91bad4246c23941b1ae2974d4d4c0d56c39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617213Reviewed-by: default avatarrajendrant <rajendrant@chromium.org>
Reviewed-by: default avatarMichael Crouse <mcrouse@chromium.org>
Reviewed-by: default avatarKinuko Yasuda <kinuko@chromium.org>
Commit-Queue: rajendrant <rajendrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842428}
parent 3b51002a
...@@ -14,6 +14,8 @@ ...@@ -14,6 +14,8 @@
#include "build/build_config.h" #include "build/build_config.h"
#include "chrome/browser/data_reduction_proxy/data_reduction_proxy_chrome_settings.h" #include "chrome/browser/data_reduction_proxy/data_reduction_proxy_chrome_settings.h"
#include "chrome/browser/data_reduction_proxy/data_reduction_proxy_chrome_settings_factory.h" #include "chrome/browser/data_reduction_proxy/data_reduction_proxy_chrome_settings_factory.h"
#include "chrome/browser/login_detection/login_detection_type.h"
#include "chrome/browser/login_detection/login_detection_util.h"
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "chrome/browser/subresource_redirect/https_image_compression_infobar_decider.h" #include "chrome/browser/subresource_redirect/https_image_compression_infobar_decider.h"
#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser.h"
...@@ -301,6 +303,8 @@ class SubresourceRedirectLoginRobotsBrowserTest : public InProcessBrowserTest { ...@@ -301,6 +303,8 @@ class SubresourceRedirectLoginRobotsBrowserTest : public InProcessBrowserTest {
params["robots_rules_receive_timeout"] = "1000"; params["robots_rules_receive_timeout"] = "1000";
enabled_features.emplace_back(blink::features::kSubresourceRedirect, enabled_features.emplace_back(blink::features::kSubresourceRedirect,
params); params);
enabled_features.emplace_back(login_detection::kLoginDetection,
base::FieldTrialParams());
} }
scoped_feature_list_.InitWithFeaturesAndParameters(enabled_features, {}); scoped_feature_list_.InitWithFeaturesAndParameters(enabled_features, {});
InProcessBrowserTest::SetUp(); InProcessBrowserTest::SetUp();
...@@ -748,4 +752,50 @@ IN_PROC_BROWSER_TEST_F(SubresourceRedirectLoginRobotsBrowserTest, ...@@ -748,4 +752,50 @@ IN_PROC_BROWSER_TEST_F(SubresourceRedirectLoginRobotsBrowserTest,
{"/load_image/image.png"}); {"/load_image/image.png"});
} }
IN_PROC_BROWSER_TEST_F(
SubresourceRedirectLoginRobotsBrowserTest,
DISABLE_ON_WIN_MAC_CHROMEOS(TestNoCompressionOnLoggedInPage)) {
robots_rules_server_.AddRobotsRules(GetHttpsTestURL("/"),
{{kRuleTypeAllow, "*"}});
// Trigger OAuth login by triggering OAuth start and complete.
ui_test_utils::NavigateToURL(browser(),
GetHttpsTestURL("/simple.html?initial"));
histogram_tester_.ExpectUniqueSample(
"Login.PageLoad.DetectionType",
login_detection::LoginDetectionType::kNoLogin, 1);
ui_test_utils::NavigateToURL(
browser(), https_test_server_.GetURL("oauth_server.com",
"/simple.html?client_id=user"));
histogram_tester_.ExpectBucketCount(
"Login.PageLoad.DetectionType",
login_detection::LoginDetectionType::kNoLogin, 2);
ui_test_utils::NavigateToURL(browser(),
GetHttpsTestURL("/simple.html?code=123"));
histogram_tester_.ExpectBucketCount(
"Login.PageLoad.DetectionType",
login_detection::LoginDetectionType::kOauthFirstTimeLoginFlow, 1);
// The next navigation will be treated as logged-in.
NavigateAndWaitForLoad(browser(), GetHttpsTestURL("/load_image/image.html"));
histogram_tester_.ExpectBucketCount(
"Login.PageLoad.DetectionType",
login_detection::LoginDetectionType::kOauthLogin, 1);
// No image compression will be triggered.
histogram_tester_.ExpectTotalCount(
"SubresourceRedirect.CompressionAttempt.ResponseCode", 0);
histogram_tester_.ExpectTotalCount(
"SubresourceRedirect.CompressionAttempt.ServerResponded", 0);
histogram_tester_.ExpectTotalCount(
"SubresourceRedirect.RobotsRulesFetcher.ResponseCode", 0);
histogram_tester_.ExpectTotalCount(
"SubresourceRedirect.RobotsRules.Browser.InMemoryCacheHit", 0);
histogram_tester_.ExpectTotalCount(
"SubresourceRedirect.ImageCompressionNotificationInfoBar", 0);
robots_rules_server_.VerifyRequestedOrigins({});
image_compression_server_.VerifyRequestedImagePaths({});
}
} // namespace subresource_redirect } // namespace subresource_redirect
...@@ -4,6 +4,9 @@ ...@@ -4,6 +4,9 @@
#include "chrome/browser/subresource_redirect/subresource_redirect_observer.h" #include "chrome/browser/subresource_redirect/subresource_redirect_observer.h"
#include "chrome/browser/login_detection/login_detection_keyed_service.h"
#include "chrome/browser/login_detection/login_detection_keyed_service_factory.h"
#include "chrome/browser/login_detection/login_detection_type.h"
#include "chrome/browser/optimization_guide/optimization_guide_keyed_service.h" #include "chrome/browser/optimization_guide/optimization_guide_keyed_service.h"
#include "chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.h" #include "chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.h"
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
...@@ -145,13 +148,33 @@ void SubresourceRedirectObserver::DidFinishNavigation( ...@@ -145,13 +148,33 @@ void SubresourceRedirectObserver::DidFinishNavigation(
navigation_handle)) { navigation_handle)) {
return; return;
} }
content::RenderFrameHost* render_frame_host =
navigation_handle->GetRenderFrameHost();
// Handle login robots based compression mode. // Handle login robots based compression mode.
if (ShouldEnableLoginRobotsCheckedCompression()) { if (ShouldEnableLoginRobotsCheckedCompression()) {
auto* login_detection_keyed_service =
login_detection::LoginDetectionKeyedServiceFactory::GetForProfile(
Profile::FromBrowserContext(web_contents()->GetBrowserContext()));
if (!login_detection_keyed_service)
return;
is_https_image_compression_applied_ =
login_detection_keyed_service->GetPersistentLoginDetection(
navigation_handle->GetURL()) ==
login_detection::LoginDetectionType::kNoLogin;
mojo::AssociatedRemote<mojom::SubresourceRedirectHintsReceiver>
hints_receiver;
render_frame_host->GetRemoteAssociatedInterfaces()->GetInterface(
&hints_receiver);
hints_receiver->SetLoggedInState(!is_https_image_compression_applied_);
if (!is_https_image_compression_applied_)
return;
SubresourceRedirectDocumentHost::GetOrCreateForCurrentDocument( SubresourceRedirectDocumentHost::GetOrCreateForCurrentDocument(
navigation_handle->GetRenderFrameHost()); navigation_handle->GetRenderFrameHost());
// TODO(1149853): Handle whether page is logged-in and disable compression.
is_https_image_compression_applied_ = true;
return; return;
} }
...@@ -163,8 +186,6 @@ void SubresourceRedirectObserver::DidFinishNavigation( ...@@ -163,8 +186,6 @@ void SubresourceRedirectObserver::DidFinishNavigation(
if (!optimization_guide_decider) if (!optimization_guide_decider)
return; return;
content::RenderFrameHost* render_frame_host =
navigation_handle->GetRenderFrameHost();
optimization_guide_decider->CanApplyOptimizationAsync( optimization_guide_decider->CanApplyOptimizationAsync(
navigation_handle, optimization_guide::proto::COMPRESS_PUBLIC_IMAGES, navigation_handle, optimization_guide::proto::COMPRESS_PUBLIC_IMAGES,
base::BindOnce( base::BindOnce(
......
...@@ -36,4 +36,10 @@ interface SubresourceRedirectService { ...@@ -36,4 +36,10 @@ interface SubresourceRedirectService {
interface SubresourceRedirectHintsReceiver { interface SubresourceRedirectHintsReceiver {
// Sends the public image URL hints from the browser to renderers. // Sends the public image URL hints from the browser to renderers.
SetCompressPublicImagesHints(CompressPublicImagesHints images_hints); SetCompressPublicImagesHints(CompressPublicImagesHints images_hints);
// Sets whether the current page is determined to be logged in. This guides
// whether subresource redirect based compression will be enabled. Sent from
// the browser to renderers on navigation commit.
SetLoggedInState(bool is_logged_in);
}; };
...@@ -32,6 +32,7 @@ RedirectResult ConvertToRedirectResult( ...@@ -32,6 +32,7 @@ RedirectResult ConvertToRedirectResult(
case RobotsRulesParser::CheckResult::kAllowed: case RobotsRulesParser::CheckResult::kAllowed:
return RedirectResult::kRedirectable; return RedirectResult::kRedirectable;
case RobotsRulesParser::CheckResult::kDisallowed: case RobotsRulesParser::CheckResult::kDisallowed:
case RobotsRulesParser::CheckResult::kInvalidated:
return RedirectResult::kIneligibleRobotsDisallowed; return RedirectResult::kIneligibleRobotsDisallowed;
case RobotsRulesParser::CheckResult::kTimedout: case RobotsRulesParser::CheckResult::kTimedout:
case RobotsRulesParser::CheckResult::kDisallowedAfterTimeout: case RobotsRulesParser::CheckResult::kDisallowedAfterTimeout:
...@@ -39,12 +40,10 @@ RedirectResult ConvertToRedirectResult( ...@@ -39,12 +40,10 @@ RedirectResult ConvertToRedirectResult(
} }
} }
// Converts the robots rules CheckResult to RedirectResult and passes to the void RecordRedirectResultMetric(RedirectResult redirect_result) {
// callback. LOCAL_HISTOGRAM_ENUMERATION(
void SendRedirectResultToCallback( "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
LoginRobotsDeciderAgent::ShouldRedirectDecisionCallback callback, redirect_result);
RobotsRulesParser::CheckResult check_result) {
std::move(callback).Run(ConvertToRedirectResult(check_result));
} }
} // namespace } // namespace
...@@ -65,6 +64,8 @@ LoginRobotsDeciderAgent::ShouldRedirectSubresource( ...@@ -65,6 +64,8 @@ LoginRobotsDeciderAgent::ShouldRedirectSubresource(
ShouldRedirectDecisionCallback callback) { ShouldRedirectDecisionCallback callback) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(url.is_valid()); DCHECK(url.is_valid());
if (redirect_result_ != RedirectResult::kRedirectable)
return redirect_result_;
if (!render_frame()->IsMainFrame()) if (!render_frame()->IsMainFrame())
return RedirectResult::kIneligibleSubframeResource; return RedirectResult::kIneligibleSubframeResource;
...@@ -83,21 +84,51 @@ LoginRobotsDeciderAgent::ShouldRedirectSubresource( ...@@ -83,21 +84,51 @@ LoginRobotsDeciderAgent::ShouldRedirectSubresource(
base::Optional<RobotsRulesParser::CheckResult> result = base::Optional<RobotsRulesParser::CheckResult> result =
robots_rules_parser_cache.CheckRobotsRules( robots_rules_parser_cache.CheckRobotsRules(
url, routing_id(), url,
base::BindOnce(&SendRedirectResultToCallback, std::move(callback))); base::BindOnce(
if (result) &LoginRobotsDeciderAgent::OnShouldRedirectSubresourceResult,
return ConvertToRedirectResult(*result); weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
if (result) {
RedirectResult redirect_result = ConvertToRedirectResult(*result);
RecordRedirectResultMetric(redirect_result);
return redirect_result;
}
return base::nullopt; return base::nullopt;
} }
void LoginRobotsDeciderAgent::OnShouldRedirectSubresourceResult(
LoginRobotsDeciderAgent::ShouldRedirectDecisionCallback callback,
RobotsRulesParser::CheckResult check_result) {
// Verify if the navigation is still allowed to redirect.
if (redirect_result_ != RedirectResult::kRedirectable) {
RecordRedirectResultMetric(redirect_result_);
std::move(callback).Run(redirect_result_);
return;
}
RedirectResult redirect_result = ConvertToRedirectResult(check_result);
RecordRedirectResultMetric(redirect_result);
std::move(callback).Run(redirect_result);
}
void LoginRobotsDeciderAgent::ReadyToCommitNavigation(
blink::WebDocumentLoader* document_loader) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
redirect_result_ = RedirectResult::kUnknown;
GetRobotsRulesParserCache().InvalidatePendingRequests(routing_id());
}
void LoginRobotsDeciderAgent::SetLoggedInState(bool is_logged_in) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
redirect_result_ = is_logged_in ? RedirectResult::kIneligibleLoginDetected
: RedirectResult::kRedirectable;
}
void LoginRobotsDeciderAgent::RecordMetricsOnLoadFinished( void LoginRobotsDeciderAgent::RecordMetricsOnLoadFinished(
const GURL& url, const GURL& url,
int64_t content_length, int64_t content_length,
RedirectResult redirect_result) { RedirectResult redirect_result) {
LOCAL_HISTOGRAM_ENUMERATION( DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
"SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
redirect_result);
// TODO(crbug.com/1148980): Record coverage metrics // TODO(crbug.com/1148980): Record coverage metrics
} }
......
...@@ -6,6 +6,7 @@ ...@@ -6,6 +6,7 @@
#define CHROME_RENDERER_SUBRESOURCE_REDIRECT_LOGIN_ROBOTS_DECIDER_AGENT_H_ #define CHROME_RENDERER_SUBRESOURCE_REDIRECT_LOGIN_ROBOTS_DECIDER_AGENT_H_
#include "base/macros.h" #include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/threading/thread_checker.h" #include "base/threading/thread_checker.h"
#include "base/timer/timer.h" #include "base/timer/timer.h"
#include "chrome/renderer/subresource_redirect/public_resource_decider_agent.h" #include "chrome/renderer/subresource_redirect/public_resource_decider_agent.h"
...@@ -37,9 +38,14 @@ class LoginRobotsDeciderAgent : public PublicResourceDeciderAgent { ...@@ -37,9 +38,14 @@ class LoginRobotsDeciderAgent : public PublicResourceDeciderAgent {
void UpdateRobotsRulesForTesting(const url::Origin& origin, void UpdateRobotsRulesForTesting(const url::Origin& origin,
const base::Optional<std::string>& rules); const base::Optional<std::string>& rules);
// content::RenderFrameObserver:
void ReadyToCommitNavigation(
blink::WebDocumentLoader* document_loader) override;
// mojom::SubresourceRedirectHintsReceiver: // mojom::SubresourceRedirectHintsReceiver:
void SetCompressPublicImagesHints( void SetCompressPublicImagesHints(
mojom::CompressPublicImagesHintsPtr images_hints) override; mojom::CompressPublicImagesHintsPtr images_hints) override;
void SetLoggedInState(bool is_logged_in) override;
// PublicResourceDeciderAgent: // PublicResourceDeciderAgent:
base::Optional<RedirectResult> ShouldRedirectSubresource( base::Optional<RedirectResult> ShouldRedirectSubresource(
...@@ -49,9 +55,21 @@ class LoginRobotsDeciderAgent : public PublicResourceDeciderAgent { ...@@ -49,9 +55,21 @@ class LoginRobotsDeciderAgent : public PublicResourceDeciderAgent {
int64_t content_length, int64_t content_length,
RedirectResult redirect_result) override; RedirectResult redirect_result) override;
// Callback invoked when should redirect check result is available.
void OnShouldRedirectSubresourceResult(
ShouldRedirectDecisionCallback callback,
RobotsRulesParser::CheckResult check_result);
bool IsMainFrame() const; bool IsMainFrame() const;
// Current state of the redirect compression that should be used for the
// current navigation.
RedirectResult redirect_result_ = RedirectResult::kUnknown;
THREAD_CHECKER(thread_checker_); THREAD_CHECKER(thread_checker_);
// Used to get a weak pointer to |this|.
base::WeakPtrFactory<LoginRobotsDeciderAgent> weak_ptr_factory_{this};
}; };
} // namespace subresource_redirect } // namespace subresource_redirect
......
...@@ -73,6 +73,10 @@ class SubresourceRedirectLoginRobotsDeciderAgentTest ...@@ -73,6 +73,10 @@ class SubresourceRedirectLoginRobotsDeciderAgentTest
result_receiver.redirect_result() == RedirectResult::kRedirectable; result_receiver.redirect_result() == RedirectResult::kRedirectable;
} }
void SetLoggedInState(bool is_logged_in) {
login_robots_decider_agent_->SetLoggedInState(is_logged_in);
}
protected: protected:
void SetUp() override { void SetUp() override {
ChromeRenderViewTest::SetUp(); ChromeRenderViewTest::SetUp();
...@@ -91,6 +95,7 @@ class SubresourceRedirectLoginRobotsDeciderAgentTest ...@@ -91,6 +95,7 @@ class SubresourceRedirectLoginRobotsDeciderAgentTest
TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
TestAllowDisallowSingleOrigin) { TestAllowDisallowSingleOrigin) {
SetLoggedInState(false);
SetUpRobotsRules("https://foo.com", {{kRuleTypeAllow, "/public"}, SetUpRobotsRules("https://foo.com", {{kRuleTypeAllow, "/public"},
{kRuleTypeDisallow, "/private"}}); {kRuleTypeDisallow, "/private"}});
EXPECT_TRUE(ShouldRedirectSubresource("https://foo.com/public.jpg")); EXPECT_TRUE(ShouldRedirectSubresource("https://foo.com/public.jpg"));
...@@ -103,6 +108,7 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, ...@@ -103,6 +108,7 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
TestHTTPRulesAreSeparate) { TestHTTPRulesAreSeparate) {
SetLoggedInState(false);
SetUpRobotsRules("https://foo.com", {{kRuleTypeAllow, "/public"}, SetUpRobotsRules("https://foo.com", {{kRuleTypeAllow, "/public"},
{kRuleTypeDisallow, "/private"}}); {kRuleTypeDisallow, "/private"}});
EXPECT_FALSE(ShouldRedirectSubresource("http://foo.com/public.jpg")); EXPECT_FALSE(ShouldRedirectSubresource("http://foo.com/public.jpg"));
...@@ -116,6 +122,7 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, ...@@ -116,6 +122,7 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
} }
TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, TestURLWithArguments) { TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, TestURLWithArguments) {
SetLoggedInState(false);
SetUpRobotsRules("https://foo.com", SetUpRobotsRules("https://foo.com",
{{kRuleTypeAllow, "/*.jpg$"}, {{kRuleTypeAllow, "/*.jpg$"},
{kRuleTypeDisallow, "/*.png?*arg_disallowed"}, {kRuleTypeDisallow, "/*.png?*arg_disallowed"},
...@@ -140,6 +147,7 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, TestURLWithArguments) { ...@@ -140,6 +147,7 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, TestURLWithArguments) {
TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
TestRulesAreCaseSensitive) { TestRulesAreCaseSensitive) {
SetLoggedInState(false);
SetUpRobotsRules("https://foo.com", {{kRuleTypeAllow, "/allowed"}, SetUpRobotsRules("https://foo.com", {{kRuleTypeAllow, "/allowed"},
{kRuleTypeAllow, "/CamelCase"}, {kRuleTypeAllow, "/CamelCase"},
{kRuleTypeAllow, "/CAPITALIZE"}, {kRuleTypeAllow, "/CAPITALIZE"},
...@@ -152,4 +160,13 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, ...@@ -152,4 +160,13 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
EXPECT_FALSE(ShouldRedirectSubresource("https://foo.com/capitalize.jpg")); EXPECT_FALSE(ShouldRedirectSubresource("https://foo.com/capitalize.jpg"));
} }
TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
TestDisabledWhenLoggedIn) {
SetLoggedInState(true);
SetUpRobotsRules("https://foo.com", {{kRuleTypeAllow, "/public"},
{kRuleTypeDisallow, "/private"}});
EXPECT_FALSE(ShouldRedirectSubresource("https://foo.com/public.jpg"));
EXPECT_FALSE(ShouldRedirectSubresource("https://foo.com/private.jpg"));
}
} // namespace subresource_redirect } // namespace subresource_redirect
...@@ -84,17 +84,13 @@ class LoginRobotsDeciderInfo : public blink::URLLoaderThrottle::Delegate { ...@@ -84,17 +84,13 @@ class LoginRobotsDeciderInfo : public blink::URLLoaderThrottle::Delegate {
NOTIMPLEMENTED(); NOTIMPLEMENTED();
} }
void VerifyRedirectResult(RedirectResult expected_result) { void VerifyWillProcessResponse() {
base::HistogramTester histogram_tester;
network::mojom::URLResponseHeadPtr head = network::mojom::URLResponseHeadPtr head =
network::CreateURLResponseHead(net::HTTP_OK); network::CreateURLResponseHead(net::HTTP_OK);
head->headers->SetHeader("Content-Length", "1024"); head->headers->SetHeader("Content-Length", "1024");
bool defer = false; bool defer = false;
throttle_->WillProcessResponse(GURL("https://foo.com/img.jpg"), head.get(), throttle_->WillProcessResponse(GURL("https://foo.com/img.jpg"), head.get(),
&defer); &defer);
histogram_tester.ExpectUniqueSample(
"SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
expected_result, 1);
EXPECT_FALSE(defer); EXPECT_FALSE(defer);
} }
...@@ -150,6 +146,10 @@ class SubresourceRedirectLoginRobotsURLLoaderThrottleTest ...@@ -150,6 +146,10 @@ class SubresourceRedirectLoginRobotsURLLoaderThrottleTest
blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON)); blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON));
} }
void SetLoggedInState(bool is_logged_in) {
login_robots_decider_agent_->SetLoggedInState(is_logged_in);
}
protected: protected:
void SetUp() override { void SetUp() override {
ChromeRenderViewTest::SetUp(); ChromeRenderViewTest::SetUp();
...@@ -163,9 +163,10 @@ class SubresourceRedirectLoginRobotsURLLoaderThrottleTest ...@@ -163,9 +163,10 @@ class SubresourceRedirectLoginRobotsURLLoaderThrottleTest
&associated_interfaces_, view_->GetMainRenderFrame()); &associated_interfaces_, view_->GetMainRenderFrame());
} }
private: protected:
LoginRobotsDeciderAgent* login_robots_decider_agent_; LoginRobotsDeciderAgent* login_robots_decider_agent_;
base::test::ScopedFeatureList scoped_feature_list_; base::test::ScopedFeatureList scoped_feature_list_;
base::HistogramTester histogram_tester_;
}; };
TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
...@@ -221,6 +222,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, ...@@ -221,6 +222,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
TestGetSubresourceURL) { TestGetSubresourceURL) {
struct TestCase { struct TestCase {
int previews_state; int previews_state;
bool is_logged_in;
std::string original_url; std::string original_url;
GURL redirected_subresource_url; // Empty URL means there will be no GURL redirected_subresource_url; // Empty URL means there will be no
// redirect. // redirect.
...@@ -229,17 +231,20 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, ...@@ -229,17 +231,20 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
const TestCase kTestCases[]{ const TestCase kTestCases[]{
{ {
blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON, blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
false,
"https://www.test.com/public_img.jpg", "https://www.test.com/public_img.jpg",
GetSubresourceURLForURL(GURL("https://www.test.com/public_img.jpg")), GetSubresourceURLForURL(GURL("https://www.test.com/public_img.jpg")),
}, },
{ {
blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON, blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
false,
"https://www.test.com/public_img.jpg#anchor", "https://www.test.com/public_img.jpg#anchor",
GetSubresourceURLForURL( GetSubresourceURLForURL(
GURL("https://www.test.com/public_img.jpg#anchor")), GURL("https://www.test.com/public_img.jpg#anchor")),
}, },
{ {
blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON, blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
false,
"https://www.test.com/public_img.jpg?public_arg1=bar&public_arg2", "https://www.test.com/public_img.jpg?public_arg1=bar&public_arg2",
GetSubresourceURLForURL( GetSubresourceURLForURL(
GURL("https://www.test.com/" GURL("https://www.test.com/"
...@@ -248,14 +253,23 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, ...@@ -248,14 +253,23 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
// Private images will not be redirected. // Private images will not be redirected.
{ {
blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON, blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
false,
"https://www.test.com/private_img.jpg", "https://www.test.com/private_img.jpg",
GURL(), GURL(),
}, },
{ {
blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON, blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
false,
"https://www.test.com/public_img.jpg&private_arg1=foo", "https://www.test.com/public_img.jpg&private_arg1=foo",
GURL(), GURL(),
}, },
// No redirection when logged-in
{
blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
true,
"https://www.test.com/public_img.jpg",
GURL(),
},
}; };
blink::WebNetworkStateNotifier::SetSaveDataEnabled(true); blink::WebNetworkStateNotifier::SetSaveDataEnabled(true);
...@@ -264,6 +278,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, ...@@ -264,6 +278,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
{kRuleTypeDisallow, ""}}); {kRuleTypeDisallow, ""}});
for (const TestCase& test_case : kTestCases) { for (const TestCase& test_case : kTestCases) {
SetLoggedInState(test_case.is_logged_in);
auto throttle = CreateLoginRobotsDecider( auto throttle = CreateLoginRobotsDecider(
test_case.original_url, network::mojom::RequestDestination::kImage, test_case.original_url, network::mojom::RequestDestination::kImage,
test_case.previews_state); test_case.previews_state);
...@@ -288,6 +303,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, ...@@ -288,6 +303,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
TestRobotsRulesSentBeforeThrottle) { TestRobotsRulesSentBeforeThrottle) {
blink::WebNetworkStateNotifier::SetSaveDataEnabled(true); blink::WebNetworkStateNotifier::SetSaveDataEnabled(true);
SetLoggedInState(false);
SetUpRobotsRules("https://www.test.com", SetUpRobotsRules("https://www.test.com",
{{kRuleTypeAllow, "/public"}, {kRuleTypeDisallow, ""}}); {{kRuleTypeAllow, "/public"}, {kRuleTypeDisallow, ""}});
...@@ -296,8 +312,6 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, ...@@ -296,8 +312,6 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
CreateURLLoaderThrottleInfo("https://www.test.com/public.jpg"); CreateURLLoaderThrottleInfo("https://www.test.com/public.jpg");
auto throttle_info2 = auto throttle_info2 =
CreateURLLoaderThrottleInfo("https://www.test.com/private.jpg"); CreateURLLoaderThrottleInfo("https://www.test.com/private.jpg");
throttle_info1->VerifyRedirectResult(RedirectResult::kRedirectable);
throttle_info2->VerifyRedirectResult(RedirectResult::kRedirectable);
throttle_info1->SendStartRequestAndVerifyDeferral( throttle_info1->SendStartRequestAndVerifyDeferral(
WillStartRequestDeferralState::kRedirected); WillStartRequestDeferralState::kRedirected);
...@@ -307,9 +321,14 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, ...@@ -307,9 +321,14 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
EXPECT_FALSE(throttle_info2->did_resume()); EXPECT_FALSE(throttle_info2->did_resume());
EXPECT_FALSE(throttle_info1->did_restart_with_url_reset_and_flags()); EXPECT_FALSE(throttle_info1->did_restart_with_url_reset_and_flags());
EXPECT_FALSE(throttle_info2->did_restart_with_url_reset_and_flags()); EXPECT_FALSE(throttle_info2->did_restart_with_url_reset_and_flags());
throttle_info1->VerifyRedirectResult(RedirectResult::kRedirectable); histogram_tester_.ExpectTotalCount(
throttle_info2->VerifyRedirectResult( "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult", 2);
RedirectResult::kIneligibleRobotsDisallowed); histogram_tester_.ExpectBucketCount(
"SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
RedirectResult::kRedirectable, 1);
histogram_tester_.ExpectBucketCount(
"SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
RedirectResult::kIneligibleRobotsDisallowed, 1);
} }
// Tests the cases when robots rules are sent, after throttles are // Tests the cases when robots rules are sent, after throttles are
...@@ -317,6 +336,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, ...@@ -317,6 +336,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
TestRobotsRulesSentAfterThrottle) { TestRobotsRulesSentAfterThrottle) {
blink::WebNetworkStateNotifier::SetSaveDataEnabled(true); blink::WebNetworkStateNotifier::SetSaveDataEnabled(true);
SetLoggedInState(false);
auto throttle_info1 = auto throttle_info1 =
CreateURLLoaderThrottleInfo("https://www.test.com/public.jpg"); CreateURLLoaderThrottleInfo("https://www.test.com/public.jpg");
...@@ -344,15 +364,23 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, ...@@ -344,15 +364,23 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
EXPECT_TRUE(throttle_info2->did_restart_with_url_reset_and_flags()); EXPECT_TRUE(throttle_info2->did_restart_with_url_reset_and_flags());
EXPECT_TRUE(throttle_info2->did_resume()); EXPECT_TRUE(throttle_info2->did_resume());
throttle_info1->VerifyRedirectResult(RedirectResult::kRedirectable); throttle_info1->VerifyWillProcessResponse();
throttle_info2->VerifyRedirectResult( throttle_info2->VerifyWillProcessResponse();
RedirectResult::kIneligibleRobotsDisallowed); histogram_tester_.ExpectTotalCount(
"SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult", 2);
histogram_tester_.ExpectBucketCount(
"SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
RedirectResult::kRedirectable, 1);
histogram_tester_.ExpectBucketCount(
"SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
RedirectResult::kIneligibleRobotsDisallowed, 1);
} }
// Tests the cases when robots rules retrieval timesout. // Tests the cases when robots rules retrieval timesout.
TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
TestRobotsRulesTimeout) { TestRobotsRulesTimeout) {
blink::WebNetworkStateNotifier::SetSaveDataEnabled(true); blink::WebNetworkStateNotifier::SetSaveDataEnabled(true);
SetLoggedInState(false);
auto throttle_info1 = auto throttle_info1 =
CreateURLLoaderThrottleInfo("https://www.test.com/public.jpg"); CreateURLLoaderThrottleInfo("https://www.test.com/public.jpg");
...@@ -377,10 +405,13 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest, ...@@ -377,10 +405,13 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
EXPECT_TRUE(throttle_info2->did_restart_with_url_reset_and_flags()); EXPECT_TRUE(throttle_info2->did_restart_with_url_reset_and_flags());
EXPECT_TRUE(throttle_info2->did_resume()); EXPECT_TRUE(throttle_info2->did_resume());
throttle_info1->VerifyRedirectResult( throttle_info1->VerifyWillProcessResponse();
RedirectResult::kIneligibleRobotsTimeout); throttle_info2->VerifyWillProcessResponse();
throttle_info2->VerifyRedirectResult( histogram_tester_.ExpectTotalCount(
RedirectResult::kIneligibleRobotsTimeout); "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult", 2);
histogram_tester_.ExpectBucketCount(
"SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
RedirectResult::kIneligibleRobotsTimeout, 2);
} }
} // namespace subresource_redirect } // namespace subresource_redirect
...@@ -160,6 +160,7 @@ void PublicImageHintsDeciderAgent::RecordMetrics( ...@@ -160,6 +160,7 @@ void PublicImageHintsDeciderAgent::RecordMetrics(
break; break;
case RedirectResult::kIneligibleRobotsDisallowed: case RedirectResult::kIneligibleRobotsDisallowed:
case RedirectResult::kIneligibleRobotsTimeout: case RedirectResult::kIneligibleRobotsTimeout:
case RedirectResult::kIneligibleLoginDetected:
NOTREACHED(); NOTREACHED();
} }
mojo::PendingRemote<ukm::mojom::UkmRecorderInterface> recorder; mojo::PendingRemote<ukm::mojom::UkmRecorderInterface> recorder;
...@@ -198,4 +199,10 @@ void PublicImageHintsDeciderAgent::NotifyCompressedResourceFetchFailed( ...@@ -198,4 +199,10 @@ void PublicImageHintsDeciderAgent::NotifyCompressedResourceFetchFailed(
ClearImageHints(); ClearImageHints();
} }
void PublicImageHintsDeciderAgent::SetLoggedInState(bool is_logged_in) {
// This mojo from browser process should not be called for public image hints
// based compression.
NOTIMPLEMENTED();
}
} // namespace subresource_redirect } // namespace subresource_redirect
...@@ -42,6 +42,7 @@ class PublicImageHintsDeciderAgent : public PublicResourceDeciderAgent { ...@@ -42,6 +42,7 @@ class PublicImageHintsDeciderAgent : public PublicResourceDeciderAgent {
// mojom::SubresourceRedirectHintsReceiver: // mojom::SubresourceRedirectHintsReceiver:
void SetCompressPublicImagesHints( void SetCompressPublicImagesHints(
mojom::CompressPublicImagesHintsPtr images_hints) override; mojom::CompressPublicImagesHintsPtr images_hints) override;
void SetLoggedInState(bool is_logged_in) override;
// PublicResourceDeciderAgent: // PublicResourceDeciderAgent:
base::Optional<RedirectResult> ShouldRedirectSubresource( base::Optional<RedirectResult> ShouldRedirectSubresource(
......
...@@ -57,7 +57,10 @@ enum class RedirectResult { ...@@ -57,7 +57,10 @@ enum class RedirectResult {
// Because the robots rules fetch timedout. // Because the robots rules fetch timedout.
kIneligibleRobotsTimeout, kIneligibleRobotsTimeout,
kMaxValue = RedirectResult::kIneligibleRobotsTimeout // Because the page was detected to be logged-in.
kIneligibleLoginDetected,
kMaxValue = RedirectResult::kIneligibleLoginDetected
}; };
} // namespace subresource_redirect } // namespace subresource_redirect
......
...@@ -126,21 +126,27 @@ void RobotsRulesParser::UpdateRobotsRules( ...@@ -126,21 +126,27 @@ void RobotsRulesParser::UpdateRobotsRules(
} }
// Respond to the pending requests, even if robots proto parse failed. // Respond to the pending requests, even if robots proto parse failed.
for (auto& request : pending_check_requests_) { for (auto& requests : pending_check_requests_) {
std::move(request.first).Run(CheckRobotsRulesImmediate(request.second)); for (auto& request : requests.second) {
std::move(request.first).Run(CheckRobotsRulesImmediate(request.second));
}
} }
pending_check_requests_.clear(); pending_check_requests_.clear();
} }
base::Optional<RobotsRulesParser::CheckResult> base::Optional<RobotsRulesParser::CheckResult>
RobotsRulesParser::CheckRobotsRules(const GURL& url, RobotsRulesParser::CheckRobotsRules(int routing_id,
const GURL& url,
CheckResultCallback callback) { CheckResultCallback callback) {
std::string path_with_query = url.path(); std::string path_with_query = url.path();
if (url.has_query()) if (url.has_query())
base::StrAppend(&path_with_query, {"?", url.query()}); base::StrAppend(&path_with_query, {"?", url.query()});
if (rules_receive_state_ == RulesReceiveState::kTimerRunning) { if (rules_receive_state_ == RulesReceiveState::kTimerRunning) {
DCHECK(rules_receive_timeout_timer_.IsRunning()); DCHECK(rules_receive_timeout_timer_.IsRunning());
pending_check_requests_.emplace_back( auto it = pending_check_requests_.insert(std::make_pair(
routing_id,
std::vector<std::pair<CheckResultCallback, std::string>>()));
it.first->second.emplace_back(
std::make_pair(std::move(callback), path_with_query)); std::make_pair(std::move(callback), path_with_query));
return base::nullopt; return base::nullopt;
} }
...@@ -172,11 +178,24 @@ RobotsRulesParser::CheckResult RobotsRulesParser::CheckRobotsRulesImmediate( ...@@ -172,11 +178,24 @@ RobotsRulesParser::CheckResult RobotsRulesParser::CheckRobotsRulesImmediate(
void RobotsRulesParser::OnRulesReceiveTimeout() { void RobotsRulesParser::OnRulesReceiveTimeout() {
DCHECK(!rules_receive_timeout_timer_.IsRunning()); DCHECK(!rules_receive_timeout_timer_.IsRunning());
rules_receive_state_ = RulesReceiveState::kTimeout; rules_receive_state_ = RulesReceiveState::kTimeout;
for (auto& request : pending_check_requests_) for (auto& requests : pending_check_requests_) {
std::move(request.first).Run(CheckResult::kTimedout); for (auto& request : requests.second) {
std::move(request.first).Run(CheckResult::kTimedout);
}
}
pending_check_requests_.clear(); pending_check_requests_.clear();
RecordRobotsRulesReceiveResultHistogram( RecordRobotsRulesReceiveResultHistogram(
SubresourceRedirectRobotsRulesReceiveResult::kTimeout); SubresourceRedirectRobotsRulesReceiveResult::kTimeout);
} }
void RobotsRulesParser::InvalidatePendingRequests(int routing_id) {
auto it = pending_check_requests_.find(routing_id);
if (it == pending_check_requests_.end())
return;
for (auto& request : it->second) {
std::move(request.first).Run(CheckResult::kInvalidated);
}
pending_check_requests_.erase(it);
}
} // namespace subresource_redirect } // namespace subresource_redirect
...@@ -37,6 +37,8 @@ class RobotsRulesParser { ...@@ -37,6 +37,8 @@ class RobotsRulesParser {
kTimedout, // Timeout in retrieving the robots rules kTimedout, // Timeout in retrieving the robots rules
kDisallowedAfterTimeout, // Timeout got triggered already, and the resource kDisallowedAfterTimeout, // Timeout got triggered already, and the resource
// was disallowed // was disallowed
kInvalidated, // The result check was invalidated, before robots rules are
// received or timeout triggered.
}; };
enum class RulesReceiveState { enum class RulesReceiveState {
...@@ -73,10 +75,16 @@ class RobotsRulesParser { ...@@ -73,10 +75,16 @@ class RobotsRulesParser {
// added to |pending_check_requests_| and called when a decision can be made // added to |pending_check_requests_| and called when a decision can be made
// like when rules are retrieved, or rule fetch timeout, etc. // like when rules are retrieved, or rule fetch timeout, etc.
// The robots rules check will make use of the |url| path and query // The robots rules check will make use of the |url| path and query
// parameters.The |url| origin, ref fragment, etc are immaterial. // parameters.The |url| origin, ref fragment, etc are immaterial. |routing_id|
base::Optional<CheckResult> CheckRobotsRules(const GURL& url, // is the render frame ID for which this URL is requested for.
base::Optional<CheckResult> CheckRobotsRules(int routing_id,
const GURL& url,
CheckResultCallback callback); CheckResultCallback callback);
// Invalidate and cancel the pending requests that were added for
// |routing_id|.
void InvalidatePendingRequests(int routing_id);
private: private:
friend class SubresourceRedirectRobotsRulesParserTest; friend class SubresourceRedirectRobotsRulesParserTest;
...@@ -107,9 +115,10 @@ class RobotsRulesParser { ...@@ -107,9 +115,10 @@ class RobotsRulesParser {
// Ordered list of robots rules from longest to shortest. // Ordered list of robots rules from longest to shortest.
std::vector<RobotsRule> robots_rules_; std::vector<RobotsRule> robots_rules_;
// Contains the requests that are pending for robots rules to be received. // Contains the requests that are pending for robots rules to be received,
// Holds the URL path and the callback. // keyed by routing ID. Key is the rouging ID and the value holds the URL path
std::vector<std::pair<CheckResultCallback, std::string>> // and the callback.
std::map<int, std::vector<std::pair<CheckResultCallback, std::string>>>
pending_check_requests_; pending_check_requests_;
// To trigger the timeout for the robots rules to be received. // To trigger the timeout for the robots rules to be received.
......
...@@ -25,10 +25,16 @@ void RobotsRulesParserCache::UpdateRobotsRules( ...@@ -25,10 +25,16 @@ void RobotsRulesParserCache::UpdateRobotsRules(
base::Optional<RobotsRulesParser::CheckResult> base::Optional<RobotsRulesParser::CheckResult>
RobotsRulesParserCache::CheckRobotsRules( RobotsRulesParserCache::CheckRobotsRules(
int routing_id,
const GURL& url, const GURL& url,
RobotsRulesParser::CheckResultCallback callback) { RobotsRulesParser::CheckResultCallback callback) {
return GetRobotsRulesParserForOrigin(url::Origin::Create(url)) return GetRobotsRulesParserForOrigin(url::Origin::Create(url))
.CheckRobotsRules(url, std::move(callback)); .CheckRobotsRules(routing_id, url, std::move(callback));
}
void RobotsRulesParserCache::InvalidatePendingRequests(int routing_id) {
for (auto& entry : parsers_cache_)
entry.second->InvalidatePendingRequests(routing_id);
} }
RobotsRulesParser& RobotsRulesParserCache::GetRobotsRulesParserForOrigin( RobotsRulesParser& RobotsRulesParserCache::GetRobotsRulesParserForOrigin(
......
...@@ -34,9 +34,13 @@ class RobotsRulesParserCache { ...@@ -34,9 +34,13 @@ class RobotsRulesParserCache {
// should be returned and the |callback| will be invoked when the decision was // should be returned and the |callback| will be invoked when the decision was
// made. // made.
base::Optional<RobotsRulesParser::CheckResult> CheckRobotsRules( base::Optional<RobotsRulesParser::CheckResult> CheckRobotsRules(
int routing_id,
const GURL& url, const GURL& url,
RobotsRulesParser::CheckResultCallback callback); RobotsRulesParser::CheckResultCallback callback);
// Invalidate and cancel the pending requests for the robots rules parser.
void InvalidatePendingRequests(int routing_id);
private: private:
// Returns a reference to the robots rules parser for the |origin| from the // Returns a reference to the robots rules parser for the |origin| from the
// cache. An entry is created if it does not exist. // cache. An entry is created if it does not exist.
......
...@@ -39,6 +39,8 @@ class CheckResultReceiver { ...@@ -39,6 +39,8 @@ class CheckResultReceiver {
base::WeakPtrFactory<CheckResultReceiver> weak_ptr_factory_{this}; base::WeakPtrFactory<CheckResultReceiver> weak_ptr_factory_{this};
}; };
const int kRenderFrameID = 1;
class SubresourceRedirectRobotsRulesParserTest : public testing::Test { class SubresourceRedirectRobotsRulesParserTest : public testing::Test {
public: public:
SubresourceRedirectRobotsRulesParserTest() SubresourceRedirectRobotsRulesParserTest()
...@@ -56,10 +58,12 @@ class SubresourceRedirectRobotsRulesParserTest : public testing::Test { ...@@ -56,10 +58,12 @@ class SubresourceRedirectRobotsRulesParserTest : public testing::Test {
// Verify robots rules check result is received synchronously with the // Verify robots rules check result is received synchronously with the
// expected result. // expected result.
void CheckRobotsRules(const std::string& url_path_with_query, void CheckRobotsRules(const std::string& url_path_with_query,
RobotsRulesParser::CheckResult expected_result) { RobotsRulesParser::CheckResult expected_result,
int render_frame_id = kRenderFrameID) {
CheckResultReceiver result_receiver; CheckResultReceiver result_receiver;
auto result = robots_rules_parser_.CheckRobotsRules( auto result = robots_rules_parser_.CheckRobotsRules(
GURL(kTestOrigin + url_path_with_query), result_receiver.GetCallback()); render_frame_id, GURL(kTestOrigin + url_path_with_query),
result_receiver.GetCallback());
EXPECT_FALSE(result_receiver.did_receive_result()); EXPECT_FALSE(result_receiver.did_receive_result());
EXPECT_EQ(expected_result, result); EXPECT_EQ(expected_result, result);
} }
...@@ -67,10 +71,11 @@ class SubresourceRedirectRobotsRulesParserTest : public testing::Test { ...@@ -67,10 +71,11 @@ class SubresourceRedirectRobotsRulesParserTest : public testing::Test {
// Verify robots rules check result is received asynchronously, and returns // Verify robots rules check result is received asynchronously, and returns
// the receiver that can be used to check the result. // the receiver that can be used to check the result.
std::unique_ptr<CheckResultReceiver> CheckRobotsRulesAsync( std::unique_ptr<CheckResultReceiver> CheckRobotsRulesAsync(
const std::string& url_path_with_query) { const std::string& url_path_with_query,
int render_frame_id = kRenderFrameID) {
auto result_receiver = std::make_unique<CheckResultReceiver>(); auto result_receiver = std::make_unique<CheckResultReceiver>();
EXPECT_FALSE(robots_rules_parser_.CheckRobotsRules( EXPECT_FALSE(robots_rules_parser_.CheckRobotsRules(
GURL(kTestOrigin + url_path_with_query), render_frame_id, GURL(kTestOrigin + url_path_with_query),
result_receiver->GetCallback())); result_receiver->GetCallback()));
return result_receiver; return result_receiver;
} }
...@@ -196,10 +201,11 @@ TEST_F(SubresourceRedirectRobotsRulesParserTest, ...@@ -196,10 +201,11 @@ TEST_F(SubresourceRedirectRobotsRulesParserTest,
auto receiver1 = std::make_unique<CheckResultReceiver>(); auto receiver1 = std::make_unique<CheckResultReceiver>();
auto receiver2 = std::make_unique<CheckResultReceiver>(); auto receiver2 = std::make_unique<CheckResultReceiver>();
robots_rules_parser->CheckRobotsRules(GURL("https://test.com/foo.jpg"), robots_rules_parser->CheckRobotsRules(kRenderFrameID,
GURL("https://test.com/foo.jpg"),
receiver1->GetCallback()); receiver1->GetCallback());
robots_rules_parser->CheckRobotsRules(GURL("https://test.com/bar"), robots_rules_parser->CheckRobotsRules(
receiver2->GetCallback()); kRenderFrameID, GURL("https://test.com/bar"), receiver2->GetCallback());
EXPECT_FALSE(receiver1->did_receive_result()); EXPECT_FALSE(receiver1->did_receive_result());
EXPECT_FALSE(receiver2->did_receive_result()); EXPECT_FALSE(receiver2->did_receive_result());
VerifyTotalRobotsRulesApplyHistograms(0); VerifyTotalRobotsRulesApplyHistograms(0);
...@@ -372,4 +378,41 @@ TEST_F(SubresourceRedirectRobotsRulesParserTest, TestRulesAreCaseSensitive) { ...@@ -372,4 +378,41 @@ TEST_F(SubresourceRedirectRobotsRulesParserTest, TestRulesAreCaseSensitive) {
VerifyTotalRobotsRulesApplyHistograms(6); VerifyTotalRobotsRulesApplyHistograms(6);
} }
TEST_F(SubresourceRedirectRobotsRulesParserTest,
TestInvalidatePendingRequests) {
auto receiver1 = CheckRobotsRulesAsync("/allowed.jpg", 1 /*render_frame_id*/);
auto receiver2 =
CheckRobotsRulesAsync("/disallowed.jpg", 1 /*render_frame_id*/);
auto receiver3 = CheckRobotsRulesAsync("/allowed.jpg", 2 /*render_frame_id*/);
auto receiver4 =
CheckRobotsRulesAsync("/disallowed.jpg", 2 /*render_frame_id*/);
// Invalidate should cancel requests for that render frame ID.
robots_rules_parser_.InvalidatePendingRequests(1 /*render_frame_id*/);
EXPECT_TRUE(receiver1->did_receive_result());
EXPECT_TRUE(receiver2->did_receive_result());
EXPECT_EQ(RobotsRulesParser::CheckResult::kInvalidated,
receiver1->check_result());
EXPECT_EQ(RobotsRulesParser::CheckResult::kInvalidated,
receiver2->check_result());
EXPECT_FALSE(receiver3->did_receive_result());
EXPECT_FALSE(receiver4->did_receive_result());
VerifyTotalRobotsRulesApplyHistograms(0);
// When robots rules are retrieved, the
SetUpRobotsRules({{kRuleTypeAllow, "/allow*"}, {kRuleTypeDisallow, "/*"}});
EXPECT_TRUE(receiver3->did_receive_result());
EXPECT_TRUE(receiver4->did_receive_result());
EXPECT_EQ(RobotsRulesParser::CheckResult::kAllowed,
receiver3->check_result());
EXPECT_EQ(RobotsRulesParser::CheckResult::kDisallowed,
receiver4->check_result());
VerifyTotalRobotsRulesApplyHistograms(2);
CheckRobotsRules("/allowed.jpg", RobotsRulesParser::CheckResult::kAllowed);
CheckRobotsRules("/disallowed.jpg",
RobotsRulesParser::CheckResult::kDisallowed);
VerifyTotalRobotsRulesApplyHistograms(4);
}
} // namespace subresource_redirect } // namespace subresource_redirect
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment