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}

chrome/browser/data_saver/subresource_redirect_login_robots_browsertest.cc

@@ -14,6 +14,8 @@
 #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_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/subresource_redirect/https_image_compression_infobar_decider.h"
 #include "chrome/browser/ui/browser.h"
@@ -36,6 +38,30 @@ namespace subresource_redirect {
 const bool kRuleTypeAllow = true;
 const bool kRuleTypeDisallow = false;
 
+// TODO(rajendrant): Delete this enum after moving RedirectResult in
+// chrome/renderer/subresource_redirect/redirect_result.h out of
+// chrome/renderer.
+enum class RedirectResult {
+  kUnknown = 0,
+  kRedirectable,
+
+  // Possible reasons for ineligibility:
+  kIneligibleBlinkDisallowed,
+  kIneligibleSubframeResource,
+  kIneligibleRedirectFailed,
+
+  // Possible reasons for ineligibility due to public image hints approach:
+  kIneligibleImageHintsUnavailable,
+  kIneligibleImageHintsUnavailableButRedirectable,
+  kIneligibleImageHintsUnavailableAndMissingInHints,
+  kIneligibleMissingInImageHints,
+
+  // Possible reasons for ineligibility due to login and robots rules
+  // based approach:
+  kIneligibleRobotsDisallowed,
+  kIneligibleRobotsTimeout,
+};
+
 // Holds one allow or disallow robots rule
 struct RobotsRule {
   RobotsRule(bool rule_type, const std::string& pattern)
@@ -264,10 +290,12 @@ class SubresourceRedirectLoginRobotsBrowserTest : public InProcessBrowserTest {
  public:
   explicit SubresourceRedirectLoginRobotsBrowserTest(
       bool enable_lite_mode = true,
-      bool enable_login_robots_compression_feature = true)
+      bool enable_login_robots_compression_feature = true,
+      const std::string& logged_in_sites = "")
       : enable_lite_mode_(enable_lite_mode),
         enable_login_robots_compression_feature_(
             enable_login_robots_compression_feature),
+        logged_in_sites_(logged_in_sites),
         https_test_server_(net::EmbeddedTestServer::TYPE_HTTPS) {}
 
   ~SubresourceRedirectLoginRobotsBrowserTest() override = default;
@@ -290,7 +318,7 @@ class SubresourceRedirectLoginRobotsBrowserTest : public InProcessBrowserTest {
     std::vector<base::test::ScopedFeatureList::FeatureAndParams>
         enabled_features;
     if (enable_login_robots_compression_feature_) {
-      base::FieldTrialParams params;
+      base::FieldTrialParams params, login_detection_params;
       params["enable_public_image_hints_based_compression"] = "false";
       params["enable_login_robots_based_compression"] = "true";
       params["lite_page_robots_origin"] = robots_rules_server_.GetURL();
@@ -299,8 +327,11 @@ class SubresourceRedirectLoginRobotsBrowserTest : public InProcessBrowserTest {
       // This rules fetch timeout is chosen such that the tests would have
       // enough time to fetch the rules without causing a timeout.
       params["robots_rules_receive_timeout"] = "1000";
+      login_detection_params["logged_in_sites"] = logged_in_sites_;
       enabled_features.emplace_back(blink::features::kSubresourceRedirect,
                                     params);
+      enabled_features.emplace_back(login_detection::kLoginDetection,
+                                    login_detection_params);
     }
     scoped_feature_list_.InitWithFeaturesAndParameters(enabled_features, {});
     InProcessBrowserTest::SetUp();
@@ -328,6 +359,9 @@ class SubresourceRedirectLoginRobotsBrowserTest : public InProcessBrowserTest {
   bool enable_lite_mode_;
   bool enable_login_robots_compression_feature_;
 
+  // Logged-in sites specified from field trial.
+  const std::string logged_in_sites_;
+
   base::test::ScopedFeatureList scoped_feature_list_;
 
   // Simulates the LitePages servers that return the robots rules and compress
@@ -748,4 +782,131 @@ IN_PROC_BROWSER_TEST_F(SubresourceRedirectLoginRobotsBrowserTest,
       {"/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({});
+}
+
+// Verify that when image load gets canceled due to subsequent page load, the
+// subresource redirect for the image is canceled as well.
+IN_PROC_BROWSER_TEST_F(SubresourceRedirectLoginRobotsBrowserTest,
+                       DISABLE_ON_WIN_MAC_CHROMEOS(TestCancelBeforeImageLoad)) {
+  robots_rules_server_.set_failure_mode(
+      RobotsRulesTestServer::FailureMode::kTimeout);
+  robots_rules_server_.AddRobotsRules(GetHttpsTestURL("/"),
+                                      {{kRuleTypeAllow, ""}});
+
+  ui_test_utils::NavigateToURL(browser(),
+                               GetHttpsTestURL("/load_image/image.html"));
+  FetchHistogramsFromChildProcesses();
+
+  ui_test_utils::NavigateToURL(browser(),
+                               GetHttpsTestURL("/load_image/simple.html"));
+  FetchHistogramsFromChildProcesses();
+
+  RetryForHistogramUntilCountReached(
+      &histogram_tester_,
+      "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult", 1);
+  histogram_tester_.ExpectUniqueSample(
+      "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
+      RedirectResult::kIneligibleRobotsTimeout, 1);
+  histogram_tester_.ExpectUniqueSample(
+      "SubresourceRedirect.CompressionAttempt.ResponseCode",
+      net::HTTP_TEMPORARY_REDIRECT, 1);
+  histogram_tester_.ExpectTotalCount(
+      "SubresourceRedirect.CompressionAttempt.ServerResponded", 0);
+
+  robots_rules_server_.VerifyRequestedOrigins({GetHttpsTestURL("/").spec()});
+  image_compression_server_.VerifyRequestedImagePaths({});
+}
+
+// Test class that sets up logged-in sites from field trial.
+class SubresourceRedirectLoggedInSitesBrowserTest
+    : public SubresourceRedirectLoginRobotsBrowserTest {
+ public:
+  SubresourceRedirectLoggedInSitesBrowserTest()
+      : SubresourceRedirectLoginRobotsBrowserTest(
+            true /* enable_lite_mode*/,
+            true /* enable_login_robots_compression_feature */,
+            "https://loggedin.com") {}
+};
+
+// Verify that when image load gets canceled due to subsequent navigation to a
+// logged-in page, the subresource redirect for the image is disabled as well.
+IN_PROC_BROWSER_TEST_F(SubresourceRedirectLoggedInSitesBrowserTest,
+                       DISABLE_ON_WIN_MAC_CHROMEOS(TestCancelBeforeImageLoad)) {
+  robots_rules_server_.set_failure_mode(
+      RobotsRulesTestServer::FailureMode::kTimeout);
+  robots_rules_server_.AddRobotsRules(GetHttpsTestURL("/"),
+                                      {{kRuleTypeAllow, ""}});
+
+  ui_test_utils::NavigateToURL(browser(),
+                               GetHttpsTestURL("/load_image/image.html"));
+  FetchHistogramsFromChildProcesses();
+
+  ui_test_utils::NavigateToURL(
+      browser(),
+      https_test_server_.GetURL("loggedin.com", "/load_image/simple.html"));
+  FetchHistogramsFromChildProcesses();
+  histogram_tester_.ExpectBucketCount(
+      "Login.PageLoad.DetectionType",
+      login_detection::LoginDetectionType::kFieldTrialLoggedInSite, 1);
+
+  RetryForHistogramUntilCountReached(
+      &histogram_tester_,
+      "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult", 1);
+  histogram_tester_.ExpectUniqueSample(
+      "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
+      RedirectResult::kIneligibleRobotsTimeout, 1);
+  histogram_tester_.ExpectUniqueSample(
+      "SubresourceRedirect.CompressionAttempt.ResponseCode",
+      net::HTTP_TEMPORARY_REDIRECT, 1);
+  histogram_tester_.ExpectTotalCount(
+      "SubresourceRedirect.CompressionAttempt.ServerResponded", 0);
+
+  robots_rules_server_.VerifyRequestedOrigins({GetHttpsTestURL("/").spec()});
+  image_compression_server_.VerifyRequestedImagePaths({});
+}
+
 }  // namespace subresource_redirect

chrome/browser/subresource_redirect/subresource_redirect_observer.cc

@@ -4,6 +4,9 @@
 
 #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_factory.h"
 #include "chrome/browser/profiles/profile.h"
@@ -145,13 +148,33 @@ void SubresourceRedirectObserver::DidFinishNavigation(
                                               navigation_handle)) {
     return;
   }
+  content::RenderFrameHost* render_frame_host =
+      navigation_handle->GetRenderFrameHost();
 
   // Handle login robots based compression mode.
   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(
         navigation_handle->GetRenderFrameHost());
-    // TODO(1149853): Handle whether page is logged-in and disable compression.
-    is_https_image_compression_applied_ = true;
     return;
   }
 
@@ -163,8 +186,6 @@ void SubresourceRedirectObserver::DidFinishNavigation(
   if (!optimization_guide_decider)
     return;
 
-  content::RenderFrameHost* render_frame_host =
-      navigation_handle->GetRenderFrameHost();
   optimization_guide_decider->CanApplyOptimizationAsync(
       navigation_handle, optimization_guide::proto::COMPRESS_PUBLIC_IMAGES,
       base::BindOnce(

chrome/common/subresource_redirect_service.mojom

@@ -36,4 +36,10 @@ interface SubresourceRedirectService {
 interface SubresourceRedirectHintsReceiver {
   // Sends the public image URL hints from the browser to renderers.
   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);
 };

chrome/renderer/subresource_redirect/login_robots_decider_agent.cc

@@ -32,6 +32,7 @@ RedirectResult ConvertToRedirectResult(
     case RobotsRulesParser::CheckResult::kAllowed:
       return RedirectResult::kRedirectable;
     case RobotsRulesParser::CheckResult::kDisallowed:
+    case RobotsRulesParser::CheckResult::kInvalidated:
       return RedirectResult::kIneligibleRobotsDisallowed;
     case RobotsRulesParser::CheckResult::kTimedout:
     case RobotsRulesParser::CheckResult::kDisallowedAfterTimeout:
@@ -39,12 +40,10 @@ RedirectResult ConvertToRedirectResult(
   }
 }
 
-// Converts the robots rules CheckResult to RedirectResult and passes to the
-// callback.
-void SendRedirectResultToCallback(
-    LoginRobotsDeciderAgent::ShouldRedirectDecisionCallback callback,
-    RobotsRulesParser::CheckResult check_result) {
-  std::move(callback).Run(ConvertToRedirectResult(check_result));
+void RecordRedirectResultMetric(RedirectResult redirect_result) {
+  LOCAL_HISTOGRAM_ENUMERATION(
+      "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
+      redirect_result);
 }
 
 }  // namespace
@@ -65,6 +64,8 @@ LoginRobotsDeciderAgent::ShouldRedirectSubresource(
     ShouldRedirectDecisionCallback callback) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(url.is_valid());
+  if (redirect_result_ != RedirectResult::kRedirectable)
+    return redirect_result_;
   if (!render_frame()->IsMainFrame())
     return RedirectResult::kIneligibleSubframeResource;
 
@@ -83,21 +84,51 @@ LoginRobotsDeciderAgent::ShouldRedirectSubresource(
 
   base::Optional<RobotsRulesParser::CheckResult> result =
       robots_rules_parser_cache.CheckRobotsRules(
-          url,
-          base::BindOnce(&SendRedirectResultToCallback, std::move(callback)));
-  if (result)
-    return ConvertToRedirectResult(*result);
+          routing_id(), url,
+          base::BindOnce(
+              &LoginRobotsDeciderAgent::OnShouldRedirectSubresourceResult,
+              weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  if (result) {
+    RedirectResult redirect_result = ConvertToRedirectResult(*result);
+    RecordRedirectResultMetric(redirect_result);
+    return redirect_result;
+  }
 
   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(
     const GURL& url,
     int64_t content_length,
     RedirectResult redirect_result) {
-  LOCAL_HISTOGRAM_ENUMERATION(
-      "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
-      redirect_result);
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   // TODO(crbug.com/1148980): Record coverage metrics
 }
 

chrome/renderer/subresource_redirect/login_robots_decider_agent.h

@@ -6,6 +6,7 @@
 #define CHROME_RENDERER_SUBRESOURCE_REDIRECT_LOGIN_ROBOTS_DECIDER_AGENT_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
 #include "base/timer/timer.h"
 #include "chrome/renderer/subresource_redirect/public_resource_decider_agent.h"
@@ -37,9 +38,14 @@ class LoginRobotsDeciderAgent : public PublicResourceDeciderAgent {
   void UpdateRobotsRulesForTesting(const url::Origin& origin,
                                    const base::Optional<std::string>& rules);
 
+  // content::RenderFrameObserver:
+  void ReadyToCommitNavigation(
+      blink::WebDocumentLoader* document_loader) override;
+
   // mojom::SubresourceRedirectHintsReceiver:
   void SetCompressPublicImagesHints(
       mojom::CompressPublicImagesHintsPtr images_hints) override;
+  void SetLoggedInState(bool is_logged_in) override;
 
   // PublicResourceDeciderAgent:
   base::Optional<RedirectResult> ShouldRedirectSubresource(
@@ -49,9 +55,21 @@ class LoginRobotsDeciderAgent : public PublicResourceDeciderAgent {
                                    int64_t content_length,
                                    RedirectResult redirect_result) override;
 
+  // Callback invoked when should redirect check result is available.
+  void OnShouldRedirectSubresourceResult(
+      ShouldRedirectDecisionCallback callback,
+      RobotsRulesParser::CheckResult check_result);
+
   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_);
+
+  // Used to get a weak pointer to |this|.
+  base::WeakPtrFactory<LoginRobotsDeciderAgent> weak_ptr_factory_{this};
 };
 
 }  // namespace subresource_redirect

chrome/renderer/subresource_redirect/login_robots_decider_agent_browsertest.cc

@@ -73,6 +73,10 @@ class SubresourceRedirectLoginRobotsDeciderAgentTest
            result_receiver.redirect_result() == RedirectResult::kRedirectable;
   }
 
+  void SetLoggedInState(bool is_logged_in) {
+    login_robots_decider_agent_->SetLoggedInState(is_logged_in);
+  }
+
  protected:
   void SetUp() override {
     ChromeRenderViewTest::SetUp();
@@ -91,6 +95,7 @@ class SubresourceRedirectLoginRobotsDeciderAgentTest
 
 TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
        TestAllowDisallowSingleOrigin) {
+  SetLoggedInState(false);
   SetUpRobotsRules("https://foo.com", {{kRuleTypeAllow, "/public"},
                                        {kRuleTypeDisallow, "/private"}});
   EXPECT_TRUE(ShouldRedirectSubresource("https://foo.com/public.jpg"));
@@ -103,6 +108,7 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
 
 TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
        TestHTTPRulesAreSeparate) {
+  SetLoggedInState(false);
   SetUpRobotsRules("https://foo.com", {{kRuleTypeAllow, "/public"},
                                        {kRuleTypeDisallow, "/private"}});
   EXPECT_FALSE(ShouldRedirectSubresource("http://foo.com/public.jpg"));
@@ -116,6 +122,7 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
 }
 
 TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, TestURLWithArguments) {
+  SetLoggedInState(false);
   SetUpRobotsRules("https://foo.com",
                    {{kRuleTypeAllow, "/*.jpg$"},
                     {kRuleTypeDisallow, "/*.png?*arg_disallowed"},
@@ -140,6 +147,7 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest, TestURLWithArguments) {
 
 TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
        TestRulesAreCaseSensitive) {
+  SetLoggedInState(false);
   SetUpRobotsRules("https://foo.com", {{kRuleTypeAllow, "/allowed"},
                                        {kRuleTypeAllow, "/CamelCase"},
                                        {kRuleTypeAllow, "/CAPITALIZE"},
@@ -152,4 +160,13 @@ TEST_F(SubresourceRedirectLoginRobotsDeciderAgentTest,
   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

chrome/renderer/subresource_redirect/login_robots_url_loader_throttle_browsertest.cc

@@ -84,17 +84,13 @@ class LoginRobotsDeciderInfo : public blink::URLLoaderThrottle::Delegate {
     NOTIMPLEMENTED();
   }
 
-  void VerifyRedirectResult(RedirectResult expected_result) {
-    base::HistogramTester histogram_tester;
+  void VerifyWillProcessResponse() {
     network::mojom::URLResponseHeadPtr head =
         network::CreateURLResponseHead(net::HTTP_OK);
     head->headers->SetHeader("Content-Length", "1024");
     bool defer = false;
     throttle_->WillProcessResponse(GURL("https://foo.com/img.jpg"), head.get(),
                                    &defer);
-    histogram_tester.ExpectUniqueSample(
-        "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
-        expected_result, 1);
     EXPECT_FALSE(defer);
   }
 
@@ -150,6 +146,10 @@ class SubresourceRedirectLoginRobotsURLLoaderThrottleTest
                  blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON));
   }
 
+  void SetLoggedInState(bool is_logged_in) {
+    login_robots_decider_agent_->SetLoggedInState(is_logged_in);
+  }
+
  protected:
   void SetUp() override {
     ChromeRenderViewTest::SetUp();
@@ -163,9 +163,10 @@ class SubresourceRedirectLoginRobotsURLLoaderThrottleTest
         &associated_interfaces_, view_->GetMainRenderFrame());
   }
 
- private:
+ protected:
   LoginRobotsDeciderAgent* login_robots_decider_agent_;
   base::test::ScopedFeatureList scoped_feature_list_;
+  base::HistogramTester histogram_tester_;
 };
 
 TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
@@ -221,6 +222,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
        TestGetSubresourceURL) {
   struct TestCase {
     int previews_state;
+    bool is_logged_in;
     std::string original_url;
     GURL redirected_subresource_url;  // Empty URL means there will be no
                                       // redirect.
@@ -229,17 +231,20 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
   const TestCase kTestCases[]{
       {
           blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
+          false,
           "https://www.test.com/public_img.jpg",
           GetSubresourceURLForURL(GURL("https://www.test.com/public_img.jpg")),
       },
       {
           blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
+          false,
           "https://www.test.com/public_img.jpg#anchor",
           GetSubresourceURLForURL(
               GURL("https://www.test.com/public_img.jpg#anchor")),
       },
       {
           blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
+          false,
           "https://www.test.com/public_img.jpg?public_arg1=bar&public_arg2",
           GetSubresourceURLForURL(
               GURL("https://www.test.com/"
@@ -248,14 +253,23 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
       // Private images will not be redirected.
       {
           blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
+          false,
           "https://www.test.com/private_img.jpg",
           GURL(),
       },
       {
           blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
+          false,
           "https://www.test.com/public_img.jpg&private_arg1=foo",
           GURL(),
       },
+      // No redirection when logged-in
+      {
+          blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
+          true,
+          "https://www.test.com/public_img.jpg",
+          GURL(),
+      },
   };
   blink::WebNetworkStateNotifier::SetSaveDataEnabled(true);
 
@@ -264,6 +278,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
                                             {kRuleTypeDisallow, ""}});
 
   for (const TestCase& test_case : kTestCases) {
+    SetLoggedInState(test_case.is_logged_in);
     auto throttle = CreateLoginRobotsDecider(
         test_case.original_url, network::mojom::RequestDestination::kImage,
         test_case.previews_state);
@@ -288,6 +303,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
 TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
        TestRobotsRulesSentBeforeThrottle) {
   blink::WebNetworkStateNotifier::SetSaveDataEnabled(true);
+  SetLoggedInState(false);
 
   SetUpRobotsRules("https://www.test.com",
                    {{kRuleTypeAllow, "/public"}, {kRuleTypeDisallow, ""}});
@@ -296,8 +312,6 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
       CreateURLLoaderThrottleInfo("https://www.test.com/public.jpg");
   auto throttle_info2 =
       CreateURLLoaderThrottleInfo("https://www.test.com/private.jpg");
-  throttle_info1->VerifyRedirectResult(RedirectResult::kRedirectable);
-  throttle_info2->VerifyRedirectResult(RedirectResult::kRedirectable);
 
   throttle_info1->SendStartRequestAndVerifyDeferral(
       WillStartRequestDeferralState::kRedirected);
@@ -307,9 +321,14 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
   EXPECT_FALSE(throttle_info2->did_resume());
   EXPECT_FALSE(throttle_info1->did_restart_with_url_reset_and_flags());
   EXPECT_FALSE(throttle_info2->did_restart_with_url_reset_and_flags());
-  throttle_info1->VerifyRedirectResult(RedirectResult::kRedirectable);
-  throttle_info2->VerifyRedirectResult(
-      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 are sent, after throttles are
@@ -317,6 +336,7 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
 TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
        TestRobotsRulesSentAfterThrottle) {
   blink::WebNetworkStateNotifier::SetSaveDataEnabled(true);
+  SetLoggedInState(false);
 
   auto throttle_info1 =
       CreateURLLoaderThrottleInfo("https://www.test.com/public.jpg");
@@ -344,15 +364,23 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
   EXPECT_TRUE(throttle_info2->did_restart_with_url_reset_and_flags());
   EXPECT_TRUE(throttle_info2->did_resume());
 
-  throttle_info1->VerifyRedirectResult(RedirectResult::kRedirectable);
-  throttle_info2->VerifyRedirectResult(
-      RedirectResult::kIneligibleRobotsDisallowed);
+  throttle_info1->VerifyWillProcessResponse();
+  throttle_info2->VerifyWillProcessResponse();
+  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.
 TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
        TestRobotsRulesTimeout) {
   blink::WebNetworkStateNotifier::SetSaveDataEnabled(true);
+  SetLoggedInState(false);
 
   auto throttle_info1 =
       CreateURLLoaderThrottleInfo("https://www.test.com/public.jpg");
@@ -377,10 +405,13 @@ TEST_F(SubresourceRedirectLoginRobotsURLLoaderThrottleTest,
   EXPECT_TRUE(throttle_info2->did_restart_with_url_reset_and_flags());
   EXPECT_TRUE(throttle_info2->did_resume());
 
-  throttle_info1->VerifyRedirectResult(
-      RedirectResult::kIneligibleRobotsTimeout);
-  throttle_info2->VerifyRedirectResult(
-      RedirectResult::kIneligibleRobotsTimeout);
+  throttle_info1->VerifyWillProcessResponse();
+  throttle_info2->VerifyWillProcessResponse();
+  histogram_tester_.ExpectTotalCount(
+      "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult", 2);
+  histogram_tester_.ExpectBucketCount(
+      "SubresourceRedirect.LoginRobotsDeciderAgent.RedirectResult",
+      RedirectResult::kIneligibleRobotsTimeout, 2);
 }
 
 }  // namespace subresource_redirect

chrome/renderer/subresource_redirect/public_image_hints_decider_agent.cc

@@ -160,6 +160,7 @@ void PublicImageHintsDeciderAgent::RecordMetrics(
       break;
     case RedirectResult::kIneligibleRobotsDisallowed:
     case RedirectResult::kIneligibleRobotsTimeout:
+    case RedirectResult::kIneligibleLoginDetected:
       NOTREACHED();
   }
   mojo::PendingRemote<ukm::mojom::UkmRecorderInterface> recorder;
@@ -198,4 +199,10 @@ void PublicImageHintsDeciderAgent::NotifyCompressedResourceFetchFailed(
   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

chrome/renderer/subresource_redirect/public_image_hints_decider_agent.h

@@ -42,6 +42,7 @@ class PublicImageHintsDeciderAgent : public PublicResourceDeciderAgent {
   // mojom::SubresourceRedirectHintsReceiver:
   void SetCompressPublicImagesHints(
       mojom::CompressPublicImagesHintsPtr images_hints) override;
+  void SetLoggedInState(bool is_logged_in) override;
 
   // PublicResourceDeciderAgent:
   base::Optional<RedirectResult> ShouldRedirectSubresource(

chrome/renderer/subresource_redirect/redirect_result.h

@@ -57,7 +57,10 @@ enum class RedirectResult {
   // Because the robots rules fetch timedout.
   kIneligibleRobotsTimeout,
 
-  kMaxValue = RedirectResult::kIneligibleRobotsTimeout
+  // Because the page was detected to be logged-in.
+  kIneligibleLoginDetected,
+
+  kMaxValue = RedirectResult::kIneligibleLoginDetected
 };
 
 }  // namespace subresource_redirect

chrome/renderer/subresource_redirect/robots_rules_parser.cc

@@ -126,21 +126,27 @@ void RobotsRulesParser::UpdateRobotsRules(
   }
 
   // Respond to the pending requests, even if robots proto parse failed.
-  for (auto& request : pending_check_requests_) {
-    std::move(request.first).Run(CheckRobotsRulesImmediate(request.second));
+  for (auto& requests : pending_check_requests_) {
+    for (auto& request : requests.second) {
+      std::move(request.first).Run(CheckRobotsRulesImmediate(request.second));
+    }
   }
   pending_check_requests_.clear();
 }
 
 base::Optional<RobotsRulesParser::CheckResult>
-RobotsRulesParser::CheckRobotsRules(const GURL& url,
+RobotsRulesParser::CheckRobotsRules(int routing_id,
+                                    const GURL& url,
                                     CheckResultCallback callback) {
   std::string path_with_query = url.path();
   if (url.has_query())
     base::StrAppend(&path_with_query, {"?", url.query()});
   if (rules_receive_state_ == RulesReceiveState::kTimerRunning) {
     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));
     return base::nullopt;
   }
@@ -172,11 +178,24 @@ RobotsRulesParser::CheckResult RobotsRulesParser::CheckRobotsRulesImmediate(
 void RobotsRulesParser::OnRulesReceiveTimeout() {
   DCHECK(!rules_receive_timeout_timer_.IsRunning());
   rules_receive_state_ = RulesReceiveState::kTimeout;
-  for (auto& request : pending_check_requests_)
-    std::move(request.first).Run(CheckResult::kTimedout);
+  for (auto& requests : pending_check_requests_) {
+    for (auto& request : requests.second) {
+      std::move(request.first).Run(CheckResult::kTimedout);
+    }
+  }
   pending_check_requests_.clear();
   RecordRobotsRulesReceiveResultHistogram(
       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

chrome/renderer/subresource_redirect/robots_rules_parser.h

@@ -37,6 +37,8 @@ class RobotsRulesParser {
     kTimedout,                // Timeout in retrieving the robots rules
     kDisallowedAfterTimeout,  // Timeout got triggered already, and the resource
                               // was disallowed
+    kInvalidated,  // The result check was invalidated, before robots rules are
+                   // received or timeout triggered.
   };
 
   enum class RulesReceiveState {
@@ -73,10 +75,16 @@ class RobotsRulesParser {
   // added to |pending_check_requests_| and called when a decision can be made
   // like when rules are retrieved, or rule fetch timeout, etc.
   // The robots rules check will make use of the |url| path and query
-  // parameters.The |url| origin, ref fragment, etc are immaterial.
-  base::Optional<CheckResult> CheckRobotsRules(const GURL& url,
+  // parameters.The |url| origin, ref fragment, etc are immaterial. |routing_id|
+  // is the render frame ID for which this URL is requested for.
+  base::Optional<CheckResult> CheckRobotsRules(int routing_id,
+                                               const GURL& url,
                                                CheckResultCallback callback);
 
+  // Invalidate and cancel the pending requests that were added for
+  // |routing_id|.
+  void InvalidatePendingRequests(int routing_id);
+
  private:
   friend class SubresourceRedirectRobotsRulesParserTest;
 
@@ -107,9 +115,10 @@ class RobotsRulesParser {
   // Ordered list of robots rules from longest to shortest.
   std::vector<RobotsRule> robots_rules_;
 
-  // Contains the requests that are pending for robots rules to be received.
-  // Holds the URL path and the callback.
-  std::vector<std::pair<CheckResultCallback, std::string>>
+  // Contains the requests that are pending for robots rules to be received,
+  // keyed by routing ID. Key is the rouging ID and the value holds the URL path
+  // and the callback.
+  std::map<int, std::vector<std::pair<CheckResultCallback, std::string>>>
       pending_check_requests_;
 
   // To trigger the timeout for the robots rules to be received.

chrome/renderer/subresource_redirect/robots_rules_parser_cache.cc

@@ -25,10 +25,16 @@ void RobotsRulesParserCache::UpdateRobotsRules(
 
 base::Optional<RobotsRulesParser::CheckResult>
 RobotsRulesParserCache::CheckRobotsRules(
+    int routing_id,
     const GURL& url,
     RobotsRulesParser::CheckResultCallback callback) {
   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(

chrome/renderer/subresource_redirect/robots_rules_parser_cache.h

@@ -34,9 +34,13 @@ class RobotsRulesParserCache {
   // should be returned and the |callback| will be invoked when the decision was
   // made.
   base::Optional<RobotsRulesParser::CheckResult> CheckRobotsRules(
+      int routing_id,
       const GURL& url,
       RobotsRulesParser::CheckResultCallback callback);
 
+  // Invalidate and cancel the pending requests for the robots rules parser.
+  void InvalidatePendingRequests(int routing_id);
+
  private:
   // Returns a reference to the robots rules parser for the |origin| from the
   // cache. An entry is created if it does not exist.

chrome/renderer/subresource_redirect/robots_rules_parser_unittest.cc

@@ -39,6 +39,8 @@ class CheckResultReceiver {
   base::WeakPtrFactory<CheckResultReceiver> weak_ptr_factory_{this};
 };
 
+const int kRenderFrameID = 1;
+
 class SubresourceRedirectRobotsRulesParserTest : public testing::Test {
  public:
   SubresourceRedirectRobotsRulesParserTest()
@@ -56,10 +58,12 @@ class SubresourceRedirectRobotsRulesParserTest : public testing::Test {
   // Verify robots rules check result is received synchronously with the
   // expected result.
   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;
     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_EQ(expected_result, result);
   }
@@ -67,10 +71,11 @@ class SubresourceRedirectRobotsRulesParserTest : public testing::Test {
   // Verify robots rules check result is received asynchronously, and returns
   // the receiver that can be used to check the result.
   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>();
     EXPECT_FALSE(robots_rules_parser_.CheckRobotsRules(
-        GURL(kTestOrigin + url_path_with_query),
+        render_frame_id, GURL(kTestOrigin + url_path_with_query),
         result_receiver->GetCallback()));
     return result_receiver;
   }
@@ -196,10 +201,11 @@ TEST_F(SubresourceRedirectRobotsRulesParserTest,
   auto receiver1 = 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());
-  robots_rules_parser->CheckRobotsRules(GURL("https://test.com/bar"),
-                                        receiver2->GetCallback());
+  robots_rules_parser->CheckRobotsRules(
+      kRenderFrameID, GURL("https://test.com/bar"), receiver2->GetCallback());
   EXPECT_FALSE(receiver1->did_receive_result());
   EXPECT_FALSE(receiver2->did_receive_result());
   VerifyTotalRobotsRulesApplyHistograms(0);
@@ -372,4 +378,41 @@ TEST_F(SubresourceRedirectRobotsRulesParserTest, TestRulesAreCaseSensitive) {
   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