[Query Tiles]: bug fix and unit test for scheduler and fetcher.

- Fixed fetcher crash because ResourceRequest was null if a second
request was asked but it was build in ctor.
- Fixed a wrong usage of OneOffTaskInfo window.
- Added unit test to verify the start/end window and backoff delay
in task info.

- If debug switch instant fetch on, we always schedule a fetch task
on service startup once, don't do recurring fetching task.

Bug: 1083532
Change-Id: I8c1a29bc7c7692b6c59a2de958589b41ca7957c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2207616
Commit-Queue: Hesen Zhang <hesen@chromium.org>
Auto-Submit: Hesen Zhang <hesen@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#769910}

components/query_tiles/internal/tile_fetcher.cc

@@ -55,20 +55,21 @@ class TileFetcherImpl : public TileFetcher {
       const std::string& experiment_tag,
       const std::string& client_version,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
-      : url_loader_factory_(url_loader_factory) {
-    tile_info_request_status_ = TileInfoRequestStatus::kInit;
-    auto resource_request =
-        BuildGetRequest(url, country_code, accept_languages, api_key,
-                        experiment_tag, client_version);
-    url_loader_ = network::SimpleURLLoader::Create(
-        std::move(resource_request), kQueryTilesFetcherTrafficAnnotation);
-  }
+      : url_loader_factory_(url_loader_factory),
+        url_(url),
+        country_code_(country_code),
+        accept_languages_(accept_languages),
+        api_key_(api_key),
+        experiment_tag_(experiment_tag),
+        client_version_(client_version),
+        tile_info_request_status_(TileInfoRequestStatus::kInit) {}
 
  private:
   // TileFetcher implementation.
   void StartFetchForTiles(FinishedCallback callback) override {
-    // TODO(hesen): Estimate max size of response then replace to
-    // DownloadToString method.
+    auto resource_request = BuildGetRequest();
+    url_loader_ = network::SimpleURLLoader::Create(
+        std::move(resource_request), kQueryTilesFetcherTrafficAnnotation);
     url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
         url_loader_factory_.get(),
         base::BindOnce(&TileFetcherImpl::OnDownloadComplete,
@@ -76,28 +77,22 @@ class TileFetcherImpl : public TileFetcher {
   }
 
   // Build the request to get tile info.
-  std::unique_ptr<network::ResourceRequest> BuildGetRequest(
-      const GURL& url,
-      const std::string& country_code,
-      const std::string& accept_languages,
-      const std::string& api_key,
-      const std::string& experiment_tag,
-      const std::string& client_version) {
+  std::unique_ptr<network::ResourceRequest> BuildGetRequest() {
     auto request = std::make_unique<network::ResourceRequest>();
     request->method = net::HttpRequestHeaders::kGetMethod;
-    request->headers.SetHeader("x-goog-api-key", api_key);
-    request->headers.SetHeader("X-Client-Version", client_version);
+    request->headers.SetHeader("x-goog-api-key", api_key_);
+    request->headers.SetHeader("X-Client-Version", client_version_);
     request->headers.SetHeader(net::HttpRequestHeaders::kContentType,
                                kRequestContentType);
     request->url =
-        net::AppendOrReplaceQueryParameter(url, "country_code", country_code);
-    if (!experiment_tag.empty()) {
+        net::AppendOrReplaceQueryParameter(url_, "country_code", country_code_);
+    if (!experiment_tag_.empty()) {
       request->url = net::AppendOrReplaceQueryParameter(
-          request->url, "experiment_tag", experiment_tag);
+          request->url, "experiment_tag", experiment_tag_);
     }
-    if (!accept_languages.empty()) {
+    if (!accept_languages_.empty()) {
       request->headers.SetHeader(net::HttpRequestHeaders::kAcceptLanguage,
-                                 accept_languages);
+                                 accept_languages_);
     }
     return request;
   }
@@ -142,6 +137,7 @@ class TileFetcherImpl : public TileFetcher {
     std::move(callback).Run(tile_info_request_status_,
                             std::move(response_body));
     tile_info_request_status_ = TileInfoRequestStatus::kInit;
+    url_loader_.reset();
   }
 
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
@@ -149,6 +145,14 @@ class TileFetcherImpl : public TileFetcher {
   // Simple URL loader to fetch proto from network.
   std::unique_ptr<network::SimpleURLLoader> url_loader_;
 
+  // Params of resource request.
+  GURL url_;
+  std::string country_code_;
+  std::string accept_languages_;
+  std::string api_key_;
+  std::string experiment_tag_;
+  std::string client_version_;
+
   // Status of the tile info request.
   TileInfoRequestStatus tile_info_request_status_;
 

components/query_tiles/internal/tile_service_scheduler.cc

@@ -18,6 +18,10 @@
 namespace query_tiles {
 namespace {
 
+// Schedule window params in instant schedule mode.
+const int kInstantScheduleWindowStartMs = 10 * 1000;  // 10 seconds
+const int kInstantScheduleWindowEndMs = 20 * 1000;    // 20 seconds
+
 class TileServiceSchedulerImpl : public TileServiceScheduler {
  public:
   TileServiceSchedulerImpl(
@@ -42,6 +46,9 @@ class TileServiceSchedulerImpl : public TileServiceScheduler {
   }
 
   void OnFetchCompleted(TileInfoRequestStatus status) override {
+    if (IsInstantFetchMode())
+      return;
+
     if (status == TileInfoRequestStatus::kShouldSuspend) {
       MaximizeBackoff();
       ScheduleTask(false);
@@ -56,6 +63,12 @@ class TileServiceSchedulerImpl : public TileServiceScheduler {
   }
 
   void OnTileManagerInitialized(TileGroupStatus status) override {
+    if (IsInstantFetchMode()) {
+      ResetBackoff();
+      ScheduleTask(true);
+      return;
+    }
+
     if (status == TileGroupStatus::kNoTiles && !is_suspend_) {
       ResetBackoff();
       ScheduleTask(true);
@@ -68,9 +81,7 @@ class TileServiceSchedulerImpl : public TileServiceScheduler {
 
   void ScheduleTask(bool is_init_schedule) {
     background_task::OneOffInfo one_off_task_info;
-    bool instant_fetch_task = base::CommandLine::ForCurrentProcess()->HasSwitch(
-        switches::kQueryTilesInstantBackgroundTask);
-    if (instant_fetch_task) {
+    if (IsInstantFetchMode()) {
       GetInstantTaskWindow(&one_off_task_info.window_start_time_ms,
                            &one_off_task_info.window_end_time_ms,
                            is_init_schedule);
@@ -119,22 +130,20 @@ class TileServiceSchedulerImpl : public TileServiceScheduler {
   void GetInstantTaskWindow(int64_t* start_time_ms,
                             int64_t* end_time_ms,
                             bool is_init_schedule) {
-    auto now = clock_->Now().ToDeltaSinceWindowsEpoch().InMilliseconds();
     if (is_init_schedule) {
-      *start_time_ms = now + 10 * 1000;
+      *start_time_ms = kInstantScheduleWindowStartMs;
     } else {
       *start_time_ms = GetDelaysFromBackoff();
     }
-    *end_time_ms = now + 20 * 1000;
+    *end_time_ms = kInstantScheduleWindowEndMs;
   }
 
   void GetTaskWindow(int64_t* start_time_ms,
                      int64_t* end_time_ms,
                      bool is_init_schedule) {
-    auto now = clock_->Now().ToDeltaSinceWindowsEpoch().InMilliseconds();
     if (is_init_schedule) {
       *start_time_ms =
-          now + TileConfig::GetScheduleIntervalInMs() +
+          TileConfig::GetScheduleIntervalInMs() +
           base::RandGenerator(TileConfig::GetMaxRandomWindowInMs());
     } else {
       *start_time_ms = GetDelaysFromBackoff();
@@ -162,6 +171,11 @@ class TileServiceSchedulerImpl : public TileServiceScheduler {
     prefs_->Set(kBackoffEntryKey, *value);
   }
 
+  bool IsInstantFetchMode() const {
+    return base::CommandLine::ForCurrentProcess()->HasSwitch(
+        switches::kQueryTilesInstantBackgroundTask);
+  }
+
   // Native Background Scheduler instance.
   background_task::BackgroundTaskScheduler* scheduler_;
 

components/query_tiles/internal/tile_service_scheduler_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_command_line.h"
 #include "base/test/simple_test_clock.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "base/test/task_environment.h"
@@ -15,6 +16,7 @@
 #include "components/prefs/testing_pref_service.h"
 #include "components/query_tiles/internal/tile_config.h"
 #include "components/query_tiles/internal/tile_store.h"
+#include "components/query_tiles/switches.h"
 #include "components/query_tiles/test/test_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -34,8 +36,6 @@ class MockBackgroundTaskScheduler
   MOCK_METHOD1(Cancel, void(int));
 };
 
-// TODO(crbug.com/1082529): Verify the schedule timing range matches task_info,
-// also add more cross status situation tests.
 class TileServiceSchedulerTest : public testing::Test {
  public:
   TileServiceSchedulerTest() = default;
@@ -57,11 +57,13 @@ class TileServiceSchedulerTest : public testing::Test {
     tile_service_scheduler_ =
         TileServiceScheduler::Create(&mocked_native_scheduler_, &prefs_,
                                      &clock_, &tick_clock_, std::move(policy));
+    EXPECT_CALL(
+        *native_scheduler(),
+        Cancel(static_cast<int>(background_task::TaskIds::QUERY_TILE_JOB_ID)));
+    tile_service_scheduler()->CancelTask();
   }
 
  protected:
-  base::Clock* clock() { return &clock_; }
-  base::TickClock* tick_clock() { return &tick_clock_; }
   MockBackgroundTaskScheduler* native_scheduler() {
     return &mocked_native_scheduler_;
   }
@@ -81,27 +83,65 @@ class TileServiceSchedulerTest : public testing::Test {
   std::unique_ptr<TileServiceScheduler> tile_service_scheduler_;
 };
 
-TEST_F(TileServiceSchedulerTest, CancelTask) {
-  EXPECT_CALL(
-      *native_scheduler(),
-      Cancel(static_cast<int>(background_task::TaskIds::QUERY_TILE_JOB_ID)));
-  tile_service_scheduler()->CancelTask();
+MATCHER_P2(TaskInfoEq,
+           min_window_start_time_ms,
+           max_window_start_time_ms,
+           "Verify window range in TaskInfo.") {
+  EXPECT_TRUE(arg.one_off_info.has_value());
+  EXPECT_GE(arg.one_off_info->window_start_time_ms, min_window_start_time_ms);
+  EXPECT_LE(arg.one_off_info->window_start_time_ms, max_window_start_time_ms)
+      << "Actual window start time in ms: "
+      << arg.one_off_info->window_start_time_ms;
+
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kQueryTilesInstantBackgroundTask)) {
+    EXPECT_EQ(arg.one_off_info->window_end_time_ms -
+                  arg.one_off_info->window_start_time_ms,
+              10 * 1000)
+        << "Actual window end time in ms: "
+        << arg.one_off_info->window_end_time_ms;
+  } else {
+    EXPECT_EQ(arg.one_off_info->window_end_time_ms -
+                  arg.one_off_info->window_start_time_ms,
+              TileConfig::GetOneoffTaskWindowInMs())
+        << "Actual window end time in ms: "
+        << arg.one_off_info->window_end_time_ms;
+  }
+
+  return true;
 }
 
 TEST_F(TileServiceSchedulerTest, OnFetchCompletedSuccess) {
-  EXPECT_CALL(*native_scheduler(), Schedule(_));
+  auto expected_range_start = TileConfig::GetScheduleIntervalInMs();
+  auto expected_range_end =
+      expected_range_start + TileConfig::GetMaxRandomWindowInMs();
+  EXPECT_CALL(*native_scheduler(),
+              Schedule(TaskInfoEq(expected_range_start, expected_range_end)));
+  tile_service_scheduler()->OnFetchCompleted(TileInfoRequestStatus::kSuccess);
+}
+
+TEST_F(TileServiceSchedulerTest, OnFetchCompletedSuccessInstantFetchOn) {
+  base::test::ScopedCommandLine scoped_command_line;
+  scoped_command_line.GetProcessCommandLine()->AppendSwitchASCII(
+      query_tiles::switches::kQueryTilesInstantBackgroundTask, "true");
+
+  EXPECT_CALL(*native_scheduler(), Schedule(_)).Times(0);
   tile_service_scheduler()->OnFetchCompleted(TileInfoRequestStatus::kSuccess);
 }
 
 TEST_F(TileServiceSchedulerTest, OnFetchCompletedSuspend) {
-  EXPECT_CALL(*native_scheduler(), Schedule(_));
+  EXPECT_CALL(*native_scheduler(), Schedule(TaskInfoEq(4000, 4000)));
   tile_service_scheduler()->OnFetchCompleted(
       TileInfoRequestStatus::kShouldSuspend);
 }
 
+// Verify the failure will add delay that using test backoff policy.
 TEST_F(TileServiceSchedulerTest, OnFetchCompletedFailure) {
-  EXPECT_CALL(*native_scheduler(), Schedule(_));
-  tile_service_scheduler()->OnFetchCompleted(TileInfoRequestStatus::kFailure);
+  for (int i = 0; i <= 2; i++) {
+    EXPECT_CALL(*native_scheduler(),
+                Schedule(TaskInfoEq(1000 * pow(2, i), 1000 * pow(2, i))));
+    tile_service_scheduler()->OnFetchCompleted(TileInfoRequestStatus::kFailure);
+  }
 }
 
 TEST_F(TileServiceSchedulerTest, OnFetchCompletedOtherStatus) {
@@ -114,12 +154,24 @@ TEST_F(TileServiceSchedulerTest, OnFetchCompletedOtherStatus) {
 }
 
 TEST_F(TileServiceSchedulerTest, OnTileGroupLoadedWithNoTiles) {
-  EXPECT_CALL(*native_scheduler(), Schedule(_));
+  auto expected_range_start = TileConfig::GetScheduleIntervalInMs();
+  auto expected_range_end =
+      expected_range_start + TileConfig::GetMaxRandomWindowInMs();
+  EXPECT_CALL(*native_scheduler(),
+              Schedule(TaskInfoEq(expected_range_start, expected_range_end)));
   tile_service_scheduler()->OnTileManagerInitialized(TileGroupStatus::kNoTiles);
 }
 
+TEST_F(TileServiceSchedulerTest, OnTileGroupLoadedInstantFetchOn) {
+  base::test::ScopedCommandLine scoped_command_line;
+  scoped_command_line.GetProcessCommandLine()->AppendSwitchASCII(
+      query_tiles::switches::kQueryTilesInstantBackgroundTask, "true");
+  EXPECT_CALL(*native_scheduler(), Schedule(TaskInfoEq(10 * 1000, 10 * 1000)));
+  tile_service_scheduler()->OnTileManagerInitialized(TileGroupStatus::kSuccess);
+}
+
 TEST_F(TileServiceSchedulerTest, OnTileGroupLoadedWithFailure) {
-  EXPECT_CALL(*native_scheduler(), Schedule(_));
+  EXPECT_CALL(*native_scheduler(), Schedule(TaskInfoEq(4000, 4000)));
   tile_service_scheduler()->OnTileManagerInitialized(
       TileGroupStatus::kFailureDbOperation);
 }