Do not record original URL if it is same as final URL.

Also add the logic to skip the redirect to itself if such old data
was found in the metadata store.

BUG=741197
TEST=new tests added

Change-Id: I006104b2716baf68303f99c035ea628d45dffdc0
Reviewed-on: https://chromium-review.googlesource.com/572601
Commit-Queue: Jian Li <jianli@chromium.org>
Reviewed-by: Dmitry Titov <dimich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487582}

chrome/browser/android/offline_pages/offline_page_request_job.cc

@@ -358,7 +358,13 @@ void SucceededToFindOfflinePage(
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // If the match is for original URL, trigger the redirect.
-  if (offline_page && url == offline_page->original_url) {
+  // Note: If the offline page has same orginal URL and final URL, don't trigger
+  // the redirect. Some websites might route the redirect finally back to itself
+  // after intermediate redirects for authentication. Previously this case was
+  // not handled and some pages might be saved with same URLs. Though we fixed
+  // the problem, we still need to support those pages already saved with this
+  if (offline_page && url == offline_page->original_url &&
+      url != offline_page->url) {
     ReportRequestResult(RequestResult::REDIRECTED, network_state);
     NotifyOfflineRedirectOnUI(job, offline_page->url);
     return;

chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc

@@ -49,15 +49,18 @@ const GURL kTestUrl2("http://test.org/page2");
 const GURL kTestUrl3("http://test.org/page3");
 const GURL kTestUrl3WithFragment("http://test.org/page3#ref1");
 const GURL kTestUrl4("http://test.org/page4");
+const GURL kTestUrl5("http://test.org/page5");
 const GURL kTestOriginalUrl("http://test.org/first");
 const ClientId kTestClientId = ClientId(kBookmarkNamespace, "1234");
 const ClientId kTestClientId2 = ClientId(kDownloadNamespace, "1a2b3c4d");
 const ClientId kTestClientId3 = ClientId(kDownloadNamespace, "3456abcd");
 const ClientId kTestClientId4 = ClientId(kDownloadNamespace, "5678");
+const ClientId kTestClientId5 = ClientId(kDownloadNamespace, "9999");
 const int kTestFileSize = 444;
 const int kTestFileSize2 = 450;
 const int kTestFileSize3 = 450;
 const int kTestFileSize4 = 111;
+const int kTestFileSize5 = 450;
 const int kTabId = 1;
 const int kBufSize = 1024;
 const char kAggregatedRequestResultHistogram[] =
@@ -257,6 +260,8 @@ class OfflinePageRequestJobTest : public testing::Test {
                 const GURL& original_url,
                 std::unique_ptr<OfflinePageArchiver> archiver);
 
+  OfflinePageItem GetPage(int64_t offline_id);
+
   void InterceptRequest(const GURL& url,
                         const std::string& method,
                         const std::string& extra_header_name,
@@ -289,6 +294,7 @@ class OfflinePageRequestJobTest : public testing::Test {
   int64_t offline_id2() const { return offline_id2_; }
   int64_t offline_id3() const { return offline_id3_; }
   int64_t offline_id4() const { return offline_id4_; }
+  int64_t offline_id5() const { return offline_id5_; }
   int bytes_read() const { return bytes_read_; }
 
   TestPreviewsDecider* test_previews_decider() {
@@ -301,6 +307,7 @@ class OfflinePageRequestJobTest : public testing::Test {
       const GURL& url,
       const std::string& method,
       content::ResourceType resource_type);
+  void OnGetPageByOfflineIdDone(const OfflinePageItem* pages);
   void ReadCompleted(int bytes_read);
 
   // Runs on IO thread.
@@ -331,7 +338,9 @@ class OfflinePageRequestJobTest : public testing::Test {
   int64_t offline_id2_;
   int64_t offline_id3_;
   int64_t offline_id4_;
+  int64_t offline_id5_;
   int bytes_read_;
+  OfflinePageItem page_;
 
   DISALLOW_COPY_AND_ASSIGN(OfflinePageRequestJobTest);
 };
@@ -344,6 +353,7 @@ OfflinePageRequestJobTest::OfflinePageRequestJobTest()
       offline_id2_(-1),
       offline_id3_(-1),
       offline_id4_(-1),
+      offline_id5_(-1),
       bytes_read_(0) {}
 
 void OfflinePageRequestJobTest::SetUp() {
@@ -363,13 +373,19 @@ void OfflinePageRequestJobTest::SetUp() {
       profile(), BuildTestOfflinePageModel);
   RunUntilIdle();
 
-  OfflinePageModel* model =
-      OfflinePageModelFactory::GetForBrowserContext(profile());
+  OfflinePageModelImpl* model = static_cast<OfflinePageModelImpl*>(
+      OfflinePageModelFactory::GetForBrowserContext(profile()));
 
   // Hook up a test clock such that we can control the time when the offline
   // page is created.
   clock_.SetNow(base::Time::Now());
-  static_cast<OfflinePageModelImpl*>(model)->set_testing_clock(&clock_);
+  model->set_testing_clock(&clock_);
+
+  // Skip the logic to clear the original URL if it is same as final URL.
+  // This is needed in order to test that offline page request handler can
+  // omit the redirect under this circumstance, for compatibility with the
+  // metadata already written to the store.
+  model->set_skip_clearing_original_url_for_testing();
 
   // All offline pages being created below will point to real archive files
   // residing in test data directory.
@@ -420,6 +436,20 @@ void OfflinePageRequestJobTest::SetUp() {
 
   SavePage(kTestUrl4, kTestClientId4, GURL(), std::move(archiver4));
 
+  // Save an offline page with same original URL and final URL.
+  base::FilePath archive_file_path5 =
+      test_data_dir_path.AppendASCII("offline_pages")
+          .AppendASCII("hello.mhtml");
+  std::unique_ptr<TestOfflinePageArchiver> archiver5(
+      new TestOfflinePageArchiver(kTestUrl5, archive_file_path5,
+                                  kTestFileSize5));
+
+  SavePage(kTestUrl5, kTestClientId5, kTestUrl5, std::move(archiver5));
+
+  // Check if the original URL is still present.
+  OfflinePageItem page = GetPage(offline_id5());
+  EXPECT_EQ(kTestUrl5, page.original_url);
+
   // Create a context with delayed initialization.
   test_url_request_context_.reset(new net::TestURLRequestContext(true));
 
@@ -532,6 +562,23 @@ void OfflinePageRequestJobTest::OnSavePageDone(SavePageResult result,
     offline_id3_ = offline_id;
   else if (offline_id4_ == -1)
     offline_id4_ = offline_id;
+  else if (offline_id5_ == -1)
+    offline_id5_ = offline_id;
+}
+
+OfflinePageItem OfflinePageRequestJobTest::GetPage(int64_t offline_id) {
+  OfflinePageModelFactory::GetForBrowserContext(profile())->GetPageByOfflineId(
+      offline_id,
+      base::Bind(&OfflinePageRequestJobTest::OnGetPageByOfflineIdDone,
+                 base::Unretained(this)));
+  RunUntilIdle();
+  return page_;
+}
+
+void OfflinePageRequestJobTest::OnGetPageByOfflineIdDone(
+    const OfflinePageItem* page) {
+  ASSERT_TRUE(page);
+  page_ = *page;
 }
 
 void OfflinePageRequestJobTest::InterceptRequestOnIO(
@@ -895,6 +942,22 @@ TEST_F(OfflinePageRequestJobTest, LoadOfflinePageAfterRedirect) {
           SHOW_OFFLINE_ON_DISCONNECTED_NETWORK);
 }
 
+TEST_F(OfflinePageRequestJobTest, NoRedirectForOfflinePageWithSameOriginalURL) {
+  SimulateHasNetworkConnectivity(false);
+
+  // No redirect should be triggered when original URL is same as final URL.
+  InterceptRequest(kTestUrl5, "GET", "", "", content::RESOURCE_TYPE_MAIN_FRAME);
+  base::RunLoop().Run();
+
+  EXPECT_EQ(kTestFileSize5, bytes_read());
+  ASSERT_TRUE(offline_page_tab_helper()->GetOfflinePageForTest());
+  EXPECT_EQ(offline_id5(),
+            offline_page_tab_helper()->GetOfflinePageForTest()->offline_id);
+  ExpectOneUniqueSampleForAggregatedRequestResult(
+      OfflinePageRequestJob::AggregatedRequestResult::
+          SHOW_OFFLINE_ON_DISCONNECTED_NETWORK);
+}
+
 TEST_F(OfflinePageRequestJobTest, LoadOfflinePageFromNonExistentFile) {
   SimulateHasNetworkConnectivity(false);
 

components/offline_pages/core/offline_page_model_impl.cc

@@ -323,7 +323,11 @@ void ReportInitializationAttemptsSpent(int attempts_spent) {
 
 // protected
 OfflinePageModelImpl::OfflinePageModelImpl()
-    : OfflinePageModel(), is_loaded_(false), weak_ptr_factory_(this) {}
+    : OfflinePageModel(),
+      is_loaded_(false),
+      testing_clock_(nullptr),
+      skip_clearing_original_url_for_testing_(false),
+      weak_ptr_factory_(this) {}
 
 OfflinePageModelImpl::OfflinePageModelImpl(
     std::unique_ptr<OfflinePageMetadataStore> store,
@@ -335,6 +339,7 @@ OfflinePageModelImpl::OfflinePageModelImpl(
       policy_controller_(new ClientPolicyController()),
       archive_manager_(new ArchiveManager(archives_dir, task_runner)),
       testing_clock_(nullptr),
+      skip_clearing_original_url_for_testing_(false),
       weak_ptr_factory_(this) {
   archive_manager_->EnsureArchivesDirCreated(
       base::Bind(&OfflinePageModelImpl::OnEnsureArchivesDirCreatedDone,
@@ -731,7 +736,15 @@ void OfflinePageModelImpl::OnCreateArchiveDone(
                                     save_page_params.client_id, file_path,
                                     file_size, start_time);
   offline_page_item.title = title;
-  offline_page_item.original_url = save_page_params.original_url;
+
+  // Don't record the original URL if it is identical to the final URL. This is
+  // because some websites might route the redirect finally back to itself upon
+  // the completion of certain action, i.e., authentication, in the middle.
+  if (skip_clearing_original_url_for_testing_ ||
+      save_page_params.original_url != offline_page_item.url) {
+    offline_page_item.original_url = save_page_params.original_url;
+  }
+
   offline_page_item.request_origin = save_page_params.request_origin;
   store_->AddOfflinePage(offline_page_item,
                          base::Bind(&OfflinePageModelImpl::OnAddOfflinePageDone,

components/offline_pages/core/offline_page_model_impl.h

@@ -110,6 +110,10 @@ class OfflinePageModelImpl : public OfflinePageModel, public KeyedService {
 
   OfflineEventLogger* GetLogger() override;
 
+  void set_skip_clearing_original_url_for_testing() {
+    skip_clearing_original_url_for_testing_ = true;
+  }
+
  protected:
   // Adding a protected constructor for testing-only purposes in
   // offline_page_storage_manager_unittest.cc
@@ -285,6 +289,9 @@ class OfflinePageModelImpl : public OfflinePageModel, public KeyedService {
   // it once it is not longer needed.
   base::Clock* testing_clock_;
 
+  // Don't clear original URL if it is same as final URL. For testing only.
+  bool skip_clearing_original_url_for_testing_;
+
   base::WeakPtrFactory<OfflinePageModelImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(OfflinePageModelImpl);

components/offline_pages/core/offline_page_model_impl_unittest.cc

@@ -567,6 +567,25 @@ TEST_F(OfflinePageModelImplTest, SavePageSuccessful) {
   EXPECT_EQ("", offline_pages[0].request_origin);
 }
 
+TEST_F(OfflinePageModelImplTest, SavePageSuccessfulWithSameOriginalURL) {
+  std::unique_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(
+      kTestUrl, OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED));
+  // Pass the original URL same as the final URL.
+  SavePageWithArchiverAsync(kTestUrl, kTestClientId1, kTestUrl, "",
+                            std::move(archiver));
+  PumpLoop();
+
+  EXPECT_EQ(SavePageResult::SUCCESS, last_save_result());
+  ResetResults();
+
+  const std::vector<OfflinePageItem>& offline_pages = GetAllPages();
+
+  ASSERT_EQ(1UL, offline_pages.size());
+  EXPECT_EQ(kTestUrl, offline_pages[0].url);
+  // The original URL should be empty.
+  EXPECT_TRUE(offline_pages[0].original_url.is_empty());
+}
+
 TEST_F(OfflinePageModelImplTest, SavePageSuccessfulWithRequestOrigin) {
   EXPECT_FALSE(HasPages(kTestClientNamespace));